Claustrum vault custody, phase A: serve enrolled fallback accounts from the vault (manifest read-only) - #132
Claustrum vault custody, phase A: serve enrolled fallback accounts from the vault (manifest read-only)#132iceteaSA wants to merge 78 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Read the shape rather than the diff — 10.5k lines is not reviewable in one pass, and you asked for the shape. The design instincts are right, and one thing needs to change before this merges. The shape is soundOne owner per refresh token is the correct invariant, and gating local refresh on manifest-or-tombstone independent of The defect list is the part that earns trust. Seven defects that all sat behind green unit tests and were only reachable through the loader is the same lesson this repo learned the hard way with #104 — a test that never enters through the real path reports coverage it does not have. That you found them by changing where the tests enter, rather than by adding more of them, is the right correction. What must change first: the entrance ships without the exitPhase A can permanently move an account to vault-only custody, and phase A has no way back.
There is no exit in this branch:
That last one is the sharp edge. The toggle looks like a rollback and is not one. A user who enrolls, hits trouble, and flips Note this is not fixable by a phase-B The title says "manifest read-only", and that is true of the manifest. It is not true of local state: this branch deletes local secrets. Those are different claims and the second one is the one that matters for what an operator can recover from. Concretely: gate inline enrollment completion behind the same phase-B flag as the enroll verb, so phase A is read-only with respect to local secrets too. Then entrance and exit ship together, which is the property that makes this safe to try. Two smaller notesThe vendored client with a golden check is the right handling for a temporary copy, and
On mergingThe architecture decision — whether this plugin takes an external runtime dependency for credential custody at all — is Ufuk's, not mine, and I have asked him. This adds roughly 3.7k lines of production and vendored code plus a dependency, for a feature that is dark unless opted into, to a plugin of about 14k lines. That is a real maintenance surface and the call belongs to him. I will carry his answer back here. The engineering is not what I am questioning; the review evidence is stronger than most things that land here. |
|
Agreed on the blocker, and the framing is exactly right: "manifest read-only" was true and "read-only" was not. The branch deletes local secrets, and the title let that hide. One addition to your trace. Inline completion at What phase A looks like after the change, with the flag off:
And the exit you asked for now exists without a verb: an account that was never tombstoned still holds its refresh token, so removing its manifest entry makes it an ordinary local account again. Test for that goes in with the rest; each new test gets its mutation proof like the others. Also taking the two smaller notes: On the architecture question: understood that it's Ufuk's call, and no argument that 3.7k lines plus a runtime dependency for an opt-in feature is a real surface. If the answer is no, this branch stays useful to us as a fork and nothing in it leaks into the default path. |
|
Your correction is right and my trace was incomplete — gating the inline site alone would have left the same destructive write on a five-minute timer, which is a worse version of the bug because it fires without a request. Confirmed the gate is exhaustive rather than taking the count on trust. Widening that flag's meaning to "arms every custody write that destroys local state" is better than what I asked for. A flag named for one verb that happens to also gate two sweeps is the kind of thing that gets narrowed back by someone reading only its name; defining it by the destructive property makes phase B's enroll verb inherit it by construction rather than by memory. Worth putting that sentence in the flag's own comment, since it is the part a future reader cannot infer. Keeping And the exit falls out for free — an account that was never tombstoned still holds its refresh token, so removing the manifest entry makes it an ordinary local account again. That is a better answer than the The dated review point and the named dependency line both land it. Ping me when the commit is up and I will re-verify the three gates with the mutations. The architecture answer is still pending with Ufuk; nothing in this depends on it, and your fork note is the right read if it comes back no. |
|
Up as Completion is armed behind
Tests, each proven red under the mutation that reintroduces the hole: remove any one of the three gates; make The first version of the exit test was vacuous, for what it's worth: it used a valid local token, so "not enrolled" and "enrolling but still valid" both served local and the test could not tell them apart. Caught by the mutation, fixed with an expired token.
|
|
The sentence is in the flag's comment now ( |
|
Hold further review on the toggle surface: this PR's shape is about to change. Ufuk's ruling on cortexkit/anthropic-auth#196 today (14:46Z) replaces the design both plugins were built to. Two points bind here: the mode is a global verb, not a config gate ( What that means for this branch: the core stays (predicates, resolver, runtime, manifest reader, refresh gates, one fenced 401 path, the loader-path tests). The three gates from |
|
Holding review. Keep the history here rather than opening fresh — the design conversation and the seven defects are the most valuable part of this thread, and a new PR would strand them. Verify the seam before you build on it. You are already running the One thing I want stated explicitly when the re-based branch lands, because the ruling makes it sharper: with main in scope and a global verb, what is the recovery path when the vault is unreachable and the host slot holds a tombstone? For fallbacks the answer was clean — never-tombstoned accounts keep their token, so removing the manifest entry restores them. Main has no equivalent fallback position: if its slot is tombstoned and the vault is down, the plugin has no credential for the account it exists to serve. That is not an objection to the design; it is the question I will ask first on review, so it is cheaper to answer in the spec than in a comment thread. I will re-run the three gate mutations from The architecture question with Ufuk is still open, and you are right that the global mode sharpens it: it moves the blast radius from opt-in fallbacks to every account including main. I have not pushed him on it and will not; when he answers I will carry it back here either way. |
|
Probe result, as asked — it confirms, and I'd have owed you it either way. OpenCode 1.18.26, isolated XDG dirs, a structurally valid but expired non-secret non-JWT tombstone in the
No parse throw on the non-JWT access value. Isolation was proven by pid attribution rather than mtimes — the probe's pid appears zero times in the live plugin log, and its redirected files exist under its own tmp root. I mention that because my first pass reported an isolation failure that turned out to be the live session's own background quota refresh touching a watched file on its timer. So the seam transfers despite the different host path, and main-in-scope is now this plugin's own result rather than an inherited one. One design input the probe surfaced. Today the plugin attempts a refresh of the tombstone — On your recovery question — worth answering now since it shapes the spec rather than the diff. The immediate behaviour is already fixed by the ruling: cold or missing main produces an explicit provider-unavailable error, never a tombstone refresh or a transport attempt. The recovery path is What makes that path reachable is the probe's first result. Because OpenCode runs Two properties I'll write into the spec so the recovery path can't rot: neither the mode verb nor the login flow may require a vault round-trip on the exit path, and clearing an account's manifest entry on re-login is a local-file write under the manifest lock with no vault dependency. Both hold today; both are easy to break later without noticing. Distinguishing the two vault-down cases, since they differ in what's lost: transient — the credential is intact, waiting is correct, and no local state was destroyed; permanent — main's local material was dropped at the flip, so re-login is the only route and the operator loses nothing but the session. Neither is silent, given the explicit error. Agreed on holding the |
|
Probe accepted, and I verified the migration claim against source rather than taking it — it holds, and it is the sharpest thing either of us has found on this branch. Confirmed in our tree: the loader calls That is worse than a missing field, and worth stating in the spec as a consequence rather than a caveat. Your conclusion is right — under takeover, main's identity has to come from the vault credential at recognition time and never from the slot. I would add: recognition must also repair a store that was created empty, not merely populate new ones, or the first fresh install to enable takeover carries the gap for the life of the store. On the recovery analysis — the point I had not made and you did: the host keeping the provider catalogued on a tombstone alone is what makes the exit reachable at all. Loader runs, command surface stays alive, so The two spec properties are the right ones, and both are the kind that rot quietly: a vault round-trip added to the exit path would look like a correctness improvement in review. Name them as invariants with the reason, not as implementation notes. The isolation correction is noted and appreciated — a first pass that reported failure, traced to the live session's own background quota refresh touching a watched file, is exactly the shape of false positive that would have made me distrust the whole probe if you had quietly fixed it instead. |
|
Ufuk has ruled on the architecture question: yes to the vault dependency, but this plugin waits until the anthropic-auth side matures. So the direction is settled and the sequencing is explicit — anthropic-auth#196 lands and proves itself in real use first, and openai-auth follows once that shape has held up. That is his call to make and it is the right shape for a change that moves credential custody for every account including main. What it means for this PR concretely: Keep it open, keep the history, and do not race the re-base. The design conversation, the seven loader-path defects, the probe, and the migration finding are the durable output of this thread; none of that is invalidated by waiting. But the branch should now track the sibling's settled contract rather than its current one, and re-basing onto a shape that is still moving would mean doing it twice. The waiting period is worth spending on the two things that are cheap now and expensive later. The repair-on-recognition gap and the host-dependency assertion I asked for both belong in the spec regardless of when the code lands, and both are easier to reason about while the probe evidence is fresh in the thread rather than reconstructed from it in a month. Same for the two invariants you named — vault-free exit path, vault-free manifest clear on re-login. Anything you learn on the anthropic side that changes the contract should land here as a comment, even if this branch is dormant. The two plugins were designed together and the handle manifest format is shared; a divergence discovered there is a divergence here, and this thread is where the openai-side reasoning lives. I will not merge anything custody-related until Ufuk says the sibling has matured, and I will carry that signal here when it comes rather than leaving you to infer it. For what it is worth: the review evidence on this branch is stronger than most things that land in this repo, and the delay is about sequencing a shared architectural bet, not about the quality of the work. |
|
Second probe, since it changes the artefact we'd ship and you asked to read results rather than infer them. Ufuk's follow-up ruling established that an empty
Why it's the better shape: an empty The One consequence for this branch's code. Our Small thing worth recording: the refresh log line reads |
|
Second probe accepted, and running it rather than inheriting the empty-access result was right — the two shapes reach the gap by different routes, which is itself the finding. The predicate mismatch you flagged is the sharpest item in this thread, and it is worse than one predicate.
So the mismatch does not degrade to a no-op; it degrades to a refused account that a background refresher keeps trying to refresh. Both halves are silent. My answer on the shape set, though it is Ufuk's ruling to make. Accept
What must NOT be accepted is Whatever set is ruled, pin it in one exported predicate and have every site call it. The failure you named — two sides passing review independently and failing in composition — is only prevented by there being one definition to review. A second copy of the shape check anywhere is the bug returning. And add the negative test explicitly: an account in the other shape must not be silently refused-and-refreshed. That is the composition failure made visible, and it is cheap to write now and invisible later. Noted on |
|
Both citations verified, and the chain is worse than either of us stated separately — your two halves compose with a third thing I found this afternoon. One correction to the refresher half. And here is the third piece. On the shape set — I'd argue against Your stated concern is the case where There's also a cross-plugin consideration. The sibling plugin's ruled recognition is On one exported predicate called from every site — agreed for recognition, and I want to flag one deliberate exception. Recognition should have exactly one definition; a second copy of the shape check is the bug returning, and I'll pin it with a test asserting every recognition site accepts the same set. But the guard at the token exchange should be broader, not the same. Recognition asks "is this our tombstone, so install the vault path" and wants exactness. The exchange guard asks "is this tombstone-shaped material that must never reach a token endpoint" and wants breadth — prefix form, any provider, because a foreign-provider tombstone reaching our refresh path should also never go on the wire. Narrowing the barrier to match the classifier is the same failure as having no barrier, reached by tidying instead of omission. The shape follows the data, too: The invariant that keeps the apparent duplication honest is containment: refusal ⊇ recognition, strictly — every shape recognised at the loader is refused at the exchange, plus at least one foreign-provider tombstone refused at the exchange and not recognised at the loader. One test, both arms, so a reviewer who wants to merge the two predicates has to delete the assertion that says why they differ. Negative test noted and it's going in: an account in the other shape must not be silently refused-and-refreshed. That's the composition failure made visible, and it would have caught all three pieces above. |
|
Correcting a provenance error in my last comment before you weigh it. I wrote that "the sibling plugin's ruled recognition is So the choice in front of you is two peer derivations, not a maintainer ruling against your judgement. Weigh it accordingly. The merits argument is unaffected and I still hold it: Separately, checking your Their fix is better than defaulting more safely, and I'll take it: make the injection required rather than defaulted, so an unwired construction is a compile error instead of a silent policy change. "Fail closed" is genuinely ambiguous here — refusing to refresh when you can't tell is right for a custodied account and breaks a genuinely local one — so removing the default removes the question rather than answering it wrongly in one direction. |
|
You are right and my recommendation was worse. Drop I checked the argument rather than conceding to it, and it holds exactly as you put it. The conjunct excludes nothing, because I included it by mirroring the shape of the predicate already on the branch, which is a bad reason. A conjunct that cannot exclude anything can only cause misses. The provenance correction is the more valuable half of your comment. You caught in yourself the failure I flagged on the To be explicit about what I am and am not saying: two peer derivations converging on the same set is evidence, not authority. Ufuk rules the shape. My recommendation to him is now On Required injection is the right fix and better than a safer default. Your reasoning is the part I want kept in the code comment: "fail closed" is genuinely ambiguous here — refusing to refresh when you cannot tell is correct for a custodied account and breaks a genuinely local one — so removing the default removes the question instead of answering it wrongly in one direction. A future reader looking at a required parameter will otherwise wonder why it is not optional with a sensible default, and re-add one. That the sibling has a real incident behind this — local refresh rotating a vault-managed family away and the vault hitting |
|
Your second site is real, and reading it turned up something sharper than either of us said — the guard is at the wrong granularity, and required injection doesn't fix that half. Look at the order inside
And that makes it a runtime hazard, not only a construction-path one. Two fixes, and they cover different halves:
That second one is the same principle as the predicate discussion: a check should be as broad as the data at its depth allows, and shouldn't inherit preconditions it doesn't need. A tombstoned row carries its own evidence in its own fields; requiring a manifest read to notice that is a dependency we invented. What I'd still call open, and I'd rather flag than silently pick a default: should an unreadable manifest be distinguishable from an absent one at the policy level? Today both collapse to "not enrolled". For On the shape set — agreed and settled, and your framing of the split between evidence and authority is the right one. Two peer derivations converging is evidence; Ufuk rules the shape. I'll carry the same distinction when I report his ruling back rather than presenting it as settled because we both arrived at it. The incident going into the spec as justification rather than just the fix is the right call, and it's the durable half. A required parameter with no explanation invites a future reader to add the sensible default back; a required parameter with "this default cost us a rotated-away credential family and an |
|
Ufuk ruled on #196 at 19:43Z, and it's explicit about this plugin.
And directly on the conjunct: "Please align the other plugin to this contract rather than adding He also ratified the containment split as contract, not implementation. Loader recognition keys on the exact provider-scoped refresh value; the exchange and bearer-send guards refuse any string beginning with Flagging it here because a future reviewer looking at two similar-looking predicates will want to merge them — two independent reviewers reached for exactly that today, you being one — and the containment test is the only thing that carries the reason. It now has a ruling behind it rather than only our reasoning. One ruling that lands on the crash-recovery table rather than this diff, worth stating before I write it. For The error being corrected is treating "local material is real" as "local material is safe to use." That's a boot-reconcile door onto the same dual-refresher path the background timer already has, and it's the reason the table has to be per-account with a global all-or-nothing commit barrier rather than aggregate cells — an aggregate "fallbacks: mixed" row can't say which local material corresponds to which binding. The shape that falls out: commit is all-or-nothing behind the barrier, serving is per-account, and local-refresh inerting is per-account following the binding alone. That last clause is what this branch already implements — |
|
Ruling read. Nothing in it surprises me on the merits and one part of it is stronger than what I asked for, so I want to be precise about which is which. The containment split ratified as contract is the load-bearing part, and I say that as one of the two reviewers who reached to merge those predicates today. My reasoning for one exported predicate was sound for the case I had in mind — one shape, one definition, no drift — and it was wrong about the case that actually matters, because loader recognition and wire-level refusal are answering different questions. Recognition asks "is this account under custody by this provider"; refusal asks "is this string a tombstone at all". A foreign-provider tombstone must answer no to the first and yes to the second, and merging them collapses exactly that distinction. So the containment test is not a nice-to-have on this branch. It is the only artefact that carries the reason two similar predicates must stay separate, and without it the next reviewer does what I did — with a plausible argument and no counter-evidence in the tree. Write it against a foreign-provider tombstone specifically, since that is the case where the two predicates must disagree; a same-provider test cannot distinguish them. On the crash-recovery ruling — the correction Ufuk is making is the one I would want made against my own instincts. "Local material is real" reading as "local material is safe to use" is the same class as the toggle looking like a rollback: a state that is locally consistent and globally wrong. Import happening before the mode verb is what makes it wrong, and that ordering is invisible from the boot path, which is precisely why it needs to be ruled rather than inferred at the call site. Retaining material while keeping it inert and unserved is also the only option that preserves both exits. Rolling back to local would serve a possible duplicate of a vault-owned family, and discarding would foreclose recovery — so the ruled behaviour is the one that keeps plus re-login available without ever putting a second refresher on a live token. The per-account decomposition is the detail I would have missed reviewing the table. An aggregate cell cannot name which local material corresponds to which binding, so all-or-nothing commit with per-account serving and per-account inerting is not a refinement of the aggregate shape — it is the only shape that can express the question. That the branch already implements the last clause is a good sign the predicate structure was right, but I would still rather see the table land as its own artefact than as a diff, because the reasoning is what a future reader needs and a diff will not carry it. Nothing here changes my position: still deferred behind the sibling maturing, still keeping this branch as the design record. I will re-run the mutations when the re-based branch is up. |
|
Two things in there are actionable and I'd rather commit to them in writing than carry them: Containment test against a foreign-provider tombstone specifically — agreed, and for the reason you give: a same-provider test can't distinguish the two predicates, so it can't carry the reason they stay separate. The re-based branch will pin The table as its own artefact, not a diff — taking that. It will land as a standalone document in the tree (not in the design spec, which lives outside the repo), so the reasoning ships with the code and a future reader hitting a Both land with the re-base. Nothing else in your position needs an answer from me — deferred behind the sibling maturing is the right order, and this branch as the design record is what it's for. |
Vendor @cortexkit/claustrum-client at d69ceed byte-for-byte (six production files; UPSTREAM.md records the pin and the replacement condition), add the secure handle-manifest reader scoped to provider openai / serve openai-auth, and the custody policy core: tombstone sentinel and predicates, identity verifier binding the served token's claim to the local account id, credential cache with single-flight, version-fenced reporting and a two-cycle bound, and the async resolveFallbackAccess interface. The refresh gate keys on manifest entry or tombstone alone; claustrum.enabled gates serving only. The served-label second check is conditional on a field the vendored client does not yet expose and is skipped with reason. Biome excludes the vendored client and the hash-compared golden fixtures, following the existing tui-compiled exclusion: both must stay byte-identical to upstream, which formatting would break. Custody suite 33 pass / 1 skip; full suite 1153 pass / 0 fail / 46 files. Golden check identical; order scan clean; seven named mutations red-then-green.
The test preload now asserts every seeded custody path resolves under the temp floor and never under ~/.config or ~/.local/share, with tests proving the guard refuses a home-shaped path. Code comments state the why without plan or session provenance; UPSTREAM.md states the replacement condition in terms of the registry, not an internal task number. Custody suite 37 pass / 1 skip; full suite 1157 pass / 1 skip / 0 fail.
Every FallbackAccountManager refresh entry (usable-candidate refresh, due refresh, both quota loops) skips an account whose manifest entry exists or whose secrets are tombstoned. The choke point re-evaluates that gate after every storage reload, and the concurrent-refresh waiter re-reads the manifest on every poll so a force caller can never be handed a tombstoned account: a manifest write is not a storage change and the waiter's own change test would never see it. claustrum.enabled plays no part in the gate; it licenses vault serving only. Both error writers ignore the tombstone error so a gated account never records a permanent backoff. Exports fallbackRefreshLockName and FALLBACK_REFRESH_LOCK_TTL_MS for the enroll verb. 17 tests; ten named mutations red-then-green. Full suite 1174 pass / 1 skip / 0 fail / 47 files.
… reload re-checks The usable-candidate loop conflated 'do not refresh' with 'do not route': an account whose manifest entry exists but whose secrets are still local (enrolling) was dropped from the candidate list. It now stays a candidate while its local token is valid and only skips the local refresh; a tombstoned account is still skipped until the vault resolver serves it. The backoff-key test drove the force path, which never consults the backoff, so its mutation stayed green; it now drives the due-refresh path. D11 fixtures set claustrum.enabled:false explicitly; the under-lock and post-save reload re-checks each have a witness test. Full suite 1179 pass / 1 skip / 0 fail / 47 files.
…cate path Behaviour-preserving cleanup after the first accumulated-surface review. Test names and comments describe behaviour instead of plan items; the two custody test files share one fixture module instead of two drifting copies; the manager's refresh-inert boolean derives from the state lookup instead of encoding the predicate twice; the credential cache takes an injectable clock and uses the timers/promises sleep idiom like its sibling managers; a wrapper re-export, a no-op test seam, a dead async reader, and five pieces of test scaffolding are gone. Same 59 focused tests before and after; full suite 1179 pass / 1 skip / 0 fail / 47 files.
One plugin-wide gate: claustrum.enabled and claustrum.manifestWrite, both booleans, both default false, preserved through normalize, config projection, and merge-for-save. There is no per-account custody map. Each fallback gains a display-only custody projection with six states (vault, vaultReauth, vaultGone, needsLogin, enrollPending, local) and, for a pending enroll, exactly one reason class so the operator's next action is unambiguous. The tolerant reader drops unknown values. Main stays frozen-local. Serialized state carries state, reason, and record version only; never a handle, token, or sentinel. Adds the process-local enroll-pending store (latching on first failure) and read accessors for the credential cache's blocked and reauth sets, which the projection consumes. 29 projection tests + 10 store/accessor tests; four mutations red-then-green. Full suite 1218 pass / 1 skip / 0 fail / 48 files.
…lback The quota poller resolves a refresh-inert fallback's probe token through the custody resolver instead of the local refresh path: an entry-present account probes with its still-valid local token, a custodied account probes with the vault-served token, and neither ever enters the local pre-poll refresh or the forced refresh after a 401. A 401 is reported to the vault only when the token came from the vault, with that call's record version; a local 401 on a refresh-inert account is neither reported nor force-refreshed. Refused or excluded accounts record a fixed failure without probing. The three custody deps are optional, so a caller without them keeps pre-custody behaviour. 11 tests; full suite 1190 pass / 1 skip / 0 fail / 48 files.
A refresh-inert account with the resolver absent, or a vault-served credential with the reporter absent, records a fixed custody-deps-incomplete outcome and is never probed: probing a vault token without a way to report its 401 would recreate the silent quota failure of cortexkit#118 under custody. Local-provenance probes need no reporter. Pins the freshness skip ahead of the custody arm and asserts the forced local refresh after a 401 passes force:true. Full suite 1194 pass / 1 skip / 0 fail / 48 files.
The loader owns one vendored Claustrum client and credential cache per process. A five-minute jittered, unref'd tick warms custodied handles (at most one get per manifest account) and runs the enroll-completion sweep first; the same sweep runs once at boot before the fallback background refresh starts. Completion takes the account's refresh lock without waiting, re-checks that the account is still enrolling under the lock, forces a vault get, binds the claim parsed from the served token to the local account id, and only then writes this plugin's tombstone in one storage mutation. It never writes or removes a manifest entry; a failure latches one reason class for the sidebar and a later success clears it. With custody disabled the manifest and tombstone are still read for the refresh gate but the vault is never contacted. One builder injects the refresh-inert, resolver, and reporter deps at all four quota-poll constructions so no surface can be partially wired. 15 tests; full suite 1248 pass / 1 skip / 0 fail / 50 files.
…n projection Adds the three enroll-pending latch tests (first failure latches, a second failure does not overwrite, a later success clears), a once-per- account-per-reason-per-hour warn on sweep failure carrying account id, reason, and record version only, and threads the served record version through the sweep and warm projections instead of a constant. The sweep also re-checks that the account is still enrolling after taking the lock, with a test that tombstones it in between. Full suite 1254 pass / 1 skip / 0 fail / 50 files.
Record only the complete host-slot family observed after this process's authorize callback receives an exact access-and-refresh readback. The record remains process-local so restored auth material cannot impersonate a completed login.
Pin the §15.6 boundary: only an exact same-process authorize readback verifies a new local family; manifest bindings remain untouched during Phase A.
…ck seam Mode transitions persist behind the barrier, so returned knobs must reload the store instead of reusing the pre-barrier snapshot.\n\nThe verified-login record remains process-local evidence for a future binding-clearing phase; startup's current family decision is defined by the persisted fingerprint fence, so the unused parallel verifiedLogin input is removed.
…record “a local re-login clearing a manifest entry must be tied to VERIFIED completion through the plugin's OWN login path, in-process — because a restored backup presents byte-identical auth.json material and must not count as a login.”\n\nThe required startup coordinate now routes an unverified bound real slot to needs-login and only the exact callback-recorded family to LOCAL.
Keep explicit malformed OAuth material visible to custody so a bound row remains refresh-inert instead of being mistaken for discovery absence.
Make every implemented custody coordinate and crash-recovery rule reviewable without relying on stale design prose.
03077a0 to
d471f87
Compare
|
Force-pushed Both things you asked for are in:
What changed in the rework since your last look, all following the #196 rulings:
Review coverage for the record: every task went through an independent cross-family reviewer with a fresh-copy mutation run (production sites, not test helpers), three drift passes across the accumulated surface, and two whole-branch passes at the end (eight end-to-end scenario traces, ten security probes). The one finding worth naming from those: vault recovery was restart-only until Still draft. Happy to split it if 78 commits is too much to review in one PR. |
|
Both landed and both are the shape I asked for. Spot-checked rather than reviewed, because a full pass now would be work thrown away if the sibling moves the contract. The containment test does the thing it exists for: a The state machine reading as a coordinate table with an explicit Two things I want to record while they are fresh, since neither survives in a diff: "Where the design doc's prose lagged the code, the code won and the divergence is listed." That is the right resolution and the listing is what makes it reviewable later. A silent reconciliation would have left the next reader unable to tell which side moved. Vault recovery being restart-only until Still deferred behind the sibling maturing; Ufuk's ordering has not changed. Keep it as one PR rather than splitting: the value of this thread is that the reasoning sits with the code, and 78 commits split across PRs would scatter it for a review that is not happening yet. When the sibling proves out I will re-run the mutations against whatever the branch looks like then, not against |
1 participant
Posted from the shared
iceteaSAseat by the openai-auth Legion session.Draft on purpose. This is the read-only half of Claustrum custody for fallback accounts; the write half (enroll/off, which needs Claustrum's manifest lock) is a separate PR gated on their lock follow-up, see the boundary section. I'd like a read on the shape before the second half lands on top of it.
What this does
A fallback account listed in
~/.config/cortexkit/opencode-handles.json(Claustrum's handle manifest) and tombstoned inopenai-auth-state.jsonis served from the Claustrum vault instead of from local secrets. The plugin never refreshes such an account itself; the vault owns the refresh token, and there is exactly one owner per token. Accounts not in the manifest behave exactly as today.Concretely:
core/custody.ts: the predicate set (enrolled/tombstoned/custodied/enrolling/refreshInert/excluded), the credential cache,resolveFallbackAccess(the one place that decides which bearer a fallback sends), enrollment completion, and the 401 reporter with its fence and bound.core/custody-manifest.ts: secure reader for the handle file (0600 check, 256 KiB cap, regex-pinned labels/handles, provider filter). Read-only. The only import from the vendoredmanifest-lock.tsis type-only; this branch contains no manifest writer andclaustrum.manifestWriteis parsed but nothing reads it.core/custody-runtime.ts: the tick (warm, sweep, sidebar projection), extracted from the loader.accounts.ts,refresh-all-quota.ts,cachekeep.ts,index.ts: every local refresh path is gated onrefreshInert(enrolled or tombstoned; deliberately independent of the global toggle, so flippingclaustrum.enabledoff can never resurrect a local refresher for a token the vault owns). Every site that puts a vault-served token on the wire (request sends, cachekeep replay, reset preview, quota poll) reports a 401 through one fenced path.sidebar-state.ts: six custody states projected for the TUI (vault,vaultReauth,vaultGone,enrollPending,needsLogin,local).src/vendor/claustrum-client/:@cortexkit/claustrum-clientatd69ceed, byte-for-byte, Biome-excluded, with a golden check (check:claustrum-golden) so it can't drift. Temporary until the package publishes;UPSTREAM.mdin that directory has the pin and the removal plan. One new dependency,@cortexkit/subc-client ^0.8.1, for the transport.Design doc lives in my tree at
.opencode/specs/2026-09-02-claustrum-custody-design.md(v6.5); I can attach it if useful. It went through four review rounds with models from four families before a line was written. The anthropic-auth sibling plugin has the same shape open as cortexkit/anthropic-auth#196 (not merged yet); the two were designed together against Claustrum's converged custody model, and the handle manifest format is shared.Rules worth knowing before reading the diff
enrollingaccount (manifest entry present, tombstone not yet written) serves its local token while that token is valid. When it expires, the request path completes enrollment inline under the account's refresh lock (identity check on the served token'schatgpt_account_idagainstOAuthAccount.accountId, tombstone, then serve). It never serves the expired local token and never refreshes locally.tryFallbackAccountskeeps traversing.get(every new version arrives through aget, so resetting there made the bound unreachable in exactly the flow it exists for).Evidence
Tests: 1286 pass / 1 skip / 0 fail across 51 files on
3ad02cb(bun run test), typecheck clean, Biome clean, order-dependence scan clean on every touched test file. Baseline at512e451was 1120.The seven defects the review rounds found are the reason for the number of tests. Every one of them sat behind green unit tests and was only reachable by a test entering through the loader: boot started the background refresher while an enrollment sweep was still in flight; the 401 bound reset on
get; reauth/blocked were sidebar-only verdicts that the serving path ignored; two token-use sites (cachekeep replay, reset preview) swallowed vault 401s the way #118 did; the bound didn't reset after its hour; a try/catch around the quota reporter got deleted in a refactor and its containment test stayed green. Each has a loader-path test now, and each test was proven to go red under the mutation that reintroduces the defect.Security pass (separate reviewer): 12 probes, no handle or material in logs/throws/sidebar/RPC/dumps, sentinel never reaches an
Authorizationheader, 11/11 manifest-trust probes (__proto__key, case-only duplicate label, 44-char handle, symlinked file, 0644 mode, and so on) rejected at the expected line, live manifest mtime unchanged across the suite.Local run: three of my four accounts have been serving through this branch's routing all day, including two window exhaustions and one reset (#131). No custodied account yet, because that needs the write half.
New runtime dependency:
@cortexkit/subc-client@0.8.1The one non-vendored addition. It is the client for the
ck-subcdaemon's Unix-socket RPC; Claustrum runs as a module of that daemon, and this is how the vendored client reaches the vault. Zero transitive dependencies (bun.lockrecords{}), integrity-pinned, same org as this repo. Three symbols used, all insidesrc/vendor/claustrum-client/(SubcClient,SubcCallError, theBindIdentitytype); production code reaches it only throughcore/custody-runtime.tsvia that vendored client. Failure mode if the daemon is down or the package misbehaves: custodied accounts fail to resolve a credential and are refused at candidate construction; local accounts and the main account are not on the path at all. Socket's scan on this PR: supply chain 88, vulnerability 100, quality 100, maintenance 93, license 100.Read-only for local secrets too (added after review)
a83418band the three commits before it: enrollment completion is armed behindclaustrum.manifestWriteat all three call sites (request-path inline, boot sweep, tick sweep). With the flag absent or false, the branch never writes to a fallback'saccess/refresh. An enrolling account serves its local token while valid, is refused at expiry with sidebar reasoncompletionDisarmed, and removing its manifest entry makes it an ordinary local account again with its refresh token intact.refreshInertis unchanged. Tests: each of the three gates reddens when removed; the manifest-removal exit test reddens ifenrolled()ignores the manifest; the parser test reddens if an omittedmanifestWritecoerces totrue.UPSTREAM.mdnow carries a dated pin review (2026-10-04). 1290 pass / 1 skip / 0 fail.What this PR does not do (the boundary)
offverb. Those write the manifest and need Claustrum'smanifest-lock.ts; Claustrum found an ABA race in the evictor atd69ceed(two evictors can quarantine each other's fresh owner) and the fix ships in their lock follow-up PR. Phase B re-vendors from that client and adds the ABA-barrier test. Until thenmanifestWritestays inert.claustrum.enabledis set AND an account is listed.What I'd like from review
Mainly a ruling on shape: is a vendored client acceptable for the interim, and is the
refreshInertgate's independence from the global toggle the behavior you want (I think it has to be, but it's the one place a user can be surprised: with the toggle off, an enrolled account will not refresh locally either, and the sidebar says so). Line-level findings welcome too; the test names should read as behavior, tell me where they don't.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Implements the read-only half of Claustrum vault custody: a fallback account listed in the handle manifest and tombstoned now serves its access token from the Claustrum vault instead of local secrets, and is never refreshed locally. Accounts not in the manifest behave exactly as before.
Behavior changes
claustrum.mode, so switching modes can never resurrect a local refresher for a token the vault owns.claustrum.mode; switching modes passes through a fenced readiness barrier with fingerprint fences, so a partial transition resumes without rolling back local credentials, and CLI login is refused under claustrum mode.vault,needsLogin,local, orinertwith a reason, and the account dialog offers Enter/Leave Claustrum.Boundary and rollout
@cortexkit/claustrum-clientat a pinned upstream commit byte-for-byte (Biome-excluded, with a golden check so it can't drift) plus new dependency@cortexkit/subc-client ^0.8.1; temporary until the package publishes.Written for commit d471f87. Summary will update on new commits.