fix(spawn): set PI_CLEAR_ON_SHRINK=1 for Firstmate-launched Pi sessions - #75
Merged
Conversation
This was referenced Aug 27, 2026
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
Fix the Pi black-regions rendering issue for Firstmate-launched Pi sessions according to the investigation report. Make every Firstmate-launched Pi and pi-signed process - crewmates, scouts, secondmates, and the documented primary launch - set PI_CLEAR_ON_SHRINK=1 beside FM_PI_HARNESS while preserving Pi's explicit terminal.clearOnShrink settings-level override. Add a unit-level launch-environment regression and an opt-in live regular-TUI regression that uses an isolated scratch Pi session with Calm on, expands and collapses a viewport-filling tool result, asserts no empty region remains above the next visible content, repeats around a real compaction rebuild, and retains existing hidden-thinking coverage. Document in docs/configuration.md and the harness-adapters Pi section that Calm-induced transcript shrink relies on PI_CLEAR_ON_SHRINK=1, why it prevents black regions, that Pi 0.84.3 does not fix the issue by itself, and the flicker and superseded-scrollback tradeoffs. Do not upgrade Pi or restart running secondmates; the setting applies on next launch. Deliver through the no-mistakes pipeline as one PR, do not merge, because the captain decides go-live.
What Changed
bin/fm-spawn.shnow prefixes every Pi-family launch command withPI_CLEAR_ON_SHRINK=1alongsideFM_PI_HARNESS, so crewmates, scouts, and secondmates on bothpiandpi-signedfully clear the terminal when the transcript render shrinks instead of leaving stale empty regions; the README primary-launch commands set the same variable.tests/fm-pi-clear-on-shrink-live-e2e.test.sh, an opt-in (FM_PI_CLEAR_ON_SHRINK_LIVE_E2E=1) real-Pi regular-TUI regression on an isolated scratch session with Calm on that expands and collapses a viewport-filling tool result, asserts no empty region remains above the next visible content, and repeats the check around a real/compactrebuild; registered it inbin/fm-test-run.sh's live-e2e family, and updatedtests/fm-spawn-dispatch-profile.test.shlaunch-shape assertions to cover the new environment variable.docs/configuration.mdand the harness-adapters Pi section: why Calm-induced shrink needs the setting, that an explicitterminal.clearOnShrinksetting still takes precedence, that Pi 0.84.3 does not fix it alone, and the flicker/superseded-scrollback tradeoffs.Risk Assessment
✅ Low: The functional change is a single environment-variable prefix on Firstmate's one Pi-family launch site plus two documented primary commands, verified against Pi's own settings-precedence implementation, with matching unit-level launch assertions, accurate documentation of the override and tradeoffs, and an opt-in live regression whose helpers and fixture I traced without finding a defect.
Testing
Ran the targeted unit launch-environment regression (all Pi/pi-signed launch identities carry PI_CLEAR_ON_SHRINK=1) and then drove the redesigned opt-in live regression against real Pi 0.84.2 in both directions: it fails consistently with PI_CLEAR_ON_SHRINK=0, leaving a 9-row empty region below the last rendered row after the render shrinks, and passes consistently with PI_CLEAR_ON_SHRINK=1 both before and after a real compaction rebuild, resolving the round-1 concern that the live test guarded nothing. Captured the two real tmux viewports at the shrink assertion point and rendered them side by side as a screenshot that shows the black region present pre-fix and gone post-fix; transient test copies were removed and the worktree is clean.
Evidence: Same comparison as rendered HTML
Source: Same comparison as rendered HTML
Evidence: Fail-before/pass-after write-up for the live regression
Source: Fail-before/pass-after write-up for the live regression
Evidence: Live test run with PI_CLEAR_ON_SHRINK=0 (pre-fix launch env)
Source: Live test run with PI_CLEAR_ON_SHRINK=0 (pre-fix launch env)
not ok - pre-compaction shrink left a 9-row empty region below the last rendered rowEvidence: Live test run with PI_CLEAR_ON_SHRINK=1 (Firstmate launch env)
Source: Live test run with PI_CLEAR_ON_SHRINK=1 (Firstmate launch env)
ok - Pi 0.84.2 regular TUI leaves no stale rows when the render shrinks, before and after compactionEvidence: Raw tmux pane capture at the shrink assertion, PI_CLEAR_ON_SHRINK=0
Source: Raw tmux pane capture at the shrink assertion, PI_CLEAR_ON_SHRINK=0
Evidence: Raw tmux pane capture at the shrink assertion, PI_CLEAR_ON_SHRINK=1
Source: Raw tmux pane capture at the shrink assertion, PI_CLEAR_ON_SHRINK=1
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
tests/fm-pi-clear-on-shrink-live-e2e.test.sh:195- assert_no_empty_region_before asserts on the exact frame captured the instant the marker first appears (wait_for_text returns on first grep hit), with no settle for the in-progress redraw. Every sibling live Pi assertion in tests/fm-calm-pi-extension.test.sh (e.g. lines 2135-2141, 2170-2178) first spins until "Working..." clears or the transition completes before measuring geometry. Concrete sequence: Pi streams NEXT_VISIBLE_CONTENT and the status/spinner rows in separate diff-line writes; the capture can land between the marker line and the rows above it, showing a transient blank run and failing with "left a N-row empty region" on a correct build. Add the same settle loop (re-capture until the spinner row is gone, then re-capture the snapshot) before both assertions.tests/fm-pi-clear-on-shrink-live-e2e.test.sh:69- The awkrunaccumulator is never initialized, so when the marker is the first line of the captured viewport the block prints an empty string.[ "$blank_run" -le 3 ]then aborts with a bash "unary operator expected" error and the fallback message reads "left a -row empty region above NEXT_VISIBLE_CONTENT" - a nonsense diagnostic for what is actually the zero-blank-rows (passing) case. AddBEGIN { run = 0 }.tests/fm-pi-clear-on-shrink-live-e2e.test.sh:178- The live test hardcodes PI_CLEAR_ON_SHRINK=1 in its own tmux launch rather than launching through fm-spawn, so it characterizes Pi's rendering with the variable set and would pass unchanged if bin/fm-spawn.sh:1241 were reverted. The launch-environment regression is carried entirely by tests/fm-spawn-dispatch-profile.test.sh, which matches the intent's requested split, so this is coverage shape rather than a defect. If the intent is a true fail-before-fix regression, a negative arm running the same expand/collapse sequence with PI_CLEAR_ON_SHRINK=0 and asserting the empty region does appear would pin that the variable is what prevents it.🔧 Fix: settle Pi viewport and init awk blank-run counter
1 warning still open:
tests/fm-pi-clear-on-shrink-live-e2e.test.sh:68- settle_viewport can return during the exact in-progress redraw it was added to exclude. This fixture runs Calm on ($HOME_DIR/config/calm = on, line 118), and .pi/extensions/fm-calm.ts:145-152 calls ui.setWorkingVisible(false) while showing the animated ship, so the stock "Working..." row never appears in the pane and the! tail -12 "$SNAPSHOT" | grep -Fq "Working..."guard (line 76) is always true. The only remaining protection iscmp -sover two frames captured 50ms apart (lines 73-77), but the Calm ship widget redraws on CALM_WORKING_SHIP_TICK_MS = 220 (.pi/extensions/lib/fm-calm-working-ship.ts:52 and 224). Concrete sequence: wait_for_text (line 214) returns on the first frame containing NEXT_VISIBLE_CONTENT while the run is still active; settle_viewport captures at t and t+50ms inside the same 220ms water phase; the frames are byte-identical, cmp succeeds, settle returns, and assert_no_empty_region_before (line 216) measures that transient frame and can fail with "left a N-row empty region above NEXT_VISIBLE_CONTENT" on a correct build. About three of four sample pairs fall inside a single tick, so the added settle removes little of the flake it targets. Fix: require stability across a window longer than one ship tick (sample more than 220ms apart, or N consecutive identical samples spanning >220ms), and gate on the Calm ship row being absent rather than on the stock "Working..." row. Same applies to the post-compaction call at line 231.🔧 Fix: gate live-e2e settle on Calm ship across ship tick
1 warning still open:
tests/fm-spawn-dispatch-profile.test.sh:684- InsertingPI_CLEAR_ON_SHRINK=1betweenFM_PI_HARNESS=$harnessand the executable makesassert_not_contains "$launch" "FM_PI_HARNESS=$harness $harness"unfalsifiable. If bin/fm-spawn.sh regressed to emitting a bare command name, the launch string would beFM_PI_HARNESS=pi PI_CLEAR_ON_SHRINK=1 pi --tui-mode regular ..., which does not containFM_PI_HARNESS=pi pi, so this guard passes on exactly the regression it was written to catch. The paired positive assertion at line 682 still covers the absolute-path case, so this is a dead guard rather than a coverage hole, but it should be re-anchored (e.g. onPI_CLEAR_ON_SHRINK=1 $harnesswith a trailing space) to stay meaningful.🔧 Fix: re-anchor bare-executable guard after clear-on-shrink assignment
3 infos still open:
tests/fm-pi-clear-on-shrink-live-e2e.test.sh:126- The fixture writes{"hideThinkingBlock":true,...}but the faux model is declaredreasoning: false(line 150) and no response usesfauxThinking, so no thinking block is ever produced and the setting is inert in this test. If the intent's "retains existing hidden-thinking coverage" meant this new live test should also exercise hidden thinking, it does not: add afauxThinking(...)block to one scripted assistant message and assert its text is absent from the pane. If it only meant "do not delete the existing hidden-thinking tests", that is satisfied (nothing was removed) and the setting is harmless noise. Flagging because resolving it changes the test's content, not just its mechanics.tests/fm-pi-clear-on-shrink-live-e2e.test.sh:69-SETTLE_SAMPLE_SECONDS=0.06xSETTLE_STABLE_INTERVALS=4= 0.24s is what makes the settle correct: it must exceedCALM_WORKING_SHIP_TICK_MS = 220(.pi/extensions/lib/fm-calm-working-ship.ts:53) so an animating Calm ship always breakscmpeven when thetail -12hull grep misses the widget row. That relationship is the entire reason the helper works and it holds today with only ~20ms of nominal margin, yet nothing in the file records it. A later tweak to either constant (e.g. dropping to 3 intervals, or 0.05s sampling) silently reduces the window below one tick and reinstates the exact flake this helper was added to remove. Add a comment naming CALM_WORKING_SHIP_TICK_MS and the >220ms requirement next to the two constants.tests/fm-pi-clear-on-shrink-live-e2e.test.sh:34-capture_viewportswallows tmux failure with|| true, leaving the previous contents in $SNAPSHOT. Insidesettle_viewportthat turns a dead or unreachable pane into an immediate pass: the marker is still present in the stale file, no hull is present, andcmptrivially succeeds four times in a row, after whichassert_no_empty_region_beforemeasures geometry from a frame the terminal is no longer showing. In practice the wrapper's trailingsleep 20keeps the pane alive for the assertion window, so this is a latent false-pass rather than an observed one; havingcapture_viewportreturn tmux's status and havingsettle_viewportresetstable=0on a failed capture closes it.🔧 Fix: drop inert hidden-thinking setting from live shrink test
2 infos still open:
tests/fm-pi-clear-on-shrink-live-e2e.test.sh:180- The fixture runs with Calm on ($HOME_DIR/config/calm = on, line 125), but the shrink it exercises comes from stock Pi's tool expand/collapse, not from Calm. Calm only hides tool rows inside wrapBuiltIn() (.pi/extensions/fm-calm.ts:283 for "assistant-tool-call" and :302 for "tool-result"), which wraps Pi's seven built-in tools;viewport_fillis registered via pi.registerTool in the fixture provider and is never wrapped, so its call and result render with Pi's stock renderer and Calm's calmPresentationHides() path is never taken for them. That matches the docs' "ordinary tool-collapse transitions" half of docs/configuration.md:34 and satisfies the intent's literal "Calm on" requirement, but the Calm-specific shrink (rows collapsing tonew Container(), or a /calm toggle repainting the transcript) is not covered by this test. Noting the coverage boundary; no change required for the stated acceptance criteria.tests/fm-pi-clear-on-shrink-live-e2e.test.sh:95- assert_no_empty_region_before resetsrunto 0 on every non-blank line, so it only measures the contiguous blank run immediately above the first line containing the marker. A leftover region separated from the marker by any single non-blank row - a status/spinner row, or staleBEFORE_COMPACT_ROW_*text the un-cleared shrink left behind - yields run=0 and passes. Concretely, a viewport ofBEFORE_COMPACT_ROW_042 / BEFORE_COMPACT_ROW_043 / NEXT_VISIBLE_CONTENTreports 0 blank rows even though the transcript is corrupt. This matches the reported symptom (black/empty regions, which tmux capture-pane renders as blank rows), so the assertion is aimed at the right artifact; flagging only that its detection window is one adjacent run rather than the whole region above the marker.🔧 **Test** - 1 issue found → auto-fixed ✅
tests/fm-pi-clear-on-shrink-live-e2e.test.sh:214- The new opt-in live regression tests/fm-pi-clear-on-shrink-live-e2e.test.sh does not discriminate the bug: running an unmodified copy of it against real Pi 0.84.2 with only the launched PI_CLEAR_ON_SHRINK value changed (1 vs 0; Pi compares strictly to "1", so 0 is a genuine pre-fix launch) passes in both cases, with byte-identical tmux viewports apart from the temp path in the footer (blank_run=2 above the next visible content in all four assertions). Captures taken immediately after the Ctrl+O collapse, before the follow-up message repaints, are also identical. I additionally tried isolated scratch Pi sessions with a 96-row tool result at 100x36 and 100x60 panes and could not make a large empty region appear with clear-on-shrink off, so I could not author a fail-before/pass-after live test either. The unit-level launch-env regression IS a proper fail-before/pass-after regression, and live evidence confirms the env var really flips Pi's terminal.clearOnShrink while an explicit settings.json value still overrides it — but no artifact demonstrates that the black regions themselves are prevented. The author needs to decide whether to strengthen or re-scope the live test.bash bin/fm-test-run.sh tests/fm-spawn-dispatch-profile.test.sh— all 31 cases pass on the target commitReverted only thePI_CLEAR_ON_SHRINK=1assignment inbin/fm-spawn.sh:1241and re-rantests/fm-spawn-dispatch-profile.test.sh— fails withnot ok - pi launch did not enable clear-on-shrink ..., then restored the fix (worktree clean)FM_PI_CLEAR_ON_SHRINK_LIVE_E2E=1 bash bin/fm-test-run.sh tests/fm-pi-clear-on-shrink-live-e2e.test.sh— passes against real Pi 0.84.2Ran an unmodified copy of the live e2e twice with the launchedPI_CLEAR_ON_SHRINKset to 1 and 0, capturing tmux viewports at each assertion and immediately after the Ctrl+O collapse — viewports identical, both passManual isolated Pi repro attempts (faux provider, 96-row tool result, 100x36 and 100x60 panes, expand/collapse) with clear-on-shrink on vs off — no black-region difference observedLive/settingsinspection in tmux forPI_CLEAR_ON_SHRINK=1,PI_CLEAR_ON_SHRINK=0, andPI_CLEAR_ON_SHRINK=1+settings.json terminal.clearOnShrink:false— shows true / false / falseCaptured realfm-spawnlaunch command lines for thepicrewmate andpi-signedpersistent secondmate showingFM_PI_HARNESS=... PI_CLEAR_ON_SHRINK=1bash bin/fm-test-run.sh tests/fm-calm-pi-extension.test.sh— existing Calm hidden-thinking coverage still passes🔧 Fix: make Pi clear-on-shrink live e2e reproduce stale rows
✅ Re-checked - no issues remain.
bin/fm-test-run.sh tests/fm-spawn-dispatch-profile.test.sh— 25 launch-shape tests pass, including the Pi/pi-signedPI_CLEAR_ON_SHRINK=1assertions and the re-anchored bare-executable guardFM_PI_CLEAR_ON_SHRINK_LIVE_E2E=1 bin/fm-test-run.sh tests/fm-pi-clear-on-shrink-live-e2e.test.sh— passes against real Pi 0.84.2FM_PI_CLEAR_ON_SHRINK_LIVE_E2E=1 bash tests/fm-pi-clear-on-shrink-live-e2e.test.shrepeated 4x with PI_CLEAR_ON_SHRINK=1 — 4/4 pass (flake check)Copy of the live test with only the launched value changed toPI_CLEAR_ON_SHRINK=0— 3/3 runs fail withpre-compaction shrink left a 9-row empty region below the last rendered row(fail-before/pass-after confirmed against real Pi)Instrumented copies of both directions dumping the rawtmux capture-paneviewport at the shrink assertion point; stale-row count measured as 9 (off) vs 0 (on)Rendered the two real viewports side by side to HTML and screenshotted it with headless Chrome for reviewer-visible proof of the black regiondocs/calm.md:52- docs/calm.md's "Regression entry points" list does not include tests/fm-pi-clear-on-shrink-live-e2e.test.sh. I deliberately left it out: docs/configuration.md#pi-calm-preference-configcalm owns the PI_CLEAR_ON_SHRINK fact and already names the opt-in command, and calm.md already points there as owner. Adding the command to calm.md would create a second copy to keep in sync. Flagging in case the maintainers prefer calm.md to be the exhaustive Calm regression index instead.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.