Conversation
…pair Two independent defects made the --claude turn-end guard re-block a session forever instead of honoring its documented bound. 1. No session-lock awareness. A session that does not hold the home's session lock must not arm, drain, or repair supervision (AGENTS.md section 3), and fm-claude-stop-autoarm.sh refuses to arm from it on the same predicate. The guard nonetheless demanded that repair before allowing the stop, so a read-only session with work in flight re-blocked on every turn while the lock-owning session sat idle. Claude mode now allows the stop with an advisory whenever a different LIVE harness process holds state/.lock. The allow requires positive proof of a live foreign owner; a missing, malformed, or stale lock keeps the ordinary blocking path, so this does not widen into a general fail-open. 2. The bounded block budget did not bind. The count advanced only when the auto-arm epoch CHANGED, but a stalled auto-arm stops rewriting state/.claude-autoarm-epoch. With a frozen epoch the count never rose, the FM_CLAUDE_TURNEND_BLOCK_BUDGET ceiling was never reached, and the terminal attended fail-open became unreachable - the ceiling silently evaporated in exactly the stalled case it exists to cover. A genuine block now consumes one count per blocked stop. The allow paths keep per-epoch idempotence, so one event epoch still yields exactly one recovery turn, and a stop that was already accounted for on an allow path is not charged twice. The terminal fail-open still requires a verified failure episode; that requirement is deliberately unchanged. Verification: both new regression tests were confirmed failing against origin/main and passing against this change. Full tests/fm-turnend-guard.test.sh green at 66 assertions, 0 failures. bin/fm-lint.sh clean (shellcheck 0.11.0), bin/fm-doc-audience-check.sh ok.
The --claude foreign-lock allow decided "a different session owns this lock" with `! fm_session_lock_owned_by_self`. That helper also reports failure when this process's harness ancestry cannot be resolved, so the negation collapsed "someone else owns it" and "I cannot tell who owns it" into the same answer. With an unresolvable ancestry and state/.lock holding this session's own live harness pid, the guard allowed a blind stop for the one session permitted to repair supervision, while fm-claude-stop-autoarm.sh stayed inert on the same ambiguity: work in flight, supervision off, no signal. The header comment's claim that the allow "never widens into a general fail-open" was false. fm-session-lock-lib.sh now owns the decision through one lock-file reader and two evidence-based predicates. fm_session_lock_live_foreign_owner succeeds only on complete positive evidence: a numeric lock pid, a live harness at that pid, a RESOLVED ancestry for this process, and the lock pid outside it. A missing lock, a malformed lock, a dead owner, and an unresolvable ancestry all keep the ordinary blocking path. fm-claude-stop-autoarm.sh reads the lock through the shared fm_session_lock_stale_owner rather than the foreign-owner predicate, and this is deliberate. Its inertness on an empty or malformed lock depends on treating that as uncertainty; routing it through the foreign-owner predicate would send it down the recovery path to claim a lock nobody proved was free. The stale-owner form is outcome-equivalent to the code it replaces across all four cases (live foreign owner, unresolvable ancestry, dead pid, empty or malformed lock) while still removing the duplicated reader the two scripts had drifted apart on. The advisory no longer claims the lock owner restores supervision. An idle lock owner has no next turn, which is the incident this allow exists for, and under away mode the away supervisor owns the watcher. It now states only what was verified and takes its recovery step from fm-supervision-instructions.sh with the same --afk and --x-mode inputs the repair banner uses. Tests: the existing foreign-owner test did not control the condition it named. It ran the guard under a plain bash, so ancestry resolution depended on the host process tree - it resolved under a real session and did not on a CI runner, where the test passed through the fail-open branch instead of through positive foreign ownership. It now runs under the suite's existing fake-harness parent so the ancestry resolves. A new test pins the unresolvable case by running the guard beneath more plain shells than the walk's 16-hop bound, which is deterministic on every host, and asserts the guard blocks. Verified against a scratch copy carrying the pre-fix predicate: that test fails there with "expected exit 2, got 0".
Owner
|
Speaking as Kun's firstmate: closing this as stale. It has been waiting on a contributor update for 14+ days with no author push or comment. Reopen if you want to pick it back up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Raise the turn-end guard fix compliantly through the gate per CONTRIBUTING.md, so the repo's 'PR must be raised via no-mistakes' check passes. An earlier PR (#1592) for this same change was opened by hand with gh and fails that check.
The branch fixes three defects in bin/fm-turnend-guard.sh's --claude mode, in two commits.
Commit 1 (6938bc4), the original work:
Commit 2 (5b4b8ac), fixing what a previous run of this pipeline correctly caught in commit 1:
One deliberate deviation a reviewer should judge on its merits rather than as an oversight: bin/fm-claude-stop-autoarm.sh reads the lock through the shared fm_session_lock_stale_owner, NOT through fm_session_lock_live_foreign_owner. Its inertness on an empty or malformed lock depends on treating that as uncertainty; routing it through the foreign-owner predicate would send it down the lock-recovery path to claim a lock nobody proved was free. The stale-owner form was traced outcome-equivalent to the code it replaces across all four cases (live foreign owner, unresolvable ancestry, dead pid, empty or malformed lock) while still removing the duplicated lock-file reader the two scripts had drifted apart on.
Test work worth knowing about: the original foreign-owner test did not control the condition it named, because run_hook_claude invokes the guard under a plain bash and ancestry resolution then depends on the host process tree - it resolves under a real session and does not on a CI runner, where the test silently passed through the fail-open branch. It now runs under the suite's existing fake-harness parent. A new test pins the unresolvable case deterministically on every host by running the guard beneath more plain shells than the ancestry walk's 16-hop bound, and asserts the guard blocks.
Local verification already run against this exact head: bin/fm-lint.sh clean at pinned ShellCheck 0.11.0; bin/fm-test-run.sh tests/fm-turnend-guard.test.sh green at 36/36; and the new unresolvable-ancestry test was mutation-verified against a scratch copy carrying the pre-fix predicate, where it fails with 'expected exit 2, got 0'.
Process note: two earlier fix rounds of this pipeline died mid-edit to an upstream 'Connection closed mid-response' API error and discarded their work, so commit 2 was authored directly on the branch between runs rather than by a fix round.
What Changed
bin/fm-turnend-guard.sh--claudemode now allows a stop with a read-only advisory when a different live harness process holds this home'sstate/.lock, instead of blocking a session thatbin/fm-claude-stop-autoarm.shforbids from arming. The decision moves intofm_session_lock_live_foreign_ownerinbin/fm-session-lock-lib.sh, which requires complete positive evidence including a resolved harness ancestry, so a missing lock, malformed lock, dead owner, or unresolvable ancestry all keep the ordinary blocking path. The advisory's recovery step comes frombin/fm-supervision-instructions.shwith the same--afk/--x-modeinputsblock_stopuses, rather than promising the lock owner will repair supervision on its next turn.FM_CLAUDE_TURNEND_BLOCK_BUDGETceiling now counts blocks rather than epochs: a genuine block consumes one count per blocked stop even when a stalled auto-arm has frozenstate/.claude-autoarm-epoch, while a newBUDGET_ADVANCEDflag keeps a single stop accounted at most once in total and the allow paths keep their per-epoch idempotence.bin/fm-session-lock-lib.shgains a singlestate/.lockreader (fm_session_lock_pid) plusfm_session_lock_stale_owner, whichbin/fm-claude-stop-autoarm.shnow uses in place of its own duplicated inline lock parsing — deliberately the stale-owner predicate, not the foreign-owner one, so an empty or malformed lock leaves the auto-arm inert.tests/fm-turnend-guard.test.shadds coverage for the foreign-lock allow (under the suite's fake-harness parent), a host-independent unresolvable-ancestry block, a stale-lock block, and frozen-epoch budget exhaustion; docs for the guard, scripts, watcher continuity, and the supervision verification record are updated to match.Risk Assessment
✅ Low: The only outstanding defect from the prior round is fixed and independently verified by probe — the ancestry walk now binds to the fixture's own fake-claude pid rather than the host's real session, and the deliberately-collapsed control shape resolves nothing so the test fails loudly instead of silently passing — leaving a well-bounded change whose production logic fails closed on every incomplete-evidence path.
Testing
I ran the three targeted suites that own this change — tests/fm-turnend-guard.test.sh (including the four new --claude cases), tests/fm-claude-stop-autoarm.test.sh, and tests/fm-session-lock-ancestry.test.sh — and all passed with no failures or gate skips. Because passing tests alone do not show the incident behavior, I also drove the real Stop hook by hand in hermetic homes against the guard as it existed at the base commit, at commit 1, and at branch head, and captured the transcripts a Claude session would actually receive: the base guard blocks a non-lock-owning session with the blind-turn banner while head allows it with an evidence-only advisory; the commit-1 guard allows a blind stop when the harness ancestry cannot resolve (and still carries the disproved "ends its next turn" promise) while head blocks; and against a frozen auto-arm epoch the base guard blocks 6 out of 6 stops with its block count stuck at zero while head counts 1-2-3 and reaches the attended "SUPERVISION IS GENUINELY DOWN" fail-open. An away-mode run confirms the advisory's recovery step comes from the shared supervision-instructions line rather than a hand-written claim, and a five-state lock matrix confirms the auto-arm's new stale-owner reader decides identically to the inline code it replaced. This change is not UI-facing, so the reviewer-visible evidence is CLI hook transcripts rather than screenshots. The worktree is clean; all scratch fixtures were temp dirs removed on exit and only evidence files remain.
Evidence: Per-commit Stop-hook transcripts for all three defects (before vs after)
DEFECT 1 - foreign live session-lock owner --- before (4ee4a0a) --- exit=2 | ● TURN WOULD END BLIND - SUPERVISION IS OFF | ● 1 task(s) in flight, but no live watcher holds this home lock (last beat: never). --- after (adc0913) --- exit=0 | {"systemMessage":"FIRSTMATE SUPERVISION IS OWNED BY ANOTHER SESSION: ... Stay read-only here. Recovery belongs to whoever holds the lock: watcher supervision needs Stop-owned automatic recovery; inspect the hook registration and startup status before ending the turn."} DEFECT 2 - unresolvable harness ancestry is not proof of a foreign owner --- before (commit 1 only, 6938bc4) --- exit=0 | {"systemMessage":"FIRSTMATE SUPERVISION IS OWNED BY ANOTHER SESSION: ... The lock-owning session restores supervision when it ends its next turn. Stay read-only here."} --- after (adc0913) --- exit=2 | ● TURN WOULD END BLIND - SUPERVISION IS OFF DEFECT 3 - frozen auto-arm epoch vs FM_CLAUDE_TURNEND_BLOCK_BUDGET (default 3) --- before (4ee4a0a) --- stop #1..#6: exit=2 budget=count=0 epoch=3 (ceiling never arrives) --- after (adc0913) --- stop #1: exit=2 count=1 stop #2: exit=2 count=2 stop #3: exit=2 count=3 stop #4: exit=0 count=4 | {"systemMessage":"FIRSTMATE SUPERVISION IS GENUINELY DOWN: 1 task(s) in flight, the Stop-owned auto-arm exhausted its bounded retries and one failure notice, no watcher or automatic continuation exists, and the block budget is exhausted. Keep this session attended and diagnose the automatic Stop-hook and watcher startup before relying on unattended supervision."}Evidence: Away-mode foreign-lock advisory (recovery step sourced from fm-supervision-instructions.sh)
AWAY-MODE (state/.afk present) foreign-lock advisory, head adc0913 exit=0 | {"systemMessage":"FIRSTMATE SUPERVISION IS OWNED BY ANOTHER SESSION: this session does not hold this home's session lock, so it must not arm, drain, or repair supervision. Supervision is not running for this home right now, and this turn is allowed to end only because this session is the one that cannot repair it. Stay read-only here. Recovery belongs to whoever holds the lock: Away mode owns watcher supervision; load /afk and ensure the daemon is running instead of starting normal supervision directly."}Evidence: Auto-arm lock-reader outcome-equivalence matrix (the deliberate stale-owner deviation)
state/.lock | OLD inline reader | NEW stale_owner | guard's live_foreign_owner -------------------------------------------------------------------------------------------------------- live foreign harness pid | INERT (exit 0) | INERT (exit 0) | foreign owner dead pid | recover-lock | recover-lock | no proof empty | INERT (exit 0) | INERT (exit 0) | no proof malformed (not-a-pid) | INERT (exit 0) | INERT (exit 0) | no proof missing | INERT (exit 0) | INERT (exit 0) | no proofEvidence: Evidence capture scripts (reproduce the transcripts above)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
.agents/skills/bootstrap-diagnostics/SKILL.md- branch carries 1 commit(s) that exist on your local main branch but were never pushed to origin/main; rebasing would bundle this unrelated work (46 file(s)) into the PR:Push main to origin, or rebase your branch onto origin/main, before gating.
🔧 **Review** - 2 issues found → auto-fixed ✅
tests/fm-turnend-guard.test.sh:1572- run_hook_claude_under_fake_harness does not establish a fake-harness parent, so the foreign-owner test is still host-dependent — the exact defect the commit message claims to have fixed."$dir/fake-claude" -c 'bash "$FM_HOME/bin/fm-turnend-guard.sh" --claude'is a single simple command, which bash exec-optimizes: the fake-claude process is replaced in-place by the guard's bash, leaving no harness-named ancestor. Verified by probe under this exact invocation shape: fm_harness_ancestry_pids resolved to the host's realclaudepid (comm=claude), not to the fixture. Consequence: on a host with a live Claude session above the suite the test passes via the wrong ancestor; on a CI runner with no harness ancestor fm_harness_ancestry_pids fails, fm_session_lock_live_foreign_owner returns false, the guard falls to block_stop and exits 2, soexpect_code 0 ... must allow a stop it can never repairand the 'SUPERVISION IS OWNED BY ANOTHER SESSION' assertion both fail. Local 36/36 green does not predict CI here. Fix: keep the parent alive, e.g.-c 'bash "$FM_HOME/bin/fm-turnend-guard.sh" --claude; exit $?'— I verified that form preserves fake-claude as the parent. Note the comment cites run_integrated_autoarm (line 1072) as the mechanism being reused, but that pre-existing helper collapses the same way (bash also exec-optimizes the last command of a multi-command -c string); it is unaffected only because the pid it writes to state/.lock is dead by the time anything reads it.bin/fm-turnend-guard.sh:230- Tradeoff note, deliberate per the stated intent and AGENTS.md section 3: when a live lock owner is idle, neither session repairs supervision — the non-owner now allows the stop with an advisory and stays read-only, while the owner has no next turn to fire its Stop hook. The home therefore runs with tasks in flight and no watcher for as long as the owner process stays alive, with no escalation beyond the per-turn systemMessage. It self-heals only when the owner exits, at which point fm_session_lock_stale_owner lets the other session's auto-arm reclaim and arm. This is still strictly better than the ~15-turn un-endable block it replaces; recording it because the advisory's wording is now the only signal a human ever sees.🔧 Fix: keep the fake harness alive above the guard in tests
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-turnend-guard.test.sh— 57 checks green, including the four new --claude cases (foreign-lock allow, unresolvable-ancestry block, stale-lock block, frozen-epoch budget exhaustion)bin/fm-test-run.sh tests/fm-claude-stop-autoarm.test.sh— green, covering the auto-arm's shared-lock-reader paths (no lock, live foreign owner, dead recorded owner)bin/fm-test-run.sh tests/fm-session-lock-ancestry.test.sh— green, covering harness identification and live-vs-stale owner resolutionManual per-commit Stop-hook capture: guard binaries extracted from 4ee4a0a / 6938bc4 / adc0913 into hermetic FM_HOME fixtures, run as a real--claudeStop hook with a live harness-named process in state/.lock (capture-guard-transcripts.sh)Manual away-mode capture: same foreign-lock allow withstate/.afkpresent, confirming the advisory's recovery step is sourced fromfm-supervision-instructions.sh --afk 1(capture-afk-advisory.sh)Manual predicate matrix: auto-arm's pre-change inline lock reader vsfm_session_lock_stale_ownervsfm_session_lock_live_foreign_ownerover live/dead/empty/malformed/missing state/.lock (capture-lock-predicate-matrix.sh)docs/verification/supervision.md:158- The maintainer-verification record for the guard predicate is still the 2026-08-02 block covering the previous correction (bin/fm-lint.sh,bin/fm-doc-audience-check.sh, and a four-suiteFM_TEST_SUMMARY total=4 failed=0), and.agents/skills/firstmate-coding-guidelines/SKILL.mdrequires verification evidence to be updated when behavior changes. This branch changes the guard predicate again (live-foreign-lock allow, per-block budget accounting) and adds four tests, so that record no longer covers the current head; its recordedfm-doc-audience-check: ok surfaces=61 local_links=174line is also below the current 176 after this change. I did not author a replacement block because doing so honestly requires the exact command output from a lint and test run, and executing the pipeline's lint/test phases is outside this documentation phase. The intent already states the runs were made against this head (ShellCheck 0.11.0 clean,tests/fm-turnend-guard.test.sh36/36), so the pipeline's lint/test phase output can be pasted into a new dated paragraph in that section.🔧 Fix: refresh supervision verification record for current guard head
1 info still open:
docs/verification/supervision.md:158- Judgment call worth a reviewer's eye: I replaced the 2026-08-02 guard-predicate verification block rather than appending a second dated block beside it. The file's stated purpose is active empirical facts about current guarantees, so two dated records for the same guarantee would be duplication plus a stalelocal_links=174number. To make the supersede safe rather than lossy, I re-ran the exact same four-suite command the old block recorded (tests/fm-claude-stop-autoarm.test.sh,tests/fm-guard-stale-banner.test.sh,tests/fm-turnend-guard.test.sh,tests/fm-supervision-instructions.test.sh) at this head, so the new block covers identical ground, and I folded the old block's subject matter (auto-arm false-failure, monotonic bounded fail-open) into the new sentence alongside the session-lock split and per-block budget accounting. The two later 2026-08-02 paragraphs in the same section cover different guarantees and were left untouched. If you would rather keep the historical block verbatim and append, the new block is a self-contained unit that can be moved below it.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.