Skip to content

Optional Claustrum vault custody for fallback OAuth accounts - #175

Merged
ualtinok merged 5 commits into
cortexkit:mainfrom
iceteaSA:feat/claustrum-detect
Sep 2, 2026
Merged

Optional Claustrum vault custody for fallback OAuth accounts#175
ualtinok merged 5 commits into
cortexkit:mainfrom
iceteaSA:feat/claustrum-detect

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

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 FallbackAccountManager currently 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. refreshMainAccessToken reads getAuth() and sends freshAuth.refresh to Anthropic's token endpoint. A vault-managed main account would send a placeholder, get 400 invalid_grant, and our own classifier marks that permanent: 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 threw SubcCallError(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 identity explicitly 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:

before   SubcCallError             missing_identity
after    ClaustrumCredentialError  action=gone  class=permanent  not_found

not_found is 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. consumerIdentity and identity are 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-client reads SUBC_MODULE_ID and SUBC_LAUNCH_NONCE from process.env by 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 pass consumerIdentity: null and type-enforce it via Omit.

Separately, every managed call needs a BindIdentity: harness: 'opencode', the plugin's project directory, and a session derived as store- — the fingerprint pattern PrimeManagerRegistry already 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 showed credential.get ignores session entirely 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.get is 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 synchronous peek; 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_failure error path logged served.handle verbatim, so a daemon outage during a report persisted the bearer to opencode-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_error whose numeric status never leaves the response_start control frame. Both report call sites gated on response.status === 401, so on the default transport a vault credential could be rejected forever without one report_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_ms was 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 detached credential.get fires 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|na and vaultServed per 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 real buildDialogPayload path — 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 died invalid_grant and 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.get with minTtlMs derived 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 distinguishes vaultReauth (vault copy needs re-import) from needsReauth (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, looping 400 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 terminal SubcCallError with a 60s backoff, and latch the permanent custody error through refreshBackoffActive so 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, claustrumCredentialCache stays null; flipping the gate on mid-session bypassed the vault get and fell into local refresh — the same invalid_grant loop, 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 is mergeAccountRuntimeState — not the higher-level save — so the fence lives there: an older writer can't overwrite a newer latch, with checkedAt error-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):

seq 813  refresh_commit  actor=vault  v2 → v3
seq 822  refresh_commit  actor=vault  v3 → v4   17:25:58Z
seq 830  refresh_commit  actor=vault  v4 → v5   20:56:04Z

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 minTtlMs window = opens ~20:56Z, and the commit landed four seconds into that window — the tick phase and the min-TTL rule meshing exactly as designed. Zero auth_events, zero FetchRateAnomaly, 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 peek misses — 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.get per request indefinitely, both found by independent reviewers a layer apart: transport failures and terminal SubcCallErrors 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 failed is terminal and is genuine vault-side unavailability, which must back off — but missing_identity is 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.get served version 1 with expiry matching the import; cache residency observable only via successful decode (peek cold→resident); the running plugin's own warm proven by vault served rendering in the modal (that flag is a live in-process cache.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

full gate         1448 pass / 0 fail   (#174 head is 1366, so +82)
e2e               29 pass / 0 fail
typecheck         clean
format:check      clean
blocked-URL audit 0 vendor hits
live probe        reaches the vault, returns not_found
live custody      import → mint → get v1 → resident cache → modal renders vault-served
live soak         3 vault-owned refresh cycles (seq 813 → 822 → 830), zero anomalies

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:

remove the production identity     identity test fails: expected identity, got undefined
remove consumerIdentity: null      consumer_identity expected false, received true
remove the transport classifier    13 gets for 12 requests
revert the quota token precedence  sticky quota test fails
neuter the steady-state clear      late-error clear test fails
restore the naive whole-storage clear   both "does not roll back" race tests fail
infer provenance from account state     2 tests fail, including the stale-resident one
widen the dump input to accept an account   typecheck fails on the structural contract
log the handle on report failure again      logger blindness test fails on the sentinel
drop the relay-401 report wiring            maintainer-shape repro observes only credential.get
serve through the min-TTL window silently   the exact-numbers regression fails (no second get)
remove claustrumGate from the projection    real-path modal test fails by name
reconnect removed on terminal SubcCallError  latch test fails through get #3
revert the mergeAccountRuntimeState fence    both writer-rollback race tests fail

Not included

A warm-skip optimisation using the vault's stale_pending field, 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 explicit true — absent means not-observed, not false.

@socket-security

socket-security Bot commented Aug 29, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​cortexkit/​subc-client@​0.8.18810010093100

View full report

@cubic-dev-ai cubic-dev-ai Bot 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.

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
Loading

Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.

Re-trigger cubic

Comment thread packages/opencode/src/index.ts Outdated
Comment thread packages/opencode/src/index.ts
Comment thread packages/core/src/claustrum.ts Outdated
Comment thread packages/opencode/src/index.ts Outdated
Comment thread packages/opencode/src/index.ts Outdated
Comment thread packages/core/src/claustrum.ts Outdated
Comment thread packages/opencode/src/tests/claustrum-client.test.ts Outdated
Comment thread packages/opencode/src/tests/test-fetch.ts Outdated
Comment thread packages/opencode/src/tests/index.test.ts
Comment thread packages/opencode/src/tests/claustrum-client.test.ts Outdated
@iceteaSA

Copy link
Copy Markdown
Contributor Author

All ten review threads addressed in 8aac979, plus one finding that came out of verifying them. Gates at the pushed sha: 1365 pass / 0 fail (was 1359, +6), e2e 29/0, typecheck and format clean.

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.

  • sendWithAccessToken re-resolved a credential the caller had already resolved, and threw when the second resolution came back empty. The fallback loop does not catch, so expiry skew between the two resolutions aborted the whole attempt instead of skipping to the next account. It now takes the resolution the caller captured; the throw is gone entirely, because it no longer resolves and has nothing to throw about. Wrapping the call site in try/catch would have masked this rather than fixed it.
  • reportAuthFailure defaulted the served credential to the current cache entry. A detached warm refresh can advance the cache past the version the provider actually received — and since invalidation is version-gated, a wrong-but-current version passes the gate. Cubic was precise that this is latent rather than live, since every caller passes explicitly today. That is exactly why it needed removing: the argument for binding provenance structurally is that a future caller cannot get it wrong, and an optional parameter with a cache-reading default hands the footgun back.

The rest of the P2s:

  • Connection-file override — status probed the default path while runtime honoured OPENCODE_ANTHROPIC_AUTH_CLAUSTRUM_CONNECTION_FILE. Now resolved through one helper, so they cannot diverge again.
  • JSON payload with no token field — control fell past the catch and returned the entire blob as a bearer token. The raw-payload fallback is now restricted to the genuinely non-JSON case; a parsed object with no usable token field returns undefined and fails closed.
  • Sticky routing dropping vault accounts — confirmed reachable. buildStickyOAuthRoutes resolved a valid vault credential and then dropped the route on !account.access. A vault-managed account legitimately has no sidecar token; that is the point of vault custody. Admission now keys on the vault credential, and only when route.claustrum.served is present, so plain OAuth behaviour is untouched.

The extra finding, which is the one I would not have shipped without. Verifying the "exactly one credential.get" claim in the sticky test meant tracing the warm path, and that showed handleClaustrumCredentialError handled gone, reauth and reduce_and_retry — but had no case for transient. Meanwhile warm scheduling ran on every resolution that found no usable credential. So a reachable-but-failing vault (vault_locked, refresh_failed — states the vault's own contract calls recoverable) drew one credential.get per request, indefinitely. The in-flight map does not help: it dedups only while a call is pending, and a fast-failing get clears it immediately.

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:

backoff disabled   13 credential.get across 12 requests   (Expected: < 12, Received: 13)
backoff restored    1 credential.get across 12 requests

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. ClaustrumClient has exactly one field, readonly #client, so JSON.stringify(client) is always "{}" — the whole test held regardless of where the key lived, and not.toContain('connection-file-key') asserted the absence of a literal nothing in the codebase produces. Removed rather than patched; the real control is the 0600 connection file on disk, and that has its own test.

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.

@cubic-dev-ai cubic-dev-ai Bot 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.

All reported issues were addressed across 5 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/opencode/src/index.ts
Comment thread packages/core/src/claustrum.ts
Comment thread packages/opencode/src/index.ts Outdated
Comment thread packages/opencode/src/tests/index.test.ts Outdated
@iceteaSA

Copy link
Copy Markdown
Contributor Author

Round 2 — all five closed, 8a9dd49..76bf07a. Gates at the pushed sha: 1367 pass / 0 fail (was 1365, +2), e2e 29/0, typecheck and format clean.

The P1 was real, and it was caused by the previous round's fix. Threading the resolution into sendWithAccessToken is right for provenance within one attempt — but a 401 retry is a new attempt, and the three sibling arms had drifted apart:

main      -> refresh, route = {...route, access: fresh}   updated
fallback  -> refreshAccount, route = {...route, access}   updated
claustrum -> report the failure                            route unchanged

So sendRoute(route) resent the token the provider had just rejected, along with a resolution the report had just invalidated. Two arms updating and the third not is what made it findable.

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:

re-resolve -> usable token?  yes -> update route, retry
                             no  -> yield the route, let the caller move on

Mutation-proved: forcing it to always resend reddens does not retry a rejected sticky vault credential with the rejected bearer present (occurrence 1 → 2). The non-sticky path at index.ts:5572 was checked and does not share the defect — it reports, then advances the account loop without resending.

reportAuthFailure is now required-by-signature. Last round I removed the cache default and accepted a runtime TypeError as sufficient; the reviewer was right that an optional parameter which always throws is worse than either alternative. Omission is a compile error now, which was the original intent — a future caller should not be able to get provenance wrong. There was no genuine two-argument caller.

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 76bf07a.

@cubic-dev-ai cubic-dev-ai Bot 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.

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

Comment thread packages/opencode/src/index.ts
@iceteaSA

Copy link
Copy Markdown
Contributor Author

Pinned the claim I made when declining the forced-refresh suggestion above — cca53d1. Gates at the pushed sha: 1368 pass / 0 fail (+1), typecheck and format clean.

I had argued that a vault-enabled account does not lose ordinary OAuth recovery when the vault is unavailable, because the resolution carries no served and the 401 falls to the forced-refresh arm. That was read, not run, so it is now a test: refreshes the sidecar credential after a 401 when the vault is unavailable. Removing the forced-refresh arm reddens it with Expected: 200, Received: 401.

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 relay-worker-miniflare timeout, and I have filed the mechanism as #176 rather than keep waving at it.

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 await mf.ready. Racing it against an 8s timeout and running the full suite reproduces it directly:

error: MFREADY_TIMEOUT_startWorker
(fail) relay Worker under Miniflare > websocket hash mismatch returns 409 [8001.51ms]

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.

@iceteaSA
iceteaSA force-pushed the feat/claustrum-detect branch from cca53d1 to dbb6a18 Compare August 30, 2026 09:01
@iceteaSA

Copy link
Copy Markdown
Contributor Author

Greptile's finding is correct and now fixed in bfea004. Rebased onto the updated #174 first, so the stack is current.

Confirmed empirically before fixing, with a plain OAuth account as the positive control:

PLAIN:  tokenHits= 1 | permanent= true
VAULT:  tokenHits= 1 | permanent= true

Identical. A vault-managed account's stale sidecar refresh token went to Anthropic's token endpoint on a background timer, returned invalid_grant, and persisted a permanent refresh error — which marks the account needs-reauth and drops it from routing until re-login. isClaustrumEnabledForAccount existed and was never called from any refresh path.

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:

vault enabled + handle + resident parseable credential  -> skip local refresh (vault owns it)
cold cache, warm in flight, or failed connection        -> allow local refresh (degradation)

isFallbackAccountVaultServed(accountId, storage) on AccountManagerOptions, defaulting to false so nothing changes for callers that do not pass it. OpenCode wires it to a non-blocking cache.peek — no vault await on any path.

Verified against my own probe on the fixed tree:

PLAIN(control):      hits= 1  permanent= true      <- majority path unchanged
VAULT-SERVED:        hits= 0  permanent= null      <- no longer spends the token
VAULT-UNAVAILABLE:   hits= 1  permanent= true      <- outage recovery intact

The implementer extended the guard past the two call sites I named — refreshQuotaForDueAccounts, refreshQuotaForAllAccounts, and a defensive check in refreshAccount — because the background runner invokes both refresh methods and quota recovery can otherwise re-enter local OAuth refresh. That is the right call; a fix on one of a set of siblings is a defect this branch has already produced twice.

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.

Comment thread packages/opencode/src/index.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot 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.

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

Comment thread packages/opencode/src/index.ts Outdated
@iceteaSA

Copy link
Copy Markdown
Contributor Author

Both P1s fixed in 7395f8f. They landed on the same predicate from opposite directions and both were right.

Cubic — the predicate accepted an expired credential. peek returns the cache entry with no expiry check, and the gate only tested cached && accessToken. An expired-but-resident entry therefore reported vault-served and local refresh was skipped forever while the vault was down: the vault could not refresh it, and we would not.

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 usableClaustrumAccessToken(credential, now) used at every site — token present, and expiresAtMs === null || expiresAtMs > now. No third variant survives in packages/opencode/src; the cache's own retention check in core is a retention decision, not a routing one, and stays separate.

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 → invalid_grantpermanent: true persisted, gating routing at index.ts:2488, 5265, 5362. markClaustrumCredentialReady only cleared the in-memory sets and never touched lastRefreshError, so the account stayed excluded after the vault credential arrived, with nothing in the vault path able to clear it.

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:

expiry check removed    4 fail, incl. "attempts local refresh when a resident vault
                        credential is expired during an outage"
clearing disabled       1 fail, "clears a persisted sidecar refresh error when the
                        vault later becomes resident"
restored                21 pass / 0 fail

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: bfea004 fixed a real defect and introduced two more, one in each direction from the line it changed. Neither reviewer found both. The expiry hole came from reusing peek as if it meant "usable", and the exclusion hole from fixing the refresh path without asking what happens to the error the old path had already written.

Comment thread packages/opencode/src/index.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot 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.

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

Comment thread packages/opencode/src/index.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot 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.

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

Comment thread packages/opencode/src/tests/index.test.ts Outdated
@iceteaSA
iceteaSA force-pushed the feat/claustrum-detect branch from b73540b to 3107d57 Compare August 30, 2026 14:05
@iceteaSA

Copy link
Copy Markdown
Contributor Author

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.

CACHE: constructed | ms= 5
THREW: SubcCallError | kind= terminal
MSG:  managed call requires a BindIdentity in SubcClient.connect({ identity })
      or call(..., { identity })

That is the real daemon, reached through the production path — connectClaustrumCredentialCache({ enabled: true }) exactly as index.ts calls it, then cache.get(handle).

The cause is my own security fix, and the two things have similar names. consumerIdentity and identity are different parameters. I forced consumerIdentity: null to stop the client inheriting SUBC_MODULE_ID / SUBC_LAUNCH_NONCE from a supervising module and authenticating as it — that part is right and stays. But a managed call also requires a BindIdentity ({project_root, harness, session}), and production never passes one. connectClaustrumCredentialCache accepts identity as an option; index.ts:1647 does not supply it.

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 identity explicitly or injects a connector stub, so no test ever exercised the production construction against a client that enforces the requirement. The fake daemon does not enforce it. That is precisely the failure mode I wrote into this review round's brief — "anything that would silently succeed against a fabricated fixture but fail against the real daemon" — and it was sitting in the code while I wrote the sentence.

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 BindIdentity through the production construction, plus a test that builds the cache the way index.ts does — no injected identity — and asserts the call reaches the wire. That test must fail on today's code.

Two review notes from the same round, both worth having:

  • Transport errors bypass classification, so the transient backoff added earlier never engages for a connection-level failure — one credential.get per provider request during a vault outage, against the shared 64/60s limiter. Same shape as the retry-loop defect fixed earlier in this PR, one layer down.
  • An unknown or malformed error class falls through to transient/retry rather than failing closed.

The network-guard half (#174) came back clean from the same round — no escape path found, including IPv4 integer/hex/short forms, lying Request.url getters, and redirect loops; stub shapes match what the code parses; every absence assertion carries positive-content proof.

@iceteaSA

Copy link
Copy Markdown
Contributor Author

The feature works now. fbecf73 + 2b0f8dd. The evidence that matters is the live daemon, not the suite — I re-ran the probe myself against the committed tree:

before   SubcCallError             missing_identity
after    ClaustrumCredentialError  action=gone  class=permanent  not_found

The call now clears identity and reaches the vault; not_found is the correct answer for a handle that does not exist, and it maps to permanent → gone, which is right because Claustrum returns not_found for revoked and unknown handles alike (deliberately indistinguishable, so the API cannot be used to enumerate handles).

What the identity is, and why. harness: 'opencode', project_root from the plugin's project directory, and session derived as store-<sha256(accountStoragePath)> — reusing the fingerprint pattern PrimeManagerRegistry already uses. Stable across restarts, distinct per config store, and no new persisted field.

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 credential.get ignores session entirely and the limiter scopes on TCP connection_id + handle. The conclusion survived, the reason did not — so it is derived from existing state rather than persisted into a new schema field for a value the vault does not read.

consumerIdentity: null is untouched and still enforced: removing it reddens sends route.open without inherited consumer identity on consumer_identity expected false, received true.

Also fixed, both from the same review round:

  • Transport errors bypassed classification — found independently by two reviewers, one layer apart: in the vault client's error mapping, and in warmClaustrumCredential's catch, which handled only ClaustrumCredentialError while transport throws the disjoint SubcError. So a vault dying mid-run produced one credential.get per provider request with no backoff at all, against the shared 64/60s limiter. Fixed by classifying at the core layer, which makes the existing catch effective. Mutation: removing the classifier gives 13 gets for 12 requests — the signature of exactly this defect.
  • Quota probe token precedence at index.ts:5413account.access ?? credential.accessToken preferred a sidecar token that is stale by construction for a vault-served account, while the line three below already preferred the vault credential. Every route build sent a dead token, 401'd, armed no backoff, and left quota permanently stale for exactly the vault-served accounts.
  • Unknown error class now maps to transient-with-backoff, logged with the raw value, rather than failing closed to a permanent block. Fail-closed is right for the capability — never serve, spend, or report on something unvalidated — but a permanent block is the most operator-visible action available and should not be triggered by an unrecognised string. The failure mode that ruling avoids: the vault ships a new error class, and every account using it is excluded until a human re-authenticates.

Per-fix mutations, each reverted and restored:

production identity removed      identity test fails (expected identity, got undefined)
consumerIdentity: null removed   consumer_identity expected false, received true
transport classifier removed     13 gets for 12 requests
quota precedence reverted        sticky quota test fails (verified by me, not the implementer)

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 accounts: [{}] variant I reported on #174; those come in the next round, and I will hold the merge until they are in.

@cubic-dev-ai cubic-dev-ai Bot 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.

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

Comment thread packages/core/src/claustrum.ts
@iceteaSA
iceteaSA force-pushed the feat/claustrum-detect branch from 2b0f8dd to c2a1132 Compare August 30, 2026 15:59
@iceteaSA

Copy link
Copy Markdown
Contributor Author

Squashed to one commit on top of #174's squashed base — c2a1132, stacked on 372ee2e. git diff against the pre-squash tip is empty, so the tree is byte-identical. Gate: 1393 / 0 (the one failure across runs is #176's Miniflare hang, which solo-greens 4/4), e2e 29/0, typecheck and format clean.

Everything from the five-way review round is in:

  • The feature now works. Production never supplied a BindIdentity, so every vault call threw and the integration had never functioned. Confirmed against the live daemon in both directions: SubcCallError missing_identity before, ClaustrumCredentialError not_found after — the call clears identity and reaches the vault. not_found maps to permanent, which is right, since Claustrum returns it for revoked and unknown handles alike.
  • Terminal and transport failures both classify now, so per-handle backoff engages instead of issuing one credential.get per request against the vault's shared 64/60s limiter. Reverting either gives 13 gets for 12 requests. Terminal needed care rather than a blanket reclassification: route.open failed is terminal and is genuine vault-side unavailability, but missing_identity is also terminal and is our own bug — and the only reason it was findable is that it threw loudly. So they back off and log the raw code, because backoff protects the limiter and logging protects debuggability; they were never in conflict.
  • An unrecognised error class retries boundedly rather than blocking the account. Fail-closed is right for the capability — never serve, spend, or report on something unvalidated — but a permanent block is the most operator-visible action available, and under the other rule a vault shipping a new error class would exclude every account using it until a human re-authenticated.
  • Quota probes prefer the vault credential. account.access ?? credential.accessToken preferred a sidecar token that is stale by construction for a vault-served account, while the line three below already preferred the vault's — a precedence slip that made every route build send a dead token and left vault-served quota permanently stale.
  • The chokepoint gate is pinned, with a plain-account positive control; deleting the line previously left all three vault-gate tests green.

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.

Comment thread packages/opencode/src/index.ts
@iceteaSA

Copy link
Copy Markdown
Contributor Author

Fixed in b2f888c. Gate at the pushed sha: 1395 / 0 (baseline 1393, +2), typecheck and format clean.

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 markUsed so nothing serving a response awaits the locked write, and still going through the same atomic scoped clear rather than a second write path.

Mutation-proved myself rather than taking the implementer's counts. Neutering the trigger (2 call sites) reddens clears a late local refresh error on a subsequent warm vault request; restored, 29/29 green in that file. The two "does not roll back" race tests still redden under the naive whole-storage clear, so this did not weaken the earlier fix.

Worth recording what the shape was, because it is not the usual race. The clear was wired to a transition, and warmClaustrumCredential only runs when a peek misses — so once the cache was warm, no later event could reach the clear at all. Anything landing after the transition was permanent by construction. A race you can lose repeatedly is at least self-correcting on the next attempt; this one had no next attempt.

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.

@cubic-dev-ai cubic-dev-ai Bot 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.

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

Comment thread packages/opencode/src/index.ts Outdated
@iceteaSA
iceteaSA force-pushed the feat/claustrum-detect branch 2 times, most recently from a372481 to b6f5341 Compare August 30, 2026 17:33
@iceteaSA

Copy link
Copy Markdown
Contributor Author

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 isPermanentRefreshError, and called it an absorbing state with operator re-login as the only exit. I checked the code path properly this time and that is not what happens:

  • getUsableFallbackAccounts skips the local refresh entirely when the account is vault-served (accounts.ts:3627-3638), so it never hits the backoff throw and lands in the usable set.
  • At index.ts:5330, an account already in that set is pushed as a route without consulting isPermanentRefreshError. That check lives in the else if branch — the path for accounts that are not usable.

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, tokenNeedsRefresh is true, the permanent error triggers the backoff throw, and now it is excluded. The failure surfaces at the worst possible moment: when the sidecar fallback is the only thing left.

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 b6f5341 (restacked on #174's 81200aa; gate 1404 pass, the lone failure is #176 which solo-greens 4/4):

The clear no longer decides from the request-start snapshot. It takes a fresh loadAccounts at clear time on the detached path and stays conditional, so an error persisted mid-request is seen. I did not take the suggested "drop the gate and let the locked helper re-read", because that pays a config + state write-lock acquisition on every vault-served request to catch a rare window, and lock contention on the response path is a regression this project has already shipped once. A read is cheap; two lock acquisitions per request are not.

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 main-access); restoring the naive whole-storage clear still reddens both "does not roll back" tests. The clean path takes zero locked clears, with a positive control showing exactly one when an error is present.

@iceteaSA
iceteaSA force-pushed the feat/claustrum-detect branch 3 times, most recently from 7a2fb1f to 1f0486b Compare August 31, 2026 05:19

@ualtinok ualtinok left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Request changes

  1. [P1] Keep opaque credential handles out of logspackages/opencode/src/index.ts:4902-4904

    The PR states that opaque handles stay out of logs, but the credential.report_auth_failure error path logs served.handle verbatim. A daemon/reporting failure therefore persists the credential handle in opencode-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.

  2. [P1] Handle vault 401s from the default optimistic WebSocket relaypackages/opencode/src/index.ts:5652-5655 (also the sticky path at 6343-6356)

    Both report paths require response.status === 401. With the default WebSocket relay, PersistentRelaySession resolves an optimistic synthetic 200 before the upstream response_start; an upstream 401 is later converted into an SSE relay_upstream_error (packages/core/src/relay.ts:958-965). The plugin therefore never calls credential.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 WebSocket response_start { status: 401 }: after consuming the response, the only Claustrum call was credential.get; credential.report_auth_failure was absent. Direct and HTTP-relay tests pass, so the missing default transport path needs explicit coverage.

  3. [P2] Enforce min_ttl_ms when deciding whether a cached credential is freshpackages/core/src/claustrum.ts:438-444

    ClaustrumCredentialCache.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 with min_ttl_ms = 120000, now = 0, and expires_at_ms = 121000 loaded version 1; after advancing now to 2000 (119 seconds remaining), get() still returned version 1 and made no second credential.get call. Cache freshness should account for the minimum TTL, while the request path can continue serving a still-unexpired captured token during the detached refresh.

  4. Stack-base blocker (#174)packages/opencode/src/tests/network-guard.test.ts:46-63

    The full suite on Bun 1.3.14 produced 1700 passes and one failure. The new allows all IPv4 addresses in the loopback block test timed out after 30 seconds; it also reproduces in isolation. Bun.serve({ port: 0 }) is not reachable through 127.0.0.2 / 127.1.2.3 in 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)

@iceteaSA
iceteaSA force-pushed the feat/claustrum-detect branch from 1f0486b to 1367eb1 Compare August 31, 2026 14:32

@iceteaSA iceteaSA left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

All four findings addressed at head 1367eb1 (base 0ef96fe), each against your repro shape:

  1. Handle out of logs — the report-failure path now logs the account id, never the handle; the blindness suite gained a logger surface via __setLogTestSink with a sentinel-handle assertion (red on the old code: sentinel present in the captured record; green now).

  2. 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 the response_start control 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 served record_version; the provenance fences hold — your repro shape (vault-served record + WebSocket response_start {status: 401}) now observes credential.report_auth_failure, and a relay 401 on a non-vault route still reports nothing.

  3. min_ttl_ms freshnessget() now treats a credential inside the min-TTL window as stale: it keeps serving the still-unexpired token and fires a detached credential.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.

  4. 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 direct assertLoopback unit matrix. 127.0.0.1 keeps 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 iceteaSA left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@iceteaSA
iceteaSA force-pushed the feat/claustrum-detect branch from 1367eb1 to f0c699c Compare August 31, 2026 14:46
@iceteaSA
iceteaSA requested a review from ualtinok August 31, 2026 16:13
@iceteaSA
iceteaSA force-pushed the feat/claustrum-detect branch 8 times, most recently from 70941a5 to e5ae3aa Compare September 1, 2026 16:25
@iceteaSA
iceteaSA force-pushed the feat/claustrum-detect branch 2 times, most recently from 35eceff to 60ca70d Compare September 2, 2026 09:34
@iceteaSA

iceteaSA commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

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 reporter_source relay provenance landed after); (4) the loopback test is fixed in #174 and no longer opens a socket. Since then: reconnect sharing and 401-report dedupe from the Cubic pass. Stack gate on the current head: 1524 pass / 0 fail, e2e 30/0, typecheck clean.

@iceteaSA

iceteaSA commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

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 /api/oauth/usage answers an expired token with 429 rate_limit_error, not 401 (live A/B: sidecar → 429; the same account's vault credential → 200). The 401→refresh branch never fired, a per-process backoff armed, and every plugin process re-polled on its own cadence while the account's quota sat frozen for 21h. Now: a single resolveFallbackAccessToken seam on FallbackAccountManager (vault-first via peek, then sidecar, else no poll) covers the background timer, the prime path, and the killswitch refresh; a known-expired token is never sent (non-vault accounts refresh first); fallback 429 backoff is persisted cross-process like main's. Fixing the poll timing exposed a hole in the 401-report dedupe from 60ca70d (two reports for one served record version), closed with a monotonic per-handle last-reported-version fence; tryFallbackAccounts defers cancelling the current response until a fallback actually sends.

b24ada0 — never echo parser text from a secret-bearing JSON parse. Bun's JSON.parse quotes an unquoted token beside the failure; detectClaustrumConnection put that message in reason, which /claude-account status displays. Fixed message now. The same fix for the account-store reader on main is #184.

Cross-family review APPROVE 0/0 after SHOULDs; per-hunk mutations pasted in the commits' tests. Stack gate: 1523/0, e2e 30/0.

@cubic-dev-ai cubic-dev-ai Bot 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.

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

Comment thread packages/core/src/accounts.ts
Comment thread packages/opencode/src/index.ts Outdated
Comment thread packages/opencode/src/tests/credential-handle-blindness.test.ts
Comment thread packages/opencode/src/tests/accounts.test.ts Outdated
Comment thread packages/core/src/claustrum.ts
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.
@iceteaSA
iceteaSA force-pushed the feat/claustrum-detect branch from b24ada0 to b62f6ac Compare September 2, 2026 12:14
@ualtinok
ualtinok merged commit 9ded8f5 into cortexkit:main Sep 2, 2026
5 checks passed
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