CI mirror: fix(pi): block the turn end from agent_end instead of reacting after settle - #3
Open
sbracewell64 wants to merge 5 commits into
Open
CI mirror: fix(pi): block the turn end from agent_end instead of reacting after settle#3sbracewell64 wants to merge 5 commits into
sbracewell64 wants to merge 5 commits into
Conversation
…settle The Pi primary turn-end guard hooked agent_settled, which fires after the agent loop has already exited. That emits the idle signal first, so anything watching for idle sees a blind turn end, and the forced follow-up only re-opens a turn that already ended. Pi drains both message queues before emitting agent_end, so a follow-up queued from an agent_end handler makes _handlePostAgentRun() return true and the session calls agent.continue(). The same run keeps going and agent_settled never fires until the guard stops queuing. Pi therefore blocks genuinely, like the Claude and Codex Stop hooks, rather than passively. Measured against pi 0.81.1 with a local mock provider: the shipped agent_end guard emits one idle signal, the agent_settled version emits two. A probe that re-queued from agent_end blocked 12 consecutive times with no ceiling, so the single-follow-up limit is Firstmate's latch and not a Pi limit. The re-entrancy latch is unchanged. Because agent_end also fires at auto-retry and compaction boundaries, a run needing one of those while supervision is already unhealthy spends its latched follow-up there instead of at the final boundary; the recovery instruction still lands in the same run, so this is an ordering nuance rather than a blind turn end. Adds tests/fm-turnend-guard.test.sh:test_pi_extension_blocks_before_the_idle_signal, which models Pi's loop and fails against the agent_settled version.
…ng agent_end path
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
Ship stage 0 of a scout investigation into making Pi the single harness: fix Firstmate's own Pi primary turn-end guard so it genuinely blocks a turn end instead of reacting after one.
Background the diff cannot show: an investigation established that Firstmate previously believed Pi could not block a turn end and could only force one bounded follow-up, which was a main reason Pi-for-Claude was rated a downgrade. That belief was wrong. Pi drains both message queues before emitting agent_end, so a follow-up queued from an agent_end handler makes _handlePostAgentRun() return true and the session calls agent.continue() - the run continues and agent_settled, the idle signal, never fires. The old guard hooked agent_settled, which fires only after that loop exits, so the idle signal escaped before the follow-up re-opened the turn. Measured with pi 0.81.1 against a local mock provider: the agent_end guard emits one idle signal, the agent_settled version emits two.
Deliberate decisions the captain scoped and approved, which may look surprising in the diff:
The new test test_pi_extension_blocks_before_the_idle_signal models Pi's real loop and was verified to fail against the agent_settled version.
What Changed
.pi/extensions/fm-primary-turnend-guard.tsnow runs the sharedbin/fm-turnend-guard.shpredicate fromagent_endinstead ofagent_settled. Pi drains both message queues before emittingagent_end, so a follow-up queued there makes_handlePostAgentRun()return true and the session callsagent.continue()— the run keeps going and the idle signal never fires. The oldagent_settledwiring ran after that loop had already exited, so the turn ended blind and the follow-up only re-opened it afterwards. Measured against pi 0.81.1 with a local mock provider: one idle signal with theagent_endguard, two with theagent_settledversion.agent_settledlistener is registered that does nothing but clear the single-follow-up latch.agent_endfires again only if the run continues, so a run that dies after the guard queued its follow-up (ESC, fatal session error, discarded queue) would otherwise leave the latch set and let the next logical run end unguarded. The existing one-follow-up latch is otherwise kept unchanged — no block budget was added, since Pi can block repeatedly (a probe blocked 12 consecutive times) and that is out of scope here.docs/turnend-guard.md,docs/architecture.md,bin/fm-turnend-guard.sh's header, and the harness-adapters skill now describe Pi as a direct blocker alongside Claude and Codex, with the auto-retry/compaction ordering nuance recorded rather than fixed.docs/verification/supervision.mdgains the pi 0.81.1 probe evidence in its existing Turn-end guard section, plus an honest note that the Node-driven Pi extension cases need a Node build with TypeScript type stripping (pre-existing onorigin/main, and unverified in CI).tests/fm-turnend-guard.test.shaddstest_pi_extension_blocks_before_the_idle_signal, verified to fail against theagent_settledversion, andtest_pi_extension_releases_the_latch_when_a_run_dies_before_continuing.Risk Assessment
✅ Low: The branch is now a single well-bounded event move backed by two behavioral regression tests, with the one real correctness gap fixed and every doc, skill, and code comment reconciled to the same mechanism; this round changed only prose.
Testing
Ran the targeted tests/fm-turnend-guard.test.sh suite (all 53 cases pass) after fixing the local environment blocker — the system Node is compiled without TypeScript type stripping, exactly the pre-existing limitation the change documents, so I ran the suite under an official Node 22.22.1 build. To prove the intent rather than just the assertions, I drove the real pi 0.81.1 CLI against a local mock provider with the actual tracked extension: with the shipped agent_end guard the predicate runs before the run can settle, Pi continues the same run, the FIRSTMATE_OP recovery instruction is delivered to the model, and the idle signal fires exactly once; with the base agent_settled version the run ends first, the idle signal fires twice, and the recovery instruction never reaches the model. I also confirmed the new regression test fails when pointed at the pre-change extension, and that the changed extension passes a strict typecheck against pi 0.81.1. The only failure observed anywhere was tests/fm-pi-primary-types.test.sh tripping on the untouched fm-calm.ts, which fails identically at the base commit.
Evidence: Real pi 0.81.1 before/after ordering transcript (agent_settled vs agent_end)
--- BEFORE: base a5fe1bc, guard hooked on agent_settled --- event order: LLM_REQUEST AGENT_END GUARD SETTLED SETTLED what the model was actually sent: --- model request 2 messages, last role=user --- [{"type":"text","text":"hello"}] -> The run ended, the guard only then ran, and the idle signal fired TWICE. No second LLM_REQUEST: the recovery instruction never reached the model. --- AFTER: 6d3f045, guard hooked on agent_end --- event order: LLM_REQUEST GUARD AGENT_END LLM_REQUEST AGENT_END SETTLED what the model was actually sent: --- model request 2 messages, last role=user --- [{"type":"text","text":"hello"}] --- model request 4 messages, last role=user --- [{"type":"text","text":"FIRSTMATE_OP: v1 turn-end-guard: TURN WOULD END BLIND - supervision is off. The watcher cycle is missing, failed, or unhealthy. Follow the harness recovery instruction below before ending the turn.\n\nrun bin/fm-watch-arm.sh as its own background task to repair supervision\n"}] -> The guard ran BEFORE the run could settle, Pi continued the SAME run, the recovery instruction was delivered, and the idle signal fired exactly ONCE. One GUARD line: the second AGENT_END consumed the latch without re-running the predicate, so there is no recursion./tmp/no-mistakes-evidence/01KYFKA5WDGW852V0H5MPMMRM6/pi-e2e)Evidence: New regression test fails against the pre-change agent_settled extension
Evidence: tests/fm-turnend-guard.test.sh full output (Pi cases at lines 38-42)
ok - .pi primary extension: agent_end forces one follow-up through the shared guard ok - .pi primary extension: guard blocks the run before agent_settled emits the idle signal ok - .pi primary extension: an interrupted run releases the latch without re-opening the guard ok - .pi primary extension: no-tool and multi-tool runs each inject exactly one guard follow-up ok - .pi primary extension: delivery failure resets the logical-run latchPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 5 issues found → auto-fixed (2) ✅
.pi/extensions/fm-primary-turnend-guard.ts:145- The re-entrancy latchguardFollowupActiveis now both set and cleared inside theagent_endhandler. Unlikeagent_settled— which pi emits from the agent loop'sfinallyand is therefore guaranteed once per logical run —agent_endis emitted peragent.prompt()/agent.continue()call, so it only fires again if the run actually continues. If the run terminates after the guard queued its follow-up but before anotheragent_end(user interrupt/ESC while the follow-up is still queued, a fatal session error, or pi discarding the queue), the latch staystruefor the lifetime of the process and the next logical run's turn end is silently skipped with no guard invocation at all — one unguarded blind turn end, exactly the failure class this guard exists to prevent. Under the previousagent_settledwiring the clearing event was guaranteed, so this robustness property regressed. The natural belt-and-braces fix (also clear the latch from anagent_settledlistener, keepingagent_endas the blocking hook) is currently forbidden by the newassert_not_contains "$content" 'pi.on("agent_settled"'assertion at tests/fm-turnend-guard.test.sh:784, so this needs an author decision rather than a silent edit.bin/fm-turnend-guard.sh:14- The shared guard's header comment still reads "OpenCode, pi, and grok adapters use the same predicate and force one bounded follow-up because their turn-end events are passive." This change reclassified Pi as a direct blocker indocs/turnend-guard.md:81("Claude, Codex, and Pi block the turn end directly, while OpenCode and Grok use bounded passive follow-ups"),docs/turnend-guard.md:64, and.agents/skills/harness-adapters/SKILL.md:55-56, but the guard script itself — the file every adapter calls, and the one that points readers atdocs/turnend-guard.mdfor per-harness mechanics — was left with the old claim. A reader landing in the guard first gets the exact wrong mental model of Pi's mechanism.tests/fm-turnend-guard.test.sh:878-idleEmittedBeforeFirstFollowUpcan never become true in this harness: it is only set inside thesendUserMessagemock,sendUserMessageis only reachable from the awaitedagentEndhandler, and everyagentEndcall completes beforeidleEmitted = trueon line 874. So the assertion "idle signal was emitted before the guard blocked" is dead code, and the property the test's name and comment advertise is not actually measured. What genuinely fails against theagent_settledversion is the handler-registration throw on line 862 — which duplicates the string assertion already added at line 783. Either drop the dead flag or restructure the mock so the guard's follow-up delivery and the settle emission are independently ordered (e.g. deliver the follow-up asynchronously and emit settle only when the queue drains), so the ordering claim is really exercised.docs/verification/supervision.md:64- The section intro still reads "validated across all five harnesses on 2026-07-08 through 2026-07-12, with Claude's replacement Stop-owned path revalidated on 2026-07-24" — it was extended for Claude's revalidation but not for Pi's. The table row directly below now records a 2026-07-26 measurement on pi 0.81.1 with a different mechanism ("Blockingagent_endcallback"), which falls outside the stated range. Add Pi's 2026-07-26 revalidation to the intro sentence so the date provenance stays self-consistent.docs/turnend-guard.md:44- "Pi listens foragent_end..., runs once per logical agent run" is now imprecise:agent_endfires at every continuation, auto-retry, and compaction boundary, as the same document acknowledges at line 69. What is once-per-run is the follow-up (bounded by the latch); thebin/fm-turnend-guard.shpredicate subprocess itself is now spawned at each of those boundaries rather than once at settle. Rewording to "injects at most one follow-up per logical agent run" would keep the bullet accurate and avoid implying the predicate cost is unchanged.🔧 Fix: release Pi guard latch on the guaranteed settle event
3 issues (1 warning, 2 infos) still open:
.agents/skills/harness-adapters/SKILL.md:285- This line still reads "listens for logical-runagent_end, not per-tool-loopturn_endand not post-settleagent_settled", which the latch-release fix made false:.pi/extensions/fm-primary-turnend-guard.ts:179now registerspi.on("agent_settled", ...). This is the agent-facing harness reference, so an agent reading it would conclude the settle listener is a bug and remove it, reintroducing the stuck-latch blind turn end the new test at tests/fm-turnend-guard.test.sh:894 exists to prevent. Reword to state thatagent_endis the blocking hook andagent_settledis registered solely to release the latch, mirroring the wording already added at docs/turnend-guard.md:68-69.docs/turnend-guard.md:95- The regression-coverage sentence lists "Pi logical-run latching, Pi blocking before the idle signal" but was not extended fortest_pi_extension_releases_the_latch_when_a_run_dies_before_continuing, even though the previous commit followed exactly this convention when it addedtest_pi_extension_blocks_before_the_idle_signalto the same list. Add the latch-release-after-an-interrupted-run case so the coverage inventory stays complete..pi/extensions/fm-primary-turnend-guard.ts:179- The unconditionalguardFollowupActive = falseis safe only because pi emitsagent_settledafter the whole agent loop exits, which the inline comment and the 12-block probe in docs/verification/supervision.md:114 both establish for pi 0.81.1. If a future pi emitted settle betweenagent.continue()iterations, the latch would clear mid-loop and the guard would re-block at everyagent_endwith no block budget to stop it (the budget was deliberately deferred per the captain's scope). No action needed now; the assumption is documented in code and evidence, and this is worth rechecking on the next pi version bump rather than defending against speculatively.🔧 Fix: document the Pi settle listener as latch release only
✅ Re-checked - no issues remain.
tests/fm-pi-primary-types.test.sh:8- tests/fm-pi-primary-types.test.sh fails in this environment, but only on the unchanged .pi/extensions/fm-calm.ts (TS2322 on renderResult, TS2345 on TerminalInputHandler) when typechecked against the installed pi 0.81.1. I confirmed this is pre-existing and unrelated to this change: the changed fm-primary-turnend-guard.ts typechecks clean both at 6d3f045 and at base a5fe1bc. The test also silently skips when tsc is absent, which is why it goes unnoticed locally. No action needed for this change.bash tests/fm-turnend-guard.test.sh— all 53 cases ok under an official Node 22.22.1 build with TypeScript type stripping, includingtest_pi_extension_blocks_before_the_idle_signal,test_pi_extension_releases_the_latch_when_a_run_dies_before_continuing, and the reworkedtest_pi_extension_injects_once_per_logical_agent_run/test_pi_extension_retries_after_followup_delivery_failuretest_pi_extension_blocks_before_the_idle_signalrun in isolation with ROOT pointed at the pre-change extension (git show a5fe1bc:.pi/extensions/fm-primary-turnend-guard.ts) →not ok - Pi guard must block the turn end before the idle signal fires, confirming the regression test genuinely fails on the agent_settled version (temporary driver deleted afterwards)Real-harness ordering probe:pi -p "hello" --provider mockp --model mock-model -ne -na --no-session -nbt -e mock-provider.ts -e <guard-variant>.ts -e probe.tsagainst pi 0.81.1 with a local OpenAI-compatible mock provider, isolated HOME/FM_HOME, and a stubbin/fm-turnend-guard.shreturning exit 2 — run for both the shipped agent_end guard and the base a5fe1bc agent_settled guardCaptured the exact user message the mock provider received in each run, showing theFIRSTMATE_OP: v1 turn-end-guardrecovery instruction reaching the model in the same run only with the agent_end guardStrict no-emit typecheck of.pi/extensions/fm-primary-turnend-guard.ts+lib/fm-operational-input.tsagainst the installed@earendil-works/pi-coding-agent0.81.1 (tsc 5.9.3, same tsconfig as tests/fm-pi-primary-types.test.sh) — clean for both the changed and base versionsReproduced the documented environment limitation: system Node 22.22.1 (node_use_amaro: false) fails the.tsextension import withERR_UNKNOWN_FILE_EXTENSION, matching the note added to docs/verification/supervision.md🔧 **Document** - 1 issue found → auto-fixed ✅
docs/verification/supervision.md:126- docs/verification/supervision.md states the four Node-driven Pi extension cases "pass on the CI runners". I could not verify that claim from this worktree: the local Node build has no TypeScript type stripping, so the cases abort locally, and I have no CI access. The surrounding paragraph is maintainer-verification material where every other claim carries a version, command, or observed output; this one carries none. Consider naming the CI Node version (or the workflow job) so the claim is checkable, or softening it to what was actually observed.🔧 Fix: record Pi test Node requirement as observed, CI unverified
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.