joshtronic/igor

Watch

fix(tick): don't ship a partial worktree when claude exits nonzero (igor#305) #307

Merged

joshtronic merged 2 commits from agent/305-ship-safety-gate into master

2026-07-01 15:49:10 +00:00

igor commented

2026-07-01 15:15:04 +00:00

Collaborator

What this PR does

fix: don't ship a partial worktree when claude exits nonzero (porksicle#114 / igor#305)
fix: correct the misleading "fallback via Haiku" log strings + stale comments (igor#306)

The bug (igor#305)

tick.sh captured CLAUDE_EXIT and logged it, then never checked it. The issue-work ship path committed and shipped whatever was dirty in the worktree regardless of exit code. On porksicle.com#110, claude implemented the change, ran git stash to capture "before" screenshots, then exited 1 while the stash was still active -- before git stash pop. The harness committed the only visible file (a leftover scratch screenshot script) and auto-merged PR #114, silently dropping the real edit and falsely resolving the issue.

The fix

Gate the harness commit on a clean exit. If CLAUDE_EXIT is nonzero the run died mid-workflow and the leftover tree is not a trustworthy diff, so we refuse to commit it. Safe because:

the issue worktree is created fresh each tick from origin/<base> (git worktree add -B) and torn down by the EXIT trap -- nothing durable is lost by not committing;
the issue re-queues via the recovery sweep, so a fresh run retries next tick;
a run that keeps crashing hits the existing noop -> Status/Blocked guard, so it can't loop.

The happy path (clean exit) is byte-for-byte unchanged -- the original commit block just moves into the else branch.

The Haiku red herring (igor#306)

The PR-body/commit-subject fallback has called $AGENT_MODEL (Sonnet), not Haiku, since AGENT_MODEL_THINKING was retired -- only the log string still said "Haiku." That false log is what tricked the logwatch pass into filing #306. Fixed the log lines + stale comments; no model change.

Test plan

bin/check-sync.sh passes (outcome-sync contract intact -- no new outcome; the nonzero-exit path reuses the existing noop/blocked handling)
bash -n bin/tick.sh clean
Reviewer: confirm the intent -- a nonzero claude exit should never ship, even if that occasionally discards a run with legit-but-incomplete work (retry is cheap; a wrong-diff merge is not).

Not in this PR (tracked follow-ups)

porksicle remediation: revert the stray chore: add script to capture before screenshots commit from porksicle master + reopen porksicle#110 (separate repo, own PR flow). Keeping igor#305 open until that lands.
deep root-cause of the exit-1 itself (igor#306 bullet a): why claude exits 1 mid-run. This PR makes it non-catastrophic; it doesn't stop it happening.
stale "Haiku" comments in ideation-pipeline.sh (same cosmetic pattern, different subsystem) -- trivial sweep, left out to keep this P0 focused.

Refs igor#305, igor#306.

## What this PR does - [x] fix: don't ship a partial worktree when claude exits nonzero (porksicle#114 / igor#305) - [x] fix: correct the misleading "fallback via Haiku" log strings + stale comments (igor#306) ### The bug (igor#305) `tick.sh` captured `CLAUDE_EXIT` and logged it, then **never checked it**. The issue-work ship path committed and shipped whatever was dirty in the worktree regardless of exit code. On porksicle.com#110, claude implemented the change, ran `git stash` to capture "before" screenshots, then **exited 1 while the stash was still active** -- before `git stash pop`. The harness committed the only visible file (a leftover scratch screenshot script) and auto-merged PR #114, silently dropping the real edit and falsely resolving the issue. ### The fix Gate the harness commit on a clean exit. If `CLAUDE_EXIT` is nonzero the run died mid-workflow and the leftover tree is not a trustworthy diff, so we refuse to commit it. Safe because: - the issue worktree is created **fresh each tick** from `origin/<base>` (`git worktree add -B`) and torn down by the EXIT trap -- nothing durable is lost by not committing; - the issue re-queues via the recovery sweep, so a fresh run retries next tick; - a run that keeps crashing hits the existing noop -> `Status/Blocked` guard, so it can't loop. The happy path (clean exit) is byte-for-byte unchanged -- the original commit block just moves into the `else` branch. ### The Haiku red herring (igor#306) The PR-body/commit-subject fallback has called `$AGENT_MODEL` (Sonnet), not Haiku, since `AGENT_MODEL_THINKING` was retired -- only the **log string** still said "Haiku." That false log is what tricked the logwatch pass into filing #306. Fixed the log lines + stale comments; no model change. ## Test plan - [x] `bin/check-sync.sh` passes (outcome-sync contract intact -- no new outcome; the nonzero-exit path reuses the existing noop/blocked handling) - [x] `bash -n bin/tick.sh` clean - [ ] Reviewer: confirm the intent -- a nonzero claude exit should **never** ship, even if that occasionally discards a run with legit-but-incomplete work (retry is cheap; a wrong-diff merge is not). ## Not in this PR (tracked follow-ups) - **porksicle remediation**: revert the stray `chore: add script to capture before screenshots` commit from porksicle master + reopen porksicle#110 (separate repo, own PR flow). Keeping igor#305 open until that lands. - **deep root-cause of the exit-1 itself** (igor#306 bullet a): *why* claude exits 1 mid-run. This PR makes it non-catastrophic; it doesn't stop it happening. - stale "Haiku" comments in `ideation-pipeline.sh` (same cosmetic pattern, different subsystem) -- trivial sweep, left out to keep this P0 focused. Refs igor#305, igor#306.

igor added 1 commit

2026-07-01 15:15:04 +00:00

fix(tick): don't ship a partial worktree when claude exits nonzero (igor#305)

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 5s

Details

bd383e7ad7

The issue-work ship path captured CLAUDE_EXIT but never checked it: the
harness committed and shipped whatever was dirty in the worktree
regardless of whether the run finished cleanly or died mid-workflow. When
claude exited 1 with the real edits stashed away (a `git stash` it took to
capture "before" screenshots, before restoring), the harness committed the
leftover scratch file and auto-merged it -- silently dropping the requested
change and falsely resolving the issue (porksicle#114 -> PR #114).

Gate the harness commit on a clean exit: if CLAUDE_EXIT is nonzero, refuse
to commit the partial tree. The worktree is disposable (freshly branched
from origin/<base> each tick, torn down by the EXIT trap) and the issue
re-queues via the recovery sweep, so a fresh run retries next tick; a
persistently-crashing run hits the existing noop -> Status/Blocked guard
and stops looping. The happy path (clean exit) is unchanged.

Also correct the misleading "fallback via Haiku" log strings and stale
comments: the PR-body/commit-subject fallback has called $AGENT_MODEL
(Sonnet), not Haiku, since AGENT_MODEL_THINKING was retired. The false
"Haiku" log line is what tricked the logwatch pass into filing igor#306.

Refs igor#305, igor#306.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011KXPafSYJPY6XkWHHk1Lfs

igor requested review from joshtronic

2026-07-01 15:15:05 +00:00

igor commented

2026-07-01 15:19:04 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for bd383e7a: success

Summary: Small, well-reasoned harness fix: gates the issue-work harness commit on a clean CLAUDE_EXIT, so a nonzero (mid-workflow) claude exit no longer commits/ships a partial worktree; plus cosmetic "Haiku"→$AGENT_MODEL log/comment corrections. CI is green, the diff is ~40 lines, and the checklist matches what's in the diff. I found no blocking defect, but the correctness of the fix hinges on things that live outside this diff, and the author explicitly asks a human to confirm the intent — so I'm keeping a human in the loop rather than rubber-stamping.

Checklist honesty: Both checked items are real and present in the diff (bin/tick.sh gate + the log/comment string swaps). No fabricated claims.

What looks right:

Happy path is preserved verbatim — the original git add -A / derive_commit_subject / git commit block is moved unchanged into the else branch. Good, minimal blast radius.
The safety argument (fresh worktree per tick, EXIT-trap teardown, recovery re-queue, noop→Status/Blocked loop guard) is coherent and the discard-on-nonzero-exit tradeoff is defensible.
The Haiku changes are genuinely cosmetic (log strings + comments); no model behavior change.

Things I can't verify from the diff (why this isn't an APPROVE):

bin/tick.sh:3864 — [ "$CLAUDE_EXIT" -ne 0 ] assumes CLAUDE_EXIT is set and numeric at this point. If it can ever be empty here, the arithmetic test errors (integer expression expected), which under set -e/errexit could abort or, worse, silently fall through to the commit branch — defeating the fix. The PR says it's captured-and-logged upstream, which is plausible, but the assignment isn't in the diff. Worth a human confirming it's unconditionally set before this line.
The claim that skipping the commit cleanly lands in the "noop/blocked path (COMMITS stays 0)" is entirely off-diff. I can't confirm nothing downstream (the PR-body/ship path) executes when the tree is dirty-but-uncommitted after a nonzero exit. The $AGENT_MODEL log-string references at lines ~3996/3999 also assume that var is in scope there — again plausible but not shown.

Test coverage: No automated test exercises the new branch — the test plan is bash -n (syntax only) + check-sync.sh. For a ship-critical harness gate, a regression guard (even a shell-level unit that asserts "no commit when CLAUDE_EXIT=1 and tree dirty") would be worth it, though I recognize this subsystem may not have an easy test seam.

Scope note (non-blocking): This bundles igor#306 (cosmetic log fixes) into an igor#305 P0 fix. Small and closely related, so acceptable, but strictly it's two issues in one PR.

For the human reviewer: confirm (1) CLAUDE_EXIT is always set/numeric by the time control reaches line 3864, and (2) the intent tradeoff the author flagged — a nonzero exit should never ship even if it occasionally discards legit-but-incomplete work.

_{Independent review by the harness on claude-opus-4-8. The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `bd383e7a`: **success** **Summary:** Small, well-reasoned harness fix: gates the issue-work harness commit on a clean `CLAUDE_EXIT`, so a nonzero (mid-workflow) claude exit no longer commits/ships a partial worktree; plus cosmetic "Haiku"→`$AGENT_MODEL` log/comment corrections. CI is green, the diff is ~40 lines, and the checklist matches what's in the diff. I found no blocking defect, but the correctness of the fix hinges on things that live outside this diff, and the author explicitly asks a human to confirm the intent — so I'm keeping a human in the loop rather than rubber-stamping. **Checklist honesty:** Both checked items are real and present in the diff (`bin/tick.sh` gate + the log/comment string swaps). No fabricated claims. **What looks right:** - Happy path is preserved verbatim — the original `git add -A` / `derive_commit_subject` / `git commit` block is moved unchanged into the `else` branch. Good, minimal blast radius. - The safety argument (fresh worktree per tick, EXIT-trap teardown, recovery re-queue, noop→`Status/Blocked` loop guard) is coherent and the discard-on-nonzero-exit tradeoff is defensible. - The Haiku changes are genuinely cosmetic (log strings + comments); no model behavior change. **Things I can't verify from the diff (why this isn't an APPROVE):** - `bin/tick.sh:3864` — `[ "$CLAUDE_EXIT" -ne 0 ]` assumes `CLAUDE_EXIT` is set and numeric at this point. If it can ever be empty here, the arithmetic test errors (`integer expression expected`), which under `set -e`/`errexit` could abort or, worse, silently fall through to the commit branch — defeating the fix. The PR says it's captured-and-logged upstream, which is plausible, but the assignment isn't in the diff. Worth a human confirming it's unconditionally set before this line. - The claim that skipping the commit cleanly lands in the "noop/blocked path (COMMITS stays 0)" is entirely off-diff. I can't confirm nothing downstream (the PR-body/ship path) executes when the tree is dirty-but-uncommitted after a nonzero exit. The `$AGENT_MODEL` log-string references at lines ~3996/3999 also assume that var is in scope there — again plausible but not shown. **Test coverage:** No automated test exercises the new branch — the test plan is `bash -n` (syntax only) + `check-sync.sh`. For a ship-critical harness gate, a regression guard (even a shell-level unit that asserts "no commit when `CLAUDE_EXIT=1` and tree dirty") would be worth it, though I recognize this subsystem may not have an easy test seam. **Scope note (non-blocking):** This bundles igor#306 (cosmetic log fixes) into an igor#305 P0 fix. Small and closely related, so acceptable, but strictly it's two issues in one PR. **For the human reviewer:** confirm (1) `CLAUDE_EXIT` is always set/numeric by the time control reaches line 3864, and (2) the intent tradeoff the author flagged — a nonzero exit should never ship even if it occasionally discards legit-but-incomplete work. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-01 15:19:05 +00:00

1 minute 8 seconds

igor referenced this pull request from a commit

2026-07-01 15:36:39 +00:00

harden(tick): fail-safe the ship-safety gate against an unset CLAUDE_EXIT

igor added 1 commit

2026-07-01 15:36:39 +00:00

harden(tick): fail-safe the ship-safety gate against an unset CLAUDE_EXIT

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

21acf2e75e

Follow-up to the shadow review on PR #307. The gate `[ "$CLAUDE_EXIT"
-ne 0 ]` assumed the var is always set/numeric -- true today
(CLAUDE_EXIT=$? runs right after the claude call), but a latent
fail-OPEN: if a refactor ever moved that assignment, an empty value
would make the arithmetic test error, read as false in the `if`, and
silently fall through to the commit branch -- the exact wrong-diff ship
this gate exists to stop.

Default to 1 (`${CLAUDE_EXIT:-1}`) so an unset/empty value is treated as
a crash and refuses to ship. A safety gate should fail safe, not open.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011KXPafSYJPY6XkWHHk1Lfs

igor commented

2026-07-01 15:36:58 +00:00

Author

Collaborator

Addressed the review. Good catch on the CLAUDE_EXIT fail-open — hardened the gate to ${CLAUDE_EXIT:-1} (21acf2e) so an unset/empty value fails safe (treated as a crash, refuses to ship) instead of falling through to the commit branch. The other two flags I verified off-diff: the fresh-per-tick worktree is branched from origin/<base>, so a skipped commit leaves COMMITS=0 and the ship path is never entered; AGENT_MODEL is a global in scope at the fallback. A shell-level regression test is a fair ask but needs a test seam extracted from the inline ship path — noting it as a follow-up rather than refactoring the critical path inside a P0.

Addressed the review. Good catch on the `CLAUDE_EXIT` fail-open — hardened the gate to `${CLAUDE_EXIT:-1}` (21acf2e) so an unset/empty value fails **safe** (treated as a crash, refuses to ship) instead of falling through to the commit branch. The other two flags I verified off-diff: the fresh-per-tick worktree is branched from `origin/<base>`, so a skipped commit leaves `COMMITS=0` and the ship path is never entered; `AGENT_MODEL` is a global in scope at the fallback. A shell-level regression test is a fair ask but needs a test seam extracted from the inline ship path — noting it as a follow-up rather than refactoring the critical path inside a P0.

igor commented

2026-07-01 15:39:18 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for 21acf2e7: success

Small, focused change to bin/tick.sh that (1) gates the harness sweep-commit on a clean CLAUDE_EXIT so a run that dies mid-workflow can't ship a partial/scratch worktree, and (2) corrects stale "Haiku" log strings/comments. The reasoning is sound and the checklist is honest — but a couple of things I can't verify from the diff, plus the author explicitly asks a human to confirm the intent, so I'm keeping this in front of you rather than approving.

What checks out

Checklist is honest: both checked items (ship-safety gate + Haiku log/comment fixes) map to real hunks in the diff. The one unchecked box is a reviewer-confirmation request, correctly left open.
Gate logic is correct: [ "${CLAUDE_EXIT:-1}" -ne 0 ] → 0 commits (else branch, byte-for-byte the old block), nonzero/unset skips. The :-1 fail-safe default is a good call.
In scope (~30 lines), no CI-config changes, no security surface, CI green.

Things I'd want you to weigh (can't confirm from the diff)

The 4000-line script's downstream is not visible. The PR asserts skipping the commit leaves COMMITS=0 and control "falls through to the noop/blocked path," and that the dirty worktree is torn down by the EXIT trap with "nothing durable lost." The diff at line ~3861 doesn't show the outcome-determination block, the EXIT trap, or the worktree teardown. In particular: does teardown git worktree remove --force? A left-dirty worktree that the old (commit) path always cleaned could now fail a non-forced removal or collide with next tick's git worktree add -B. I can't confirm either way — worth a glance.
No test for the new safety gate. This is a behavioral change to a critical ship path and nothing exercises "nonzero exit ⇒ no commit." bin/check-sync.sh + bash -n only cover contract-sync and syntax, not the new branch. If there's genuinely no harness for testing tick.sh runtime behavior, fine — but the core safety property is going in untested.
Intent tradeoff (the unchecked box). The gate discards any dirty tree on nonzero exit, including a run that did legit-but-incomplete work. Note the gate only affects the harness sweep-commit — if claude made real commits earlier in the run and then crashed, those committed changes are not covered by this gate and could still ship. That matches the reported bug (real edit was stashed, not committed) but is worth being explicit about if "never ship on nonzero exit" is the intended guarantee.

Minor / non-blocking

The skip log uses raw $CLAUDE_EXIT ("...claude exited $CLAUDE_EXIT mid-run...") while the gate uses ${CLAUDE_EXIT:-1}. If CLAUDE_EXIT is ever unset, the log prints an empty value ("exited mid-run") even though the gate correctly treated it as a crash. Cosmetic.

No blocking defect found, but the unverifiable downstream/teardown behavior, the absent test on a P0 ship path, and the author's own request for intent sign-off are enough that a human should take the final look.

_{Independent review by the harness on claude-opus-4-8. The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `21acf2e7`: **success** Small, focused change to `bin/tick.sh` that (1) gates the harness sweep-commit on a clean `CLAUDE_EXIT` so a run that dies mid-workflow can't ship a partial/scratch worktree, and (2) corrects stale "Haiku" log strings/comments. The reasoning is sound and the checklist is honest — but a couple of things I can't verify from the diff, plus the author explicitly asks a human to confirm the intent, so I'm keeping this in front of you rather than approving. **What checks out** - Checklist is honest: both checked items (ship-safety gate + Haiku log/comment fixes) map to real hunks in the diff. The one unchecked box is a reviewer-confirmation request, correctly left open. - Gate logic is correct: `[ "${CLAUDE_EXIT:-1}" -ne 0 ]` → `0` commits (else branch, byte-for-byte the old block), nonzero/unset skips. The `:-1` fail-safe default is a good call. - In scope (~30 lines), no CI-config changes, no security surface, CI green. **Things I'd want you to weigh (can't confirm from the diff)** - **The 4000-line script's downstream is not visible.** The PR asserts skipping the commit leaves `COMMITS=0` and control "falls through to the noop/blocked path," and that the dirty worktree is torn down by the EXIT trap with "nothing durable lost." The diff at line ~3861 doesn't show the outcome-determination block, the EXIT trap, or the worktree teardown. In particular: does teardown `git worktree remove --force`? A left-dirty worktree that the old (commit) path always cleaned could now fail a non-forced removal or collide with next tick's `git worktree add -B`. I can't confirm either way — worth a glance. - **No test for the new safety gate.** This is a behavioral change to a critical ship path and nothing exercises "nonzero exit ⇒ no commit." `bin/check-sync.sh` + `bash -n` only cover contract-sync and syntax, not the new branch. If there's genuinely no harness for testing tick.sh runtime behavior, fine — but the core safety property is going in untested. - **Intent tradeoff (the unchecked box).** The gate discards *any* dirty tree on nonzero exit, including a run that did legit-but-incomplete work. Note the gate only affects the harness sweep-commit — if claude made real commits earlier in the run and *then* crashed, those committed changes are not covered by this gate and could still ship. That matches the reported bug (real edit was stashed, not committed) but is worth being explicit about if "never ship on nonzero exit" is the intended guarantee. **Minor / non-blocking** - The skip log uses raw `$CLAUDE_EXIT` (`"...claude exited $CLAUDE_EXIT mid-run..."`) while the gate uses `${CLAUDE_EXIT:-1}`. If `CLAUDE_EXIT` is ever unset, the log prints an empty value ("exited mid-run") even though the gate correctly treated it as a crash. Cosmetic. No blocking defect found, but the unverifiable downstream/teardown behavior, the absent test on a P0 ship path, and the author's own request for intent sign-off are enough that a human should take the final look. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-01 15:39:18 +00:00

1 minute 22 seconds