Claustrum vault custody: serve the main account and enrolled fallbacks from the vault - #132
Conversation
|
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. |
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 |
d471f87 to
0d4635e
Compare
|
Rebased onto One conflict, in The rebase touched no custody file — the diff against the pre-rebase branch is exactly your transport work plus that one test hunk. Gates at Two things found while verifying, both worth your attention more than mine: Six order-dependent tests, now fixed ( That latch looks like a bug on |
0d4635e to
482a7ff
Compare
|
Rebased onto One conflict, in Gates on the rebased head: 1446 pass, 1 skip, 0 fail; The one thing I watched for: Still draft, still waiting on your review of the substance. |
|
The anthropic-auth counterpart is merged, so the deferral I put on this in September is lifted. I read the shipped implementation at What shipped, and where
Plus The four things this branch should match1. Tombstone value and recognition — as ruled, confirmed in code.
2. The containment split is two separate predicates, as ruled. 3. The manifest is scoped by a resolution rejects outright when 4. Crash recovery is a verdict table, not ad-hoc branching. Claustrum mode with real local material and an unavailable vault never rolls back to local serving — it stays incomplete until the vault returns. That is the behaviour I ruled for this side too, and it is now a table you can port rather than prose to reinterpret. The verb
Same shape here, under One protection worth copying verbatimThe account store discards a credential write that lands on a tombstone ( Still open from my earlier review, unchanged by any of thisThe How I would like to take thisRebase onto |
482a7ff to
2835bac
Compare
|
Rebased onto v0.7.2 and aligned. Head Going through your four points, plus the two you left open. 1 and 2 — already matchedRecognition keys on 3 — constants were right, and one thing you described is not what the sibling does
On "rejects outright": the sibling treats an own-provider serve mismatch as Discovery: What did land is the absolute-path requirement on 4 — equivalent on four rows, one I could not confirm
5 — not implemented, and here is exactly whyThe write path a stale refresh takes: Your We do have a provenance marker — Still open from your earlier reviewBoth were already done and I should have said so sooner: One correction you should haveThe credential-id validator now scopes on the provider segment — The reason it is worth a paragraph: our fixtures used |
|
Rebase and alignment confirmed. Four answers, and one correction to something I wrote. 3 — you are right and I was impreciseI wrote "rejects outright". The sibling does not throw; The absolute-path requirement on Discovery via
|
|
Implementing the sticky tombstone, and it turned up a gap in the ruling worth your ruling rather than my guess.
|
|
Sticky tombstone is in — The guardSits in Mode comes from the config read that Confirmed the race you described before touching anything: BehaviourUnder ProofsBoth directions, and the second is the one that matters:
That second mutation exists because a guard proving only the discard half would pass every test while permanently bricking On the mode-awareness resolutionFlagged separately in my previous comment and unchanged: this is the deviation from your literal wording, since the exit verb writes mode rather than credentials. If you would rather Gates: 1457 pass / 1 skip / 0 fail, typecheck, Biome, order scanner 6/6 isolated, clean tree. |
|
Mode-awareness is right, and it is better grounded than either of us put it — the sibling's own verdict table already depends on it. Why your option 3 is the correct oneYou rejected clearing the tombstone on exit because it would lose the Local mode with a tombstoned main is a reviewed, named, non-error state in the reference implementation. It must remain reachable. Clearing on exit would make it unreachable and replace a slot that says "the vault had this, log in again" with a slot that says nothing. So: mode-awareness, as you built it. The guard discards over a tombstone only while On my original wordingYou are right that I named a writer that does not write. What I was reaching for is what you built: the replacement is permitted because the operator has durably declared the slot is ours again. Mode-as-declaration is a better expression of it than a permit flag — a flag is a claim any caller can make, including one still in claustrum mode, which is the ambiguity the rule exists to resolve. Your rejection of option A is the same reasoning I used to reject The config readSourcing mode from the read The second mutationThis is the part of the delivery I would point at if someone asked what good test evidence looks like. A guard that proves only the discard half passes every test while permanently bricking Verified myself before replying: Where this leaves the PRThis was the last item I was waiting on. Doing the full review pass now against head |
|
Stop before the full review pass — What landed
Three of the files this branch edits moved packages:
What that means for the tombstone guard specifically
The custody modules themselves ( What I verified while waitingThree things I had open, all confirmed on
What I would likeRe-base onto If the rebase turns up something where the new path-parameter shape and custody disagree, bring it back rather than forcing it; that seam is three days old and I would rather change it than have custody work around it. |
|
Stopped before re-basing, as you asked. I started one to measure it, hit the decision below in the second commit, and aborted rather than resolve it 80 more times. Branch is untouched at The rebase itself is tractable
The problem is that our custody layer does not split along your package boundary
Two things fall out of that, and both are yours to rule on rather than mine to pick mid-rebase.
Core would import a host-coupled module. Three ways:
I lean to the third. It matches how core already handles provider-specific behaviour, it keeps core free of custody vocabulary entirely, and it does not require me to guess which half of the transition module you would consider shared. But you have just spent 72 files deciding what belongs in core, so you have the clearer view. One correction to something I nearly sent youMy first pass reported Say which shape you want and I will rebase onto it in one pass. |
|
Before you rebase —
|
| gone | where the same guard belongs |
|---|---|
cli.ts add path |
packages/opencode/src/auth/methods.ts, the menu's Add account action |
| — | executeAccountCommand's add branch in packages/core/src/commands.ts, which both hosts reach |
The core one is the load-bearing one: it is the single body OpenCode's slash command and Pi's openai-account add both call, so a guard there covers every host at once. The menu action is a separate call site and needs its own check, or needs to route through the same body.
Worth deciding deliberately rather than porting mechanically: Pi now writes to the same store format, so "custody is active" is a question Pi can ask too. If the guard lives only in core, both hosts get it for free.
Everything else that moved
Since the layout note I sent this morning, main went from dfd36fc to c40f8ee. Relevant to this branch:
packages/opencode/src/core/accounts.ts→packages/core/src/accounts.ts, with every store entry point taking caller-supplied config and state paths. No defaults, and core reads no environment variable —rg 'process\.env' packages/core/srcmatching nothing is an enforced criterion, so the mode read insidesaveAccountStateneeds to keep that property.- A new
packages/opencode/src/auth/directory holds the login methods and the doctor. If custody wants a startup verdict surfaced to an operator, the doctor is now the natural place for it rather than a bespoke path. packages/opencode/src/logger.tsis a thin host shim; the logger itself is in core.
The custody modules themselves are untouched by all of this — they are new files, so they rebase cleanly. The friction is confined to the two files above.
On the head
GitHub shows this branch still at 75237c9 on top of v0.7.2, so I read this morning's update as metadata rather than a push. If you have rebased locally and not pushed, ignore the above and just confirm which base you landed on; I will review against whatever is on the branch.
No rush from my side. I would rather you rebase once, onto something current, than twice.
75237c9 to
b676809
Compare
|
Ported onto It is one commit now, and that is a change worth explainingThe branch was 82 commits. Replaying them across the package extraction produced conflicts in files that no longer live where those commits think they do, and the intermediate states were only ever meaningful against the old layout — a commit that edits That turned 82 conflict rounds into one pass of 15 files, which is also what made the next part visible. What nearly shippedOne resolution pass took upstream's side wholesale on Recovered with a proper three-way merge against base, ours and upstream: 8 real conflicts rather than an all-or-nothing choice. Your three rulingsThe account-add guard is in Core purity holds. The The test count moved and the delta reconciles1457 before the layout change, 1332 now, all in one package then and two now. Pristine Ready for the full review pass. |
b676809 to
2d40c4f
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
iceteaSA
changed the title
2d40c4f to
116bc1a
Compare
|
Green on The two CI failures were both mine, and both were me checking the wrong thingThe dependency. The format gate. I had been running I have stopped guessing at the gate set — the five commands come out of the workflow file now, and all five pass locally: Two Biome findings I checked instead of autofixing
Ready for the full review pass. |
|
Full review pass done against First, the gate, run myself rather than taken from the branch: 146 core / 1332 opencode / 14 pi, typecheck clean. My first run showed two failures in 1.
|
Squashed for the port onto the shared-core layout; the 82-commit history is preserved at tag backup/custody-pre-0c90408.
116bc1a to
c4b98eb
Compare
|
All three fixed, plus the small one, on 1.
|
|
CI green on It was draft originally because the design was still moving and I wanted a read on the shape before building further on it. That is no longer true: the design is settled, you have reviewed it against the shipped sibling, and the state machine and verdict handling came through your pass unchanged. One thing worth restating now that it is mergeable, because the title changed under it: this covers main and enrolled fallbacks, not fallbacks alone. The original description promised the narrower thing and was two revisions stale; it is rewritten. Phase B — the manifest writer and the enroll verb — stays separate, and it now has a real unblock: Claustrum's manifest lock merged upstream, so that PR re-vendors from the merge commit and adds the ABA-barrier test. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
40 issues found across 57 files
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/index.ts">
<violation number="1" location="packages/opencode/src/index.ts:1554">
P1: When the main slot is a recognized Claustrum tombstone, the request path still uses `getAuth()`'s tombstone and never reads the main handle from the custody cache. Because `refreshMainWithLease` explicitly rejects that tombstone, main-account requests are sent with an empty/tombstone bearer instead of the vault credential; resolve and thread the cached main credential into `primaryAccess` and its account identity before routing.</violation>
</file>
<file name="packages/opencode/src/core/custody-runtime.ts">
<violation number="1" location="packages/opencode/src/core/custody-runtime.ts:223">
P1: When the process starts in local mode, `/openai-account claustrum` cannot initialize custody because boot exits before creating the cache that its preflight requires. Make the runtime acquire the cache when the persisted mode changes to Claustrum.</violation>
<violation number="2" location="packages/opencode/src/core/custody-runtime.ts:296">
P1: After `/openai-account local`, this runtime still treats the process as Claustrum-enabled because it gates ticks on the loader-time storage snapshot. Read and enforce the current persisted mode before completion or warm passes so local credentials are never tombstoned or sent to the vault.</violation>
<violation number="3" location="packages/opencode/src/core/custody-runtime.ts:1034">
P1: When `CLAUSTRUM_SUBC_CONNECTION` points to a valid connection file, detection succeeds but cache setup ignores that path and connects to the default location. Reuse the same resolved connection path for detection and cache connection.</violation>
</file>
<file name="packages/core/src/custody.ts">
<violation number="1" location="packages/core/src/custody.ts:566">
P1: When force and non-force reads overlap for one handle, this condition starts two vault requests instead of sharing one. The first completion can also remove the newer flight from `#inflight`, allowing further duplicate requests and nondeterministic resident-record ordering; join any existing flight regardless of its force flag.</violation>
<violation number="2" location="packages/core/src/custody.ts:635">
P1: Concurrent 401s for the same handle and `recordVersion` bypass this fence because the version is marked only after the await. Deduplicate in-flight reports keyed by handle and version, otherwise duplicate 401s can count as two distinct failures and enter the one-hour reauth state prematurely.</violation>
<violation number="3" location="packages/core/src/custody.ts:836">
P2: If the manifest handle changes between the pre-lock read and the under-lock reread, this sweep still fetches the stale handle after validating the new manifest. Recompute the handle from `recheckManifest` under the lock, or abort when the manifest revision changes, before tombstoning the local account.</violation>
</file>
<file name="packages/core/src/custody-manifest.ts">
<violation number="1" location="packages/core/src/custody-manifest.ts:97">
P1: When the configured manifest path is a FIFO, this blocking open waits for a writer before `isFile()` can reject it, hanging manifest reads and startup. Add `O_NONBLOCK` so non-regular files are rejected without blocking.</violation>
<violation number="2" location="packages/core/src/custody-manifest.ts:181">
P2: A single descriptor read can return fewer bytes than requested, but this code treats that short read as EOF and parses a truncated manifest. Loop until EOF or `HANDLE_FILE_MAX_BYTES + 1` bytes have been read.</violation>
<violation number="3" location="packages/core/src/custody-manifest.ts:252">
P2: A manifest containing duplicate owning provider blocks is accepted even though the manifest-lock contract rejects duplicate provider IDs, and duplicate labels then silently use the last block's handle. Reject duplicate provider IDs while parsing instead of accepting this ambiguous custody state.</violation>
</file>
<file name="packages/opencode/src/core/custody-transition.ts">
<violation number="1" location="packages/opencode/src/core/custody-transition.ts:442">
P1: When `auth.all()` returns a partial non-empty map, this check passes and `auth.set` can rewrite the host auth store from a torn snapshot, potentially deleting other credentials. Require a complete coherent auth-store read before calling `set`, or defer the transition on any snapshot mismatch.</violation>
</file>
<file name="packages/opencode/src/tui/command-dialogs.tsx">
<violation number="1" location="packages/opencode/src/tui/command-dialogs.tsx:704">
P2: Disabled fallbacks can never reach this new `Enable` action because the sidebar state filters them out before `buildFallbackAccountOptions` is called. Load disabled accounts from account storage or include them in the sidebar projection so the dialog can re-enable them.</violation>
</file>
<file name="packages/opencode/src/core/custody-host-slot.ts">
<violation number="1" location="packages/opencode/src/core/custody-host-slot.ts:236">
P2: When the custody manifest is unreadable, this function inspects the host auth slot before returning `manifest-unreadable`, violating the documented precedence and potentially propagating an auth-store read failure. Classify the manifest first and return the inert verdict before calling `confirmMainAuthSlot`.</violation>
</file>
<file name="packages/opencode/src/auth/methods.ts">
<violation number="1" location="packages/opencode/src/auth/methods.ts:253">
P1: If Claustrum is enabled while the OAuth flow is waiting, this one-time preflight still lets `mutateAccounts` add the newly logged-in local token. Recheck the mode under the transition/store lock immediately before the upsert, or abort when it changed.</violation>
</file>
<file name="packages/core/src/accounts.ts">
<violation number="1" location="packages/core/src/accounts.ts:511">
P1: When a new custody tombstone is saved over an existing local state entry, the timestamp merge can keep the local bearer instead of the tombstone. Make an incoming tombstone authoritative before applying the newer-token comparison.</violation>
<violation number="2" location="packages/core/src/accounts.ts:864">
P2: When a row becomes `corrupt`, saving it does not clear previously persisted access and refresh values because the empty runtime patch is merged as an older token. Delete the account's existing runtime state when persisting a corrupt OAuth row.</violation>
</file>
<file name="packages/core/src/quota-manager.ts">
<violation number="1" location="packages/core/src/quota-manager.ts:508">
P2: When `refreshAllFallbacks` processes a vault-owned tombstoned fallback, this assertion throws before the per-account `try/catch`, rejecting the whole batch and skipping every later fallback. Skip custody-owned accounts or catch this sentinel inside the loop instead of allowing the batch method to throw.</violation>
</file>
<file name="packages/opencode/src/tests/setup-env.ts">
<violation number="1" location="packages/opencode/src/tests/setup-env.ts:136">
P2: When a parent process or CI supplies an intentional path override, these preload assertions reject it unless it happens to be inside this run's random `FLOOR_DIR`, causing the entire test harness to fail before tests start. Either always replace inherited values with the floor values, or only assert values that this preload seeded; do not preserve arbitrary overrides and then require them to equal the floor.</violation>
</file>
<file name="packages/opencode/scripts/check-claustrum-golden.ts">
<violation number="1" location="packages/opencode/scripts/check-claustrum-golden.ts:9">
P2: The documented command does not exist in `packages/opencode/package.json`, so this checker cannot run via `bun run check:claustrum-golden` and fixture drift cannot gate CI. Add the package script and wire it into the relevant CI check.</violation>
<violation number="2" location="packages/opencode/scripts/check-claustrum-golden.ts:58">
P2: When GitHub’s raw endpoint stalls, this check can hang the CI job indefinitely because `fetch` has no timeout. Pass a bounded abort signal so network failures fail the gate promptly.</violation>
</file>
<file name="packages/opencode/src/tests/integration.test.ts">
<violation number="1" location="packages/opencode/src/tests/integration.test.ts:2714">
P2: This test must restore the custody manifest environment to the test floor, not delete it. Deleting it makes later tests fall through to the operator's default manifest, and an exception before these cleanup lines can also leak the mocked `fetch`; wrap all process-global cleanup in `finally` and restore `FLOOR_CLAUSTRUM_HANDLES`.</violation>
</file>
<file name="packages/core/src/refresh-all-quota.ts">
<violation number="1" location="packages/core/src/refresh-all-quota.ts:445">
P2: When the resolver completes an enrollment whose original account has no `accountId`, this probe sends an empty `chatgpt-account-id` and caches quota without the served identity. Reload the account after resolution and use its current `accountId` for both `whamFn` and `setFallback`.</violation>
</file>
<file name="packages/opencode/src/core/custody-manifest.ts">
<violation number="1" location="packages/opencode/src/core/custody-manifest.ts:44">
P2: When `XDG_CONFIG_HOME` is relative or `HOME` is unset, `defaultCustodyManifestPath` returns a relative manifest path and makes custody depend on the process working directory. Reject relative XDG values and use the OS home directory fallback instead of returning `.config` relative to the working directory.</violation>
</file>
<file name="packages/opencode/src/core/cachekeep.ts">
<violation number="1" location="packages/opencode/src/core/cachekeep.ts:738">
P2: When a vault auth-failure report stalls, this await holds `prewarm()` and leaves `tickInFlight` true, so every subsequent tick returns and all sessions stop warming. Dispatch the report without blocking the cachekeep tick and catch its rejection, as the normal response path does.</violation>
</file>
<file name="packages/core/src/commands.ts">
<violation number="1" location="packages/core/src/commands.ts:433">
P2: When leaving Claustrum races with `enable`, the mode can become local between the custody check and the account write, leaving a vault-tombstoned fallback enabled but unusable locally. Serialize this mutation with the mode transition, or recheck the mode inside the same transaction that writes the account.</violation>
<violation number="2" location="packages/core/src/commands.ts:437">
P2: When `<id>` refers to an API-key fallback, this branch treats the existing account as missing because it only finds OAuth accounts. Find the account by ID for ordinary enable/disable operations, and apply the custody-binding check only when enabling an OAuth account in Claustrum mode.</violation>
<violation number="3" location="packages/core/src/commands.ts:959">
P2: If custody auth-failure reporting throws, one 401 prevents the reset preview from returning its per-account error row and aborts the whole account list. Catch reporting failures separately, log them, and still return the original preview failure.</violation>
</file>
<file name="packages/opencode/src/sidebar-state.ts">
<violation number="1" location="packages/opencode/src/sidebar-state.ts:91">
P2: After the first quota/sidebar machine write, the new main custody status is lost because the normal machine snapshot has no `main.custody` and replaces the persisted main row. Preserve or reproject the main custody value on every machine-state write so the sidebar does not revert to an unprojected main account.</violation>
</file>
<file name="packages/opencode/src/tests/custody.test.ts">
<violation number="1" location="packages/opencode/src/tests/custody.test.ts:134">
P2: The manifest-writing tests leak every descriptor returned by `openSync`. Close the descriptor in a `finally` block, or use the already-imported `writeFileSync`, and apply the same fix to the version test.</violation>
<violation number="2" location="packages/opencode/src/tests/custody.test.ts:1589">
P3: This golden-fixture assertion does not pin the fixture prefix or bytes; `fixturePath.endsWith('handles.json')` is tautological. Assert the expected prefix/content (or invoke the byte-for-byte golden check) so fixture drift cannot pass silently.</violation>
</file>
<file name="packages/opencode/src/tests/custody-runtime.test.ts">
<violation number="1" location="packages/opencode/src/tests/custody-runtime.test.ts:195">
P2: This reset removes the state needed to test hourly expiry, so the test cannot catch a broken time-window implementation. Remove the reset and rely on the advanced `clock` to prove the same key emits again after one hour.</violation>
</file>
<file name="packages/opencode/src/tests/custody-quota.test.ts">
<violation number="1" location="packages/opencode/src/tests/custody-quota.test.ts:693">
P2: This test does not guard its stated safety property: with `injectCustodyDeps: 'none'`, the quota loop deliberately falls through to local refresh, and the assertion confirms that call. Wire the inert predicate while omitting the resolver, then assert `refreshAccount` is not called and `CUSTODY_DEPS_INCOMPLETE` is returned, or rename this test to describe the pre-custody choke-point behavior.</violation>
</file>
<file name="packages/opencode/src/tests/custody-request.test.ts">
<violation number="1" location="packages/opencode/src/tests/custody-request.test.ts:289">
P2: This assertion does not verify that the enable command rejected the identity mismatch; it only verifies that the reason exists in a constant. Preserve and assert the command's actual failure/result, and do not swallow unexpected errors, so regressions in the binding guard cannot pass this test.</violation>
</file>
<file name="packages/opencode/src/tests/custody-transition.test.ts">
<violation number="1" location="packages/opencode/src/tests/custody-transition.test.ts:206">
P2: This fixture does not distinguish UTF-8 ordering from JavaScript’s UTF-16 ordering, so the test can pass with the wrong comparator. Use a BMP/non-BMP pair such as `\uE000` and `\u{10000}` and arrange the expected byte order.</violation>
</file>
<file name="packages/opencode/docs/custody-state-machine.md">
<violation number="1" location="packages/opencode/docs/custody-state-machine.md:7">
P2: Phase A also creates discovered fallback rows and updates `rowHistory`; it does not only write existing-row tombstones. Update this scope statement to distinguish the unsupported manifest writer/enroll verb from runtime discovery of local tombstone rows.</violation>
<violation number="2" location="packages/opencode/docs/custody-state-machine.md:172">
P3: The barrier description lists the wrong account-lock order: the implementation sorts `main` together with fallback IDs, so some fallbacks are acquired before `main`. Document the lexicographic participant order to keep lock-order guidance consistent with the code and test.</violation>
</file>
<file name="packages/opencode/src/tests/custody-main.test.ts">
<violation number="1" location="packages/opencode/src/tests/custody-main.test.ts:144">
P2: When the test runs without the preload-provided floor values, this cleanup deletes the path environment variables while detached loader work may still be running. Restore the test-process floor values instead of deleting them, so late writes cannot target the operator's default auth, state, log, or manifest paths.</violation>
</file>
<file name="packages/opencode/src/core/account-paths.ts">
<violation number="1" location="packages/opencode/src/core/account-paths.ts:28">
P3: `fallbackRefreshLockName` is unused here because runtime callers use the identical core helper. Remove this duplicate and its `createHash` import, or migrate all callers to one shared definition.</violation>
</file>
<file name="packages/opencode/src/tests/custody-authorize.test.ts">
<violation number="1" location="packages/opencode/src/tests/custody-authorize.test.ts:576">
P3: The final test asserts `record.hasVerifiedInProcessMainLogin(restoredSlot)` is `false` at the start, but production stores the fingerprints in a module-level `Set` (`verifiedInProcessMainLoginFingerprints` in `packages/opencode/src/core/custody-host-slot.ts`) that is never cleared. The assertion only holds because no earlier test in the same process verified a login with exactly `verified-access`/`verified-refresh`. This is an order- and process-dependent guarantee: any future test (or rerun pattern) that records the same credential family flips this assertion silently, and if test files are ever sharded into one process the check races. Use a token family unique to this test that cannot collide, or reset the set in `beforeEach`.</violation>
</file>
<file name="packages/core/src/tests/export-manifest.ts">
<violation number="1" location="packages/core/src/tests/export-manifest.ts:56">
P3: The comment says "The three vendored-client names" but the block lists four, and only three (ClaustrumClient, detectClaustrumConnection, getDefaultClaustrumConnectionPath) come from the vendored client. ClaustrumCredentialCache is defined in packages/core/src/custody.ts (line 466) and reaches internal.ts via `export * from './custody'`, not from ./vendor/claustrum-client/index, which exports neither it nor the internal.ts comment's promise that "the host needs exactly these two". The overstated grouping misleads a maintainer updating the surface list.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (auth.type !== 'oauth') return {} | ||
|
|
||
| const mainSlot = classifyMainAuthSlot(auth) | ||
| const recognizedMainTombstone = |
There was a problem hiding this comment.
P1: When the main slot is a recognized Claustrum tombstone, the request path still uses getAuth()'s tombstone and never reads the main handle from the custody cache. Because refreshMainWithLease explicitly rejects that tombstone, main-account requests are sent with an empty/tombstone bearer instead of the vault credential; resolve and thread the cached main credential into primaryAccess and its account identity before routing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/index.ts, line 1554:
<comment>When the main slot is a recognized Claustrum tombstone, the request path still uses `getAuth()`'s tombstone and never reads the main handle from the custody cache. Because `refreshMainWithLease` explicitly rejects that tombstone, main-account requests are sent with an empty/tombstone bearer instead of the vault credential; resolve and thread the cached main credential into `primaryAccess` and its account identity before routing.</comment>
<file context>
@@ -1190,16 +1550,22 @@ export async function CodexAuthPlugin(
if (auth.type !== 'oauth') return {}
+ const mainSlot = classifyMainAuthSlot(auth)
+ const recognizedMainTombstone =
+ mainSlot.kind === 'tombstone' || mainSlot.kind === 'empty'
+
</file context>
| }, | ||
| async runTick() { | ||
| if (closed) return | ||
| if (options.storage?.claustrum?.mode !== 'claustrum') return |
There was a problem hiding this comment.
P1: After /openai-account local, this runtime still treats the process as Claustrum-enabled because it gates ticks on the loader-time storage snapshot. Read and enforce the current persisted mode before completion or warm passes so local credentials are never tombstoned or sent to the vault.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/core/custody-runtime.ts, line 296:
<comment>After `/openai-account local`, this runtime still treats the process as Claustrum-enabled because it gates ticks on the loader-time storage snapshot. Read and enforce the current persisted mode before completion or warm passes so local credentials are never tombstoned or sent to the vault.</comment>
<file context>
@@ -0,0 +1,1035 @@
+ },
+ async runTick() {
+ if (closed) return
+ if (options.storage?.claustrum?.mode !== 'claustrum') return
+ // Re-read manifest (hot-reload on mtime) so an operator edit lands at
+ // the next tick without a restart.
</file context>
| // or the bounded single-flight guarantee collapses to N. | ||
| const force = !!options.force | ||
| const existingInflight = this.#inflight.get(handle) | ||
| if (existingInflight && existingInflight.force === force) { |
There was a problem hiding this comment.
P1: When force and non-force reads overlap for one handle, this condition starts two vault requests instead of sharing one. The first completion can also remove the newer flight from #inflight, allowing further duplicate requests and nondeterministic resident-record ordering; join any existing flight regardless of its force flag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/custody.ts, line 566:
<comment>When force and non-force reads overlap for one handle, this condition starts two vault requests instead of sharing one. The first completion can also remove the newer flight from `#inflight`, allowing further duplicate requests and nondeterministic resident-record ordering; join any existing flight regardless of its force flag.</comment>
<file context>
@@ -0,0 +1,909 @@
+ // or the bounded single-flight guarantee collapses to N.
+ const force = !!options.force
+ const existingInflight = this.#inflight.get(handle)
+ if (existingInflight && existingInflight.force === force) {
+ return existingInflight.promise
+ }
</file context>
| if (existingInflight && existingInflight.force === force) { | |
| if (existingInflight) { |
| // without round-tripping to the daemon. A cleared resident (after a | ||
| // successful get re-fetching a higher version) lets a report at a | ||
| // higher version bypass the fence again. | ||
| const lastReported = this.#reported.get(handle) |
There was a problem hiding this comment.
P1: Concurrent 401s for the same handle and recordVersion bypass this fence because the version is marked only after the await. Deduplicate in-flight reports keyed by handle and version, otherwise duplicate 401s can count as two distinct failures and enter the one-hour reauth state prematurely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/custody.ts, line 635:
<comment>Concurrent 401s for the same handle and `recordVersion` bypass this fence because the version is marked only after the await. Deduplicate in-flight reports keyed by handle and version, otherwise duplicate 401s can count as two distinct failures and enter the one-hour reauth state prematurely.</comment>
<file context>
@@ -0,0 +1,909 @@
+ // without round-tripping to the daemon. A cleared resident (after a
+ // successful get re-fetching a higher version) lets a report at a
+ // higher version bypass the fence again.
+ const lastReported = this.#reported.get(handle)
+ if (lastReported !== undefined && lastReported >= recordVersion) {
+ return
</file context>
| ((candidate: string) => | ||
| nodeOpen( | ||
| candidate, | ||
| constants.O_RDONLY | constants.O_NOFOLLOW, |
There was a problem hiding this comment.
P1: When the configured manifest path is a FIFO, this blocking open waits for a writer before isFile() can reject it, hanging manifest reads and startup. Add O_NONBLOCK so non-regular files are rejected without blocking.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/custody-manifest.ts, line 97:
<comment>When the configured manifest path is a FIFO, this blocking open waits for a writer before `isFile()` can reject it, hanging manifest reads and startup. Add `O_NONBLOCK` so non-regular files are rejected without blocking.</comment>
<file context>
@@ -0,0 +1,328 @@
+ ((candidate: string) =>
+ nodeOpen(
+ candidate,
+ constants.O_RDONLY | constants.O_NOFOLLOW,
+ ) as unknown as Promise<CustodyDescriptor>)
+
</file context>
| constants.O_RDONLY | constants.O_NOFOLLOW, | |
| constants.O_RDONLY | constants.O_NOFOLLOW | constants.O_NONBLOCK, |
| export type { AccountPaths } | ||
| export { ACCOUNT_FILE_NAME, ACCOUNT_STATE_FILE_NAME, deriveStatePath } | ||
|
|
||
| export function fallbackRefreshLockName(accountId: string) { |
There was a problem hiding this comment.
P3: fallbackRefreshLockName is unused here because runtime callers use the identical core helper. Remove this duplicate and its createHash import, or migrate all callers to one shared definition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/core/account-paths.ts, line 28:
<comment>`fallbackRefreshLockName` is unused here because runtime callers use the identical core helper. Remove this duplicate and its `createHash` import, or migrate all callers to one shared definition.</comment>
<file context>
@@ -24,6 +25,13 @@ import {
export type { AccountPaths }
export { ACCOUNT_FILE_NAME, ACCOUNT_STATE_FILE_NAME, deriveStatePath }
+export function fallbackRefreshLockName(accountId: string) {
+ return `fallback-oauth-refresh-${createHash('sha256')
+ .update(accountId)
</file context>
|
|
||
| ## Barrier | ||
|
|
||
| Entering claustrum uses the process-local custody mutex, then the renewable `claustrum-mode` lock, then renewable account locks in sorted identity order (main before fallback ids). Main custody work uses the `main-refresh` lock. A main login retains a process-local exclusion lease until host `auth.get()` readback observes the written access/refresh pair, or the 5-second readback lease expires and logs a warning before release. |
There was a problem hiding this comment.
P3: The barrier description lists the wrong account-lock order: the implementation sorts main together with fallback IDs, so some fallbacks are acquired before main. Document the lexicographic participant order to keep lock-order guidance consistent with the code and test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/docs/custody-state-machine.md, line 172:
<comment>The barrier description lists the wrong account-lock order: the implementation sorts `main` together with fallback IDs, so some fallbacks are acquired before `main`. Document the lexicographic participant order to keep lock-order guidance consistent with the code and test.</comment>
<file context>
@@ -0,0 +1,249 @@
+
+## Barrier
+
+Entering claustrum uses the process-local custody mutex, then the renewable `claustrum-mode` lock, then renewable account locks in sorted identity order (main before fallback ids). Main custody work uses the `main-refresh` lock. A main login retains a process-local exclusion lease until host `auth.get()` readback observes the written access/refresh pair, or the 5-second readback lease expires and logs a warning before release.
+
+The barrier is:
</file context>
| Entering claustrum uses the process-local custody mutex, then the renewable `claustrum-mode` lock, then renewable account locks in sorted identity order (main before fallback ids). Main custody work uses the `main-refresh` lock. A main login retains a process-local exclusion lease until host `auth.get()` readback observes the written access/refresh pair, or the 5-second readback lease expires and logs a warning before release. | |
| Entering claustrum uses the process-local custody mutex, then the renewable `claustrum-mode` lock, then renewable account locks in lexicographically sorted participant-id order. Main custody work uses the `main-refresh` lock. A main login retains a process-local exclusion lease until host `auth.get()` readback observes the written access/refresh pair, or the 5-second readback lease expires and logs a warning before release. |
| 'claustrum-golden', | ||
| 'handles.json', | ||
| ) | ||
| expect(fixturePath.endsWith('handles.json')).toBe(true) |
There was a problem hiding this comment.
P3: This golden-fixture assertion does not pin the fixture prefix or bytes; fixturePath.endsWith('handles.json') is tautological. Assert the expected prefix/content (or invoke the byte-for-byte golden check) so fixture drift cannot pass silently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/custody.test.ts, line 1589:
<comment>This golden-fixture assertion does not pin the fixture prefix or bytes; `fixturePath.endsWith('handles.json')` is tautological. Assert the expected prefix/content (or invoke the byte-for-byte golden check) so fixture drift cannot pass silently.</comment>
<file context>
@@ -0,0 +1,1784 @@
+ 'claustrum-golden',
+ 'handles.json',
+ )
+ expect(fixturePath.endsWith('handles.json')).toBe(true)
+ const source = JSON.parse(readFileSync(fixturePath, 'utf8')) as {
+ version: number
</file context>
| expires: 60_000, | ||
| } | ||
|
|
||
| expect(record.hasVerifiedInProcessMainLogin(restoredSlot)).toBe(false) |
There was a problem hiding this comment.
P3: The final test asserts record.hasVerifiedInProcessMainLogin(restoredSlot) is false at the start, but production stores the fingerprints in a module-level Set (verifiedInProcessMainLoginFingerprints in packages/opencode/src/core/custody-host-slot.ts) that is never cleared. The assertion only holds because no earlier test in the same process verified a login with exactly verified-access/verified-refresh. This is an order- and process-dependent guarantee: any future test (or rerun pattern) that records the same credential family flips this assertion silently, and if test files are ever sharded into one process the check races. Use a token family unique to this test that cannot collide, or reset the set in beforeEach.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tests/custody-authorize.test.ts, line 576:
<comment>The final test asserts `record.hasVerifiedInProcessMainLogin(restoredSlot)` is `false` at the start, but production stores the fingerprints in a module-level `Set` (`verifiedInProcessMainLoginFingerprints` in `packages/opencode/src/core/custody-host-slot.ts`) that is never cleared. The assertion only holds because no earlier test in the same process verified a login with exactly `verified-access`/`verified-refresh`. This is an order- and process-dependent guarantee: any future test (or rerun pattern) that records the same credential family flips this assertion silently, and if test files are ever sharded into one process the check races. Use a token family unique to this test that cannot collide, or reset the set in `beforeEach`.</comment>
<file context>
@@ -0,0 +1,612 @@
+ expires: 60_000,
+ }
+
+ expect(record.hasVerifiedInProcessMainLogin(restoredSlot)).toBe(false)
+ await withFreshLocalFactory(restoredSlot, async (start) => {
+ await start()
</file context>
| 'CUSTODY_OWNING_SHAPE', | ||
| 'CUSTODY_REFUSE', | ||
| 'CUSTODY_TOMBSTONE_PREFIX', | ||
| // The three vendored-client names the host needs to locate and open the vault |
There was a problem hiding this comment.
P3: The comment says "The three vendored-client names" but the block lists four, and only three (ClaustrumClient, detectClaustrumConnection, getDefaultClaustrumConnectionPath) come from the vendored client. ClaustrumCredentialCache is defined in packages/core/src/custody.ts (line 466) and reaches internal.ts via export * from './custody', not from ./vendor/claustrum-client/index, which exports neither it nor the internal.ts comment's promise that "the host needs exactly these two". The overstated grouping misleads a maintainer updating the surface list.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/tests/export-manifest.ts, line 56:
<comment>The comment says "The three vendored-client names" but the block lists four, and only three (ClaustrumClient, detectClaustrumConnection, getDefaultClaustrumConnectionPath) come from the vendored client. ClaustrumCredentialCache is defined in packages/core/src/custody.ts (line 466) and reaches internal.ts via `export * from './custody'`, not from ./vendor/claustrum-client/index, which exports neither it nor the internal.ts comment's promise that "the host needs exactly these two". The overstated grouping misleads a maintainer updating the surface list.</comment>
<file context>
@@ -45,7 +45,24 @@ export const INTERNAL_VALUE_EXPORTS = [
+ 'CUSTODY_OWNING_SHAPE',
+ 'CUSTODY_REFUSE',
+ 'CUSTODY_TOMBSTONE_PREFIX',
+ // The three vendored-client names the host needs to locate and open the vault
+ // socket. Named deliberately: the vendored client also carries transport
+ // internals that are not part of this package's surface.
</file context>
|
Merged as The manifest shim. I suggested a re-export; you used named re-exports instead, with the reason written down — The warning. Moved to the custody logger, and you also fixed the repeat I mentioned in passing — The vendor directory. Consolidated into core with Verified on the merge rather than the branch: 146 core / 1336 opencode / 14 pi, typecheck and biome clean, One thing I am recording rather than asking you to changeThe no-environment-variable rule for I checked before deciding, and I am accepting it. Those reads locate the Claustrum daemon socket, which is machine-global and identical for both hosts — it is not host store-path resolution, which is the thing the rule exists to prevent. The rule's purpose is that Pi must never resolve an OpenCode path through core; that property is intact. The file is also a byte-for-byte upstream snapshot we have committed not to edit, so bending it locally would be worse than the exception. Two corrections that follow from it, both mine: the criterion was never test-enforced — I had been verifying it with a grep and describing it as enforced — and it now needs stating as "core's own modules read no environment variable, What happens nextThis is merged but not released. The sibling in Thanks for the rebase — and for pushing back on the exit-path ruling earlier. You were right that I had named a writer that does not write, and the mode-as-declaration shape you built in its place is what the sibling's own verdict table turns out to depend on. |
2 participants
Posted from the shared
iceteaSAseat by the openai-auth Legion session.Draft on purpose. Rebased onto
0c90408and aligned to the settled custody contract; the write half (the manifest writer and the enroll verb) is a separate PR.This description was rewritten on 2026-09-18. The original described an earlier design that covered fallback accounts only, behind a per-account toggle. That is no longer what this branch does, and a stale description is worse than none — it sends a reviewer looking for a shape that is not there.
What this does
Under
claustrummode, both the main account and enrolled fallbacks are served from the Claustrum vault rather than from local secrets. The local slot holds a tombstone —{ type: 'oauth', access: '', refresh: 'claustrum-tombstone:v1:openai', expires: 0 }— and the vault owns the refresh token. One owner per token, no split-brain refresh.This matches the anthropic-auth sibling, which shipped the same shape first (cortexkit/anthropic-auth#196 and follow-ups). The handle manifest, the tombstone format and the credential-id rules are shared contracts, not parallel implementations.
Mode is global and operator-declared:
/openai-account claustrumand/openai-account local. There is no per-account enable.Main-slot custody, which is the part that is new here
migrateIfNeeded, because migration parses the main access token and a sentinel would silently losemainAccountIdon a fresh store. Under takeover, main's identity is derived from the vault credential instead.auth.type === 'oauth' && auth.refresh === custodyTombstoneKey('openai').accessandexpiresare written but are not conjuncts, so a tombstone whose other fields drifted is still recognised.claustrum-tombstone:prefix, for any provider. Refusal ⊋ recognition, and a foreign-provider tombstone proves both directions in tests.Auth.setwritesauth.jsonwhole, without locking or atomic rename, so a torn read followed by a write destroys every other provider's credentials. That path is withdrawn by construction; recovery goes throughck auth migrate-plugin.packages/opencode/docs/custody-state-machine.md, 46 rows over mode × main × fallbacks × evidence. Claustrum mode with real local material and an unavailable vault never falls back to serving locally; it stays incomplete until the vault returns.Fallbacks
A fallback listed in the handle manifest and tombstoned is served from the vault.
resolveFallbackAccessis the single place that decides which bearer a fallback sends. Refusals happen at candidate construction, never as a throw from the send, sotryFallbackAccountskeeps traversing. The request path is peek-only and never blocks on the vault; refill happens on the tick.An
enrollingaccount — manifest entry present, tombstone not yet written — serves its local token while that token is valid, and is refused rather than serving an expired one.A tombstone is sticky
applyNewerTokenStatepicks the token source bylastRefreshedAt, thenexpires. A local refresh already in flight when a takeover writes the tombstone completes afterwards, carries a later timestamp, and wins — resurrecting local material under a vault-owned account.So: while mode is
claustrum, a credential write landing on a tombstone has itsaccess,refreshandexpiresdiscarded and logs at warn; non-credential fields merge normally. Underlocalthe tombstone is no longer authoritative and a re-login writes normally. The operator's durable mode declaration is the permit, rather than a flag a caller passes — a flag is a claim any caller can make, including one still in claustrum mode, which is the ambiguity the rule exists to resolve.Manifest credential ids
credential_id.split(':')[1]must equal the provider block it sits in. The kind segment is an open set —oauth,chatgpt,apikey,antigravityare all live — so it is never allowlisted, and the label segment is never consulted. Rule and conformance rows come from Claustrum's contract text incortexkit/claustrum,docs/opencode-custody-design.md.Worth flagging for review rather than leaving in the diff: our fixtures previously used
oauth:openai:*, which is valid under that rule and wrong only because no vault record carries it — our real credential ischatgpt:openai. The conformance rows cannot catch a regression back to it, since both shapes pass all of them. The validator and the fixture correction therefore ship together, with a test pinned to the real id.Layout
This branch was ported onto the shared-core extraction.
custody.tsand the manifest parser live inpackages/core/src/; the env and XDG path resolution stayed in the host and passes a resolved path in, sorg 'process\.env' packages/core/srcstill matches nothing.custody-host-slot.ts,custody-transition.ts,custody-state.tsandcustody-runtime.tsare host-side. No compatibility wrappers, no deep imports intopackages/core/src/*, and no new core exports were needed.The account-add guard that used to live in
cli.ts— refusingaddwhile custody is active — is now inexecuteAccountCommand'saddbranch inpackages/core/src/commands.ts, so both hosts inherit it, with a separate check on the auth menu's Add account action.History note: the branch was 82 commits and is now one. Replaying those across the package extraction produced intermediate states that were meaningless by construction, since they edit files at paths that no longer exist. The full history is preserved at
backup/custody-pre-0c90408.Evidence
Core 146 pass / 0 fail. OpenCode 1332 pass / 1 skip / 0 fail. Both typechecks clean, Biome clean, order-dependence scan clean on touched test files. Pristine
upstream/mainruns 1037 in the opencode package, so this branch adds 295 there plus core's 146.The design went through four review rounds across four model families before implementation, and the defects that shaped it were mostly invisible to unit tests — they only appeared to tests entering through the loader. Each has a loader-path test, and each test was proven to go red under the mutation that reintroduces the defect.
A separate security pass ran 12 probes: no handle or credential material in logs, throws, sidebar state, RPC payloads or dumps; the sentinel never reaches an
Authorizationheader; 11/11 manifest-trust probes rejected at the expected line.What this does not do
localmode or without a handle manifest.account_idagainst the JWT claim, currently inert and skipped. The field ships on the wire but the vendored client'sServedCredentialdoes not carry it; Claustrum is widening the client and that branch goes live on the re-vendor. Flagging it because the skip's original comment claimed the wire lacked the field, which was never true.