fix(pi): stop Calm claiming built-in tool names that refuse Pi's launch - #2
Merged
Conversation
Pi refused to start inside a Firstmate worktree whenever the operator's own Pi extension overrode a built-in tool name Calm also claimed, dropping the crewmate straight back to a shell with no agent running. Root cause, reproduced end to end against the operator's real global extensions and Pi 0.84.0: Calm registered wrapped ToolDefinitions for read/bash/edit/write/grep/find/ls, and Pi's ResourceLoader.detectExtensionConflicts runs one pass over the fully loaded extension set and records an error diagnostic for every later claim of an already-registered name. main.js exits the process with status 1 on any error diagnostic before the TUI opens. Both extensions stay loaded and precedence still follows load order, but the launch itself is refused. Firstmate's project-local extension is resolved first, so the operator's extension took the diagnostic and the whole session died. The reported observation predates the gate added in kunchenguid#1724, which is why it appeared with the preference off; that gate reduced the exposure to Calm-on sessions but left them refusing the launch outright. No in-extension check can avoid the conflict at registration time: getAllTools() throws before extension binding, Pi exposes no pre-bind list of the extensions it is about to load, and deferring to session_start loses restored tool rows, which Pi builds from a pre-session_start registry snapshot. Registration cannot be made both safe and complete, so Calm no longer registers a tool at all. lib/fm-calm-tool-row-layout.ts collapses the seven rows at render time through ToolExecutionComponent.render instead, reaching the same zero-height path the registered wrappers used. That removes the collision class entirely, applies to restored rows with no load-order constraint, lifts the previously documented bound against rows drawn before a first /calm activation, and restores two behaviors the wrappers dropped: Pi builds its own built-ins from the live session cwd and from the operator's read auto-resize-images and bash command-prefix and shell-path settings. Coverage is unchanged: the same seven names, disclosed images and third-party tool rows still visible, byte-identical stock rendering while Calm is off. The watcher and turn-end-guard extensions were checked too; only fm_watch_arm_pi is claimed, and a test now pins that every tracked extension stays inside the Firstmate namespace. Verified by launching the real Pi TUI in a worktree with the operator's own ~/.pi/agent/extensions present, with Calm off and with Calm on: both start clean, and both previously aborted. No operator-owned configuration was touched. tests/fm-calm-pi-extension.test.sh gains test_no_tool_name_claims, test_tool_row_collapse_without_name_claims, and test_launch_beside_foreign_builtin_extension, which drives the real pi executable through its own conflict-diagnostic path with no model turn and carries a positive control so it cannot pass vacuously. All three fail against the pre-fix extension. Also repairs a stale fixture stub that predated Pi 0.84.0's getMarkdownTransformers, which was failing test_rendering_and_session_lifecycle on the default branch.
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
2 participants
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 defect that makes Pi refuse to start inside a firstmate worktree when the operator has their own Pi extension registering the same built-in tools.
DELIVERY TARGET: this work ships to the captain's own fork, https://github.com/Bloh09/firstmate.git, which is now origin. It must never be pushed to, or opened as a PR against, kunchenguid/firstmate - the captain does not own that repository and has a standing rule against opening or merging PRs into repositories they do not own. A previous run of this exact work failed only at the push step with a 403 because origin was still the upstream repo; origin has since been repointed to the fork and the gate re-initialized. Nothing else about the change has altered.
This is a re-run of an already-validated change, preserved at head 915af5d. The prior run passed intent, rebase, review, test, document and lint, and its review and document fix commits are already part of this branch (60ef61b, 915af5d). Only push failed, so expect this to move quickly.
Reproduction the captain hit: spawning a Pi crewmate into a firstmate worktree printed 'Failed to load extension "~/.pi/agent/extensions/zz-read-only-mode.ts": Tool "read" conflicts with /.pi/extensions/fm-calm.ts' (and grep/find/ls), then dropped straight back to a shell with no agent running.
Diagnosis established with evidence, not assumption. The brief's leading hypothesis - that loadCalmPreference() cannot resolve config/calm from a crewmate worktree and falls back to on - was FALSIFIED: it catches and returns false, so an unresolvable preference means Calm off. The real cause is that Pi's ResourceLoader.detectExtensionConflicts makes one pass over the fully loaded extension set and records an ERROR diagnostic for every later claim of an already-registered tool name, and main.js exits status 1 on any error diagnostic before the TUI opens. Both extensions stay loaded and precedence still follows load order, but the launch is refused. Firstmate's project-local extension resolves before the operator's global one, so the operator's extension took the diagnostic and the session died. The captain's observation predates PR kunchenguid#1724 (verified by reproducing the exact output at 71f0b3f^); that PR narrowed the exposure to Calm-on sessions but left them aborting the launch outright, which was the live defect.
Requirements the captain set: (1) Calm off must claim nothing; (2) firstmate must never prevent Pi from starting, even with Calm genuinely on; (3) the same bug class must not recur silently in fm-primary-pi-watch.ts or fm-primary-turnend-guard.ts. Constraints: do not weaken Calm's presentation to dodge the collision; do not touch ~/.pi or any operator-owned configuration; do not change how any other harness launches.
Deliberate design decision, explicitly authorized by the captain's brief. The brief's preferred shape - have the load-time path do the same contested-name check as activateBuiltInsIfNeeded() - is provably unreachable: pi.getAllTools() throws before extension binding, Pi exposes no pre-bind list of the extensions it is about to load, and deferring registration to session_start loses restored tool rows because Pi builds them from a pre-session_start registry snapshot. Registration cannot be made both safe and complete. So Calm now registers NO ToolDefinition at all, and .pi/extensions/lib/fm-calm-tool-row-layout.ts collapses the seven built-in rows at render time by patching ToolExecutionComponent.render, reaching the exact zero-height path the registered wrappers used. This deletes the collision class rather than working around it. The large deletion in fm-calm.ts (wrapBuiltIn, wrappedBuiltIns, builtInsRegistered, contestedBuiltIns, activateBuiltInsIfNeeded, reportBuiltInLosses, realpathOrSelf) is intended: every one of those existed only to manage a collision that can no longer occur. The adapter's Pi import is deliberately type-only, with the runtime class reached through the namespace import, so the typeof probe stays the only failure path and fm-calm.ts can catch it and skip just this adapter - a static value import would fail at ESM link time and defeat that contract.
Intended behavior changes, all improvements, none accidental: rows restored before a first /calm activation now collapse too, lifting the previously documented non-retroactive bound; Calm's coverage now applies even when another extension owns the name, while that extension keeps its own execution; and Pi again builds its built-ins from the live session cwd and from the operator's read auto-resize-images and bash command-prefix/shell-path settings, which Calm's wrappers had silently discarded. Coverage is otherwise deliberately unchanged - the same seven names (bash, read, edit, write, grep, find, ls), disclosed built-in images still visible, third-party custom tool rows still visible, byte-identical stock rendering while Calm is off.
On requirement 3: fm-primary-turnend-guard.ts registers no tool, and fm-primary-pi-watch.ts registers only the namespaced fm_watch_arm_pi; a test pins that every tracked firstmate Pi extension stays inside the fm_/fm- namespace.
Verification and its constraint: the captain's Pi account has NO usage quota, so no Pi model turn was run and none may be. Acceptance is that Pi OPENS correctly. Verified by launching the real Pi TUI with cwd inside this firstmate worktree and the captain's real ~/.pi/agent/extensions in place (including zz-read-only-mode.ts), with Calm off and again with Calm on: both start with zero extension-load errors and stay up, where both previously aborted. Nothing under ~/.pi was modified. Automated coverage added in the repo's existing colocated test style: test_no_tool_name_claims, test_tool_row_collapse_without_name_claims, and test_launch_beside_foreign_builtin_extension, the last of which drives the real pi executable through its own conflict-diagnostic path with no model turn and carries a positive control so it cannot pass vacuously. All three fail against the pre-fix extension.
This is firstmate's own shared tracked material, so .agents/skills/firstmate-coding-guidelines/SKILL.md applies: docs/calm.md and docs/calm-mode-feasibility.md were patched in place as the one owner of the user-facing contract and the Pi-source evidence respectively.
One incidental repair is included because it blocked verifying this change: tests/fm-calm-pi-extension.test.sh had a fixture stub predating Pi 0.84.0's getMarkdownTransformers, already failing test_rendering_and_session_lifecycle on the default branch.
KNOWN PRE-EXISTING FAILURES, deliberately NOT fixed here and confirmed by the previous run to reproduce identically on base commit 70aeba8 without this change, in areas this change does not touch: test_operational_followup_turn_e2e ('Pi follow-up loaded_on case rendered a duplicate captain answer') and test_interactive_terminal_e2e ('/export did not complete while calm mode was on'). The captain's explicit instruction is to report those rather than absorb them, so do not expand scope to them.
What Changed
.pi/extensions/fm-calm.tsno longer registers anyToolDefinition. The seven built-in wrappers and the collision machinery they needed (wrapBuiltIn,wrappedBuiltIns,builtInsRegistered,contestedBuiltIns,activateBuiltInsIfNeeded,reportBuiltInLosses,realpathOrSelf) are deleted, so Pi'sdetectExtensionConflictscan no longer record a fatalTool "<name>" conflicts with ...diagnostic against an operator extension that claimsbash,read,edit,write,grep,find, orls. Coverage moves to a new.pi/extensions/lib/fm-calm-tool-row-layout.ts, which patchesToolExecutionComponent.renderto collapse those seven rows at render time, reuses Pi's own per-row image state so disclosed images stay visible, and is installed through the existinginstallCalmPresentationAdapterprobe so a missing seam degrades this adapter alone. Its Pi import is type-only with the runtime class read off the namespace import, keeping thetypeofprobe the only failure path.tests/fm-calm-pi-extension.test.shreplacestest_builtin_gate_load_timeandtest_calm_activation_collision_and_regression_boundwithtest_no_tool_name_claims,test_tool_row_collapse_without_name_claims, andtest_launch_beside_foreign_builtin_extension, the last driving the realpiexecutable beside a foreign extension claiming all seven names, with a positive control. The same file's fixture stub was updated for Pi 0.84.0'sgetMarkdownTransformers, which was already failingtest_rendering_and_session_lifecycle.tests/fm-pi-primary-types.test.shandtests/fm-pi-primary-live-e2e.test.shnow copy the new adapter into their fixtures.docs/calm.mdstates that Calm registers no tool, collapses rows regardless of which extension owns the name, and drops the previous non-retroactive first-toggle bound;docs/calm-mode-feasibility.mdrecords theResourceLoader.detectExtensionConflicts/main.jsexit evidence, why no in-extension check can avoid it, and extends the verified Pi range to 0.84.0;.agents/skills/harness-adapters/SKILL.mdpins thefm_namespace rule for tracked Firstmate Pi extensions.Risk Assessment
✅ Low: The change deletes a whole failure class rather than patching a symptom - Calm now registers no ToolDefinition, so Pi's fatal tool-name conflict diagnostic is unreachable from Firstmate - and the replacement render-time collapse reproduces Pi's own zero-height path exactly, degrades independently behind a probe, and is pinned by a real-
pi-binary launch regression test carrying a positive control; the only remaining exposure is the same undocumented-Pi-internal dependency the two pre-existing sibling adapters already carry, and both findings are cosmetic.Testing
I validated the fix at the level the captain actually hit it: the real Pi 0.84.0 TUI launching inside a Firstmate project beside the operator's own global zz-read-only-mode.ts. With the pre-fix extension restored from base 70aeba8, Pi printed the exact reported errors ('Tool "read" conflicts with .../fm-calm.ts', plus grep, find and ls) and dropped back to the shell with exit 1; with this change Pi opens and stays up with Calm off, with Calm already on at launch, and after /calm is toggled live, with both extensions listed in Pi's own [Extensions] banner and zero load errors. A before/after screenshot of those four real terminal states is the primary artifact. On top of that, the three new automated tests pass on HEAD and all three fail against the pre-fix extension, so the regression is genuinely pinned; the real-pi launch test also carries a positive control that I saw hold. No Pi model turn was run and nothing under ~/.pi was modified, per the captain's quota and configuration constraints. The two E2E tests the intent flags as pre-existing failures do still fail, but they fail identically on base 70aeba8, so they are not this change's doing; the touched type test gate-skipped locally because tsc is not installed, leaving that contract to CI. Transient fixtures and temp test drivers were removed and the worktree is clean.
/tmp/no-mistakes-evidence/01KZER916F3Y3RTBF9VR455RXC/pi-launch-evidence.png)Evidence: Same evidence as a rendered HTML page (ANSI-faithful terminal captures)
Evidence: BEFORE - pre-fix fm-calm.ts, Calm on: real Pi refuses to start
Error: Failed to load extension ".../agent/extensions/zz-read-only-mode.ts": Tool "read" conflicts with .../project/.pi/extensions/fm-calm.ts Error: Failed to load extension ".../agent/extensions/zz-read-only-mode.ts": Tool "grep" conflicts with .../project/.pi/extensions/fm-calm.ts Error: Failed to load extension ".../agent/extensions/zz-read-only-mode.ts": Tool "find" conflicts with .../project/.pi/extensions/fm-calm.ts Error: Failed to load extension ".../agent/extensions/zz-read-only-mode.ts": Tool "ls" conflicts with .../project/.pi/extensions/fm-calm.ts Hint: Start without extensions using "pi -ne". PI_EXIT=1Evidence: AFTER - this change, Calm off: Pi TUI opens with both extensions loaded
pi v0.84.0 [Extensions] algal/pi-openai-server-compaction:src, ask-user-question.ts, auto-compact-75.ts, codex-usage-bar.ts, context.ts, custom-header.ts, DietrichGebert/ponytail:pi-extension, filechanges, fm-calm.ts, fm-primary-pi-watch.ts, fm-primary-turnend-guard.ts, herdr-agent-state.ts, md-link.ts, memory.ts, pi-gpt-fast-mode, pi-herd.js, pi-web-access@0.14.0, usage-bar, zz-read-only-mode.ts ~/project $0.000 (sub) 0.0%/500k (auto) (xai) grok-4.5 - high Grok usage check failed: {"code":"personal-team-blocked:spending-limit", ...} <- no model turn was runEvidence: AFTER - /calm activated live in the running session
Tool output: collapsed (persisted preference after the toggle: config/calm = on; session remained up)Evidence: Targeted test run on HEAD 915af5d
ok - Calm claims no tool name in either preference state, and no tracked Firstmate Pi extension claims a tool name or CLI flag outside its own namespace ok - Calm collapses its seven built-in tool rows at render time without claiming a name, covers a row a foreign extension owns while leaving that tool's execution intact, keeps disclosed images and third-party rows visible, and restores byte-identical stock rendering when it turns off ok - the real Pi executable starts with no extension-load error or tool-name conflict, with Calm off and with Calm on, beside an extension that overrides all seven built-in tool names ok - Pi calm centralizes transcript visibility, preserves execution/export data, keeps Pi's stock working row visible while no run is active, and persists its choice across session startsEvidence: Same three tests run against the pre-fix extension from base 70aeba8 - all fail
not ok - Pi tracked-extension tool-name claim contract failed: Error: Calm registered ["read","bash","edit","write","grep","find","ls"] while config/calm was on not ok - Pi refused to start with Calm on beside an extension overriding the built-in tool names: Error: Failed to load extension ".../zz-foreign-builtins.ts": Tool "read" conflicts with .../fm-calm.ts (and bash, edit, write, grep, find, ls) not ok - Pi calm tool-row collapse contract failed: Error: Calm registered a tool; it must claim no tool name at all at activateBuiltInsIfNeeded (.../fm-calm.ts:347)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
.agents/skills/afk/SKILL.md- branch carries 72 commit(s) that exist on your local main branch but were never pushed to origin/main; rebasing would bundle this unrelated work (260 file(s)) into the PR:Push main to origin, or rebase your branch onto origin/main, before gating.
.pi/extensions/fm-calm.ts:44-calmPresentationHidesis left in the import list but is no longer referenced anywhere in fm-calm.ts - it was only used by the deletedwrapBuiltInrenderCall/renderResult bodies. Nothing in the pipeline catches it: tests/fm-pi-primary-types.test.sh's tsconfig setsstrictbut notnoUnusedLocals, and bin/fm-lint.sh is shellcheck-only overbin/*.shandtests/*.sh, so no TS linter runs over .pi/extensions. Drop the import line..pi/extensions/lib/fm-calm-tool-row-layout.ts:3- The file header states thatinstallCalmToolRowLayout()"probes that exact method and those exact fields and throws if any is missing", but onlyToolExecutionComponentand.prototype.renderare probed at install time (lines 66-72).imageComponents/imageSpacersare per-instance fields and cannot be probed then; they are checked at render time (line 82) and degrade via theimageShapeUnavailablelatch plus a console diagnostic, never a throw. Since this file's whole contract is how it fails, the header should describe the two-stage behavior rather than a single throw path.tests/fm-calm-pi-extension.test.sh- Pre-existing failure, NOT caused by this change:test_operational_followup_turn_e2ein tests/fm-calm-pi-extension.test.sh fails with "Pi follow-up loaded_on case rendered a duplicate captain answer". I confirmed it reproduces identically against base commit 70aeba8 with the pre-fix .pi/extensions in place, so this change neither introduces nor worsens it. Reported per the captain's explicit instruction not to absorb it into this change's scope.tests/fm-calm-pi-extension.test.sh- Pre-existing failure, NOT caused by this change:test_interactive_terminal_e2ein tests/fm-calm-pi-extension.test.sh fails with "/export did not complete while calm mode was on". I confirmed it reproduces identically against base commit 70aeba8 with the pre-fix .pi/extensions in place. Reported per the captain's explicit instruction not to absorb it into this change's scope.tests/fm-pi-primary-types.test.sh- tests/fm-pi-primary-types.test.sh gate-skipped locally ("skip: tsc not found for Pi extension typecheck"), so the type-only Pi import contract added in 60ef61b for .pi/extensions/lib/fm-calm-tool-row-layout.ts is not typechecked in this environment. Remote CI owns that check. No action needed locally.bash tests/fm-calm-pi-extension.test.shscoped totest_no_tool_name_claims,test_tool_row_collapse_without_name_claims,test_launch_beside_foreign_builtin_extension,test_rendering_and_session_lifecycle,test_pi_compat_no_upper_bound,test_pi_compat_degraded_adapter,test_pi_compat_missing_adapter_exports- all pass on HEAD 915af5dRegression proof:git checkout 70aeba8 -- .pi/extensionsthen re-rantest_no_tool_name_claims,test_tool_row_collapse_without_name_claims,test_launch_beside_foreign_builtin_extension- all three fail pre-fix; worktree restored cleanManual E2E: launched the realpi0.84.0 TUI under tmux with cwd in a project carrying the worktree's.pi/extensions, andPI_CODING_AGENT_DIRholding a copy of the captain's real global extensions includingzz-read-only-mode.ts- pre-fix + Calm on reproduces the four conflict errors and exits 1 with no TUIManual E2E: same launch with this change,config/calm=off- Pi TUI opens,[Extensions]banner lists bothfm-calm.tsandzz-read-only-mode.ts, zero extension-load errorsManual E2E: same launch with this change,config/calm=onat startup - Pi TUI opens with the same extension set and zero errorsManual E2E: typed/calmin the live session - Calm activates ("Tool output: collapsed"), session stays up, preference persists toconfig/calmConfirmed no Pi model turn ran (status bar shows the account'spersonal-team-blocked:spending-limitstate) and no file under~/.piwas written (mtime sweep)bash tests/fm-calm-pi-extension.test.shscoped totest_operational_followup_turn_e2eon HEAD and on base 70aeba8 - fails identically in bothbash tests/fm-calm-pi-extension.test.shscoped totest_interactive_terminal_e2eon HEAD and on base 70aeba8 - fails identically in bothbash tests/fm-pi-primary-types.test.sh- gate-skipped (tscnot installed)docs/calm-mode-feasibility.md:401- Judgment call, not fixed: docs/calm-mode-feasibility.md ends with a series of dated verification records (2026-07-23, 2026-07-26, two 2026-07-30), but this change added no dated record for the Pi 0.84.0 collision-free launch verification described in the intent. I did not author one because I do not have the captured launch transcript from that verification run, and the repo's maintainer-verification rule requires exact commands and exact output rather than reconstructed prose. The current guarantee is instead carried by reproducible automated coverage already named in the doc (test_no_tool_name_claims, test_tool_row_collapse_without_name_claims, test_launch_beside_foreign_builtin_extension) plus the 0.84.0 version statement in the Compatibility evidence section, which is arguably stronger than a one-time dated record. If the captain wants the dated record for parity with the earlier sections, it needs the real output from that run pasted in.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.