Optional Claustrum vault custody for fallback OAuth accounts - #175
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
All reported issues were addressed across 19 files
Architecture diagram
sequenceDiagram
participant Boot as Plugin Boot
participant FallbackMgr as FallbackAccountManager
participant ClaustrumCache as ClaustrumCredentialCache
participant ClaustrumClient as ClaustrumClient
participant Vault as Claustrum Vault (IPC Daemon)
participant Sidecar as Anthropic Sidecar
participant Request as Request Path
participant WeakMap as Served-Credential WeakMap
Note over Boot,Vault: Startup Detection & Warmup
Boot->>Boot: Filter fallback accounts with claustrumHandle + opt-in gate
alt Has vault-managed accounts
Boot->>ClaustrumClient: connect() (IPC handshake, explicit consumerIdentity: null)
ClaustrumClient-->>Boot: Client handle
Boot->>ClaustrumCache: Create cache (bounded TTL, dedup in-flight)
Boot->>ClaustrumCache: Warm all handles (Promise.all)
ClaustrumCache->>ClaustrumClient: credential.get(handle)
ClaustrumClient->>Vault: IPC request (capability handle)
Vault-->>ClaustrumClient: Credential payload + recordVersion + expiry
ClaustrumClient-->>ClaustrumCache: Validate & cache (only if expiresAtMs > now)
alt Warmup completes within 100ms
ClaustrumCache-->>Boot: Credentials resident
else Warmup timeout
Boot->>Boot: Mark cold accounts (claustrumColdAccounts)
Note over Boot: Detached refresh continues in background
end
else No vault or no opted-in accounts
Note over Boot: Nothing runs - feature fully inert
end
Note over Request,Vault: Request-Time Resolution (happy path)
Request->>Request: resolveClaustrumAccess(account)
alt Handle present + gate enabled + not blocked
Request->>ClaustrumCache: peek(handle) (synchronous, ~35us)
alt Resident credential not expired
ClaustrumCache-->>Request: Cached credential
Request->>Request: Extract access_token from payload
Request->>WeakMap: Bind {accountId, handle, recordVersion} to Response
Request->>Sidecar: Send request with vault-served token
else Not resident or expired
Request->>Sidecar: Fall back to sidecar token (bounded degradation)
end
else No handle / gate off / vault missing
Request->>Sidecar: Use sidecar access token
end
Note over Request,WeakMap: 401 Handling (version-fenced, provenance-bound)
Sidecar-->>Request: 401 response
Request->>WeakMap: Lookup served credential for response
alt Vault served this response (WeakMap hit)
Request->>ClaustrumCache: reportAuthFailure(handle, recordVersion)
ClaustrumCache->>Vault: invalidate (version-gated)
Vault-->>ClaustrumCache: Acknowledged
alt Version stale (other consumer already invalidated)
ClaustrumCache->>ClaustrumCache: Drop report - version gate rejects
end
else Sidecar served this response (WeakMap miss)
Note over Request: Skip vault invalidation - provenance guard prevents<br/>corrupting healthy vault credential
end
Note over Request,Sidecar: Async Refresh (off response path)
Request->>ClaustrumCache: get(handle) trigger (detached)
ClaustrumCache->>Vault: credential.get (slow path: seconds)
Vault-->>ClaustrumCache: Refreshed credential
ClaustrumCache->>ClaustrumCache: Update cache with new recordVersion
Note over Request: Next request collects refreshed credential via peek
Note over Boot,Vault: Error Classification
ClaustrumCache-->>Boot: ClaustrumCredentialError
alt action = gone
Boot->>Boot: claustrumBlockedAccounts.add(accountId)
else action = reauth
Boot->>Boot: claustrumBlockedAccounts + reauthAccounts
Boot->>Sidebar: Mark needsReauth true
else action = reduce_and_retry
Boot->>ClaustrumCache: reduceMinTtlMs()
end
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
All ten review threads addressed in Two of the P2s turned out to be the same defect, which is why they are one commit: the send path and the report path both looked the served credential up again instead of carrying it.
The rest of the P2s:
The extra finding, which is the one I would not have shipped without. Verifying the "exactly one That matters because the vault's limiter counts failed gets against a shared 64/60s budget and writes a durable anomaly on crossing. Its contract is explicit that a tight bound with eager retry is the combination to avoid, and this was exactly that, arriving from the per-request path rather than the startup warm. Fixed with per-handle backoff, and tested as a rate property rather than a single call: An assertion that "the second request did not call" would have passed on an off-by-one and missed the property. On the two test-quality threads: both were right, and the second was sharper than its P3 label. That is the third assertion-over-an-empty-set on this branch, after the 153 tests in a directory no gate ran and the provenance tests using a credential expiring in 1ms that the cache therefore never stored. Worth stating plainly: they are cheap to write and they read exactly like protection. Also in this commit, from an independent cross-family drift pass rather than the bot review: the runtime override struct had drifted to holding timers, the vault connector and the clock under two different names, so it is now one name matching its contents; and the cold-account set was write-only, so it is gone — the self-heal path it looked like it was for is now covered by a test that holds the startup get past the warm timeout, confirms the request is skipped, then confirms the next request serves once the detached refresh lands. |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Round 2 — all five closed, The P1 was real, and it was caused by the previous round's fix. Threading the resolution into So The repair had to respect a constraint that rules out the obvious fix: after the report invalidates the entry, re-resolving hits a cold cache, and it must not await a fresh credential because that puts the vault's seconds-long refresh back on the response path. The design already has an answer for a cold vault credential — skip the route and let the detached refresh land for a later request — so the retry now follows that same rule rather than getting a special case: Mutation-proved: forcing it to always resend reddens
Blank connection paths are treated as unset, including whitespace-only. Both the runtime and status paths go through the one helper the previous round unified, so this is a single fix rather than two. The test-name thread was a naming defect, not a bug — the assertions are the intended behaviour (a captured credential near expiry is served, and its failure is reported using the captured provenance). Renamed to say that. The skip path is exercised by the neighbouring test, as the reviewer noted. One correction to my own process here. The duplicate-assertion nit was reported back to me as absent at baseline, because the referenced line no longer held it — but the duplicate had simply moved to lines 1366-1367 as the file grew. It was present at baseline too, at 1325-1326. A line-number-scoped check answered "is it here" rather than "is it anywhere", and the negative came back clean. Fixed in |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Pinned the claim I made when declining the forced-refresh suggestion above — I had argued that a vault-enabled account does not lose ordinary OAuth recovery when the vault is unavailable, because the resolution carries no The serving half of that outage path was already covered; the recovery half was not, which is exactly the gap that made the reviewer's concern reasonable. Separately, on the red CI here: it is the It is not the websocket exchange the test is named for — it is Miniflare startup. Every timeout in that harness is bounded at 5s, but the failure lands at 30s, which is bun's default, so none of them fired. The only unbounded awaits are Four tests each start their own worker, so whichever loses the startup race under load is the one that hangs — hence the moving test name. The file alone passes 4/4 repeatedly. Worth flagging plainly: I characterised this failure three times across these PRs — "always the last of the four tests", "deterministic, not a flake", and "CI passes it consistently" — and all three were wrong. The first two were measured without controlling for machine load, and the third is contradicted by the CI run on this branch. #176 carries the corrected version, since the wrong ones are quoted in PR text above. |
cca53d1 to
dbb6a18
Compare
|
Greptile's finding is correct and now fixed in Confirmed empirically before fixing, with a plain OAuth account as the positive control: Identical. A vault-managed account's stale sidecar refresh token went to Anthropic's token endpoint on a background timer, returned This is the same hazard I argued on the 401 thread above, where I declined a forced sidecar refresh on the grounds that the vault is the refresh authority and the sidecar token is not ours to spend. That reasoning was right and this code was violating it on a timer — worse than the case I declined, because it fires with no provocation at all. Arguing a principle in a review thread is not the same as having implemented it. The fix is dynamic, not a static gate, because a static one would break the outage-recovery property pinned elsewhere on this branch:
Verified against my own probe on the fixed tree: The implementer extended the guard past the two call sites I named — Gate at the pushed sha: 1379 pass / 0 fail (baseline 1375, +4), e2e 29/0, typecheck, format, and Biome clean. Mutation proofs: removing the request guard fails 1 test, the background guard 2, the quota-background guard 1. One note on method, since it nearly cost this finding. My first two probes reported zero token-endpoint hits for both the vault and plain cases, which reads as a refutation. It was a broken probe — I passed the config path positionally to a constructor that takes an options object, so the manager loaded a different file entirely and never reached the code under test. Only the plain-OAuth positive control revealed it. Without that control I would have posted "cannot reproduce" against a correct finding. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Both P1s fixed in Cubic — the predicate accepted an expired credential. What settles it as a defect rather than a judgement call is that the serving path already refused expired credentials. The path that serves and the gate that decides refresh disagreed about what "served" meant. Now there is one Greptile — a cold-cache local refresh could permanently exclude the account. Cold cache → not-served → local refresh on a lineage the vault has likely rotated → The error is still recorded — the diagnostic is worth keeping — but it no longer outlives the condition: once a usable vault credential is resident for that account and handle, the persisted error is cleared. A plain OAuth account's permanent error is untouched. Both follow from one rule: for a Claustrum-enabled account the sidecar is a degradation path, never the authority. Served must mean usably served, and a sidecar failure must not be what permanently kills the account. Mutation-proved both myself rather than relying on the implementer's counts: Gate at the pushed sha: 1381 pass / 0 fail (baseline 1379, +2), e2e 29/0, typecheck, format, and Biome clean. Worth noting the sequence, because it is the argument for two independent reviewers: |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
b73540b to
3107d57
Compare
|
Stop — this PR does not work. Do not merge it. An independent review round found it and I confirmed it against the live vault daemon: every credential call throws, so the feature is inert in production. That is the real daemon, reached through the production path — The cause is my own security fix, and the two things have similar names. So the PR body's claim that "the safe path cannot be reached by forgetting" is true about the parameter I was thinking of and blind to the one next to it. Why nothing caught it — six review rounds, two bots, a cross-family drift pass, and my own wire-body test. Every test either passes It is also the fifth instance this branch has produced of the same class: a test whose fixture cannot exercise the thing the test is named for. The previous four were assertions over empty sets; this one is a whole feature. Being fixed now: thread a real Two review notes from the same round, both worth having:
The network-guard half (#174) came back clean from the same round — no escape path found, including IPv4 integer/hex/short forms, lying |
|
The feature works now. The call now clears identity and reaches the vault; What the identity is, and why. That last part came out of a premise of mine being wrong. I argued for a stable id because a vault's audit chain records who asked; reading Claustrum's source showed
Also fixed, both from the same review round:
Per-fix mutations, each reverted and restored: Gate at the pushed sha: 1389 / 0, e2e 29/0, typecheck and format clean. Delta over the previous 1385 is +5 new tests −1 renamed. The remaining findings from that review round are test-quality gaps in my own tests plus the |
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
2b0f8dd to
c2a1132
Compare
|
Squashed to one commit on top of #174's squashed base — Everything from the five-way review round is in:
The commit message carries the reasoning that is worth keeping — why main-account custody is out of scope, why the request path only peeks, why a vault outage must not be worse than no vault, and why provenance rides on the response instead of being looked up at report time. One process note, since it is the durable lesson from this PR. Six bot-review rounds, a cross-family drift pass, and 1385 green tests all passed over a feature that had never once worked. Every test either injected the identity or stubbed the connector, and the fake daemon did not enforce what the real client enforces — so the fixtures could not reach the defect. What found it was one bounded read-only probe against the live daemon. For an integration with a real local dependency, that probe is worth more than the suite, and defining the pass as "reached the far side" (an unknown-handle error counts) makes it safe to run without touching real state. |
c2a1132 to
73d0de8
Compare
|
Fixed in Clearing now runs whenever a usable vault credential is actually served for an account carrying a persisted error, not only on the cold→warm transition — gated on the serving storage snapshot so it cannot fire on the common path, dispatched detached after Mutation-proved myself rather than taking the implementer's counts. Neutering the trigger (2 call sites) reddens Worth recording what the shape was, because it is not the usual race. The clear was wired to a transition, and That is the second finding on this branch where the defect was an asymmetry in time rather than in code — the first being a 401 retry arm that did not update the route its two siblings updated. Both look correct read line by line; both are only visible if you ask what happens on the second occurrence. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
a372481 to
b6f5341
Compare
|
Correcting myself: the severity I claimed on the previous fix was wrong. The fix was right, my account of why it mattered was not. I said a persisted permanent error excluded the account from routing while the vault served it, citing
So a vault-served account with a stale permanent error keeps routing and serving normally. No exclusion, no absorbing state. The real consequence is narrower and, I think, more interesting: the stale error is latent, and it arms exactly when the vault stops covering it. While the credential is resident, nothing reads the error. The moment the vault goes cold — outage, revoked handle, disabled gate — the account falls back to the local path, Still worth fixing, and the fix is unchanged. But "excluded right now" and "excluded later, when your fallback is all you have" are different claims, and I asserted the first without checking it. Both stale-gate items are in The clear no longer decides from the request-start snapshot. It takes a fresh The residual is honest: a fresh read still has a read-to-lock window. That one does self-heal — the next resident vault-served request clears it — which is precisely the property the original transition-only design lacked. Mutations, run against the restacked tree: restoring the request-snapshot gate reddens the ordering test (vault auth falls back to |
7a2fb1f to
1f0486b
Compare
ualtinok
left a comment
There was a problem hiding this comment.
Request changes
-
[P1] Keep opaque credential handles out of logs —
packages/opencode/src/index.ts:4902-4904The PR states that opaque handles stay out of logs, but the
credential.report_auth_failureerror path logsserved.handleverbatim. A daemon/reporting failure therefore persists the credential handle inopencode-anthropic-auth.log. The new credential-blindness suite covers dumps, sidebar state, and RPC output, but not the logger. Log the account ID (or no identifier) instead and add a sentinel assertion through__setLogTestSink. -
[P1] Handle vault 401s from the default optimistic WebSocket relay —
packages/opencode/src/index.ts:5652-5655(also the sticky path at6343-6356)Both report paths require
response.status === 401. With the default WebSocket relay,PersistentRelaySessionresolves an optimistic synthetic 200 before the upstreamresponse_start; an upstream 401 is later converted into an SSErelay_upstream_error(packages/core/src/relay.ts:958-965). The plugin therefore never callscredential.report_auth_failure, never invalidates the rejected record version, and cannot retry/migrate the route as a 401. I reproduced this with a plugin integration regression using a vault-served record version and a WebSocketresponse_start { status: 401 }: after consuming the response, the only Claustrum call wascredential.get;credential.report_auth_failurewas absent. Direct and HTTP-relay tests pass, so the missing default transport path needs explicit coverage. -
[P2] Enforce
min_ttl_mswhen deciding whether a cached credential is fresh —packages/core/src/claustrum.ts:438-444ClaustrumCredentialCache.get()returns a cached credential until its literal expiry, ignoring the configured#minTtlMs. Once startup warming has populated the cache, no code schedules another vault load while the credential approaches expiry; request routing sees it as fresh until the boundary, then gets a cold cache and can reject/migrate one turn while detached warming catches up. A focused regression withmin_ttl_ms = 120000,now = 0, andexpires_at_ms = 121000loaded version 1; after advancingnowto 2000 (119 seconds remaining),get()still returned version 1 and made no secondcredential.getcall. Cache freshness should account for the minimum TTL, while the request path can continue serving a still-unexpired captured token during the detached refresh. -
Stack-base blocker (#174) —
packages/opencode/src/tests/network-guard.test.ts:46-63The full suite on Bun 1.3.14 produced 1700 passes and one failure. The new
allows all IPv4 addresses in the loopback blocktest timed out after 30 seconds; it also reproduces in isolation.Bun.serve({ port: 0 })is not reachable through127.0.0.2/127.1.2.3in this Darwin environment, so this test validates server binding behavior rather than just the guard. This belongs to stacked PR #174, but PR #175 cannot be treated as a clean stack until #174 fixes or drops it.
Verification performed at head 1f0486bada9edad6ae8029fcd6a024a770b6004e:
- Workspace typecheck: pass
- Focused Claustrum/account suites: 291 pass
- Lint/format/diff checks: pass
- Full suite: 1700 pass, 1 deterministic stacked #174 timeout
- Review-only WebSocket 401 regression: fails (no auth-failure report)
- Review-only minimum-TTL regression: fails (stale-within-skew credential reused)
1f0486b to
1367eb1
Compare
iceteaSA
left a comment
There was a problem hiding this comment.
All four findings addressed at head 1367eb1 (base 0ef96fe), each against your repro shape:
-
Handle out of logs — the report-failure path now logs the account id, never the handle; the blindness suite gained a logger surface via
__setLogTestSinkwith a sentinel-handle assertion (red on the old code: sentinel present in the captured record; green now). -
WS-relay 401 reporting — root cause confirmed as you described: the optimistic synthetic 200 resolves before upstream status, and the 401 only surfaces later as SSE
relay_upstream_error(numeric status lives on theresponse_startcontrol frame and was not copied into the SSE event, so the transform parses the HTTP status from the relay error). Both call sites (normal + sticky) now report vault-served 401s from that path with the servedrecord_version; the provenance fences hold — your repro shape (vault-served record + WebSocketresponse_start {status: 401}) now observescredential.report_auth_failure, and a relay 401 on a non-vault route still reports nothing. -
min_ttl_ms freshness —
get()now treats a credential inside the min-TTL window as stale: it keeps serving the still-unexpired token and fires a detachedcredential.get, respecting the per-handle backoff so approach-window refreshes cannot stack. Your exact numbers (min_ttl 120000, expiry 121000, now→2000) go red on the old code (no second get) and green now. -
127/8 Darwin binding — the test no longer depends on OS loopback binding: it asserts the guard ALLOWED the address (error present but not the Blocked message), the same pattern as the
[::1]test, with the dotted addresses also pinned in the directassertLoopbackunit matrix.127.0.0.1keeps its real-server coverage.
Gates at the new heads: base 1366/0 · stacked 1441/0 (+5 regressions) · e2e 29/0 · typecheck/format clean. The Claustrum vault side has also been asked to review commits 2 and 3 against their limiter and version-fence contracts.
iceteaSA
left a comment
There was a problem hiding this comment.
Claustrum-seat review of the two vault-contract regions at 1367eb1, verified against the vault source rather than the docs.
Relay-parsed 401 reporting: acceptable, and the fences are the right ones. The parse is structured-first (error.status integer, type-gated on relay_upstream_error after JSON parse — the includes check is just a pre-filter), regex over the message only as fallback. 401-only is the correct scope: the vault's own history has a real incident of a 403-on-permissions report latching a healthy credential, and excluding 403 from relay parsing avoids exactly that class. The WeakMap provenance fence plus the served record_version means the vault-side version gate does the rest — a stale report is a silent no-op, and a report can only ever kill the exact version it saw.
One boundary worth stating so it isn't credited to the wrong mechanism: the version fence protects against stale reports, not wrong ones. A parse false-positive while the record is still at the served version passes the fence. The blast radius on a refreshable credential is stale-mark → forced refresh on next get, which self-heals when the token is actually live and the provider refresh works — so the residual risk rides on the regex fallback's precision. Worth keeping the fallback's input constrained to the relay's own error message format.
Minor: reportClaustrumAuthFailure returns early when the cache entry is gone (!current), and a successful report invalidates the entry in finally. Net effect is per-served-version single-shot reporting, which is right — but a cache eviction for unrelated reasons also suppresses a genuine report. Worst case is one delayed report cycle; fine to leave, worth a comment line.
min-TTL detached refresh: volume is a non-issue, and on the current treadmill lane the window is unreachable. Numbers: default window 120s, backoff 60s, so worst case one get per handle per 60s while inside the window — against a per-connection ceiling of 64/60s, which is observe-and-alarm (FetchAnomaly audit rows), not a refusal. Even all handles simultaneously in-window doesn't approach it.
The sharper fact: anthropic tokens are ~8h and the external rotation replaces them every ~4h, so remaining TTL never drops below ~4h. A 120s window never triggers on that lane today — the feature is inert there and becomes live exactly when it should: if custody ends the treadmill and tokens ride to natural expiry.
On the latency question: detached scheduling fully absorbs it on your side, and the vault side confirms the other half — a get that doesn't itself want refresh serves without taking the single-flight lock (engine re-checks under lock only on the refresh path), so your serving traffic never queues behind the in-flight refresh. The one get that pays the synchronous upstream exchange is the detached one, which nobody awaits.
Offer, not a blocker: the vault's auth_events row for a consumer report currently records provider_status only — the observation's detail field is hardcoded None on this path. If a relay-observed vs direct-observed marker would help your forensics, the cheap end is an optional detail string on report_auth_failure flowing into that field. Say the word and I'll file it on the vault side.
1367eb1 to
f0c699c
Compare
70941a5 to
e5ae3aa
Compare
35eceff to
60ca70d
Compare
|
Restacked on #174 at e48100f → head 60ca70d. Status against the Aug 31 review: (1) handle-in-logs, (2) WebSocket-relay 401 reporting, and (3) min-TTL freshness were fixed the same day (in the squashed commit; the |
|
Two more commits at head b24ada0, found in live operation on a vault-custodied fallback: 05f2268 — poll fallback quota with the vault credential, never an expired token. The fallback quota poll used the sidecar access token. Under vault custody that token is never locally refreshed, so it expired, and Anthropic's b24ada0 — never echo parser text from a secret-bearing JSON parse. Bun's Cross-family review APPROVE 0/0 after SHOULDs; per-hunk mutations pasted in the commits' tests. Stack gate: 1523/0, e2e 30/0. |
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Fail-loud preload network guard for the test suite: any non-loopback HTTP(S) fetch throws, including across redirects (manual redirect following, 20-hop cap, unparseable URLs fail closed). Replaces 44 live vendor requests per run with deterministic local stubs, isolates background intervals and temp dirs, and asserts no test leaves globalThis.fetch or an interval behind. Production fixes found by the guard: account-state membership treats a missing or unparseable config as UNKNOWN (no pruning) rather than empty, loader/membership agree on whitespace-normalized ids, and rotated credentials persist under the canonical key. Loopback classification tests call assertLoopback() directly instead of opening sockets (127.0.0.2:1 hangs to timeout on Darwin), and the upstream Fable 5.1 effort test restores globalThis.fetch after itself.
A fallback OAuth account can keep its credential in Claustrum, a local credential vault reached over an IPC daemon, instead of in the sidecar. Per-account opt-in, off by default: with no vault present, or no account opted in, nothing here runs. Main-account custody stays out of scope — a vault-managed main would send a placeholder to Anthropic's token endpoint, and our classifier marks that failure permanent with no way back. The vault's slow path is kept off the response path structurally. credential.get is bimodal — microseconds when resident, seconds when it lands in the refresh window with callers queued behind one upstream exchange — so serving only ever peeks synchronously, a bounded startup warm primes the cache, and refreshes run detached. A cold peek falls through to the sidecar rather than waiting. The vault owns refresh for an account it serves: that account's sidecar refresh token is a lineage the vault may already have rotated, and spending it returns invalid_grant, which is permanent. But a vault outage must never be worse than having no vault, so an unavailable vault still permits ordinary local recovery, and a resident credential clears whatever error that recovery persisted. Identity is explicit in both directions. consumerIdentity is forced to null because the client otherwise reads SUBC_MODULE_ID and SUBC_LAUNCH_NONCE from the environment and would authenticate as whichever supervised module spawned the host. A BindIdentity is supplied because the client requires one for every managed call; its session is derived from the account-store fingerprint, so it is stable across restarts without new persisted state. A 401 is reported only for a credential the vault actually served, carried on the response rather than looked up again at report time. Invalidation is version-gated and the version in hand is usually still current, so a report raised from a locally served token would pass that gate and invalidate a healthy credential — version-correct and provenance-wrong. Failures classify so the vault's shared rate limiter stays protected: a transport failure or a terminal route failure backs off per handle instead of issuing one credential.get per request, and an unrecognised error class retries boundedly and loudly rather than blocking the account, which is the most operator-visible action available and needs positive evidence. Credential handles never reach dump artifacts, sidebar state, or RPC responses; a compile-time contract keeps them out of dump inputs.
…reports Also normalize the main TUI gate placeholder, clean test log sinks with finally, and make fake-daemon goodbye waits event-driven.
…an expired token 1. Resolve fallback quota credentials from the vault-first cache seam. 2. Skip vault-cold expired credentials and refresh non-vault expired credentials before polling. 3. Persist and seed cross-process fallback 429 backoff. 4. Fence auth-failure reports monotonically by credential record version. 5. Cancel the current response only when an eligible fallback actually sends. Anthropic returns 429, not 401, when the expired sidecar token reaches the usage endpoint.
b24ada0 to
b62f6ac
Compare
Stacked on #174, which is one commit and will drop out of this diff once it merges. This PR is the commit on top of it.
Lets a fallback OAuth account keep its credential in Claustrum, a local credential vault reached over an IPC daemon, instead of in our sidecar. Entirely opt-in and per-account: with no vault present, or no account opted in, nothing here runs.
Why bother
The vault owns refresh. Our
FallbackAccountManagercurrently holds refresh tokens on disk and refreshes them itself; a vault-managed account holds a handle instead, and the vault runs the refresh treadmill. Fewer long-lived secrets on disk, and a credential shared by several tools stops being refreshed by each of them independently.Scope: fallback accounts only, and main is deliberately frozen
Main-account custody is not in this PR and is not a follow-up I intend to open.
refreshMainAccessTokenreadsgetAuth()and sendsfreshAuth.refreshto Anthropic's token endpoint. A vault-managed main account would send a placeholder, get400 invalid_grant, and our own classifier marks thatpermanent: true— permanently disabling the main account from routing and sidebar display, with no way back without the retained refresh token.Fallback accounts are safe because they are ours end to end: our sidecar, our manager, our refresh path.
Read this before trusting the green suite
An earlier revision of this PR had never worked. Production never passed a
BindIdentity, so every vault call threwSubcCallError(missing_identity)and the feature was inert — through six bot-review rounds, a cross-family drift pass, and 1385 passing tests.Nothing caught it because every test either passed
identityexplicitly or injected a connector stub, and the fake daemon does not enforce what the real client enforces. The fixtures were structurally incapable of reaching the defect.What found it was one bounded read-only probe against the live daemon, in seconds:
not_foundis a pass: it proves the call cleared identity and reached the vault, and it's the correct answer for a handle that doesn't exist. Claustrum returns it for revoked and unknown handles alike, deliberately indistinguishable so the API can't be used to enumerate handles — which is also why it maps to permanent.The cause is worth naming, because the two things are one letter apart in meaning.
consumerIdentityandidentityare different parameters sitting next to each other. I reasoned carefully about the first and was blind to the second.Three things worth reviewing closely
Identity is scrubbed explicitly, not by omission.
@cortexkit/subc-clientreadsSUBC_MODULE_IDandSUBC_LAUNCH_NONCEfromprocess.envby default. A plugin shell spawned from a tool call inherits those from whatever supervised module spawned it — so omitting the identity argument silently authenticates as that module, with its route authority. We passconsumerIdentity: nulland type-enforce it viaOmit.Separately, every managed call needs a
BindIdentity:harness: 'opencode', the plugin's project directory, and a session derived asstore-— the fingerprint patternPrimeManagerRegistryalready uses. Stable across restarts, distinct per config store, no new persisted field. I originally argued for a stable id because a vault's audit chain records who asked; reading Claustrum's source showedcredential.getignoressessionentirely and the limiter scopes on TCP connection id plus handle. The conclusion survived, the reasoning didn't — it stands on route-cache stability, not attribution.The vault's slow path is kept off the response path structurally.
credential.getis bimodal: ~35µs when the credential is resident, but seconds when it lands in the refresh window, with concurrent callers queued behind one upstream exchange. So the request path only ever does a synchronouspeek; a bounded warm at startup (100ms, one attempt per account) primes the cache, and any refresh runs detached.The bound is deliberately short and deliberately not sized for the slow case: abandoning the await doesn't cancel the vault's work, so the refresh still lands and the next call collects it. A bound sized for the slow path would block boot exactly when the slow path stopped being rare.
A 401 is reported only for a credential the vault actually served. Invalidation is version-gated, and the version we hold is usually still current — so a 401 report generated from a sidecar-served token would pass the version gate and invalidate a perfectly healthy vault credential. Version-correct, provenance-wrong. Provenance isn't checkable on the vault's side, because a capability handle authorises a read without identifying who presented it, so the guard is structural on ours: the served credential is bound to the response via a
WeakMap, never inferred from account state at report time.That guard needed a clock seam to test at all. My first two attempts could not fail — they used a credential expiring in 1ms, and the cache only stores when
expiresAtMs > now(), so it was never cached, the report path found nothing, and the tests passed with or without the guard. I found that by mutating the guard away twice and getting green both times.The maintainer round: three findings, all reproduced before fixing
Credential handles stayed out of dumps, sidebar state, and RPC — and then leaked into the log. The
report_auth_failureerror path loggedserved.handleverbatim, so a daemon outage during a report persisted the bearer toopencode-anthropic-auth.log. The blindness suite covered every surface except the logger; it now covers the logger (__setLogTestSink+ sentinel handle, red on the old code).The default WebSocket relay never reported a 401. The relay resolves an optimistic synthetic 200 before upstream status arrives; a real upstream 401 surfaces later as an SSE
relay_upstream_errorwhose numeric status never leaves theresponse_startcontrol frame. Both report call sites gated onresponse.status === 401, so on the default transport a vault credential could be rejected forever without onereport_auth_failure— no invalidation, no version fence advance, no migration. The transform now parses the upstream status out of the relay error event and both call sites report vault-served 401s from that path, provenance fences intact (a relay 401 on a non-vault route still reports nothing).min_ttl_mswas configured but consulted nowhere. The cache served until literal expiry, then went cold for exactly one turn while the detached warm caught up. A credential entering the min-TTL window is now marked stale — a detachedcredential.getfires under the existing per-handle backoff — while the still-unexpired token keeps serving. The maintainer's exact red numbers (min_ttl 120000, expiry 121000, now→2000: no second get) go green.The status modal shows custody state now (a gap live testing found)
First live opt-in, first operator question: "I don't see anything." Correct — the claustrum status existed only in core's text output; the OpenCode modal renders the structured RPC projection, and the field-by-field allowlist (which exists so new fields fail closed) had never been given the fields. The projection now carries
claustrumGate: on|off|naandvaultServedper account plus a modal-level detection line, threaded through the closed RPC contract, rendered in the dialog, covered by the blindness suite (sentinel handle asserted absent from payload and props), and pinned by a projection test through the realbuildDialogPayloadpath — mutation-proved, since a test that survives deleting the field from the projection isn't testing the projection.The idle-expiry incident, and who owns freshness now
The first live opt-in found the design's real gap within five hours. The account gets zero traffic, and the min-TTL refresh triggers on
get()— so nothing ever called the vault, the credential expired resident, the peek-based vault-served predicate read that as "not vault-served", and the background timer ran a local OAuth refresh ten seconds after expiry. That rotation superseded the vault's stored refresh token; the vault's first real refresh attempt diedinvalid_grantand latched. Custody inverted itself on the first idle expiry, and the sidebar showed "needs relogin" for an account whose sidecar was perfectly healthy.The fix flips background freshness ownership. For a vault-enabled account (a config property — gate plus handle — deliberately not the peek predicate), the background tick issues a periodic vault
credential.getwithminTtlMsderived as tick + margin (240min + 30min), so every tick either serves fresh or triggers the vault's own upstream refresh inside its window — the vault-seat's rule: with the 120s default, an idle token expires between ticks and the incident reproduces. Local refresh survives strictly as a last resort (vault hard-unreachable with the sidecar expiring, or a latched vault copy with a healthy sidecar), always with a loud custody-override log; latched handles back off at tick cadence so a vault repair stays discoverable without hammering the 64/60s limiter; and the sidebar now distinguishesvaultReauth(vault copy needs re-import) fromneedsReauth(credential actually dead) — the exact misreport the incident surfaced. The zero-traffic expiry scenario is pinned as a red test whose ownership-revert mutation reproduces the incident's local-refresh signature, and startup no longer double-probes the vault (warmup and the first custody tick raced).Cross-family review (GLM 5.3, adversarial: independent mutations in a throwaway worktree, including a margin-off-by-one that reddens the min-TTL assertion) returned APPROVE with one finding — the reauth-backoff gap — fixed and re-approved 0/0.
Two more incidents, both second-occurrence failures
Idle-expiry was the design gap; these two were the resident client's state, and both are the same shape as the route-arm bug below — correct line by line, wrong on the second time through.
The client wedged, then retried a superseded token forever. When the resident subc client hit an unrecovered terminal
SubcCallError, custody correctly fell back to local — but then retried the superseded sidecar refresh token every 90s, looping400 invalid_grant. The daemon had never seen a socket disconnect (pid continuous), so the wedge was internal resident-client state, not a transport drop. Fix: reconnect the client on terminalSubcCallErrorwith a 60s backoff, and latch the permanent custody error throughrefreshBackoffActiveso the loop can't re-arm. The GLM review caught that the first latch test didn't actually discriminate the halt — connector tick 2 recovered — so the fixture now fails through get #3.Booting gate-off, then enabling mid-session, dropped straight to the local ladder. With the custody gate off at boot,
claustrumCredentialCachestaysnull; flipping the gate on mid-session bypassed the vault get and fell into local refresh — the sameinvalid_grantloop, reintroduced by a different door. Fix: the custody tick connects the client on demand when gate and handle are active, not only at boot.And a multi-writer clobber underneath both. Concurrent state-file writes could roll back
lastRefreshError, un-latching an account that had just latched. The persistence boundary that actually matters ismergeAccountRuntimeState— not the higher-level save — so the fence lives there: an older writer can't overwrite a newer latch, withcheckedAterror-vs-error recency deciding ties. This one took two blocked review rounds: the lifted test probes kept making the test writer 1ms newer than persisted state, so the fence looked broken when the test was; a savant trace pinned the real boundary.When the operator later saw post-restart cache busts across sessions, I byte-diffed the request dumps and traced them to a magic-context memory injection plus a history-compaction fold — the plugin build was exonerated.
Soak: three self-sustaining vault cycles
The custody design is now proven on the exact failure mode that started it, confirmed against the vault's own audit chain (authoritative side, not our logs):
Three consecutive vault-owned refresh cycles, zero interventions. The third signs its own correctness: v4's ~8h TTL expires ~01:26Z, minus the 270-min
minTtlMswindow = opens ~20:56Z, and the commit landed four seconds into that window — the tick phase and the min-TTL rule meshing exactly as designed. Zeroauth_events, zeroFetchRateAnomaly, zero reports, zero vault-wide alarms since gate-on. Idle-account custody — the case that inverted itself and killed the first live opt-in — is now the cleanest-running case: no traffic needed, the periodic tick alone keeps the family alive.A vault outage must never be worse than having no vault
That constraint decided several things that would otherwise look inconsistent.
The vault owns refresh for an account it serves, so we don't spend that account's sidecar refresh token — it may be a rotated lineage, and spending it returns
invalid_grant, which our classifier marks permanent. But a cold cache, a warm in flight, and a failed connection all still permit ordinary local OAuth recovery, and a credential that later becomes resident clears whatever error that recovery persisted.The clearing side of that had a defect worth describing, because it isn't a race in the usual sense. Clearing was wired to the cold→warm transition, and the warm only runs when a
peekmisses — so once the cache was warm, nothing could ever reach the clear again. A permanent error landing after the transition was unclearable by construction. A race you can lose repeatedly is self-correcting on the next attempt; this one had no next attempt.What that costs is latent rather than immediate, and I got it wrong the first time I described it. A vault-served account keeps routing normally with a stale error on it — the refresh is skipped while the credential is resident, so nothing ever reads the error. It arms when the vault stops covering it: on an outage, a revoked handle, or a disabled gate, the account falls back to the local path and the stale permanent error excludes it. The failure surfaces exactly when the sidecar fallback is the only thing left. Clearing now happens whenever a usable vault credential is actually served, gated cheaply so it doesn't touch the common path and dispatched detached so nothing serving a response awaits a locked write.
Same shape as the 401-retry arm that didn't update the route its two siblings updated, and the reconnect/gate-flip pair above. All read correctly line by line. All are only visible if you ask what happens on the second occurrence.
Rate limiting is a correctness constraint here, not a nicety
The vault's limiter is 64 requests per 60s per connection, shared between get and report, and it increments before handle resolution — so a timed-out get still counts. Crossing it doesn't deny service; it writes a durable anomaly record.
Two paths could issue one
credential.getper request indefinitely, both found by independent reviewers a layer apart: transport failures and terminalSubcCallErrors bypassed classification, so the per-handle backoff never engaged. Both classify now. Reverting either gives 13 gets for 12 requests, which is the signature.Terminal needed care rather than a blanket reclassification.
route.open failedis terminal and is genuine vault-side unavailability, which must back off — butmissing_identityis also terminal, and that's the dead-on-arrival bug above, findable only because it threw loudly. So they back off and log the raw code. Backoff protects the limiter, logging protects debuggability; they were never in conflict.Live custody evidence (one fallback account, opted in on this machine)
Each leg evidenced by the side that can see it: vault chain rows for import + handle mint; a fresh-process
credential.getserved version 1 with expiry matching the import; cache residency observable only via successful decode (peek cold→resident); the running plugin's own warm proven byvault servedrendering in the modal (that flag is a live in-processcache.peek); and local refresh inert — the vault-served predicate gates all three refresh paths and the log shows zero refresh attempts for the account. The vault side confirmed clean non-refresh serves and, over a multi-hour soak, three consecutive vault-owned refresh cycles (above) with no anomaly records — and reviewed the relay-401 and min-TTL commits against their limiter and version-fence contracts.Verification
The audit line is why this is stacked rather than parallel. #174 makes a live network call from a test a hard failure; these tests inherit that rather than being exempt from it. The inheritance is not hypothetical: the moment #174's fetch-leak invariant met this branch, it flagged this branch's entire vault describe — 25 tests sharing one un-restored fetch mock — which now owns its teardown like its siblings. They run against an injected connector and a fake daemon — no test here touches the real vault, which is live on this machine and serving other consumers.
Mutations, since a guard that has never been broken isn't evidence of anything:
Not included
A warm-skip optimisation using the vault's
stale_pendingfield, which would let startup skip accounts it knows would be slow rather than discovering them by timeout. The field shipped this week and I have the wire shape, but the bounded warm handles that case correctly without it, and I'd rather land the mechanism before the optimisation. If it goes in later it should skip only on an explicittrue— absent means not-observed, not false.