Skip to content

fix(billing): fence durable key recovery - #550

Open
drewstone wants to merge 7 commits into
mainfrom
feat/billing-durability-20260903
Open

fix(billing): fence durable key recovery#550
drewstone wants to merge 7 commits into
mainfrom
feat/billing-durability-20260903

Conversation

@drewstone

@drewstone drewstone commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes durable workspace-key recovery gaps exposed by Physim's provisioning audit.

  • Persists one idempotencyKey per provisioning row and sends it on create retries.
  • Retains the published DurableWorkspaceKeyStore lifecycle method signatures.
  • Adds optional DurableWorkspaceKeyConditionalWrites compare-and-set writes.
    A false result fences a stale creator and triggers remote compensation.
  • Probes empty crash rows with their persisted request identity.
  • Keeps unresolved rows revocation_pending instead of orphaning unknown remotes.
  • Adds recoveryProvisioner for historical get/revoke/find operations after source credential rotation.
    It never mints and never stores raw credentials.
  • Keeps current createWorkspaceKeyManager behavior unchanged.

Consumer migration

Add nullable idempotency_key storage to durable rows and persist it for new rows.
Wire store.conditionalWrites to atomic writes guarded by status='provisioning'.
Wire a documented control-plane provisioner for historical child-key reads, revokes, and discovery.
Pre-identity rows use the deterministic row-id fallback only for recovery.
Rows without a recoverable name or source identity fail closed.

Stores that omit conditionalWrites remain source-compatible with the published API.
They retain the legacy lifecycle behavior and should add the optional capability before relying on cross-worker fencing.

Proof

  • Focused durable-manager tests: 27/27 on Node 24.13.0.
  • Full signoff on 6364bf5be479: install, typecheck, incident gates, build, generated projects, knip, and two shuffled unit runs passed.
    Seeds: 1400786161, 1164662825.
  • Full signoff wall time: 345.5 seconds.
  • Commit-level Codex review on 6364bf5: no discrete bug found.
  • GitHub's full reviewer approved the implementation with zero blockers and four low findings.

Physim's live consumer migration and operator proof remain outside this library PR.

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 17f79afb

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-09-03T02:19:00Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 2 (2 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 407.4s (2 bridge agents)
Total 407.4s

💰 Value — sound-with-nits

Hardens the just-shipped identity-bound workspace-key manager's crash/source-rotation recovery (idempotent re-create, status fencing, fail-closed probe, control-plane recovery provisioner) using patterns already established in the repo — a good change with one dead-seam nit.

  • What it does: Adds four recovery mechanisms to src/billing/identity-bound.ts (the durable key manager added hours earlier in #549): (1) persists a per-row idempotencyKey (fallback workspace-key:<rowId>) and sends it on create retries so idempotent providers can't double-mint after a crash (identity-bound.ts:300-303, 734, 749-755); (2) re-documents markProvisioningRemote/markActive/`markRevocationPending
  • Goals it achieves: Makes the durable child-key state machine survive the failure modes its predecessor left open: a create that commits remotely but crashes before persisting (idempotent re-probe instead of double-mint), a concurrent worker retiring a row mid-create (status fencing), an empty crash row that never committed (probe + retry rather than orphan), and source-credential rotation that revokes a child key's
  • Assessment: Good. The change is tightly scoped to one module and each addition mirrors an existing codebase idiom rather than inventing one: idempotency keys match the sandbox module's makeSandboxProjectRef/dispatchPrompt retry identity (src/sandbox/index.ts:2528), fail-closed-on-unknown matches the /spend ownership rules (undecidable → fail closed), and status-conditional writes match the /record and
  • Better / existing approach: none — this is the right approach. Searched for existing idempotency/recovery primitives: grep across src/ shows idempotency keys live in sandbox, spend, chat-routes, and missions, but none of them are reusable for billing-key minting (they govern turns, boxes, and settlement rows, not child-key create/revoke). createTcloudKeyProvisioner (src/billing/index.ts:55) is the source-side adapter and i
  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 3
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error event without a message: {"type":"error","timestamp":1788402113526,"sessionID":"ses_f9aed664cffe2CucLw8jUePfcI","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_d9fc4fd8"}}}; opencode/zai-coding-plan/glm-5.2: opencode: opencode error event without a message: {"type":"error",

🎯 Usefulness — sound-with-nits

A coherent, additive hardening of the identity-bound durable key manager that closes real crash-recovery and source-rotation gaps, built in the existing /billing seam pattern; only a minor adoption caveat remains.

  • Integration: Reachable and additive. The manager itself landed one commit earlier (29c2f2f, 'feat(billing): add identity-bound durable key manager') and has no in-repo consumer yet — its caller is the external Physim product, which is the normal 'capability slightly ahead of first caller' case, not dead surface. The new surface is wired end-to-end: recoveryProvisioner defaults to options.provisioner (src/billi
  • Fit with existing patterns: Follows the established /billing seam contract (structural, no SDK dep): recoveryProvisioner is a Pick<getKey|revokeKey|findCreatedKeys> that never mints, and minting always stays on options.provisioner (identity-bound.ts:527). This is the right split for the stated goal — a rotated source can still read/revoke historical children via a control-plane client while fresh mints use the live credentia
  • Real-world viability: The probe path (identity-bound.ts:520-546) handles the provider permutations correctly: name-search finds already-committed creates, native-idempotency providers return the same key on retry, and a provider with neither still gets the persisted name as the search key — the residual zombie risk is only for a provider with no name-search AND no idempotency, which the doc comment explicitly calls out
  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 3
  • Bridge warning: opencode/zai-coding-plan/glm-5.2: opencode: opencode error event without a message: {"type":"error","timestamp":1788402319439,"sessionID":"ses_f9aed6571ffe26tUQOp6kLWD9N","error":{"name":"APIError","data":{"message":"Rate limit reached for requests","statusCode":429,"isRetryable":true,"responseHeaders":{"alt-svc":"h3=":443"; ma=3600","connection":"keep-alive","date":"Thu, 03 Sep 2026 02:25:19 GM

💰 Value Audit

🟡 markOrphaned store method is now dead in the manager [maintenance] ``

The change removes the only manager call to markOrphaned (old identity-bound.ts:521 no unowned remote child matched...), replacing it with markRevocationPending. markOrphaned now appears only in the DurableWorkspaceKeyStore interface (identity-bound.ts:128) and the test harness — an orphaned seam. Since the module shipped in 0.46.54 minutes ago and the PR body already declares a consumer migration, consider dropping the method (and its test markOrphaned stub) now rather than carrying

🎯 Usefulness Audit

🟡 Fence correctness lives in the consumer's store, not the manager [robustness] ``

The headline guarantee — a paused creator cannot reactivate a retired row — depends entirely on the consumer implementing the newly-documented status-conditional markProvisioningRemote/markActive/markRevocationPending writes (identity-bound.ts:108-124, and the test harness's if (row.status !== 'provisioning') return guard at tests/billing-identity-bound.test.ts). The manager's only backstop is the post-markActive getActive re-read (identity-bound.ts:820-835), which detects the case where the s


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260903T022839Z

@tangletools

Copy link
Copy Markdown

✅ No Blockers — 17f79afb

Review health 100/100 · Reviewer score 83/100 · Confidence 95/100 · 4 findings (4 low)

deepseek: Correctness 83 · Security 83 · Testing 83 · Architecture 83

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 7/7 planned shots over 8 changed files. Global verifier still owns final merge decision.

🟡 LOW Non-idempotent providers mint a throwaway key per recovery retry — src/billing/identity-bound.ts

For a provider without native idempotency whose findCreatedKeys consistently returns [] (e.g. a name-index lag or a query mismatch), each retry of a stuck provisioning row calls options.provisioner.createKey, minting a fresh budget-bearing child key that is then revoked, until one revoke succeeds. This consumes parent-key headroom once per retry (bounded by retryDelay backoff and terminating on the first successful revoke). The interface comment acknowledges the non-idempotent case relies on name search, so this is a documented limitation, but the probe mechanism is strictly more mint-happy than the previous cleanup (which never re-created). No change required unless a provider reports sustained findCreatedKeys failures; then a cap or a distinct 'unrecoverable' terminal state would avoid r

🟡 LOW Probe fallthrough branch is untested — src/billing/identity-bound.ts

After createKey returns no usable id, probeProvisioning falls through to provisioningCandidates({...row, name, keyId: 'provisioning:'+idempotencyKey})?.candidates ?? [] to discover a remotely-committed key by name. This branch (createKey committed but returned empty id during RECOVERY, as opposed to the fresh-mint path) is exercised by no test: 'cleans a remote key when create returns no id' goes through the mint() catch path at line 787 (revokePending), not through probeProvisioning. The branch is defensive and low-risk, but a provider that returns no id on a recovery re-mint would silently route to the generic 'no remote child matched' pending state rather tha

🟡 LOW Idempotency contract has no platform-adapter support yet — src/billing/index.ts

DurableWorkspaceKeyProvisioner.createKey now requires idempotencyKey (DurableWorkspaceKeyCreateInput) and the interface documents 'reusing idempotencyKey must be safe'. But the concrete platform adapter in this package, TcloudKeyClient.createKey (index.ts:35), accepts name/product/budgetUsd/expiresAt/parentKeyId/allowedModels/rpmLimit and has no idempotencyKey field, and was not updated in this PR (index.ts diff is only the export). A consumer wiring createTcloudKeyProvisioner-style plumbing has no field to pass the key through, so the idempotent-recovery guarantee silently degrades to the non-idempotent 'create a new key, revoke it' path until the platform SDK exposes idempotency. Not merge-blocking because the name-based findCreatedKeys fallback still provides crash-safe cleanup, but the

🟡 LOW idempotencyKey null-fallback (pre-identity rows) is untested — tests/billing-identity-bound.test.ts

insertProvisioning always assigns a non-null idempotencyKey (workspace-key:row-${rowNumber}), so no test exercises the migration path where idempotencyKey is null/empty. The source explicitly documents idempotencyKey: string | null as 'Null only for pre-identity rows' and idempotencyKeyForRecord() falls back to workspace-key:${row.id} when the value is absent. That fallback is the exact branch a pre-migration production row takes on recovery, and it is unverified — a regression there would re-key a legacy row with an unstable id, defeating the crash-safe idempotency this PR adds. Add one case calling insertProvisioning({ idempotencyKey: null }) (or a raw legacy record) and asserting the probe's idempotencyKey equals workspace-key:${row.id}.


tangletools · 2026-09-03T02:56:46Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — 4 non-blocking findings — 17f79afb

Full multi-shot audit completed 7/7 planned shots over 8 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-09-03T02:56:46Z · immutable trace

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 6364bf5b

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-09-03T03:14:57Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 4 (4 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 566.9s (2 bridge agents)
Total 566.9s

💰 Value — sound-with-nits

A same-day hardening of the just-shipped durable workspace-key manager: it adds persisted idempotency keys, optional CAS fencing of lifecycle writes, identity-gated crash probes, and a separate recovery client — coherent, in-grain, well-tested; only minor surface-hygiene nits.

  • What it does: Extends createIdentityBoundWorkspaceKeyManager (src/billing/identity-bound.ts, introduced in #549/0.46.54 hours earlier) in four ways: (1) every provisioning row now persists an idempotencyKey ('workspace-key:' at identity-bound.ts:799) that is sent on createKey and reused on retries, with a deterministic row-id fallback for pre-identity rows (identity-bound.ts:326-329); (2) an optional Dur
  • Goals it achieves: Closes real crash-recovery and split-brain gaps in a billing-safety module: a stale creator resuming after lease expiry can no longer activate a row another worker retired (fencing closes what the time-based withDurableLease cannot — leases are liveness, CAS is safety); a create whose response was lost is recovered deterministically via idempotency key + persisted name search instead of being gues
  • Assessment: Good on its merits. The design choices match the house grain: additive optional port capability (mirroring SpendLedgerStorePort.listLiveBetween and the AGENTS.md additive-export rule) instead of the breaking boolean-return change that was tried and deliberately reverted mid-PR (commits de9c5cc30f7b73/e0de1c3, 'preserve lifecycle store api'); mints stay on the live provisioner while historical r
  • Better / existing approach: none — this is the right approach. Searched for reusable fencing/CAS/idempotency primitives: missions' guarded update(id, guard, patch) (src/missions/service.ts:148-153) is a domain store port, not a shareable primitive; /spend's ledger is deliberately NOT CAS by design (src/spend/store.ts:7); prewarm-claim-d1's lease table and /turn-stream's DurableTurnLock are environment-specific (D1, Cloudflar
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error event without a message: {"type":"error","timestamp":1788405443694,"sessionID":"ses_f9aba952dffeo1HCYESNJxV161","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_1bf28339"}}}

🎯 Usefulness — sound-with-nits

A well-built hardening pass on the identity-bound key manager that shipped in #549 hours earlier, closing real crash-recovery and cross-worker fencing gaps the base version genuinely had, with every new seam internally reachable and an imminent named consumer (Physim) whose migration this explicitly

  • Integration: Reachable now, not speculative. The base surface createIdentityBoundWorkspaceKeyManager is already merged to main and released (d9bb1b2/#549, v0.46.54 per git log), and every new path in this PR sits on the hot path of the existing API: idempotencyKey is persisted in mint (src/billing/identity-bound.ts:799) and sent on create (:819); probeProvisioning is reached from cleanupProvisioning
  • Fit with existing patterns: Exactly the established /billing grain: structural store/provisioner/crypto seams with the product as a parameter, mirroring the existing KeyProvisioner/WorkspaceKeyStore pattern (src/billing/index.ts:22-100), fail-closed posture, additive optional capability. conditionalWrites? is an optional field so legacy void-method stores stay source-compatible — pinned by a dedicated test (tests/billi
  • Real-world viability: Built for the crash and concurrency cases, not the happy path. The CAS fence: a stale creator whose row was retired gets false from markProvisioningRemote/markActive, throws, and compensates by revoking the already-minted remote key (:860-887, pinned by the 'fences a paused creator' test at tests/billing-identity-bound.test.ts:719-731). Create-failure, missing-id, no-secret, post-activation
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 markOrphaned and the 'orphaned' status are now unreachable from the manager [maintenance] ``

rg over src/ shows store.markOrphaned declared at src/billing/identity-bound.ts:147 with zero call sites in the manager (only the interface and the test harness mock implement it); the sole writer was the old cleanupProvisioning else-branch, replaced by markRevocationPending retry at identity-bound.ts:641-648. Every consumer must now implement a required store method that can never fire, and 'orphaned' rows can only be written by consumer code directly. Since 0.46.54 shipped the same day and the

🟡 Permanent dual write path: void lifecycle methods + conditionalWrites side by side [maintenance] ``

When conditionalWrites is present, the void markProvisioningRemote/markActive are never consulted (identity-bound.ts:331-353), so a store carries two implementations of the same writes that can drift — the test harness already delegates one to the other (tests/billing-identity-bound.test.ts:101-106), which should become the documented consumer pattern. This is the accepted cost of not breaking the hours-old published interface; consider noting in the store docs that conditionalWrites implementat

🎯 Usefulness Audit

🟡 Required idempotencyKey on DurableWorkspaceKeyRecord is a type-level break of the just-released 0.46.54 interface — release as a minor [ergonomics] ``

DurableWorkspaceKeyRecord.idempotencyKey: string | null (src/billing/identity-bound.ts:45) is a new REQUIRED property on an interface that shipped to npm in 0.46.54 hours before this PR (release commit c289abb follows the #549 merge). Any store implemented against the published 0.46.54 shape stops typechecking until it adds the column. The repo's own invariant is 'additive subpaths... never a breaking change to an existing export' (AGENTS.md, when-you-add-a-module). Practical blast radius is ~

🟡 Crash-row probing can mint-then-revoke a real remote key on providers without native idempotency [robustness] ``

probeProvisioning (src/billing/identity-bound.ts:570-596) resolves an empty crashed row by re-issuing createKey with the persisted identity when the retrying caller matches; on a provider that ignores idempotencyKey this creates a fresh remote child key that is then immediately revoked (:624-637). Money-safe and self-cleaning, but each retry cycle costs a real create+revoke pair on the platform API until the row resolves. The interface doc acknowledges the trade ('providers without native


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260903T032649Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — f8c36800

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-09-03T03:36:40Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Coverage 1 of 2 lenses (value)
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 511.2s (2 bridge agents)
Total 511.2s

⚠️ Partial audit — the verdict covers only value. usefulness: agent returned no usable verdict. Treat the missing lens as unexamined, not as clear.

💰 Value — sound

Hardens the day-old durable workspace-key manager's crash recovery with a persisted idempotency identity, optional CAS fencing for stale creators, fail-closed pending states, and a separate recovery credential seam — additive, source-compatible, and in the grain of this codebase's existing fencing p

  • What it does: Extends createIdentityBoundWorkspaceKeyManager (added one release ago in PR #549, src/billing/identity-bound.ts) in four ways: (1) every provisioning row now persists an idempotencyKey (default workspace-key:<rowId>, identity-bound.ts:326-329,799) that is sent on createKey retries so a crashed create is re-issued under the same request identity; (2) an optional `DurableWorkspaceKeyConditio
  • Goals it achieves: Closes the stranded-billable-key and double-active-key windows in a money-path lifecycle. Before: a crashed create recovered only by name search, a probe with a fresh identity could mint a second key, a transiently-empty name search marked the row terminal orphaned (permanently abandoning a possibly-committed remote key that keeps billing), a stale worker whose lease expired mid-create could act
  • Assessment: Good and coherent. The design choices match both the problem and the repo's conventions: fail-closed pending states over terminal guesses (repo convention: fail-closed defaults for money/data integrity); fencing as an OPTIONAL capability object rather than changing published method return types, which the commit history shows was deliberately converged on (30f7b73 and e0de1c3 'preserve lifecycle s
  • Better / existing approach: Searched for an existing primitive to reuse or extend: rg 'conditionalWrites|compare-and-set|fenc|guarded|idempotency' across src/. The codebase has three vertical-local fencing precedents — missions guarded-CAS (src/missions/service.ts:97-111), prewarm lease-fencing (src/sandbox/prewarm-claim-d1.ts:37), turn-stream lock fences (src/turn-stream/core.ts:267) — plus idempotent-create recovery in s
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error event without a message: {"type":"error","timestamp":1788407086828,"sessionID":"ses_f9aa18291ffesGQYRdXUyTufXK","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_8cd9fc8e"}}}

🎯 Usefulness — error

usefulness agent produced no parseable value-audit JSON.

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 3
  • Bridge error: opencode/zai-coding-plan/glm-5.2: opencode: opencode error event without a message: {"type":"error","timestamp":1788407592201,"sessionID":"ses_f9aa18298ffewcRfuTVl645Nj1","error":{"name":"APIError","data":{"message":"Rate limit reached for requests","statusCode":429,"isRetryable":true,"responseHeaders":{"alt-svc":"h3=":443"; ma=3600","connection":"keep-alive","date":"Thu, 03 Sep 2026 03:53:12 GM

💰 Value Audit

🟡 Store port retains dead surface: markOrphaned is required but never called, and fenced stores implement unreachable void writes [maintenance] ``

This PR removes the manager's only call to markOrphaned (grep: sole remaining reference is the interface declaration at src/billing/identity-bound.ts:147 and the test harness at tests/billing-identity-bound.test.ts:124), yet the method stays REQUIRED on DurableWorkspaceKeyStore — every new store implements a method the manager can never invoke. Likewise, a store that provides conditionalWrites must still implement the void markProvisioningRemote/markActive (identity-bound.ts:123,125), which beco


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260903T035316Z

@tangletools

Copy link
Copy Markdown

⚠️ Review Incomplete — f8c36800

At least one required reviewer lane failed closed. No approval or request-changes review was published. This is a reviewer run failure, not a PR quality score.

Trigger a fresh review on the current PR head.

tangletools · 2026-09-03T04:17:24Z

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — f8c36800

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-09-03T04:47:14Z

@tangletools

Copy link
Copy Markdown

⚠️ Review Interrupted — f8c36800

The review runner stopped before publishing a final verdict: webhook_restarted.

State Detail
Interrupted webhook restarted

No review verdict was produced for this run. Trigger a fresh review on the current PR head if the PR is still open.

tangletools · #550 · model: kimi-for-coding · updated 2026-09-03T05:07:42Z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants