feat: registry-optional spawn/resume with raw CLI fallback + parity CI (#392) - #453
Conversation
The repoGolem launcher registry was mandatory at both doors, violating the
AGENTS.md law that a fresh install has none of Etan's launchers:
- spawnPreflight threw when ~/.config/ralphtools/launchers.zsh was absent or
the repo was unregistered, so a brew-install user could not spawn anything.
- buildResumeCommand fell back to a NAIVE `${repo}${Suffix}` guess when no
launcher_name was recorded, emitting resume commands that point at binaries
which do not exist. Crash recovery inherited this.
The registry is now an optional enhancement:
- New non-throwing registry probe (loadLauncherRegistrySnapshot) plus
resolveLauncherNameFromRegistryOrNull / resolveRepoRootFromLauncherRegistryOrNull.
A malformed registry (ambiguous alias, relative path) still throws -- broken
config is not the same as absent config.
- New src/repo-root-fallback.ts resolves a cwd from the repo param alone, in a
documented order (CMUXLAYER_REPO_HOME, the running checkout, its sibling,
~/Gits, ~), and names every path it searched when it misses.
- buildLaunchCommand gained a raw mode emitting `cd <root> && <cli> <skip-flag>`
with flags verified against each CLI's --help.
- buildResumeCommand emits the raw form when no launcher was recorded, and
carries a cd so cwd-keyed harnesses actually find their session.
- CMUXLAYER_REQUIRE_LAUNCHER_REGISTRY=1 restores the old hard failure.
Registered installs are untouched: "raw" is only ever requested by preflight
when the registry does not answer, and every stubbed-preflight caller keeps
the launcher default.
Parity CI: tests/spawn-resume-parity.test.ts runs the spawn/resume contract
through both lanes for claude/codex/cursor/gemini and asserts the receipts are
identical apart from launcher_name. A launcher-parity CI matrix runs it on a
runner with no registry (a real fresh install) and with one planted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a13e2e4a-8376-45d9-aaca-6f5a19ca376f) |
|
Warning Review limit reached
Next review available in: 27 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const cwd = resumeCwdForAgent(record); | ||
| // Never advertise a cwd-keyed raw resume we cannot aim: it would start a | ||
| // fresh session under the same command, which reads as a successful resume. | ||
| if (!record.launcher_name && !cwd && rawResumeNeedsCwd(record.cli)) { |
There was a problem hiding this comment.
🟠 High src/agent-facade.ts:45
An invalid nonempty launcher_name makes resumeCommandForAgent advertise a cwd-less raw resume for cwd-keyed CLIs, so claude, cursor, or gemini can start a new session while resumable is reported as true. The guard checks the raw value at line 45, but buildResumeCommand checks the cleaned value and falls back to raw mode; gate on cleanLauncherName(...) or otherwise distinguish that fallback.
Also found in 2 other location(s)
src/agent-command.ts:90
buildResumeCommandfalls back to a raw resume whenevercleanLauncherNamerejects a nonempty recorded launcher. Callers such asresumeCommandForAgentonly suppress cwd-keyed raw resumes whenlauncher_nameitself is absent, so a legacy/corrupt value like"bad value"with no known cwd bypasses that guard and returns cwd-lessclaude/cursor/geminiresume commands. Those commands can start a new session while being advertised as a valid resume. The fallback must expose whether it selected raw mode, or the caller must gate on the cleaned launcher value.
src/agent-engine.ts:8089
resolveAgentRoutenow delegates toresumeCommandForAgent, whose cwd-safety check treats any nonemptylauncher_nameas a usable launcher. For a legacy record with an invalid nonempty launcher such as"bad name", nolaunch_cwd/worktree_path, and a cwd-keyed CLI,buildResumeCommandrejects the launcher internally and falls back to a raw resume withoutcd. The route therefore reportsresumable: trueand advertises a command that can start a new session instead of resuming. The gate must use the cleaned/usable launcher result (or otherwise distinguish raw fallback), not truthiness oflauncher_name.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-facade.ts around line 45:
An invalid nonempty `launcher_name` makes `resumeCommandForAgent` advertise a cwd-less raw resume for cwd-keyed CLIs, so `claude`, `cursor`, or `gemini` can start a new session while `resumable` is reported as `true`. The guard checks the raw value at line 45, but `buildResumeCommand` checks the cleaned value and falls back to raw mode; gate on `cleanLauncherName(...)` or otherwise distinguish that fallback.
Also found in 2 other location(s):
- src/agent-command.ts:90 -- `buildResumeCommand` falls back to a raw resume whenever `cleanLauncherName` rejects a nonempty recorded launcher. Callers such as `resumeCommandForAgent` only suppress cwd-keyed raw resumes when `launcher_name` itself is absent, so a legacy/corrupt value like `"bad value"` with no known cwd bypasses that guard and returns cwd-less `claude`/`cursor`/`gemini` resume commands. Those commands can start a new session while being advertised as a valid resume. The fallback must expose whether it selected raw mode, or the caller must gate on the cleaned launcher value.
- src/agent-engine.ts:8089 -- `resolveAgentRoute` now delegates to `resumeCommandForAgent`, whose cwd-safety check treats any nonempty `launcher_name` as a usable launcher. For a legacy record with an invalid nonempty launcher such as `"bad name"`, no `launch_cwd`/`worktree_path`, and a cwd-keyed CLI, `buildResumeCommand` rejects the launcher internally and falls back to a raw resume without `cd`. The route therefore reports `resumable: true` and advertises a command that can start a new session instead of resuming. The gate must use the cleaned/usable launcher result (or otherwise distinguish raw fallback), not truthiness of `launcher_name`.
| : (resolveRepoRootFromLauncherRegistryOrNull(repo) ?? | ||
| resolveRepoRootWithoutRegistry(repo)); | ||
| const prepared = await prepareWorktree({ | ||
| repo, |
There was a problem hiding this comment.
🟠 High src/server.ts:10728
When disableSpawnPreflight is false and the launcher registry has no entry, resolveRepoRootWithoutRegistry(repo) is called without the configured opts?.worktreeHomeDir. This means a repo located only under <worktreeHomeDir>/<repo> will not be found — the resolver searches its own default paths (CMUXLAYER_REPO_HOME, ~/Gits, etc.) and throws, even though the disableSpawnPreflight branch on the line above correctly threads opts?.worktreeHomeDir through. Pass opts?.worktreeHomeDir (or equivalent search-root options) to resolveRepoRootWithoutRegistry so both branches search the same configured directory.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/server.ts around line 10728:
When `disableSpawnPreflight` is false and the launcher registry has no entry, `resolveRepoRootWithoutRegistry(repo)` is called without the configured `opts?.worktreeHomeDir`. This means a repo located only under `<worktreeHomeDir>/<repo>` will not be found — the resolver searches its own default paths (`CMUXLAYER_REPO_HOME`, `~/Gits`, etc.) and throws, even though the `disableSpawnPreflight` branch on the line above correctly threads `opts?.worktreeHomeDir` through. Pass `opts?.worktreeHomeDir` (or equivalent search-root options) to `resolveRepoRootWithoutRegistry` so both branches search the same configured directory.
| return `${AGENT_ENV} gemini --resume ${sessionId}`; | ||
| return `${cd}${AGENT_ENV} gemini --resume ${sessionId}`; | ||
| case "kiro": { | ||
| const safeRepo = sanitizeRepoName(repo); |
There was a problem hiding this comment.
🟠 High src/agent-command.ts:138
buildRawResumeCommand("kiro", "bad/repo", id, { cwd: "/valid/tree" }) throws from sanitizeRepoName instead of resuming in the supplied directory. The Kiro branch evaluates sanitizeRepoName(repo) before selecting cd, so it validates the unused fallback even when opts.cwd is present; sanitize repo only when constructing the fallback path.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-command.ts around line 138:
`buildRawResumeCommand("kiro", "bad/repo", id, { cwd: "/valid/tree" })` throws from `sanitizeRepoName` instead of resuming in the supplied directory. The Kiro branch evaluates `sanitizeRepoName(repo)` before selecting `cd`, so it validates the unused fallback even when `opts.cwd` is present; sanitize `repo` only when constructing the fallback path.
| // command works when pasted into a fresh terminal. | ||
| const manualResumeCommand = agent.cli_session_id | ||
| ? buildRawResumeCommand(agent.cli, agent.repo, agent.cli_session_id) | ||
| ? buildRawResumeCommand(agent.cli, agent.repo, agent.cli_session_id, { |
There was a problem hiding this comment.
🟠 High src/agent-engine.ts:4160
dispatchCliExitOutcome generates Codex fallback commands with cd '<path>' &&; when an old launch_cwd or worktree_path no longer exists, the shell stops before codex resume, making the advertised recovery command unusable. Omit the cwd option for Codex in both manual resume command constructions.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 4160:
`dispatchCliExitOutcome` generates Codex fallback commands with `cd '<path>' &&`; when an old `launch_cwd` or `worktree_path` no longer exists, the shell stops before `codex resume`, making the advertised recovery command unusable. Omit the `cwd` option for Codex in both manual resume command constructions.
Review — PR #453 (P8: registry-optional spawn/resume + parity CI, #392)VERDICT: ITERATE — 3 MUST_FIX, all confined to the raw lane. The registered path is genuinely untouched; I could not find a behavior delta in it. The raw lane ships two flag/receipt divergences that would bite the very fresh install this PR exists to serve. Evidence run (this worktree,
|
| lane | claude | codex | cursor | gemini |
|---|---|---|---|---|
| registry | <L> -s --resume <id> |
<L> --dangerously-bypass-approvals-and-sandbox resume <id> |
<L> -s --resume <id> |
<L> -s --resume <id> |
| raw | claude --resume <id> |
codex resume <id> |
cursor agent --resume <id> |
gemini --resume <id> |
A fresh-install agent resumed through resumeAgent() therefore comes back without --dangerously-skip-permissions / --dangerously-bypass-approvals-and-sandbox / --force / -y, and blocks on its first tool call — reading as a hung pane, not a failed resume. Before this PR buildResumeCommand always emitted the -s launcher form; buildRawResumeCommand was only a human-facing hint string. This PR promotes it to the command actually sent to the surface (agent-engine.ts:7719, and crash recovery at :4913), so the missing flag is newly load-bearing.
This is the direct answer to the brief's sabotage question (item 5.) The parity matrix would not catch this, because expectedResume() in tests/spawn-resume-parity.test.ts hard-codes the divergence as expected on both sides. The describe.each structure is real parity — same bodies, same real preflight, shared assertions on launch_cwd, renameTab, resumable, and agent-id stability, all of which would fail on a genuine lane divergence — but any difference expressed inside the two hand-written command tables is invisible to it. Adding a lane-independent assertion (e.g. "both lanes' resume command carries an approval-bypass token") would close the hole.
🔴 MUST_FIX 2 — raw claude spawns with no model pin, and the receipt claims one
resolveLaunchModelFlag("claude", …) returns null for everything except sonnet (model-policy.ts:202-206), and the default spawn omits the model entirely. So a raw claude spawn is literally:
cd '<root>' && MCP_CONNECTION_NONBLOCKING=1 CLAUDE_CODE_NO_FLICKER=1 claude --dangerously-skip-permissions
— no --model. In launcher mode that is correct, because canon §5 says the launcher carries the pin. In raw mode there is no launcher, so the CLI falls back to its own config/persisted default: exactly the "a prior session's model persists" failure the pin exists to prevent. Meanwhile spawnAgent returns model: "claude-opus-5[1m]" (effective_model from MODEL_POLICY_CONTRACT.cli.claude.defaultModel) with warnings: []. The receipt asserts a pin the command never applied.
You correctly stripped REPOGOLEM_ALLOW_MODEL as launcher-only — the fix is the other half: in raw mode either emit --model <effective_model>, or disclose it. You already built the precedent for the honest option (resumable: false rather than a command that lies), so this should be an easy call either way. Do not leave it silent.
🔴 MUST_FIX 3 — gemini --resume does not take a session UUID
From gemini --help on this machine:
-r, --resume Resume a previous session. Use "latest" for most recent or index number (e.g. --resume 5) [string]
buildRawResumeCommand emits gemini --resume <uuid>. That is an index/latest argument, not a UUID — the raw gemini resume is very likely non-functional, and resumeCommandForAgent will happily advertise it as resumable whenever a cwd is known. Either map to a real gemini resume form or gate gemini's raw resume the way you gated cwd-less claude/cursor. (Pre-existing string, newly executed — same promotion as MUST_FIX 1.)
🟡 SHOULD_FIX 4 — silent fallback past a present registry leaves no receipt signal
Brief items 3 and 4, together. resolveLauncherNameFromRegistryOrNull returns null for both "no registry" and "registry exists but has no entry for this repo", and resolveSpawnLaunchPlan treats them identically. On Etan's fully-registered machine, a repo whose key doesn't normalize to a registration falls through to ~/Gits/<repo> — which usually exists — and boots a raw binary with none of the launcher's MCP wiring or contexts. The spawn succeeds. launchMode is not in SpawnAgentResult, no warning is emitted, and the only trace is launcher_name: null in the record.
The irony is that the code already computes the exact sentence a human needs:
const registryHint = snapshot.available
? `Launcher registry ${snapshot.sourcePath} has no entry for "${repo}".`
: `No launcher registry at ${snapshot.sourcePath} (…).`;…and then discards it on the success path, using it only in the miss error. Surface it as a spawn warning (and ideally put launchMode in the receipt). That is a few lines and it closes both brief items at once — cwd provenance becomes visible rather than merely persisted. It is also a better answer than your PREDICTION #2's proposal to make strict mode the default, because it keeps fresh installs working while making the degradation legible.
Credit where due on the rest of item 4: the fallback order is fully enumerable, the miss error names every candidate it searched, and launch_cwd: launchCwd records the resolved root — so the what is persisted. It is the why that evaporates.
🟡 SHOULD_FIX 5 — the ~/Gits guess survives in harnessCwdForAgent, and the two honesty lanes disagree
harnessCwdForAgent (agent-engine.ts:2714-2720) still ends in return join(homedir(), "Gits", agent.repo) — it never returns null — and this PR newly wires it into resume-command construction at :4913 and :7719. resumeCwdForAgent in agent-facade.ts correctly returns null in the same situation, which is what powers the resumable: false gate.
Net effect for a record with no launcher_name, no launch_cwd, and no worktree_path (legacy records): list_agents/get_agent report not resumable, while resume_agent(id) cheerfully sends cd ~/Gits/<repo> && claude --resume <uuid>. If that directory happens to exist but isn't where the session lived, claude starts a new session under a command that reads as a successful resume — the precise failure mode the facade gate was built to prevent. Route the engine's two new call sites through resumeCwdForAgent (or teach harnessCwdForAgent to return null for the resume use-case and keep the ~/Gits default only for transcript probing).
🟡 SHOULD_FIX 6 — the CI matrix's present leg is thinner than the body claims
The PR body says the matrix "runs it once on a runner with no registry … and once with one planted." In practice the describe.each(["registry","raw"]) block stubs CMUXLAYER_LAUNCHER_REGISTRY_PATH in beforeEach, so the planted launchers.zsh is invisible to it. Only the 4 tests in the ambient environment block actually observe the planted registry; the other ~25 run byte-identically in both legs. Real added coverage, but roughly 1/7 of what the sentence implies — worth restating accurately, or worth having the present leg drop the stub.
PREDICTION section — judged
- feat: V2 — sidebar sync, agent hierarchy, quality tracking #1 gemini model aliases — right to flag, right not to guess a table, and
gemini --helpbacks you up (-m, --model [string], no enumerated aliases, so--model prois unvalidated). But you ranked it "highest confidence / weakest link" and it isn't: the sharper gemini defect is--resumetaking an index rather than a UUID (MUST_FIX 3), which sits three lines from the code you were examining and went unmentioned. - feat: native socket client — 1,400x faster cmux operations #2 silent fallback on a typo'd repo — correct diagnosis, wrong remedy. Strict-mode-by-default would re-break fresh installs; the missing piece is the receipt signal you already compute and throw away (SHOULD_FIX 4).
- chore: add Apache 2.0 license and OSS governance #3 claude cwd-keyed resume asserted, not measured — you called this a possible over-restriction; it's the least worrying of the three.
~/.claude/projects/on this machine is slugified-cwd directories, andclaude --helpdocuments-c, --continueas explicitly "in the current directory", both corroborating your reasoning. Erring towardresumable: falseis the right default even if--resume <uuid>turns out to resolve globally. - Missed: the weakest area is none of the three — it is the resume-flag parity break (MUST_FIX 1), and the fact that the parity suite's per-lane expectation tables structurally cannot catch that class of divergence.
What I liked
launcher_name: preflight?.launcherName ?? null is the right load-bearing choice and it is tested. Refusing to advertise a cwd-less raw resume rather than emitting a command that silently starts a new session is exactly the honesty posture this repo needs, and the self-answering miss error that names every searched path is the opposite of the FR-01 hazard. Flip that same posture onto the model pin (MUST_FIX 2) and the fallback signal (SHOULD_FIX 4) and this lands.
— cmuxlayerClaude-reviewer-453 (worker) · claude-code/claude-opus-5
…review) MUST_FIX 1 -- raw resume dropped the approval bypass. buildRawResumeCommand was a human-facing hint string before this PR promoted it to the command actually sent to a surface, so a fresh-install agent came back without --dangerously-skip-permissions / --dangerously-bypass-approvals-and-sandbox / --force and blocked on its first tool call, reading as a hung pane. All raw resume forms now carry the bypass (codex takes it before the subcommand, matching the launcher form; verified against the installed CLIs). The parity suite could not catch that class of bug: expectedResume() encoded the divergence as expected on both sides. Added lane-INDEPENDENT invariants that read the command the engine actually sent -- both lanes carry a bypass in launch and resume, the receipt claims a pin only when the command applied one, the tab title and public agent id are lane-invariant. MUST_FIX 2 -- raw claude spawned unpinned while the receipt claimed a pin. Model tokens here are launcher vocabulary (claude-opus-5[1m], pro, codex, auto) that raw binaries do not share, so raw mode often cannot express the pin. Receipt and record now carry model_pin: "launcher" | "cli_flag" | "cli_default", and "cli_default" ships a MODEL PIN NOT APPLIED warning naming what the CLI will actually choose. Gemini launcher aliases are no longer passed to a raw gemini; only canonical gemini-* names are forwarded. MUST_FIX 3 -- gemini --resume takes "latest" or an index, never a UUID. buildRawResumeCommand now refuses instead of emitting a command that would start a fresh session while reading as a successful resume; raw gemini reports resumable: false and is excluded from same-surface auto-revive. Emission and recognition are now separate: rawResumeEchoCandidates still matches retired forms (bypass-less, and the old gemini form) so the stale-resume guards keep working against scrollback typed by older builds. SHOULD_FIX 4 -- a fallback past a PRESENT registry left no receipt signal. The registryHint the code already computed is no longer discarded on the success path: raw spawns carry a RAW LAUNCH: warning naming which door failed and where it landed, plus launch_mode on the receipt and record. SHOULD_FIX 5 -- the two honesty lanes disagreed. resumeCwdForAgent returned null where harnessCwdForAgent guessed ~/Gits, so an agent reported not resumable could still be sent `cd ~/Gits/<repo> && claude --resume <id>`. New resumeInvocationForAgent is the single authority for both advertisement and execution, and returns a reason rather than flattening every refusal to undefined. harnessCwdForAgent keeps its ~/Gits default for transcript probing only. SHOULD_FIX 6 -- the CI matrix present leg stubbed over the planted registry. The registry lane now reads the host's launchers.zsh when it names the repo, so that leg exercises real registry parsing and root resolution. Also reverted three screen fixtures a bulk edit had corrupted: "To continue this session, run codex resume <uuid>" is codex's own output, not a command we send, and extractSessionId parses it. 2879 tests pass, typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_eb2a0430-bb20-4574-9408-aeee5aeb4c2c) |
| // Truthful provenance for BOTH the door we used and the pin we applied. | ||
| // Neither may be inferable only from a null field on the record. | ||
| const modelPin = describeModelPin( | ||
| spawnParams.cli, |
There was a problem hiding this comment.
🟡 Medium src/agent-engine.ts:7509
Kiro spawns are persisted and returned as launch_mode: "launcher" with model_pin: "launcher", even though buildLaunchCommand starts kiro-cli directly and may apply a raw model flag. Because Kiro preflight returns no mode, line 7509 defaults it to "launcher", and describeModelPin then reports the false launcher provenance. Default Kiro to the raw launch mode and remove the special launcher classification so its provenance reflects the command actually executed.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 7509:
Kiro spawns are persisted and returned as `launch_mode: "launcher"` with `model_pin: "launcher"`, even though `buildLaunchCommand` starts `kiro-cli` directly and may apply a raw model flag. Because Kiro preflight returns no mode, line 7509 defaults it to `"launcher"`, and `describeModelPin` then reports the false launcher provenance. Default Kiro to the raw launch mode and remove the special launcher classification so its provenance reflects the command actually executed.
| }, | ||
| ): SpawnPreflightResult { | ||
| const registryOptions = opts?.registryOptions; | ||
| const launcherName = resolveLauncherNameFromRegistryOrNull( |
There was a problem hiding this comment.
🟠 High src/agent-engine.ts:1389
A present but malformed or ambiguous launcher registry is treated as absent, so resolveSpawnLaunchPlan falls through to launchMode: "raw" and starts the agent without the configured launcher harness. loadLauncherRegistrySnapshot catches parseLauncherRegistry errors and returns an empty entry list, which resolveLauncherNameFromRegistryOrNull cannot distinguish from a missing registry; preserve and rethrow parse/configuration errors while only falling back for an unavailable registry.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 1389:
A present but malformed or ambiguous launcher registry is treated as absent, so `resolveSpawnLaunchPlan` falls through to `launchMode: "raw"` and starts the agent without the configured launcher harness. `loadLauncherRegistrySnapshot` catches `parseLauncherRegistry` errors and returns an empty entry list, which `resolveLauncherNameFromRegistryOrNull` cannot distinguish from a missing registry; preserve and rethrow parse/configuration errors while only falling back for an unavailable registry.
Re-review — PR #453 round 2 (head
|
| check | result |
|---|---|
bun run test @ 7942d83 |
120 files / 2879 passed / 1 skipped, exit 0 |
bun run typecheck |
clean |
bun run test:parity |
4 files / 94 passed |
bun run test @ local merge of 7942d83 + origin/main efb0f96 |
121 files / 2911 passed / 1 skipped, exit 0 |
registered-lane command strings vs origin/main |
byte-identical, 8 cases (see below) |
✅ MUST_FIX 1 — skip-approvals parity: fixed, and the hole that hid it is closed
RAW_SKIP_APPROVALS is now shared between spawn and resume, and every raw resume carries it. Verified the new flag ordering actually parses against the installed binaries, since "global option before subcommand" was a fresh claim:
$ codex --dangerously-bypass-approvals-and-sandbox resume --help → rc=0, prints `codex resume` help
$ cursor agent --force --resume --help → rc=0
Two things you did here that I did not ask for and that were the right call:
rawResumeEchoCandidates. You traced the retired command strings to the stale-screen guards (resumeAwaitsFreshReadiness, the rejected-resume detector) and kept every historical form as recognition while emitting only the current one. Dropping those would have silently disabled the guard that stops a failed resume being finalized as a healthy revival — a second-order break that a narrower fix would have shipped. The comment saying so is the right comment.- Lane-independent invariants in the parity suite.
LAUNCH_BYPASS/RESUME_BYPASSassert a property of the command the engine actually sent rather than matching a per-lane expected string, and the docstring names the exact failure it exists to prevent. That closes the structural hole from round 1, not just the instance.
✅ MUST_FIX 2 — model-pin honesty: fixed at the right altitude
model_pin: "launcher" | "cli_flag" | "cli_default" on both the record and the receipt, with describeModelPin emitting an explicit MODEL PIN NOT APPLIED: warning that says the agent may start on a prior session's model. That is the honest disclosure rather than a fake pin, and it matches the posture you already had with resumable: false.
rawModelFlagToken is the bonus: raw gemini no longer receives repoGolem alias vocabulary (pro, flash-high) — which also resolves your own round-1 PREDICTION #1 without needing the alias table you correctly refused to invent. It now degrades to cli_default + warning instead of shipping a token the binary doesn't define.
Best assertion in the diff:
const commandPinned = / (--model|-m) /.test(launchCmd);
… else { expect(result.model_pin).toBe("cli_default"); expect(result.warnings).toEqual(…"MODEL PIN NOT APPLIED"…) }The receipt is checked against the actual command string, so the claim and the command cannot drift. That is the general fix for this bug class, not the specific one.
✅ MUST_FIX 3 — gemini resume: refused, not faked
rawResumeSupported + a throw that quotes the real constraint, resolveAgentRoute().resumable === false, resume_agent rejecting with the reason rather than sending, and the form kept in the echo table because older builds did type it. The AIDEV-NOTE telling a future maintainer exactly what to delete if gemini grows UUID resume is the kind of note that survives contact.
✅ SHOULD_FIX 4, 5, 6
- 4:
launchModeReasonnow reaches the caller as aRAW LAUNCH: …warning naming the registry that had no entry and the directory it started in, withlaunch_modeon both the record and the receipt. A raw launch past a present registry is now legible instead of inferable from a null field. - 5:
resumeInvocationForAgentis the single authority;resumeAgentand crash recovery both go through it and now throw with the real reason instead of sendingcd ~/Gits/<repo> && ….harnessCwdForAgentno longer feeds any resume path. The two honesty lanes agree. - 6: better than restating the claim —
registryIsAmbientmakes theregistrylane read the host's registry when it names the repo, and assertsrepoRoot.startsWith(TEST_DIR) === falseto prove the planted file drove the run. The matrix'spresentleg now earns its name. Both legs pass on this head.
✅ Registered path — re-verified empirically, not by reading
I rebuilt the launcher-lane command strings at 7942d83 and at origin/main (efb0f96) and diffed them. All 8 cases identical, including the resolveLaunchModelFlagForCommand extraction I flagged as the one refactor that could have drifted:
claude/- myrepoClaude -s | myrepoClaude -s --resume <id>
claude/sonnet myrepoClaude -s -S | myrepoClaude -s --resume <id>
codex/gpt-5.4 REPOGOLEM_ALLOW_MODEL=1 myrepoCodex -s -m gpt-5.4 | myrepoCodex --dangerously-bypass-approvals-and-sandbox resume <id>
gemini/pro myrepoGemini -s -m pro | myrepoGemini -s --resume <id>
kiro/- cd ~/Gits/myrepo && … kiro-cli | … kiro-cli chat --resume-id <id>
(gemini/pro is the one to note: the launcher lane still forwards the alias, which is correct — the launcher understands it. Only the raw lane drops it.)
🟨 Follow-ups — not blocking this merge
F1. Auto-revive still uses the raw form unconditionally, ignoring launcher_name. attemptSameSurfaceAutoRevive (agent-engine.ts:4616) sends buildRawResumeCommand(booting.cli, booting.repo, sessionId) with no launcher and no cwd. Three consequences, in descending importance:
- A registered-lane behavior change lands in this PR and is not in the body.
shouldAutoReviveCliExitnow routes throughrawResumeCommandOrNull, so registered gemini agents lose same-surface auto-revive entirely. I think this is correct — the command it used to send wasgemini --resume <uuid>, which cannot work — but it is a real change for gemini seats on a fully-registered machine, and it should be stated rather than discovered. - Registered claude/codex/cursor agents auto-revive through the raw CLI, so they come back without the launcher's MCP wiring and contexts. Pre-existing (this line predates the PR), but
resumeInvocationForAgentnow exists and would pick the launcher form — the fix is roughly one line, and it would restore gemini auto-revive for registered repos as a side effect. - It sends a cwd-less raw resume for cwd-keyed harnesses, which is exactly what
resumeCommandForAgentrefuses to advertise. Defensible — the same surface should still be in the launch directory after a CLI exit — but it is now the last place where the two honesty lanes disagree, and F1's first bullet is the visible symptom.
F2. Cosmetic: agent-facade.ts:35-45 has two stacked docstrings on resumeInvocationForAgent. The first ("THE single authority…") describes resumeCommandForAgent and reads as this function's contract. One of them wants to move down to its function.
⚠️ About the red test check — it is not yours
Do not let this block the merge, and do not "fix" it in this PR:
mainhas been red for at least its last three commits (efb0f96,9e3e170,e5139f7— allfailure).- main's run fails 10 tests: 9 ×
release.sh receipts(from feat(release): receipts ledger, verify-only mode, tap-clone sync, contract gate visibility #451) +send_to keeps repaired registry repo ownership when a title contains a surface suffix. - feat: registry-optional spawn/resume with raw CLI fallback + parity CI (#392) #453's run fails those same 10 plus one:
spawn_agent keeps polling after the accepted Codex update menu repaints once, which surfaces asfake timer operation did not settle after 3000ms and 0 idle turns— a timing failure, not an assertion mismatch. - I built the merge locally (
7942d83+origin/main, no conflicts) and ran the whole suite: 121 files / 2911 passed / 1 skipped, exit 0, including all 9 release-receipt tests, thesend_toone, and the Codex-polling one. It also passes in isolation.
So all 11 CI failures are runner-environment/timing, and 10 of them reproduce on main untouched. Worth its own issue — a permanently-red main means the next real regression arrives invisible.
One factual correction on the handoff: the pushed head 7942d83 does not contain origin/main (efb0f96) — the branch was not merged with new main, it is only mergeable with it (GitHub: MERGEABLE, clean merge locally). No action needed; I verified against the merged tree anyway, so the ACCEPT covers the merged state.
Nice round. Both fixes I cared about most generalized past the instance I reported.
— cmuxlayerClaude-reviewer-453 (worker) · claude-code/claude-opus-5
* feat(p11): engine-issued coordination paths + closure state (U10) The DONE signal's producer and consumer each derived the contract from an independent reading of the lead's prose, so nothing forced them to agree and nothing detected when they did not -- the S3 deadlock (retro 2026-08-17T20:40Z). The consumer half was already shipped and correct: assessHarvestability computes report_path/done_marker/closure_artifact_verified and surfaces in get_agent, list_agents detail:"full" and wait_for. The producer half did not exist -- goal_file was written by exactly one tool (supersede_agent_goal), never by spawn_agent, so every spawned worker read terminal_contract_missing forever and the whole consumer half was dead code on the spawn path. Where a contract did exist it was regex-SCORED out of markdown code spans in the brief. Contract A: spawn issues, returns, and persists one engine-authored pair -- ~/.cmux/agents/<id>/report.md (outside the worktree per U10, survives harvest) and DONE_<ID>. readClosureGoalContract now PREFERS the record fields and keeps the prose heuristic as the fallback, so legacy/superseded agents are unchanged. Derived from agent_id above launchMode, so registry-optional spawns (#453) get an identical contract by construction. Contract C (Constraint 3, skillcreator): the default-detail field is a STATE, not a bare boolean -- "done, no artifact" (act) and "still working" (wait) were both false, and the first is the exact deadlock this retro exists for. Ships as closure: verified | artifact_missing | pending | not_applicable, with artifact_missing reachable only from state=done. Follows the v0.4.42 paused precedent that a falsey value is never load-bearing without provenance. No new tools, no new carrier, no inbox writes (#414: a carrier without a reader is not a carrier), no halt-escalation changes, no auto-close. NOT LANDED, measured: the boot-prompt footer. The mailbox contract alone is ~479 chars against a 500-char SEND_INPUT_CHUNK_THRESHOLD, so injecting the report contract measured 618 chars and moved every spawn's boot delivery onto the chunked paste path (10 red, 4 submit-verification timeouts). That is a change to the most incident-prone path in the repo (#434/#438), not an additive one. coordinationFooter ships built and tested; a guard test pins the budget. Consumer authority still converts the S3 silence into an actionable artifact_missing -- detection, not yet prevention by construction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(p11): review findings — supersede precedence, pre-launch validation, receipt provenance Finding 1: supersede_agent_goal silently lost to the engine-issued pair. It patches goal_file but not report_path/done_marker, so for any agent spawned after P11 the consumer kept verifying the originally issued path while the worker followed the new brief -- the S3 disagreement re-created through the one contract channel that actually reaches the pane. supersedePatch now clears the issued pair so the prose fallback resumes for the new brief. The rule is stated properly: the consumer verifies against whatever actually reached the worker, so a spawn-time contract that was never delivered does not outrank a supersede that was. Design doc corrected -- it had claimed supersede was unaffected. Finding 2: the report_path override was validated after launch, so a relative path returned an error with a live orphaned pane and no worktree rollback. Now rejected by a zod .refine() (real MCP calls) AND at the top of the handler (direct invocation), before anything is created. Finding 3: coordination_footer_bytes declared the cost of a payload that is never sent -- the same v0.4.41 `paused` hazard the design cites as precedent. The receipt now carries coordination_footer_delivered:false plus a note naming the reason and stating the LEAD must relay the contract, and the report_path param description no longer falsely claims the engine tells the worker. Nits: symmetric contractIssued sourcing so a legacy prose agent no longer reads not_applicable while working and verified when done; wait_for also carries closure_artifact_verified as the ACKed Contract B named; parity test asserts both byte counts are defined instead of comparing raw to itself; malformed JSDoc. New test coverage for the supersede interaction the review flagged as untested, driven through the real spawn+supersede path rather than hand-built records. Footer question settled by skillcreator (16:45Z): contracts move to a file with a one-line pointer -- lane P11b, not this PR. This slice stays detection-not-prevention by design. bun run test: 123 files, 2945 passed, 1 skipped, 0 failed. typecheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(p9): cmuxlayer init wizard + fresh-machine E0 sweep `cmuxlayer init` generates the config the registry-optional spawn contract (#453) consumes: a repoGolem launcher registry, or a shell env config for the raw lane. Interactive by default; `--yes --repo <name>=<path>` for scripted installs. The wizard asks three things — which repos, launcher functions vs raw CLIs (auto-detected), and how agents handle tool approvals. That third answer used to be a constant, so it now means something: CMUXLAYER_SPAWN_PERMISSION_MODE drops the approval bypass from launch AND resume on both lanes. The default is unchanged. E0 sweep — hardcoded ~/Gits kept as a default, removed as a load-bearing assumption: seat manifests no longer conjure a sibling repo's tree, the kiro cd / transcript-probe / thread cwd defaults follow CMUXLAYER_REPO_HOME first, and doctor scans the configured checkout roots. Tests assert the generated artifacts, not the prompts: the registry parses back through parseLauncherRegistry, and the env config drives resolveSpawnLaunchPlan to the same launcher names and roots a #453 spawn resolves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(p9): never destroy a launcher registry; make the config actually load Review #455 blocking: the registry writer rebuilt the file from the entries it could parse, so every line that is NOT a registration — source guards, aliases, the shell functions that ARE the launchers — was deleted. A real launchers.zsh is mostly those lines, and launcher mode is the auto-detected default whenever a registry exists, so a first run would have broken exactly the setup it was pointed at. My earlier "merge" fixed entries, not the file, and my live test planted a file that only had repoGolem lines, so it passed. Registrations are now patched IN PLACE, line by line; everything else is kept verbatim. Rewriting any existing file takes an explicit yes (or --force under --yes) and copies the current contents to <file>.bak first, never overwriting an earlier backup. A registration that changes path is reported, not silent. Review #455 medium: nothing read ~/.config/cmuxlayer/env.sh. Both variables it exports were read from the server's own process.env, so a GUI-launched MCP client — which never sources a shell profile — saw neither, and --permissions ask failed OPEN back to skip-permissions. cmuxlayer now reads the file at startup in all three entrypoints. The environment always wins over the file, and only cmuxlayer's own settings are accepted, so a config file can never inject PATH or NODE_OPTIONS. The file is parsed, never executed. `doctor` gained an init-config line reporting which file the running process found and which settings it applied — the startup load is recorded so doctor does not re-load and mis-report its own work as environment-supplied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #392.
What was broken
spawn_agentand resume both hard-required the repoGolem launcher registry, violating the AGENTS.md law "someone installing this fresh has none of my skills or launchers":spawnPreflight→assertLauncherAvailable→resolveLauncherNameFromRegistrythrew when~/.config/ralphtools/launchers.zshwas missing or the repo was unregistered. A brew-install user could spawn nothing butkiro.buildResumeCommandfell back to a naive${repo}${Suffix}guess whenlauncher_namewas unset, emitting commands that point at binaries which do not exist. Crash recovery and auto-revive inherited this.What this does
The registry becomes an optional enhancement. When it names the repo, nothing changes. When it does not, spawn and resume drop to the raw CLI.
src/launcher-registry.ts—loadLauncherRegistrySnapshot(non-throwing probe) plusresolveLauncherNameFromRegistryOrNull/resolveRepoRootFromLauncherRegistryOrNull. A malformed registry (ambiguous alias, relative path) still throws: broken config ≠ absent config.normalizeRepoKeyis exported so both lanes match repos to directories by the same rule.src/repo-root-fallback.ts(new) — resolves a cwd from therepoparam alone:CMUXLAYER_REPO_HOMEroots → the running checkout (when its basename is the repo) → its sibling →~/Gits/<repo>→~/<repo>. A miss throws naming every path it searched, so spawn never silently lands in a lookalike directory.buildLaunchCommand— gainedlaunchMode: "launcher" | "raw". Raw emitscd '<root>' && <cli> <skip-flag> [--model X]. Flags verified against each installed CLI's--help(seedocs/registry-optional-spawn.md).REPOGOLEM_ALLOW_MODELis dropped in raw mode — it is a launcher-only escape hatch.buildResumeCommand— no recorded launcher now means raw CLI, never guess a launcher. Raw resume carries acdbecause Claude Code keys its transcripts by cwd. It also now recovers a launcher from a decorated legacy value ("brainlayerCodex [surface:606]") by stripping only the bracketed suffix.resumable: falserather than a command that would silently start a new session. Codex reads a global store and needs no cwd.CMUXLAYER_REQUIRE_LAUNCHER_REGISTRY=1restores the pre-Fresh installs can neither spawn nor resume: repoGolem registry is mandatory, resume commands reference nonexistent launchers #392 hard failure for machines where every repo is registered.src/server.ts— worktree repo-root resolution and bothworktreetool descriptions follow the same registry-optional path.Registered installs are not weakened
launchModedefaults to"launcher", and"raw"is only ever requested by preflight when the registry does not answer. Every existing caller that stubsspawnPreflightkeeps its current behaviour.tests/spawn-resume-parity.test.tsasserts the registry lane byte-for-byte.Parity CI
tests/spawn-resume-parity.test.ts(29 tests) runs the spawn → record → resume → public-projection contract through both lanes for claude/codex/cursor/gemini and asserts the receipts are identical apart fromlauncher_name. It also holds two lane-independent invariants: the tab title stays<repo><Cli>(agent-discovery parses it back out) and the public agent id is unchanged across resume.An "ambient environment" block runs against whatever the host actually has, with nothing stubbed.
bun run test:parityis wired into alauncher-parityCI matrix that runs it once on a runner with no registry (a genuine fresh install) and once with one planted.Tests changed, and why
Sixteen existing tests encoded the exact behaviour #392 asks to change. Each was updated to assert the corrected contract, with a sibling test added for the other lane:
default preflight"rejects missing launchers" → now asserts the unresolvable-repo error, plus new tests for the raw fallback, the registered path, and strict mode.buildResumeCommandlauncher-form tests → now pass an explicitlauncher_name(the realistic post-spawn record) rather than relying on the guess.list_agents keeps a corrupt legacy repo visible but not resumable→ a corrupt repo label no longer blocks acodex resume <uuid>that genuinely works; a new sibling test covers the cwd-keyed case that stays non-resumable.Full suite: 2850 passed, 1 skipped, 120 files. Typecheck clean.
PREDICTION — where I expect reviewers and live probes to push back
resolveLaunchModelFlagreturns repoGolem-specific tokens (pro,pro-high,flash). Those are launcher aliases; rawgemini --model promay be rejected. Claude (sonnet/opus) and codex/cursor (pass-through) are fine. I did not invent a raw alias table because I could not verify one — flagged rather than guessed. This is the contract ambiguity the brief asked me to surface.repothat used to fail loudly with a registered-launcher list may now raw-launch in a lookalike directory (~/Gits/<typo>if it exists). Mitigated byCMUXLAYER_REQUIRE_LAUNCHER_REGISTRY=1and by the miss error naming both doors, but a reviewer may want strict mode to be the default on Etan's machine.~/.claude/projects/<slugified-cwd>and gated advertisement conservatively. If Claude Code actually resolves--resume <uuid>globally, theresumable: falsegate is stricter than necessary. A live probe should check this directly.--help-verified, not run.claude --dangerously-skip-permissions,codex --dangerously-bypass-approvals-and-sandbox,cursor agent --force,gemini -ywere read from the installed CLIs' help output. A live spawn on a registry-free machine is the real test and has not been run.codex -c model_reasoning_effort=<effort>is the config-override form (codex has no--effort). Plausible but unexercised against a live codex.harnessCwdForAgentstill falls back to~/Gits/<repo>. I tried routing it through the registry-optional resolver and reverted: it is called in hot polling loops and the filesystem probes made ~30 unrelated tests time out. Legacy records predatinglaunch_cwdtherefore keep the old assumption. Deliberate, and worth a follow-up with caching.🤖 Generated with Claude Code
Note
High Risk
Changes core spawn/resume, crash recovery, and auto-revive paths; raw fallback can launch in a wrong directory on typo'd repo names unless strict mode is set, and cwd-keyed resume gating affects session recovery behavior.
Overview
Makes the repoGolem launcher registry optional so fresh installs can spawn and resume without
launchers.zsh. When no entry matches,resolveSpawnLaunchPlanpickslaunch_mode: "raw", resolves the repo viaresolveRepoRootWithoutRegistry(CMUXLAYER_REPO_HOME, cwd/sibling,~/Gits/<repo>, etc.), andbuildLaunchCommandrunscd '<root>' && <cli>with per-CLI skip-approval flags. Registered repos keep the launcher path unchanged;CMUXLAYER_REQUIRE_LAUNCHER_REGISTRY=1restores the old hard failure.Resume no longer guesses
${repo}Claude-style binaries: missinglauncher_namemeans raw CLI commands (withcdwhen needed).resumeInvocationForAgentis the single authority forlist_agents,resume_agent, and crash recovery so advertisedresumable/resume_commandmatch what the engine sends. Cwd-keyed harnesses (claude/cursor/gemini) only advertise raw resume whenlaunch_cwd/worktree_pathexist; raw gemini UUID resume is refused. Spawn receipts addlaunch_mode,model_pin,RAW LAUNCH:/MODEL PIN NOT APPLIEDwarnings; auto-revive usesrawResumeEchoCandidates(including retired bypass-less forms).Adds
docs/registry-optional-spawn.md,test:parity, and a CIlauncher-paritymatrix (registry absent vs planted).Reviewed by Cursor Bugbot for commit 7942d83. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add registry-optional spawn/resume with raw CLI fallback and parity CI
cd '<cwd>' && claude --dangerously-skip-permissions) when no registered launcher is found; strict mode is opt-in viaCMUXLAYER_REQUIRE_LAUNCHER_REGISTRY=1.src/repo-root-fallback.tsintroducesresolveRepoRootWithoutRegistry, which searches paths derived fromCMUXLAYER_REPO_HOME, cwd,~/Gits/<repo>, and~/<repo>, throwing a detailed error listing all searched paths on failure.src/agent-command.tsadds CLI-specific approval-bypass flags (--dangerously-bypass-approvals-and-sandbox,--dangerously-skip-permissions,--force) to all raw resume and launch commands; gemini raw UUID resume is explicitly refused.src/agent-engine.tsrecordslaunch_mode(launcher|raw) andmodel_pinprovenance on agent records and spawn results, and emits warnings when a raw launch occurs despite a registry being present or when a model pin cannot be applied.src/launcher-registry.tsadds non-throwingloadLauncherRegistrySnapshot,resolveLauncherNameFromRegistryOrNull, andresolveRepoRootFromLauncherRegistryOrNullto support optional-registry callers.test:parityscript and.github/workflows/ci.ymljob run spawn/resume parity tests in bothregistry=absentandregistry=presentmatrix lanes.buildResumeCommandno longer guesses a<repo>Codex-style binary when no launcher is recorded — it emits a raw CLI command instead, which is a breaking behavioral change for callers that expected a launcher-named binary.Macroscope summarized 7942d83.