Repository navigation
fix(claude): stop a fail-once ownership fault from deleting the credentials it just wrote - #17993
brennanb2025 wants to merge 7 commits into
Conversation
…ntials it just wrote During managed-account add, `writeOauthAccount` re-runs the ownership gate after `writeCredentials` has already written `.credentials.json`. A transient fault there (a cold WSL distro, a probe timeout, an antivirus lock) threw out of `persist` into `cleanupFailedAdd`, whose re-gate then succeeded — the fault was transient — and `rmSync`'d the whole account directory, credentials included. Three collapses made that unavoidable, and Claude had no typed errors at all, so no caller could branch on any of them: - `assertOwnedWsl` reported `timedOut` and every non-zero exit as definitive absence, and its catch-all relabelled everything (including that absence) as a trust verdict. - `resolveOwnedClaudeManagedAuthPath` returned the same `null` for a stranger's directory and for a `realpathSync` that failed. - `cleanupFailedAdd` deleted on any add failure. Adds the `owned | untrusted | indeterminate` verdict and the two error classes the Codex lane already has (mirrored, not extracted — merging the lanes is STA-5616), classifies both probes against it, and stops cleanup from deleting on an unproven observation: it leaks the directory and rethrows the original failure instead. The WSL guest now states its observation on a tagged line rather than encoding it in an exit status no caller could tell apart from `wsl.exe` failing to start. User-requested removal is unchanged and still deletes everything, keychain credentials included. Fixes STA-5674.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Typed ownership verdicts replace the catch-all. New
ClaudeManagedAuthVerdict(owned/untrusted/indeterminate) plusUntrustedManagedClaudeAuthErrorandManagedClaudeAuthTemporarilyUnavailableErrorsplit a proven trust failure from "could not look".isUnprovenManagedClaudeAuthErrorwalkscausechains so cleanup can tell them apart. - Cleanup no longer deletes on an unproven answer.
cleanupFailedAddskipsremoveManagedAuthwhen the add errored on an indeterminate probe, so the re-gate that previously authorized deleting the just-written credentials is never reached. The directory is leaked instead of the credential. - The add's own error surfaces.
runCleanupFailedAddwraps cleanup so a throw from any cleanup step never replaces the login/write error the caller must see. - WSL probe becomes a tagged, base64 protocol.
buildWslManagedAuthProbeScript/classifyWslManagedAuthProbeemit and parse a canonical-path tag instead of guessing from exit codes, so cold distros, timeouts, and spawn failures read asindeterminaterather than "not yours". - Tests reproduce the loss on real files and assert survival. Host and WSL lanes plus controls (no fault, fault at the first write, user-requested removal, cleanup-error reporting) and a fault-injection classifier suite; assertions are exact rather than loose.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
…second probe Follow-up to the #17993 review. The tri-state landed in one lane and one caller; five places still turned a failed observation into a verdict, and one turned a user's explicit request into a refusal. - Explicit removal no longer probes at all. The user's request is the authority, so a gate that cannot complete must not leave credentials on disk with nothing in the UI still pointing at them. The directory is derived lexically from the account ID instead, which is stricter than the probe it replaces: the old path deleted `resolve(canonicalAuthPath, '..')` and therefore followed a symlink out of the managed root. Automatic failed-add cleanup keeps requiring a dispositive verdict, and the two authorities are now wired to two different methods rather than sharing one. - runtime-auth had a second, separate WSL probe whose null cleared the user's active account. It now shares the verdict resolver, and sync leaves the selection alone on an indeterminate one. Once ownership is proven, the credential read reuses that path: a second probe could fail where the first succeeded and reach sync as "missing credentials", which clears the selection just the same. - The guest script proved a directory was readable and searchable before believing anything it did not find. `test -f` reports an unsearchable directory exactly as it reports an empty one, so an EACCES was arriving as a dispositive `missing-marker`. It also rejects symlinked candidates and markers now, matching the host gate. - `isDefinitiveAbsence` and the unproven-error predicate are total. Reading `.code` or `.cause` off a caught value can itself throw, which escaped the very path meant to fail closed. A shape that cannot be inspected counts as unproven, because deletion requires proof. - The host-visible WSL branch validates marker contents against the account ID and rejects symlinks, instead of accepting any directory with a marker file. The guest script's tagged-line behaviour is now covered by executing it under bash against a real temp tree, including the mode-000 cases.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Split deletion into two authorities.
ClaudeManagedAuthStoragegainedremove(user-requested: no probe, gated by account-ID spelling viaresolveOwnSpellingAccountDir) andremoveAfterFailedAdd(Orca's own cleanup: probesresolveVerdictand leaks instead of deleting onindeterminate).service.tsnow wires selection removals to the former and registration cleanup to the latter. - Shared the WSL verdict.
resolveWslManagedAuthVerdictmoved intowsl-managed-auth-probe.tsand is used by both the storage gate andruntime-auth, so the two lanes can no longer drift on what a failed probe means. - Stopped clearing the selection on a cold distro.
runtime-auth-sync.tsnow branches onresolveManagedAuthVerdict: anindeterminateverdict leaves the active account selected (warn only), while onlyuntrustedrestores the system default. A newreadManagedCredentialsAtentry point avoids re-probing a path already proven owned. - Hardened the "unproven" walk.
isUnprovenManagedClaudeAuthErrornow traverses any wrapper (not justError) with aseenset for cycles and fails closed — an unreadable shape answerstrue, which keeps the directory. - Tightened the host-visible guest path.
resolveHostVisibleGuestVerdictnow rejects symlinked candidates/markers and marker contents naming another account; the WSL probe gainedtest -hsymlink refusals and aDISPOSITIVE_TAGSallowlist. - Extracted
readErrnoCodeintodefinitive-filesystem-absence.tswith a guard so the classifier cannot throw on a hostile error shape. - Added controls and real-script coverage. New tests cover user-requested removal (foreign path not deleted, unprovable probe still deletes), cleanup after a non-ownership add failure, symlink/marker-mismatch refusals, and execute the probe script under real
bash.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
…ed predicate to STA-5616 Reverts this branch's edit to `src/shared/definitive-filesystem-absence.ts`. `349510a18e` on `brennanb2025/sta-5616-codex-wsl-ownership` already guards `isDefinitiveAbsence`'s errno read, with the reasoning written out there; two branches rewriting the same shared file only produces a conflict. This branch inherits that fix when the two land, and the reasoning is not restated here. Note the sibling keeps its `readErrnoCode` private, so the import this branch had added would not have resolved even after that change landed. What remains in-lane is the Claude-specific read: the legacy-marker adoption path classifies its own write failure, and `EEXIST` was being read straight off a caught value. A `catch` receives whatever was thrown, so a throwing `code` accessor escaped the branch meant to classify it. Only `EEXIST` is proof of a marker that is already there; anything unreadable is indeterminate. Also spelling out what the previous commit understated: replacing the ownership probe in explicit removal with a lexically derived directory is a security hardening, not a refactor. The path it replaces deleted `resolve(canonicalAuthPath, '..')` — the parent of the *canonicalised* path — so a symlink at the auth directory redirected a recursive delete to the link target's parent, outside the managed root entirely. Deriving `<managedAccountsRoot>/<accountId>` from the account ID and requiring the persisted path to spell `<root>/<accountId>/auth` bounds the delete to the directory Orca created, and recursive removal unlinks a symlink rather than following it. Refs STA-5674.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Reverted the shared predicate edit, deferring it to STA-5616.
readErrnoCodeand its test were removed fromsrc/shared/definitive-filesystem-absence.ts, restoring the file to its base state so this branch no longer rewrites a file the Codex WSL-ownership branch is also editing; the errno-read guard will land with that branch. - Guarded the adoption errno read in-lane.
resolveMarkerVerdictnow classifies the legacy-marker adoption write'sEEXISTvia a localisExistingMarkerCollision, whose try/catch prevents a hostilecodeaccessor from escaping the classifier as a throw — onlyEEXISTreads asuntrusted, everything unreadable reads asindeterminate. - Added a hostile-write fault test. The verdict test now injects a thrown value whose
codegetter throws and asserts the adoption path yieldsindeterminate, so the guard is pinned against mutation.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
The code-quality lint pass runs a rule the pre-commit hook does not, so a duplicated `node:fs` import passed locally and failed static analysis in CI.
…e probe Round 2 of review on #17993. The tri-state had reached the ownership probe and one caller; five other places still turned a failed observation into an answer, and one turned a user's request into a refusal. Enumerated every path in the lane that can produce a dispositive verdict — path checks, marker reads, credential reads, error classification, and the four decisions that clear or delete durable state — and fixed them together rather than one at a time. - The credentials read is now a result, not `string | null`. Its null said "there are no credentials", which `doSyncForCurrentSelection` acts on by clearing the user's account; a locked file, an unsearchable directory, or a failed realpath produced the same null. Both sync branches now leave the selection alone on an indeterminate read and clear only on a completed absence or invalid content. The darwin keychain read is classified the same way: its helper resolves null only for a genuine not-found, and every other failure throws. - The account path check requires exactly one segment between the managed root and `auth`. A shell `*` matches `/`, so the guest accepted `<root>/other/acct/auth` as account `acct`, and the host-side `endsWith` agreed. Orca only ever creates `<root>/<accountId>/auth`. - `isDefinitiveAbsence` guards its errno read again. Reverting it last commit was wrong: the sibling STA-5616 fix is on another branch, so this branch shipped a fail-closed predicate that throws. Written byte-identical to `349510a18e` so the merge is a no-op rather than a conflict. - The unproven-error predicate is total. `instanceof` runs a prototype lookup a Proxy can trap and throw from, so it moves inside the guard; a chain longer than the inspection depth counts as unproven, because those links were never looked at. A cycle still answers `false` — every reachable link was seen. - Explicit removal surfaces a failed unlink instead of reporting success. The caller already rolls its settings change back and rethrows, so the account returns and the user can retry; telling them it is gone while the credentials are still on disk is the same silent retention this path was fixed to stop. A root spelling that could not be canonicalised is reported too: with one spelling to compare against, "not ours" was never established. Three tests asserted a failure scenario without injecting the failure — the removal test armed a probe that no longer runs, a darwin test used a one-shot rejection an earlier call consumed, and keychain call counts accumulated across tests. Each now proves the fault reached the code under test. Refs STA-5674.
There was a problem hiding this comment.
Important
A test added in this commit fails on Linux, so the suite is red on the Linux CI leg until it is fixed.
Reviewed changes
- Made
isUnprovenManagedClaudeAuthErrorfail closed on untrappable shapes.instanceofmoved inside thetry, so a Proxy that traps prototype lookup now answers "unproven" instead of escaping the classifier, and a cause chain that exhausts the 8-link depth now also answers "unproven" rather than "proven clean". - Split the removal authority into
own/foreign/unresolvable.ClaudeManagedAuthStorage.removenow throws when the accounts root cannot be canonicalized, skips deletion on aforeignspelling, and lets anrmSyncfailure propagate soClaudeAccountSelectionrolls the settings change back instead of silently orphaning files. - Added a typed credentials read (
readClaudeManagedAuthFileResult).present/absent/indeterminatereplaces the lossynull, and runtime-auth sync now keeps the active selection onindeterminate— a locked credentials file no longer logs the user out, for host, WSL, and Darwin keychain reads. - Tightened the WSL guest-path shape check to be segment-aware. Both the JS pre-check and the guest script now require exactly one segment between the managed root and
auth, rejecting nested paths that merely share the/claude-accounts/<id>/authsuffix. - Guarded the shared absence predicate.
isDefinitiveAbsencereadserror.codethrough a localreadErrnoCodeso a hostilecodeaccessor cannot escape the catch blocks it is called from.
ℹ️ Shared predicate re-touched despite the STA-5616 deferral
The prior commit (d080890) deliberately reverted this exact edit to src/shared/definitive-filesystem-absence.ts and recorded "leave the shared predicate to STA-5616", while the PR body still states "Codex has the same shape and is untouched". This commit re-adds a private readErrnoCode helper to that shared file. The change is behavior-preserving for well-formed ErrnoExceptions (it only makes the predicate non-throwing on hostile shapes, which is strictly safer for the Codex callers too), so this is not blocking — but it reverses an explicit deferral decision and re-opens the merge-conflict surface with the in-flight Codex WSL-ownership branch. Worth confirming that's intended.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
…identity is unreadable
Item 18 of the enumeration, which I had deferred as "clears nothing durable".
That was wrong, and the review's reading of runtime-auth-sync is right.
`readManagedOauthAccount`'s null is the third disjunct of the test that decides
whether Orca restores the user's system default before dropping a managed
selection. `runtimeOauthAccountMatches(null)` is false, so a failed read did not
cause a wrong restore -- it caused NO restore, while the
`updateSettings({ activeClaudeManagedAccountId: null })` a few lines later ran
regardless. The runtime kept holding the managed account's credentials with
nothing in Orca pointing at them: not a wrong undo, a missing one.
It is also not a rare corner. The disjunct only decides when the account is
unchanged AND `hasMaterializedRuntimeAuth` is false, and the service constructor
seeds `lastSyncedAccountId` from persisted settings without materializing -- so
that is the state of the FIRST sync after every app start with a selected host
managed account, and the oauth identity is the only evidence available.
The read is now tri-state, and an indeterminate one defers the whole transition:
neither the restore nor the clear. Deferring rather than restoring-anyway is
deliberate -- `restoreSystemDefaultSnapshotForMissingManagedCredentials` consumes
the same oauth value for its own ownership checks, so running it on a failed read
would degrade exactly the checks that decide what is safe to overwrite. Malformed
JSON stays dispositive: that is a completed observation of the file.
The two teardown blocks were byte-identical once both used the hoisted read, so
they are now one method, and the selection-clearing helpers moved to their own
module to stay under the line cap.
Refs STA-5674.
…ng it from the OS CI caught this and local green could not: the removal test that asserts a refusal when the accounts root cannot be canonicalised only reaches that branch when the persisted path differs from the literal root. On macOS it does, because /var is a symlink and the ownership gate stores the canonical spelling. On Linux CI it does not, so the literal spelling matched on the first pass, ownership was genuinely established from a successful observation, and removal correctly proceeded -- the test asserted a branch that had nothing to reach it. The scenario is now constructed: userData is a symlink to a real directory, so the canonical persisted path cannot equal the literal root on any platform. Two preconditions assert that mismatch before the fault is armed, so the test cannot silently decay back into a one-spelling test. Every fault-injecting test in this file now asserts its fault actually fired. `credentialsAtFault === null` in particular was passing both when the gate saw no credentials and when the fault never ran at all. No product code changed; the behaviour CI exercised was correct. Refs STA-5674.
There was a problem hiding this comment.
Important
The new commit is clean and correct, but I am not approving: the Linux-red test flagged in the prior review (a user-requested removal that cannot resolve the accounts root refuses rather than reporting success in src/main/claude-accounts/claude-managed-auth-fail-once-credential-loss.test.ts:483) is unchanged by this commit and still fails — removeAccount resolves instead of rejecting because /tmp is not a symlink on Linux, so canonical equals lexical and the unresolvable branch is never reached. That finding is already tracked; it just still blocks merge.
Reviewed changes
- Tri-state outgoing oauth read.
readManagedOauthAccountnow delegates to a newreadManagedOauthAccountResult, which returnspresent/absent/indeterminate(ClaudeManagedOauthRead), so a failed read of the outgoing account'soauth-account.jsonis distinguishable from a genuinely missing one. - Defer rather than half-complete a teardown. The two byte-identical host teardown blocks in
runtime-auth-sync.tsare unified intorestoreDefaultAndDropHostSelection, which returns without restoring or clearing when the outgoing oauth read isindeterminate, leaving the whole transition to the next sync instead of clearing the selection while the runtime still holds the account's credentials. - Selection helpers extracted.
clearClaudeSelectionForTargetandshouldDeferOnUnreadableOauthmoved toruntime-auth-selection-teardown.tsand are now shared by the WSL and host lanes. - Fault-consumption-guarded tests. Three new host-lane cases (locked oauth read defers, malformed oauth JSON still clears, switch with unreadable outgoing defers) each assert a
> 0hit on the injected fault so an armed-but-unconsumed fault fails loudly.
The deferral semantics hold up on inspection: an untrusted verdict or malformed JSON both classify as absent (dispositive, so the clear proceeds), while only indeterminate defers, and previousManagedOauthRead is non-null only when previousAccount exists, so a first-ever sync with no prior account still clears. The three new tests pass (17/17 in the file).
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Deterministic two-spelling removal fixture. The previously Linux-red test now builds the canonical/lexical divergence itself — a real
userDatadirectory symlinked behind a sibling path — instead of inheriting it from the OS (/tmpis a symlink on macOS but not Linux). A precondition asserts the persisted path is canonical and the literal root is not a prefix of it, so the test cannot silently decay back into a one-spelling no-op. - Fault-consumption guards.
realpathHits,rmHits, andfaultsFiredcounters assert the injected fault was actually consumed (> 0) rather than merely armed, andextraTempDirsclean up the fixture on teardown.
The full file now passes 11/11. This resolves the sole outstanding finding from the prior review.
DeepSeek Pro (free via Pullfrog for OSS) | 𝕏
Review status: completeThree review rounds plus a CI-caught defect. All findings resolved; CI green (17 checks, 0 failures). What this fixes. The Claude managed-auth lane treated a failed filesystem observation as a dispositive one — "I could not look" and "the answer is no" shared a representation, and the second can clear a user's account selection or delete credentials. Every path that can produce such a verdict was enumerated (27 sites), and each now states what it does on a genuine negative answer versus on a failed operation. Only a dispositive verdict may clear state. Verification. 17 guards mutation-tested, each mutated at a single asserted anchor with the file byte-compared after restore; all caught. Four survived a first pass, and in every case the guard was load-bearing and the test was not discriminating — a surviving mutant is now read as a suspect test rather than a redundant guard. Two evidence failures are documented in the description, because they are the same category error as the bug: a verdict drawn from an uncontrolled comparison. One compared a batch test run against a solo one; the other let the operating system supply a test's precondition, producing a green that meant nothing on macOS and a red in CI. The second generalises: mutation-testing a guard proves the test discriminates on the machine you ran it on. That mutant was reported "caught" on macOS while the test it validated was unreachable on Linux — so the mutation evidence inherited the same blind spot. Anything whose reachability depends on path spelling is now exercised under both spellings. Not in scope, filed separately: STA-6379 (legacy WSL drain scripts can overwrite an active |
|
Closing: superseded by #26801 (per-account Claude folders), which removed the credential-write path this PR guarded. |

Fixes STA-5674.
Adding a Claude account on Windows could delete the account — and the credentials it had just
saved — because of a single momentary hiccup.
What changes
writes the OAuth record. If the ownership check in between failed once — antivirus holding a
file, a WSL distro waking up, a filesystem blip — cleanup ran, re-checked, succeeded the second
time, and deleted the whole account folder including the credentials from a moment earlier. A
check that did not complete is now treated as "could not tell", and Orca does not delete on
"could not tell".
— a timeout, a distro that was not running, a process that could not start — was reported as
though Orca had positively determined the folder did not belong to your account. Those are now
distinct answers, and only a real answer can trigger cleanup.
the error you see is the reason the add failed, not a confusing message about cleanup.
failure. When you ask for an account to be removed, it is removed, keychain entry included.
ELI5
Saving an account is two steps: write the password, then write the account record. Before each
step Orca checks "is this folder really mine?"
If that check failed on the second step, Orca assumed the worst and tidied up — deleted the folder.
But the check failing did not mean the folder was not Orca's; it often just meant Orca could not
reach it for a moment. And by then the password was already in there. So a momentary glitch threw
away the thing that had just been saved.
Now Orca separates "I checked, and it is not mine" from "I could not check". It only cleans up for
the first one. If it cannot tell, it leaves the folder alone — a leftover folder is easy to remove
later, a deleted password is not.
Why it was possible
The Windows/WSL ownership probe collapsed every outcome into one error string. A 5-second timeout, a
non-zero exit, a spawn failure and a genuine "this is not yours" were indistinguishable, and the
catch-all then relabelled even the "does not exist" case as a trust failure. Nothing downstream
could tell them apart because the Claude account code had no typed errors at all — so cleanup, which
should only ever run on a proven answer, ran on any answer.
The delete itself was correctly gated: it re-checks ownership first. That is why a persistent
fault was safe and only a transient one caused loss — the retry succeeded and the gate opened.
Testing
Reproduced before fixing: the test demonstrated the deletion against real files on unfixed code,
then flipped to asserting survival. It covers both the WSL lane and the host lane, so the result is
not an artifact of Windows-specific mocking, and it carries two controls — no fault means no loss,
and injecting the fault at the first write proves nothing because no credentials exist yet.
Every new guard was mutation-tested: reverted one at a time, with the specific assertion confirmed
failing without it, and each mutation confirmed to have actually applied before its result was
trusted.
tsc --noEmitclean.What this does not do
Codex lane's naming rather than sharing a module with it; extracting the common piece is
STA-5616 and belongs in its own change.
how long Orca waits.
A note on how the test evidence itself was checked
Two findings from this branch came from the tests lying rather than the product, and both are the
same category error the ticket is about — a verdict drawn from a comparison that was not controlled.
A batch run compared against a solo run.
git-command-timeout-behavior.test.tsfailed inside asix-file batch and passed when re-run at the merge base — but the merge-base run was solo. Read
naively that is "my change broke it". Run apples-to-apples, solo, it passes three times out of three
both at base and with the change: the failure is co-execution only, on a wall-clock deadline
assertion, from a file with no import path to this diff. The difference was in the harness, not the
code.
A test whose precondition came from the operating system. A removal test asserted a refusal when
the accounts root cannot be canonicalised, but only reaches that branch when the persisted path
differs from the literal root. macOS supplies that difference for free —
/varis a symlink and theownership gate stores the canonical spelling — so the test passed locally while asserting nothing on
Linux, where it failed in CI. It now builds the two-spelling situation itself, with preconditions
asserted before the fault is armed so it cannot decay back.
The general lesson applied throughout: mutation-testing a guard proves the test discriminates on
the machine it ran on. Where reachability depends on the filesystem, that has to be checked under
both spellings, and every fault-injecting test asserts the fault was actually reached rather than
only that the outcome looked right.