fix(launch): don't force the picker for a nested non-TTY launch - #132
Conversation
cue relocates CLAUDE_CONFIG_DIR to the per-profile runtime dir when it launches an agent, so every child spawned inside a cue session inherits a non-default CLAUDE_CONFIG_DIR and trips the account-alias branch. That branch forced the picker with no TTY guard — unlike the CUE_ALWAYS_PICK branch right next to it — so a nested `claude -p …` (gitguardex's AI review gate, a hook, any script) hit a picker it could not answer and exited 1 on "no profile resolved and stdin is not a TTY". Same class of bug launchDepth already fixed for CUE_LAUNCHING, whose doc comment names an AI code review as the motivating nested launch, and with the same tell: callers hand-stripping the env var to get through (failures.ts:349-353, and the launch.e2e.test.ts harness). - shouldForcePicker(): --cue-pick still always wins; CUE_ALWAYS_PICK and account-alias now both require a TTY. Collapses the duplicated `!override && isTTY` conditions into one tested place. - shouldInheritSessionProfile(): off a TTY with nothing resolved, fall back to the session's own profile via the existing detectActiveProfile() instead of exiting 1. A cwd pin / repo-default / global-default still wins, and --cue-pick opts out — a picker that cannot open is a real error, not something to paper over. Verified end to end: `echo "" | cue launch claude -p "…"` inside a cue session went from the hard error to resolving the session profile and answering. Full suite 3088 pass / 9 fail, and the same 9 fail with the touched files reverted to base — failing set unchanged.
NagyVikt
left a comment
There was a problem hiding this comment.
GitGuardex code-assist found 5 issue(s).
| inheritedProfile = detectActiveProfile(); | ||
| if (inheritedProfile) debug("launch:inherited-session-profile", inheritedProfile); | ||
| } | ||
| const resolved = inheritedProfile |
There was a problem hiding this comment.
MEDIUM Inheriting the live session's profile makes a nested non-TTY launch re-enter materialization for the runtime dir the parent agent is actively using. The runtime dir is keyed by profile name only (runtime/<profile>/claude), but the project loadout is computed from the launch cwd/pinDir — and the inheritance branch only fires when cwd resolved nothing, i.e. a cwd whose signals almost certainly differ from the parent's pinned repo. Different loadout → different .cue-hash → materializeRuntime does rename(runtimeDir, trash) + rename(tmp, runtimeDir) + rm -rf trash (runtime-materializer.ts:846-853) under the running parent session. Failure scenario: a cue session is running profile P pinned in ~/repo-a; it spawns claude -p (gitguardex's review gate) with cwd ~/scratch (no pin, no repo-default); the child inherits P, computes a smaller loadout, and swaps runtime/P/claude out from under the live parent, which then reads a skills/ tree and .claude.json that no longer contain what it was launched with.
Suggested fix:
| const resolved = inheritedProfile | |
| When the inherited profile equals the profile the current CLAUDE_CONFIG_DIR runtime belongs to, skip the rebuild path — e.g. reuse the parent runtime as-is (exec with the inherited CLAUDE_CONFIG_DIR) or force the cache-hit path by reusing the parent's loadout/pinDir instead of recomputing it from the child's cwd. |
| expect(shouldForcePicker({ ...base, isAccountAlias: true, isTTY: false })).toBe(false); | ||
| }); | ||
|
|
||
| test("CUE_ALWAYS_PICK does NOT force the picker off a TTY", () => { |
There was a problem hiding this comment.
LOW shouldForcePicker's new tests cover only negative cases for CUE_ALWAYS_PICK; nothing asserts that it still forces the picker on a TTY. Deleting the isAlwaysPickEnabled(opts.alwaysPickEnv) || term from the return would leave the whole suite green (the pre-existing isAlwaysPickEnabled tests only exercise the string parser, not the decision). Since this refactor moved that behavior into the new helper, the positive case is now untested anywhere.
Suggested fix:
| test("CUE_ALWAYS_PICK does NOT force the picker off a TTY", () => { | |
| Add: expect(shouldForcePicker({ ...base, alwaysPickEnv: "1" })).toBe(true); |
| })) { | ||
| const { detectActiveProfile } = await import("./summon"); | ||
| inheritedProfile = detectActiveProfile(); | ||
| if (inheritedProfile) debug("launch:inherited-session-profile", inheritedProfile); |
There was a problem hiding this comment.
LOW Session inheritance is silent — the only trace is debug(), which is off by default. A non-interactive launch in a directory with no pin/repo-default/global-default previously failed loudly with an actionable message; it now runs under whatever CUE_PROFILE is ambient in the environment (any shell descended from a cue session keeps it after cd elsewhere), with no indication of which profile was used or why. Failure scenario: a script run from a shell inside a cue session materializes and execs a heavy inherited profile (MCPs, skills) in an unrelated repo, and the user has no output explaining where the profile came from.
Suggested fix:
| if (inheritedProfile) debug("launch:inherited-session-profile", inheritedProfile); | |
| Mirror the loadout/rebuild lines and write one stderr note, e.g. `[cue] no profile for this directory — inheriting session profile "<name>"`. |
| }); | ||
| const resolvedForCwd = forcePicker ? { source: "none" as const } : existingResolved; | ||
| let inheritedProfile: string | null = null; | ||
| if (shouldInheritSessionProfile({ |
There was a problem hiding this comment.
LOW The regression this PR fixes is not locked at the integration level. Both helpers are pure and unit-tested, but the wiring in run() — the detectActiveProfile() import, { source: "session" }, and the interaction with the account-alias detection that caused the bug — has no test, and the existing e2e harness still deletes CLAUDE_CONFIG_DIR (launch.e2e.test.ts:35) precisely to dodge this path. A future refactor that reintroduces an unconditional force-picker on account alias would keep every test green; per notes.md the end-to-end proof was manual only.
Suggested fix:
| if (shouldInheritSessionProfile({ | |
| Add an e2e case that runs `cue launch claude --cue-dry-run` non-TTY with CLAUDE_CONFIG_DIR set to a runtime path and CUE_PROFILE set, asserting exit 0 and the inherited profile — and drop the now-unneeded `delete cleanEnv.CLAUDE_CONFIG_DIR` workaround (and the one at failures.ts:353) so the harness exercises the real environment. |
| } | ||
|
|
||
| /** | ||
| * Whether to fall back to the running session's profile (`CUE_PROFILE`, or the |
There was a problem hiding this comment.
LOW docs/launch.md "Resolve precedence" enumerates the stop-at-first-match order as flag → .cue.profile → repo-defaults.json → default-profile → picker. This adds a sixth, non-obvious step (off a TTY with nothing matched, fall back to CUE_PROFILE / the runtime path in CLAUDE_CONFIG_DIR) and changes step 5 (the picker no longer opens off a TTY for account aliases), so the documented flow no longer matches the code — AGENTS.md points readers at that file as the source for the resolve→materialize→exec flow.
Suggested fix:
| * Whether to fall back to the running session's profile (`CUE_PROFILE`, or the | |
| Add the session-inheritance fallback to the precedence list in docs/launch.md and note that the alias/CUE_ALWAYS_PICK picker triggers are TTY-only. |
NagyVikt
left a comment
There was a problem hiding this comment.
GitGuardex code-assist found 3 issue(s).
| const forcePicker = parsed.forcePick || alwaysPick || (isAccountAlias && !parsed.override); | ||
| const resolved = forcePicker ? { source: "none" as const } : existingResolved; | ||
| const isTTY = process.stdin.isTTY === true; | ||
| const forcePicker = shouldForcePicker({ |
There was a problem hiding this comment.
MEDIUM The behavior this PR fixes is not locked by any test. Both new helpers are unit-tested in isolation, but nothing exercises the composition in run(): that isTTY is derived from stdin (not stdout), that isAccountAlias is passed through, or that an account-alias + non-TTY launch now resolves instead of exiting 1. Reverting this whole hunk to the old forcePicker = parsed.forcePick || alwaysPick || (isAccountAlias && !parsed.override) line leaves all 9 new assertions green. The one harness that could catch it, launch.e2e.test.ts:35, still does delete cleanEnv.CLAUDE_CONFIG_DIR specifically to dodge this path, so the workaround the notes call out is still hiding the regression.
Suggested fix:
| const forcePicker = shouldForcePicker({ | |
| Add a run()-level test (or drop the `delete cleanEnv.CLAUDE_CONFIG_DIR` in launch.e2e.test.ts and assert the launch succeeds) with CLAUDE_CONFIG_DIR pointed at a cue runtime dir, stdin non-TTY, and CUE_PROFILE set — asserting exit 0 and the inherited profile, not the 'no profile resolved and stdin is not a TTY' error. |
| expect(shouldForcePicker({ ...base, isAccountAlias: true, isTTY: false })).toBe(false); | ||
| }); | ||
|
|
||
| test("CUE_ALWAYS_PICK does NOT force the picker off a TTY", () => { |
There was a problem hiding this comment.
LOW shouldForcePicker's CUE_ALWAYS_PICK trigger is only asserted in its negative form (off-TTY → false). No test asserts {...base, alwaysPickEnv: "1"} on a TTY returns true, and no test asserts the bare base returns false. Deleting isAlwaysPickEnabled(opts.alwaysPickEnv) || from launch.ts:1598 would silently disable CUE_ALWAYS_PICK entirely while every test in this file still passes (the isAlwaysPickEnabled describe above only covers the env parser, not that shouldForcePicker consults it).
Suggested fix:
| test("CUE_ALWAYS_PICK does NOT force the picker off a TTY", () => { | |
| Add: `expect(shouldForcePicker({ ...base, alwaysPickEnv: "1" })).toBe(true)` and `expect(shouldForcePicker(base)).toBe(false)`. |
| isTTY, | ||
| })) { | ||
| const { detectActiveProfile } = await import("./summon"); | ||
| inheritedProfile = detectActiveProfile(); |
There was a problem hiding this comment.
LOW The inherited name is used verbatim as a profile name, but detectActiveProfile's CLAUDE_CONFIG_DIR fallback returns the runtime directory segment, which is runtimeKey, not profileName. Under authmux per-account isolation runtimeKey is ${profileName}@${accountTag} (launch.ts:1958) and the child's CLAUDE_CONFIG_DIR is <configDir>/runtime/<profile>@<tag>/claude, so summon.ts:102's /\/cue\/runtime\/(.+?)\/claude\/?$/ captures core+foo@acct and loadProfile fails with a confusing 'profile not found' instead of launching. Narrow in practice — cue sets CUE_PROFILE on the child (launch.ts:307) and that wins — so it only bites when something strips CUE_PROFILE while keeping CLAUDE_CONFIG_DIR, which is exactly the case this fallback exists to serve.
Suggested fix:
| inheritedProfile = detectActiveProfile(); | |
| Strip the account tag before using the fallback, e.g. `inheritedProfile = detectActiveProfile()?.split("@")[0] ?? null` (or have detectActiveProfile do it, mirroring authmuxAccountTag's `profile@tag` separator contract). |
A nested `cue launch` of the profile already running destroyed its own runtime dir: 69 self-referential symlinks — sessions/, projects/, history.jsonl, keybindings.json, .session-stats.json and the rest, each pointing at its own path — and a runtime that answered "Not logged in · Please run /login" until the next launch rewrote .credentials.json. pickClaudeCredentialsSource() returned CLAUDE_CONFIG_DIR unconditionally. cue points that variable at <configDir>/runtime/<profile>/claude when it launches an agent, so every process spawned inside a cue session inherits it, and a nested launch ran with credentialsSource === runtimeDir. overlaySourceState() then linked each unmanaged entry to <runtimeDir>/<name>, and step 6's tmp→runtimeDir rename left every link pointing at itself. .credentials.json survived only because step 6 moves it across explicitly. An explicit CLAUDE_CONFIG_DIR still wins — that is how authmux hands cue a per-account config — but not when it names cue's own runtime tree. Falling through reaches the existing ~/.claude / authmux ladder, i.e. a source outside the dir being rebuilt. The check is an exported pure helper, matching this file's style, and compares resolved paths on a `root + sep` boundary so a sibling like runtime-backup/ stays usable. cue #132 is what unmasked this: before it, a nested non-TTY launch died at the picker and never reached the materializer. Verified with the exact env a nested launch inherits — before: `nested -> /tmp/probe-cfg/cue/runtime/some+profile/claude`; after: `nested -> ~/.claude`; `authmux -> ~/.claude-account2` unchanged on both. Full suite branch 33 fail vs base 34 in the same shell, failing-set diff empty (the one differing name passes 2/2 in isolation on both sides — flake, not a fix). Co-authored-by: NagyVikt <nagy.viktordp@gmail.com>
Summary
Test plan