fix(runtime): narrow the self-overlay guard to the dir being rebuilt - #139
Conversation
#137 refused any CLAUDE_CONFIG_DIR under cue's runtime tree. That is wider than the bug: the corruption needs source === the dir being rebuilt, and under an authmux account the two differ. The outer session execs the child with <runtime>/<profile>@account2/claude; the nested launch's own target is <runtime>/<profile>/claude, because authmuxAccountTag() returns undefined for a runtime path. Source never equalled target there — and that overlay is the only thing carrying account2's credentials into the child. #137 rejected it and fell back to ~/.claude, silently running the nested agent as account1. Reproduced against main: `B authmux run -> ~/.claude`. isSelfOverlaySource() replaces isCueRuntimeDir(): an exact resolved-path comparison against the runtime dir this launch will write, threaded through pickClaudeCredentialsSource / resolveClaudeCredentialsSource. Callers that are not rebuilding a runtime omit it and keep the plain CLAUDE_CONFIG_DIR answer. launch.ts resolves accountTag/runtimeKey before the credentials source — a pure reorder, since authmuxAccountTag(ccd, homedir()) never depended on it. Also closes #137's second finding: its tests all targeted the pure predicate, so deleting the guard kept them green. The new pickClaudeCredentialsSource block drives CLAUDE_CONFIG_DIR directly and is mutation-checked — replacing the guarded return with an unconditional one fails "refuses CLAUDE_CONFIG_DIR when it is the dir being rebuilt". Probe across all three shapes: self-overlay -> ~/.claude (the #137 fix, kept); authmux-nested -> the account2 runtime (regression undone); plain authmux dir -> unchanged. Full suite 33 fail on branch and base, failing sets byte-identical.
NagyVikt
left a comment
NagyVikt
left a comment
There was a problem hiding this comment.
🛡️ GitGuardex code-assist
2 finding(s) — 🟠 1 high · 🔵 1 low
⛔ Merge gate: blocked — 1 blocking finding(s) (blocks on high/critical).
| Severity | Location | Finding |
|---|---|---|
| 🟠 high | src/lib/runtime-install.ts:134 |
Making the self-overlay guard opt-in via options.runtimeDir re-opens #137's corruption for cue sync and cue install, which rebuild |
| 🔵 low | src/lib/runtime-install.test.ts:123 |
This test's fall-through path depends on the host machine's real $HOME and can spawn an external authmux subprocess, making a unit test |
Provider claude · commit eb19aca
| // rebuild. Falling through then reaches a source outside that dir. | ||
| const envConfigDir = process.env.CLAUDE_CONFIG_DIR; | ||
| if (envConfigDir && !isCueRuntimeDir(envConfigDir)) return envConfigDir; | ||
| if (envConfigDir && !isSelfOverlaySource(envConfigDir, options.runtimeDir)) return envConfigDir; |
There was a problem hiding this comment.
Warning
🟠 HIGH · correctness
Making the self-overlay guard opt-in via options.runtimeDir re-opens #137's corruption for cue sync and cue install, which rebuild runtimes but never pass a runtimeDir. Before this change the guard was unconditional inside pickClaudeCredentialsSource(), so it covered every caller.
Why this matters
Now src/commands/sync.ts:146 and src/commands/install.ts:249 both call resolveClaudeCredentialsSource({ healFromRuntime: false }) with no runtimeDir, yet both feed the result straight into prepareRuntime(), whose runtime dir is join(configDir(), "runtime", runtimeKey ?? profile.name, "claude") — exactly what CLAUDE_CONFIG_DIR points at when the command is run from inside a cue-launched Claude session (the normal case). credentialsSource then equals the dir being rebuilt, and overlaySourceState() (src/lib/runtime-materializer.ts:1138-1153) rms each target entry and re-symlinks it to the identical path, producing the self-referential links #137 documented; worse, .credentials.json takes the isCopyFile branch, so the rm deletes it and the subsequent copyFile(sourcePath, targetPath) fails with ENOENT into a silent catch, leaving the runtime with no credentials at all. This fires on both the warm path (line 229) and the rebuild path, and cue sync drops the hash first so the rebuild always runs. Fix by threading the target dir at both call sites, e.g. runtimeDir: runtimeDirFor(key, agent) in sync.ts and runtimeDirFor(profileName, agent) in install.ts, and add a regression test for at least one of them.
|
|
||
| test("refuses CLAUDE_CONFIG_DIR when it is the dir being rebuilt", async () => { | ||
| process.env.CLAUDE_CONFIG_DIR = target; | ||
| expect(await pickClaudeCredentialsSource({ runtimeDir: target })).not.toBe(target); |
There was a problem hiding this comment.
Note
🔵 LOW · tests
This test's fall-through path depends on the host machine's real $HOME and can spawn an external authmux subprocess, making a unit test environment-coupled. Once the guard rejects CLAUDE_CONFIG_DIR, pickClaudeCredentialsSource checks existsSync(join(homedir(), ".claude", ".credentials.json")) and, when that is absent (typical in CI), runs spawnSync("authmux", ["parallel", "--list", "--json"]) with a 3s
Why this matters
timeout and may write to stderr. The .not.toBe(target) assertion happens to hold in every environment, so this is not a flake today, but the test exercises real filesystem and process state rather than the guard alone; injecting the home dir (or asserting against a fixture root) would keep it hermetic.
NagyVikt
left a comment
NagyVikt
left a comment
There was a problem hiding this comment.
🛡️ GitGuardex code-assist
3 finding(s) — 🟡 2 medium · 🔵 1 low
✅ Merge gate: pass — no blocking findings (blocks on high/critical).
| Severity | Location | Finding |
|---|---|---|
| 🟡 medium | src/commands/launch.ts:1983 |
A nested launch under an authmux account now writes that account's credentials into the UNTAGGED runtime dir, defeating the per-account |
| 🟡 medium | src/lib/runtime-install.ts:115 |
Narrowing to an exact-path test admits every OTHER runtime dir as an overlay source, which chains runtime dirs together and can still form |
| 🔵 low | src/lib/runtime-install.test.ts:128 |
The one test that exercises the guard's fall-through runs the real, environment-dependent resolution chain, so it is non-hermetic and |
Provider claude · commit eb19aca
| // recently as a proxy for "the one you actually use." | ||
| const credentialsSource = agentKind === "claude-code" | ||
| ? await resolveClaudeCredentialsSource({ | ||
| runtimeDir: runtimeDirFor(runtimeKey, "claude-code"), |
There was a problem hiding this comment.
Important
🟡 MEDIUM · correctness
A nested launch under an authmux account now writes that account's credentials into the UNTAGGED runtime dir, defeating the per-account isolation runtimeKey exists for.
Why this matters
The inherited CLAUDE_CONFIG_DIR in that case is <runtime>/<profile>@account2/claude; authmuxAccountTag() only recognizes ~/.claude-accounts[-sessions]/<name> prefixes (launch.ts:1536-1541), so it returns undefined for a runtime path and runtimeKey collapses back to the bare profileName. The target passed here is therefore <runtime>/<profile>/claude — the same dir a plain account1 launch of that profile uses — and because source ≠ target the guard lets the overlay copy account2's .credentials.json there and swap .claude.json identity (runtime-materializer.ts:1103-1116, 1145-1150). That is exactly the "two authmux accounts alternate on one profile" thrash the account-identity guard comments describe (runtime-materializer.ts:789-802): each alternating launch discards the other account's per-profile session state, and a concurrently running account1 session has its live config dir re-identified underneath it. On the base branch the fallback wrote account1's creds into account1's dir, so this cross-account write is new. Fixing it needs the tag derived from a <profile>@<tag> runtime path AND the source resolved to that account's real dir (uuid-matched via listKnownAccountDirs), otherwise re-tagging alone reintroduces #137's regression through the sameAccount reseed.
| return target === root || target.startsWith(root + sep); | ||
| export function isSelfOverlaySource(dir: string, runtimeDir: string | undefined): boolean { | ||
| if (!runtimeDir) return false; | ||
| return resolve(dir) === resolve(runtimeDir); |
There was a problem hiding this comment.
Important
🟡 MEDIUM · correctness
Narrowing to an exact-path test admits every OTHER runtime dir as an overlay source, which chains runtime dirs together and can still form the self-referential symlink cycle #137 fixed — one hop deeper.
Why this matters
overlaySourceState() links each unmanaged entry with symlink(join(sourceDir, name), join(targetDir, name)) (runtime-materializer.ts:1153), so launching profile B from inside a profile-A session (a normal cwd-resolved nested launch) leaves B/sessions -> A/sessions, B/history.jsonl -> A/history.jsonl, etc. Two consequences follow. (1) A→B→A nesting is permitted — MAX_LAUNCH_DEPTH is 3 and blocks only at depth ≥ 3 (launch.ts:1643-1652) — and the third launch rebuilds A from B, leaving A/x -> B/x -> A/x, an ELOOP cycle that the exact-path check cannot see. (2) Runtime GC is on by default at 30 days (DEFAULT_GC_DAYS = 30, runtime-gc.ts:27; swept from maybeAutoGc at launch.ts:2742), so pruning A's runtime dangles every link B holds into it. The base branch's subtree check made both impossible. A guard that still permits the authmux case would reject any source under the runtime root except the <profile>@<tag> sibling of the dir being written, rather than only the identical path.
|
|
||
| test("keeps an authmux per-account CLAUDE_CONFIG_DIR", async () => { | ||
| process.env.CLAUDE_CONFIG_DIR = "/home/u/.claude-account2"; | ||
| expect(await pickClaudeCredentialsSource({ runtimeDir: target })).toBe("/home/u/.claude-account2"); |
There was a problem hiding this comment.
Note
🔵 LOW · tests
The one test that exercises the guard's fall-through runs the real, environment-dependent resolution chain, so it is non-hermetic and cannot distinguish why the guard fired.
Why this matters
Once the guard rejects CLAUDE_CONFIG_DIR, pickClaudeCredentialsSource checks the developer's actual ~/.claude/.credentials.json and, when that is absent, spawnSync("authmux", ["parallel", "--list", "--json"]) with a 3s timeout (runtime-install.ts:136-166) — a real subprocess plus statSync on whatever account dirs that machine has. The assertion .not.toBe(target) passes for any of those outcomes, including ones unrelated to the guard, and the result differs between a dev box with authmux installed and CI without it. Pinning the fallback (injecting the home dir / authmux lookup, or asserting against a temp HOME) would make this the regression test the notes claim it is.
…140) #139 made the self-overlay guard opt-in via options.runtimeDir. launch.ts passes it; `cue install` and `cue sync` do not — and both rebuild runtimes through prepareRuntime(). So either command run from inside a cue session still took CLAUDE_CONFIG_DIR, its own runtime dir, as the overlay source and reproduced #137's self-referential symlinks. The guard was off exactly where the materialization happens. Both callers now pass the dir they are about to write: runtimeDirFor(profile.name, agent) in install.ts, matching prepareRuntime's own `runtimeKey ?? profile.name` default, and runtimeDirFor(key, agent) in sync.ts, where `key` is already the runtimeKey passed two lines below. This was raised on #139 and committed there as 91c6601d, but never reached the remote before that PR merged — main got the narrowing alone. Confirmed on origin/main afterwards: isSelfOverlaySource present, `runtimeDir:` in install.ts/sync.ts absent. Cherry-picked here onto the post-#139 main. Also carries the comment answering #139's LOW: the wiring test asserts not.toBe(target) rather than a concrete fall-through path because os.homedir() reads the passwd entry, not $HOME, so a temp-HOME fixture does not pin the branch (measured — it still resolved the real ~/.claude). No fall-through branch can return the target, so the assertion holds anywhere and stays mutation-proof. Full suite 33 fail on branch and base, failing sets identical both ways. Co-authored-by: NagyVikt <nagy.viktordp@gmail.com>
1 participant
Summary
Test plan