fix(billing): fence durable key recovery - #550
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
🟡 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(fallbackworkspace-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-documentsmarkProvisioningRemote/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/dispatchPromptretry identity (src/sandbox/index.ts:2528), fail-closed-on-unknown matches the/spendownership rules (undecidable → fail closed), and status-conditional writes match the/recordand - 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:521no unowned remote child matched...), replacing it withmarkRevocationPending.markOrphanednow appears only in theDurableWorkspaceKeyStoreinterface (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 testmarkOrphanedstub) 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') returnguard 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.
✅ No Blockers —
|
tangletools
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
🟡 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 de9c5cc → 30f7b73/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
createIdentityBoundWorkspaceKeyManageris 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:idempotencyKeyis persisted inmint(src/billing/identity-bound.ts:799) and sent on create (:819);probeProvisioningis reached fromcleanupProvisioning - Fit with existing patterns: Exactly the established /billing grain: structural store/provisioner/crypto seams with the product as a parameter, mirroring the existing
KeyProvisioner/WorkspaceKeyStorepattern (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
falsefrommarkProvisioningRemote/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-issuingcreateKeywith the persisted identity when the retrying caller matches; on a provider that ignoresidempotencyKeythis 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.
tangletools
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
🟢 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 anidempotencyKey(defaultworkspace-key:<rowId>, identity-bound.ts:326-329,799) that is sent oncreateKeyretries 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.
|
tangletools
left a comment
There was a problem hiding this comment.
✅ 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
|
| 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
Summary
Closes durable workspace-key recovery gaps exposed by Physim's provisioning audit.
idempotencyKeyper provisioning row and sends it on create retries.DurableWorkspaceKeyStorelifecycle method signatures.DurableWorkspaceKeyConditionalWritescompare-and-set writes.A false result fences a stale creator and triggers remote compensation.
revocation_pendinginstead of orphaning unknown remotes.recoveryProvisionerfor historicalget/revoke/findoperations after source credential rotation.It never mints and never stores raw credentials.
createWorkspaceKeyManagerbehavior unchanged.Consumer migration
Add nullable
idempotency_keystorage to durable rows and persist it for new rows.Wire
store.conditionalWritesto atomic writes guarded bystatus='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
conditionalWritesremain 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
6364bf5be479: install, typecheck, incident gates, build, generated projects, knip, and two shuffled unit runs passed.Seeds:
1400786161,1164662825.6364bf5: no discrete bug found.Physim's live consumer migration and operator proof remain outside this library PR.