Skip to content

[Bugfix #1267] Builder clean-exit relaunch reruns fresh instead of resuming the ended conversation - #1317

Merged
waleedkadous merged 11 commits into
mainfrom
builder/bugfix-1267
Aug 1, 2026
Merged

[Bugfix #1267] Builder clean-exit relaunch reruns fresh instead of resuming the ended conversation#1317
waleedkadous merged 11 commits into
mainfrom
builder/bugfix-1267

Conversation

@waleedkadous

Copy link
Copy Markdown
Contributor

Summary

A builder's while true launch loop baked one command in. On the resume path
that command is <cmd> --resume <id>, so the Enter-gated relaunch after a
clean exit restarted the very conversation the user had just deliberately
ended — the opposite of the rule #1264 (fixed for architects, PR #1266) states:
clean exit → rerun fresh, no recovery.

The loop now carries two invocations and switches between them.

Root Cause

spawn-worktree.ts generated, for a resumed builder:

while true; do
  claude --resume 'abc-1234-uuid'
  status=$?
  if [ "$status" -eq 0 ]; then; read -r || exit 0
    continue          # ← reruns the same line: --resume, again
  fidone

LAUNCH_LOOP_TAIL was a shared constant with nowhere to put a second command,
so the clean-exit branch had only continue available to it. Reproduced before
touching anything, with a fake agent standing in for the harness:

LAUNCH: --resume abc-1234-uuid     ← entry
LAUNCH: --resume abc-1234-uuid     ← after clean exit + Enter (should be fresh)

Fix

buildLaunchLoop(initial, fresh) is now the single generator behind all five
script variants.

  • initial — entry, and what a crash restarts. On the resume path that
    stays --resume <id>: recovery is exactly what an unnatural death wants.
  • fresh — what the keypress relaunches. It is the ordinary role-injected,
    prompt-carrying command a non-resume spawn would have used, so "fresh" needs
    no definition of its own.
  • The switch is sticky: a crash after a clean exit restarts fresh, never
    reviving the session the user walked away from.
  • When the two are equal — every non-resume variant — the generator returns the
    historical single-command loop byte for byte.

Generated resume script:

codev_launch_initial() {
  claude --model opus --resume 'abc-1234-uuid'
}
codev_launch_fresh() {
  claude --model opus --append-system-prompt "$(cat '/wt/.builder-role.md')" "$(cat '/wt/.builder-prompt.txt')"
}
codev_launch=codev_launch_initial
while true; do
  "$codev_launch"
  status=$?
  if [ "$status" -eq 0 ]; then
    clear
    echo "Agent exited at your request. Press Enter to relaunch fresh, or close this terminal."
    read -r || exit 0
    codev_launch=codev_launch_fresh
    continue
  fidone

Bash functions rather than a command string in a variable: the fresh invocation
carries its own quoting (--append-system-prompt "$(cat '…')"), which would be
word-split if re-expanded. There is a test for exactly that.

Two consequences worth flagging:

  • Role injection and the harness worktree files are now prepared on the resume
    path too. The relaunch is a genuine fresh launch and needs them.
  • .builder-prompt.txt is still never rewritten on resume. afx reset reads
    the spawn-time ## Mode: STRICT|SOFT heading out of it precisely because
    --resume does not regenerate it, and resolveMode cannot recover a spawn-time
    --soft from protocol defaults — regenerating it would silently flip a soft
    builder to strict. The fresh command reads that file.

The relaunch prompt says "relaunch fresh" now, in all variants — accurate for
each of them, and on the resume path it corrects a genuinely misleading message.

Deliberately not done

Issue #1267's second question — whether builders should keep the Enter gate
(#1244) or auto-rerun like architects — is left open, as the issue frames it.
Only what the relaunch runs changes here, not when it runs. Builders are
porch-driven and a fresh conversation's interaction with porch task state still
wants thinking through before auto-rerun is safe; that is a separate decision,
not a side effect of this fix.

Test Plan

New bugfix-1267-launch-loop.test.ts executes the generated bash against a
fake agent that logs its argv and exits on a scripted sequence of codes, then
asserts the exact invocation order. String assertions could not have caught this
bug — the old string was "correct"; the loop semantics were wrong.

Case Asserted
clean exit → Enter [--resume abc, --fresh-args]
crash (exit 1) [--resume abc, --resume abc] — recovery preserved
clean exit → crash [--resume abc, --fresh-args, --fresh-args] — sticky
EOF at the prompt [--resume abc] — exits, relaunches nothing
quoting fresh command's "$(cat …)" arrives as one argument
initial === fresh collapses to the single-command loop, no codev_launch
afx reset detection still names claude through the dual-launcher shape

Verified these fail against the pre-fix loop: 3 of 7 fail when buildLaunchLoop
is forced back to single-command, pass after.

  • pnpm build
  • Full suite: 4051 passed, 0 failed, 48 skipped (202 files) ✓
  • Both generated script variants inspected by hand; the non-resume one is
    unchanged apart from the message.

Fixes #1267

… --resume

A resumed builder baked `<cmd> --resume <id>` in as the single command its
`while true` loop reran, so the Enter-gated relaunch after a deliberate quit
started the very conversation the user had just ended — the opposite of the rule
#1264 established for architects (clean exit → rerun, no recovery).

The loop now carries two invocations. `initial` is the entry point and what a
crash restarts, so recovery still resumes — that is what an unnatural death
wants. `fresh` is what the keypress relaunches: the ordinary role-injected,
prompt-carrying command a non-resume spawn would have used, so "fresh" needs no
definition of its own. The switch is sticky, so a crash *after* a clean exit
restarts fresh rather than reviving the abandoned session.

They are bash functions, not a command string in a variable: the fresh
invocation carries its own quoting (`--append-system-prompt "$(cat '…')"`),
which would be word-split if re-expanded. All five generated-script sites go
through one `buildLaunchLoop`; when the two commands are equal — every
non-resume variant — it returns the historical single-command loop byte for
byte.

Role injection and the harness worktree files are consequently prepared on the
resume path too: the relaunch is a genuine fresh launch and needs them.
`.builder-prompt.txt` is the deliberate exception — `afx reset` reads the
spawn-time `## Mode:` heading out of it precisely because `--resume` never
rewrites it, and `resolveMode` cannot recover a spawn-time `--soft`. The fresh
command reads that file; it does not regenerate it.

Issue #1267's second question — whether builders should keep the Enter gate or
auto-rerun like architects — is left open by design. Only *what* the relaunch
runs changes here, not *when* it runs.
…launches

The bug was bash-level: the generated string was "correct", the loop semantics
were not. So the new suite runs the real loop under bash with a fake agent that
logs its argv and exits on a scripted sequence of codes, then asserts the exact
invocation order — clean exit relaunches fresh, a crash still resumes, the
switch is sticky, EOF exits without relaunching, and the fresh command's quoting
survives (one argument, not word-split). Three of the seven fail against the
pre-fix single-command loop.

Also guards the cross-module edge: `afx reset` refuses to type into a builder
whose harness it cannot name, and names it by scanning `.builder-start.sh` for a
command-position binary — the new `codev_launch=…` / `"$codev_launch"` lines
must neither shadow the harness line nor be mistaken for one.

The #929 resume assertions move from the whole script to the *entry* launcher,
which is what they were always about; the script now legitimately contains a
second, role-injected command.
…n means something

CMAP (codex): the fake agent logged "$*", which renders one argument "two words"
identically to two arguments "two" "words" — so the assertion that the fresh
command's "$(cat …)" survives as a single argument was not actually testing
anything. Log "$@" one-per-`|` instead: `two words|` vs `two|words|`.
From claude's review, none blocking:

- The fake agent ran off the end of its scripted exit codes into `exit ""`,
  which bash rejects with 255 — a *crash*, so the loop would auto-restart and
  spin until the runner's 30s timeout. A future regression that adds an
  unexpected relaunch should fail as a wrong invocation list, not a hang;
  default the code to 0.
- Document why `.builder-prompt.txt` is guaranteed present on the resume path:
  it is an invariant of `startBuilderSession` (every non-resume branch writes
  it, and worktree-mode spawns are generated elsewhere and never reach it), not
  an assumption about the worktree.
- `reset/reorient.ts` said `.builder-role.md` is "the copy injected at spawn";
  resume now refreshes it too.
@waleedkadous

Copy link
Copy Markdown
Contributor Author

CMAP review (3-way)

Model Verdict Confidence
gemini APPROVE HIGH
codex COMMENT HIGH
claude APPROVE HIGH

No blocking issues from any reviewer. Four findings, all addressed:

codex — the quoting test wasn't testing anything. The fake agent logged
"$*", which renders one argument "two words" identically to two arguments
"two" "words" — so the assertion that the fresh command's "$(cat …)" survives
as a single argument proved nothing. Now logs "$@" one-per-|; verified
boundary-sensitive (--x|two words| vs --x|two|words|).

claude — the fake agent could hang instead of failing. Running past the
scripted exit codes hit exit "", which bash rejects with 255 — a crash, so
the loop auto-restarted and spun until the runner's 30s timeout. A future
regression that adds an unexpected relaunch should surface as a wrong invocation
list, not a hang. Defaults to 0 now.

claude — .builder-prompt.txt on the resume path. The fresh relaunch cats
a file the resume path never writes. This is an invariant of
startBuilderSession rather than an assumption about the worktree: every
non-resume branch writes it, and a resume is by definition a second launch into
a worktree a prior one set up (worktree-mode spawns, which have no prompt file,
are generated by buildWorktreeLaunchScript and never reach this function).
Documented in place rather than guarded — a runtime fallback would hide a broken
worktree rather than surface it.

claude — stale comment. reset/reorient.ts described .builder-role.md as
"the copy injected at spawn"; resume refreshes it now too. Corrected.

claude also flagged one unreproducible 6 failed run of the broader
src/agent-farm directory, then closed it itself: 3 clean full-directory runs,
10 clean isolated runs of the new file, clean main baseline. Environmental.

Full suite after the fixes: 4051 passed, 0 failed, 48 skipped (202 files).

@waleedkadous
waleedkadous merged commit 6e5d374 into main Aug 1, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Builder launch loop: clean-exit relaunch should rerun the harness fresh (no --resume), matching architect semantics from #1264

1 participant