Skip to content

fix(bin): let a read-only session end its turn instead of re-blocking forever - #4552

Open
tknguyen29032002 wants to merge 2 commits into
kunchenguid:mainfrom
tknguyen29032002:fix/turnend-guard-readonly-session
Open

tknguyen29032002 wants to merge 2 commits into
kunchenguid:mainfrom
tknguyen29032002:fix/turnend-guard-readonly-session

Conversation

@tknguyen29032002

Copy link
Copy Markdown

Intent

The user (the fleet captain) was stuck in a second Claude Code session in the firstmate home whose every Stop was re-blocked by bin/fm-turnend-guard.sh --claude with 'TURN WOULD END BLIND - SUPERVISION IS OFF', dozens of times in a row, and asked in their own words: 'can you please check what happen leading to this and please solve this probelm always'.

Diagnosis that motivates this change: another Claude session (pid 11268) held state/.lock, so this session was correctly lock-refused and read-only per AGENTS.md section 3 (it may not arm, drain, spawn, steer, or repair supervision). The lock holder's watcher had died because Claude Code killed its Stop auto-arm at the 28800-second hook timeout; that half is already fixed upstream by #4474 (HUP/TERM/INT translated into a durable failure and exit-2 rewake) and is NOT part of this change. The read-only half is not fixed anywhere: the read-only session's own auto-arm is inert by identity, so the ledger never records a verified failure episode, the block budget never reaches the attended fail-open, and the guard re-blocks that session forever. Existing backlog item fm-turnend-guard-readonly asked for the same thing: state the problem, name the lock holder, allow the stop.

Decisions made deliberately: (1) Stand down only on POSITIVE proof that a different live verified harness holds the fleet lock - new fm_session_lock_held_by_other_harness in bin/fm-session-lock-lib.sh requires a resolved harness ancestry, a lock pid that is none of those ancestors, and a live verified harness at that pid; a missing, malformed, or dead lock and an unresolvable ancestry keep the ordinary block, and the lock owner's own Stop boundary is guarded exactly as before. (2) Applies in every guard mode, since blocking a read-only session can never cause recovery in any harness; only Claude mode prints a notice, as a stdout systemMessage like the existing terminal fail-open, shown once per session and lock holder via a small record state/.turnend-readonly-advised, because the captain explicitly complained about the repetition; other modes allow silently. (3) The stand-down spends none of the block budget and does not touch the failure episode. (4) The existing frozen-epoch fail-open test used a foreign live lock holder to freeze the ledger, which is exactly the read-only case this change now stands down on, so that test now freezes the ledger by the other documented inert cause (a Stop hook that never fires again) and keeps all of its budget and fail-open assertions; the now-unused run_integrated_autoarm_unowned helper was removed. (5) New regression test runs the guard under a fake harness-named parent so its ancestry is deterministic even when the suite runs inside a real harness; it fails against the old guard (verified: 'expected exit 0, got 2') and passes with the fix. Also verified end to end against a copy of the real home state from inside the stuck session: the old guard printed the blind banner, the new guard printed one notice naming pid 11268 and then stayed silent. Scope is deliberately narrow: no change to the auto-arm, the watcher, the block budget rules, or away mode.

What Changed

  • Added fm_session_lock_held_by_other_harness to bin/fm-session-lock-lib.sh. It prints the lock pid only when there is positive proof that a different session holds state/.lock. That means the current process's harness ancestry resolves, the lock pid is not one of its ancestors, and that pid is a live verified harness. A missing, malformed, or dead lock, or an ancestry that can't be resolved, returns failure.
  • bin/fm-turnend-guard.sh now uses that check to let a read-only session's stop through in every mode. This uses none of the block budget. In --claude mode it prints one systemMessage naming the lock holder, once per session and holder. Those pairs are recorded in state/.turnend-readonly-advised, which keeps the last 50 lines. Other modes allow the stop without a message. The lock owner's own Stop is still blocked as before.
  • Updated docs/turnend-guard.md and the AGENTS.md state listing to cover the new behavior and record file. In tests/fm-turnend-guard.test.sh, the frozen-epoch fail-open test now freezes the ledger with a Stop hook that never fires, instead of a foreign lock holder, and the unused run_integrated_autoarm_unowned helper is removed. New cases run the guard under a fake harness parent and cover the read-only stand-down, the notice shown once per holder, and the lock-owner and dead-holder blocks.

🤖 Generated with Claude Code

Risk Assessment

✅ Low: The stand-down happens only when there is positive proof that a different live harness holds the fleet lock, and every uncertain case keeps the old block. Other harness hooks (the Codex, Pi and OMP session-start checks and the Cursor park) already rely on the same ownership test from hook context, so the lock owner is still guarded. The fix-round's line-per-key notice record correctly shows the notice once per session and holder, and stays bounded.

Testing

I ran the three affected guard tests from tests/fm-turnend-guard.test.sh on HEAD, and all passed. The new read-only test fails against the base-commit guard, so it reproduces the original endless re-block. I also drove the real guard script (not the full test suite) end to end on a fixture home: a task was in flight, no watcher was running, and another live harness held the fleet lock. The old guard re-blocked every stop with the blind banner. The new guard showed one notice naming the lock holder and then stayed silent, including when two read-only sessions alternated. It spent none of the block budget and still blocked the lock owner. I also confirmed the notice record stays capped at 50 lines. Evidence is two text transcripts; no screenshot applies because the change is a CLI hook with no rendered UI.

Evidence: Read-only notice record stays bounded at 50 lines

Source: Read-only notice record stays bounded at 50 lines

seeded 60 lines; new session stop exit=0, notice shown: yes
record lines after: 50
last line: session=sess-new holder=30266
repeat stop exit=0, output: (none)
Evidence: New regression test against the base (old) guard
not ok - a read-only Claude session must be allowed to end its turn: expected exit 0, got 2

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 1 issue found → auto-fixed ✅
  • ⚠️ bin/fm-turnend-guard.sh:238 - The read-only notice record holds only one key (printf '%s\n' "$NOTICE_KEY" > "$READONLY_NOTICE"). The intent requires the notice to show "once per session and lock holder", but this breaks that when more than one read-only Claude session is open in the same home. Concrete sequence: session A holds state/.lock, and sessions B and C are both read-only. B stops, writes session=B holder=A and shows the notice. C stops, sees a different key, overwrites it with session=C holder=A and shows the notice. B stops again, the key differs again, and B gets the notice again. Each alternating Stop re-shows the systemMessage in both sessions, which is the repetition the captain complained about. Smallest fix, keeping the same file: treat the record as a set of lines for the current holder. Show the notice and append only when grep -qxF "$NOTICE_KEY" misses. Drop lines for other holders so the file stays small. This corrects the existing record's semantics and adds no new state.

🔧 Fix: Show read-only notice once per session across sessions
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • tests/fm-turnend-guard.test.sh filtered to test_hook_stands_down_while_another_live_session_holds_the_fleet_lock, test_hook_claude_mode_frozen_epoch_reaches_bounded_fail_open, test_hook_claude_mode_frozen_epoch_without_verified_failure_spends_budget_and_keeps_blocking on HEAD 795d3ac: all ok
  • Same new read-only test run against a git archive 2da3c5e tree (old guard): not ok - a read-only Claude session must be allowed to end its turn: expected exit 0, got 2, confirming it reproduces the bug
  • Side-by-side transcript on a fixture primary home with a task in flight, no watcher, and state/.lock held by a live harness-named process. Guard ran under a fake harness parent as fm-turnend-guard.sh --claude with stop_hook_active=true. Old guard: 3 stops, all exit 2 with TURN WOULD END BLIND. New guard: stop 1 exit 0 with one systemMessage naming the holder pid, stops 2-3 exit 0 silent. Second session B got its own notice once. A and B alternating stayed silent. No .turnend-claude-blocks was created. The lock owner's own stop still exits 2 with the banner
  • Bound check: seeded state/.turnend-readonly-advised with 60 lines. A new session got the notice, the record was trimmed to 50 lines ending with the new session=... holder=... key, and the repeat stop was silent
  • git status --short clean after removing the temporary /tmp runner scripts and the base-tree export
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

… forever

A session that did not get the fleet lock may not arm, drain, or repair
supervision, and its Stop auto-arm is inert by identity. When supervision was
down, the Claude turn-end guard still blocked every stop in that session: the
block budget only reaches its attended fail-open after a verified auto-arm
failure, which an inert auto-arm never records, so the session re-blocked with
the repair banner indefinitely.

The guard now stands down in every mode when fm_session_lock_held_by_other_harness
proves a different live harness holds state/.lock. Claude mode names the holder
in one systemMessage per session and holder. A missing, malformed, or dead lock
and an unresolvable ancestry keep the ordinary block, and the lock owner's own
Stop boundary is guarded exactly as before.

Claude-Session: https://claude.ai/code/session_01Hh9RANeMbhu28ZryHFQqRy

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 795d3ac8af

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread bin/fm-turnend-guard.sh
Comment on lines +239 to +243
NOTICE_TMP="$READONLY_NOTICE.tmp.$$"
if { tail -n 49 "$READONLY_NOTICE" 2>/dev/null; printf '%s\n' "$NOTICE_KEY"; } > "$NOTICE_TMP" 2>/dev/null; then
mv -f "$NOTICE_TMP" "$READONLY_NOTICE" 2>/dev/null || true
fi
rm -f "$NOTICE_TMP" 2>/dev/null || true

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep the read-only path free of state writes

When this branch is reached, the caller has positively been identified as a lock-refused session, yet it creates, replaces, and removes a file under the shared state directory. This violates the read-only-session boundary and lets a non-owner race home-state changes with the actual lock holder; emit the notice without persisting from the refused session, or arrange persistence through the lock owner.

AGENTS.md reference: AGENTS.md:L173-L174

Useful? React with 👍 / 👎.

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.

1 participant