fix(pi): stop nested Pi CLI from replacing a live session binding - #60
Merged
Merged
Conversation
A short-lived child such as fm-spawn's pi --help probe was treated as lock-owned through ancestry and overwrote both markers with a pid that died immediately, causing false supervision alarms.
…-end ownership code
…s Pi change. They fail with the same tests on main at the base commit 09dc7b3 (CI run 35613893238). Two tests that failed here but not visibly on main (fm-kimi-harness and fm-spawn-compact-adviser-disable-remote) were in main's shard 9, which stopped at an actionlint download error before any test ran. Two regressions came in when the fork's own commits were rebased onto upstream on Sep 21. As you asked, I kept the previous agent's two partial fixes. I checked each one against the logs and locally, and made no other changes. 1. bin/fm-spawn.sh: restored the short staged launch line. Upstream kunchenguid#4994 (a452a79) writes the full launch command to a private file and types only `. <launch-file>` into the pane, because typed lines over about 1,024 bytes get cut off. The fork's #57 (a32fce8) put the old `spawn_send_literal "$T" "$LAUNCH"` back while resolving a merge, so it typed the whole command again. That broke fm-claude-trust, fm-backend-orca, fm-kimi-harness, fm-spawn-dispatch-profile, both fm-spawn-compact-adviser-disable suites, fm-remote-secondmate-trace-context and fm-remote-secondmate-parent-binding. The fix is one line that restores kunchenguid#4994's `spawn_send_literal "$T" ". $(shell_quote "$LAUNCH_FILE")"` and keeps #57's `SPAWN_LAUNCH_SENT=1`. 2. tests/fm-remote-reply.test.sh: the fixture also resolves the `default` key. Upstream kunchenguid#3764 added two decision lines with no key (`needs-decision [at=...]: which base branch?`), and these count under the key `default`. The fork's #48 made the automatic recovery repost wait while the mate has any open decision, so "the one automatic recovery repost was not sent". I confirmed this by printing the open decisions at that point: only `default needs-decision which base branch?` was open. Only the fixture's setup changed; every assertion is unchanged, and #48's wait-while-open rule still applies. Verification on macOS: all 8 spawn test files and fm-remote-reply pass through bin/fm-test-run.sh. With the spawn line reverted, fm-claude-trust fails with the same message as CI ("the launch command did not carry the brief the worker must read"). bash -n and shellcheck -S warning pass on both files. No Pi code was touched
tiago-peixoto
marked this pull request as ready for review
September 21, 2026 22:28
tiago-peixoto
added a commit
that referenced
this pull request
Sep 22, 2026
A short-lived child such as fm-spawn's pi --help probe was treated as lock-owned through ancestry and overwrote both markers with a pid that died immediately, causing false supervision alarms. Only the process the session lock names, or one about to claim a free or dead lock, may now publish the marker.
tiago-peixoto
added a commit
that referenced
this pull request
Sep 22, 2026
A short-lived child such as fm-spawn's pi --help probe was treated as lock-owned through ancestry and overwrote both markers with a pid that died immediately, causing false supervision alarms. Only the process the session lock names, or one about to claim a free or dead lock, may now publish the marker.
tiago-peixoto
added a commit
that referenced
this pull request
Sep 25, 2026
A short-lived child such as fm-spawn's pi --help probe was treated as lock-owned through ancestry and overwrote both markers with a pid that died immediately, causing false supervision alarms. Only the process the session lock names, or one about to claim a free or dead lock, may now publish the marker.
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
1 participant
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
Authorize repair of the verified false Pi supervision alarms caused by short-lived nested Pi commands replacing a live session binding, with regression coverage.
What Changed
fm-primary-pi-watch.tsandfm-primary-turnend-guard.tsnow write their loaded markers only when the state lock names this exact process, or when the lock is missing, invalid-free, or held by a dead pid. A short-lived Pi CLI child of a live session (such as fm-spawn'spi --helpprobe) no longer overwrites the markers with its own pid, which previously died and caused falsePI_WATCH_EXTENSION: not loadedand supervision-off alarms. Watcher arming in the watch extension still treats ancestry as ownership; the turn-end guard's now-unused ancestry walk (lockOwnership/parentPid) is removed.tests/fm-pi-watch-extension.test.shloads both extensions as a stubbed Node child and checks that a live ancestor binding is kept, that a free or dead lock is still claimed, and that a live lock naming the loading process still binds both markers. The new token-free live guardtests/fm-pi-nested-probe-marker-live-e2e.test.sh(registered inbin/fm-test-run.sh) runs the realpi --helpand a refused Pi-harness spawn against a live binding, and fails naming the installed Pi version.tests/fm-pi-primary-live-e2e.test.shnow writes the lock from a subshell thatexecspi, so the lock names the Pi process itself under the stricter rule, anddocs/verification/runtime-backends.mdrecords the fix and its verification against Pi 0.86.1.Risk Assessment
✅ Low: The change narrows one marker-writing condition, duplicated in the two Pi extensions, to match what session start already requires (marker pid equals lock pid, and fm-lock.sh records the Pi process itself), and it adds regression tests that fail against the old ancestry rule.
Testing
I ran the committed live guard against the real Pi 0.86.1 CLI; it passes on this branch and fails on the base code at the reported bug. The Pi watch-extension regression suite also passes on this branch, and its new nested-child test fails on the base code. I then drove a real Pi terminal session in a private tmux lab for both base and fix. Every step ran from inside the session with Pi's
!!shell command, so no model was involved and no tokens were spent. On base, the false alarm reproduced; on the fix, it did not across nestedpi --help, a refused Pi-harness spawn, and/reload. Evidence is CLI and terminal transcripts (this change has no visual UI surface). The lab was torn down and the worktree is clean.pi --helprun inside a live Pi primary session leaves the session's markers in place, and the next session start prints no PI_WATCH_EXTENSION alarm!!pi --help, and!!bin/fm-session-start.shprintedno PI_WATCH_EXTENSION alarm(tui-fix-pi-session.txt, live-pi-tui-before-after.md)pi --helpreplaces both markers with its dead pid and session start prints the false alarmPI_WATCH_EXTENSION: not loaded; fm_pi_extension_owns_supervision returned false (tui-base-pi-session.txt, base-…pi --helpprobe still runs) leaves the live binding in place and session start stays quiet/reload, and the watcher still arms/reloadwith lock=91873, both marker mtimes advanced and they still name 91873; the stub arm log shows arm calls from ppid 91873; session start afterward printed no alarm!!bin/fm-session-start.shprintedlock acquired: harness pid 91873andprimary harness: pi, with no alarm. The live guard's positi…pi --help replaced a live watch binding), unit-suite-fix.txt (exit 0), unit-suite-base.txt (`watch marker pid was replaced by a nested Pi pr…Evidence: Before/after table of the live Pi TUI lab (markers, lock, session-start alarm per step)
Source: Before/after table of the live Pi TUI lab (markers, lock, session-start alarm per step)
Evidence: Fix: full Pi TUI transcript (session start, nested pi --help, refused fm-spawn probe, /reload, no alarm)
Source: Fix: full Pi TUI transcript (session start, nested pi --help, refused fm-spawn probe, /reload, no alarm)
Evidence: Base: full Pi TUI transcript showing the false PI_WATCH_EXTENSION alarm after nested pi --help
Source: Base: full Pi TUI transcript showing the false PI_WATCH_EXTENSION alarm after nested pi --help
Evidence: Base: refused fm-spawn Pi probe transcript (markers replaced by the probe pid)
Source: Base: refused fm-spawn Pi probe transcript (markers replaced by the probe pid)
Evidence: Base: the false session-start alarm line
Source: Base: the false session-start alarm line
Evidence: Live guard on fix (3/3 ok)
Source: Live guard on fix (3/3 ok)
Evidence: Live guard on base (fails: pi --help replaced a live watch binding)
Source: Live guard on base (fails: pi --help replaced a live watch binding)
Evidence: Regression suite on fix
Source: Regression suite on fix
Evidence: Base vs fix session-start alarm
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (3) ✅
tests/fm-turnend-guard.test.sh:1203- Simplification: the two new turn-end tests (test_pi_turnend_nested_cli_does_not_replace_live_bindingandtest_pi_turnend_mark_loaded_claims_free_or_dead_lock) and their helpers (pi_turnend_extension_version,load_pi_turnend_extension_as_child, about 80 lines) repeat coverage the change already adds.tests/fm-pi-watch-extension.test.shhastest_pi_nested_cli_does_not_replace_live_bindingandtest_pi_mark_loaded_claims_free_or_dead_lock. Both load the samefm-primary-turnend-guard.tsin a child Node process, and both assert the.pi-turnend-extension-loadedpid for the nested case and for the free/dead-lock case. The intent asks for regression coverage, and one copy of that coverage is enough. The second copy doubles the fixture code that must change whenever the writer rule changes. Recommended remedy: remove these additions from fm-turnend-guard.test.sh, or keep them only if you deliberately want per-file ownership of the turn-end writer tests.tests/fm-pi-watch-extension.test.sh:96-fm_test_pi_extension_versionis a copy offm_pi_extension_versionfrom bin/fm-wake-lib.sh (the shasum/sha256sum/cksum ladder), andpi_turnend_extension_versionin tests/fm-turnend-guard.test.sh:1177 is a third copy. This same file already sources fm-wake-lib.sh insideassert_pi_supervision_bound, and the new live e2e calls the library function directly. Separately,assert_pi_supervision_bound(line 129) callsfm_pi_extension_loadedfor both markers right afterfm_pi_extension_owns_supervision, which already runs those exact checks against the same versions and lock. Suggested fix: seed the markers using the sourcedfm_pi_extension_versionand drop the two repeatedfm_pi_extension_loadedcalls. No behavior changes: if the hashing rule ever drifts, the tests fail loudly either way, so this only removes a parallel copy.🔧 Fix applied.
1 warning still open:
docs/verification/runtime-backends.md:2182- This line saystests/fm-pi-watch-extension.test.shandtests/fm-turnend-guard.test.shboth cover the new marker-writing rule with a stubbed Node child. That is no longer true. The previous fix round (commit 1f0b9ca) removed the turn-end copies of those tests from fm-turnend-guard.test.sh and did not update this line. The file now has no nested-child or free/dead-lock marker test. What still covers the turn-end marker istest_pi_nested_cli_does_not_replace_live_bindingandtest_pi_mark_loaded_claims_free_or_dead_lockin fm-pi-watch-extension.test.sh, which load both extensions and check both marker files. Anyone reading this verification record would believe the rule is pinned in a second suite that no longer has those tests. Fix: removeand tests/fm-turnend-guard.test.shso the line names only fm-pi-watch-extension.test.sh, and say that its tests check both the watch marker and the turn-end marker.🔧 Fix applied.
2 issues (1 warning, 1 info) still open:
tests/fm-pi-watch-extension.test.sh:4244- No test that runs by default checks the branch every real Pi session depends on: the lock names this process and that process is alive, so the marker must be written. The new rule in both extensions only allows that write because of the newlockPid !== String(process.pid)check. The new tests cover the other cases. The nested test puts the parent shell's pid in the lock, so nothing is written. The free/dead test uses a missing or dead lock, so the marker is written. The live guard's positive control also uses a free lock. The only test that runs a real session with a lock naming the Pi process is the edited tests/fm-pi-primary-live-e2e.test.sh, and it only runs on request (fm_live_gate opt-in FM_PI_LIVE_E2E). Failure case: a later edit drops or inverts the self-pid check. Every live Pi primary then stops writing its markers once fm-lock.sh records its pid (pidAlive(self) is true), and session start prints the same falsePI_WATCH_EXTENSION: not loadedalarm this change fixes. All default tests would still pass. The verification line in docs/verification/runtime-backends.md:2182 says this suite "pins the same writer rule", so it overstates what is covered. Fix: add a case in which the Node child first writes${process.pid}tostate/.lock, the way the existing tests at line 178 and nearby already do. Then import both extensions and assert that both markers name that pid. For example, add an env switch toload_pi_extensions_as_childor a third step intest_pi_mark_loaded_claims_free_or_dead_lock..pi/extensions/fm-primary-turnend-guard.ts:40- This change leaves dead code in the turn-end guard.markLoadedwas the only caller oflockOwnership(), and it now reads the lock itself. SolockOwnership()(lines 40-55), its helperparentPid()(lines 25-29, which runspsto walk parent pids), andtype LockOwnership(line 15) are now called only by each other. The watch extension still needs its own copy for arming, but this file no longer uses any of it. A reader will assume the turn-end guard still makes ancestry-based ownership decisions. Fix: delete those three definitions. KeeppidAliveand thespawnSyncimport, which the taskkill path at line 166 still uses. Behavior does not change.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
pi --helprun inside a live Pi primary session leaves the session's markers in place, and the next session start prints no PI_WATCH_EXTENSION alarm!!pi --help, and!!bin/fm-session-start.shprintedno PI_WATCH_EXTENSION alarm(tui-fix-pi-session.txt, live-pi-tui-before-after.md)pi --helpreplaces both markers with its dead pid and session start prints the false alarmPI_WATCH_EXTENSION: not loaded; fm_pi_extension_owns_supervision returned false (tui-base-pi-session.txt, base-…pi --helpprobe still runs) leaves the live binding in place and session start stays quiet/reload, and the watcher still arms/reloadwith lock=91873, both marker mtimes advanced and they still name 91873; the stub arm log shows arm calls from ppid 91873; session start afterward printed no alarm!!bin/fm-session-start.shprintedlock acquired: harness pid 91873andprimary harness: pi, with no alarm. The live guard's positi…pi --help replaced a live watch binding), unit-suite-fix.txt (exit 0), unit-suite-base.txt (`watch marker pid was replaced by a nested Pi pr…bin/fm-test-run.sh tests/fm-pi-nested-probe-marker-live-e2e.test.shon this branch (real Pi 0.86.1 CLI): 3/3 okSame live guard run against agit archive 09dc7b39tree: fails withnot ok - pi --help replaced a live watch binding (Pi 0.86.1)bin/fm-test-run.sh tests/fm-pi-watch-extension.test.shon this branch: exit 0, including the nested-child, free/dead-lock and self-lock testsThe newtests/fm-pi-watch-extension.test.shrun against base extensions: fails withnot ok - watch marker pid was replaced by a nested Pi processLive lab, fix tree: plainpiTUI in private tmux socketfm-lab-pimarker-fix, then!!bin/fm-session-start.sh,!!pi --help,!!bin/fm-session-start.sh,!!bin/fm-spawn.sh probe-x /nonexistent-project --scout --harness pi --backend tmux,!!bin/fm-session-start.sh,/reload,!!bin/fm-session-start.sh; marker, lock and mtime checks after each stepLive lab, base tree: same flow infm-lab-pimarker-base; the false PI_WATCH_EXTENSION alarm appears after the nestedpi --help; a restarted base session shows the fm-spawn probe also replacing both markersfm_pi_extension_owns_supervision(bin/fm-wake-lib.sh) run against the live lab state for fix and baseTeardown: both lab tmux servers killed, lab Pi processes confirmed exited, /tmp lab dir removed, worktree clean✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.