Skip to content

fix: restore Pi session tool loadout on resume - #436

Merged
oxwen11 merged 3 commits into
mainfrom
fix/restore-deferred-tools-on-resume
Oct 6, 2026
Merged

oxwen11 merged 3 commits into
mainfrom
fix/restore-deferred-tools-on-resume

Conversation

@oxwen11

@oxwen11 oxwen11 commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Requirement

When a session is resumed, the deferred MCP tools that tool_search loaded into it disappear, so the model can no longer call them (Tool mcp__deferred_tools__weather_lookup not found). Found while verifying #433. Extends the Pi 1.0.2 pnpm patch.

Expected behavior

Resuming a Session whose transcript declares a tool loadout:

  • restores that loadout, including MCP tools loaded by tool_search, once their servers reconnect;
  • keeps built-in tools the session had turned off turned off, including after compaction;
  • keeps extension tools that activate on registration, including ones from an extension installed after the session was created. This matches what Pi's reload() does.

New Sessions and an explicit --tools / --no-tools are unchanged.

Changes and risks

  • Root cause is in Pi 1.0.2, not Pie, and upstream 0.99.1 has the same failure.
    • createAgentSession always passes initialActiveToolNames and falls back to the default tools.
    • AgentSession restores the transcript loadout only when that value is undefined, so on this path the restore never runs.
    • As a result, _pendingToolNames, which re-adds tools after MCP reconnects, is never armed, and the first run records deferred tools as removed.
  • Fix: one hunk in patches/@earendil-works__pi-coding-agent@1.0.2.patch.
    • When the session uses the default tools (_usesDefaultTools, which is always the case in Pie), it restores the transcript loadout.
    • It also keeps the extension tools that are active at startup because they activate on registration.
    • A fresh session has no system message, so nothing is restored.
  • pnpm-lock.yaml: only the patch hash changes.
  • Review found that a transcript-only restore would drop extension tools added after the session was created. The second commit fixes that by keeping registration-activated extension tools, the same policy as reload().
  • Known tradeoffs:
    • An extension tool the session deliberately turned off comes back on resume, as it already does after reload().
    • A built-in tool newly added to the defaultTools setting is not turned on for older sessions.
    • Restored tools must re-register before the first prompt after resume (Pi design).
  • Should also go upstream to Pi; Pi 1.0.4 still has the bug.

Verification

Tested revision: 0183aa59.

  • resume-tool-loadout.test.ts runs the installed Pi cli.js over RPC. It drops bash and runs a turn, then resumes by --session-id with a newly installed extension tool (probe_tool). It asserts that bash stays off and probe_tool is active.
    • Fails without the patch (bash comes back).
    • Fails with the first patch version (probe_tool missing).
    • Passes now.
  • Built pie-pi-process bundle, with an isolated PI_CODING_AGENT_DIR and MCP fixtures, resumed the way Pie does:
    • the MCP harness passes all 18 checks, including the deferred tool being restored after reconnect;
    • the reviewer's probe script now shows probe_tool active after resume, and calling it returns probe ok.
  • pnpm check passes. Server tests: 647 passed, 1 skipped.
  • Not verified:
    • a real model or real MCP servers;
    • the reconnect race.

@oxwen11
oxwen11 added this pull request to stack #437 October 5, 2026 09:37
@pkg-pr-new

pkg-pr-new Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
npx https://pkg.pr.new/oxwen11/pie/@getpie/cli@436

commit: 0183aa5

Base automatically changed from chore/upgrade-pi-1.0.2 to main October 5, 2026 21:17
@oxwen11
oxwen11 force-pushed the fix/restore-deferred-tools-on-resume branch 3 times, most recently from 32c9ed9 to aff3ff7 Compare October 6, 2026 07:48
@oxwen11

oxwen11 commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Rebased onto main db2acfa after #433, #443 and #438 merged. New head: aff3ff7.

  • The Pi patch is regenerated on top of main's patch, which includes the bash exports from feat(server): background long bash commands after 60 seconds #390. The resume hunk is unchanged. pnpm-lock.yaml still changes only the patch hash.
  • Following refactor: emit ESM entries as .js and package tools/test #422's fixture convention, the fixture is renamed loadout-extension.mjs → .js and added to the untyped-fixture override in oxlint.config.mts.
  • Re-verified at this head:
    • pnpm check passes.
    • Server tests: 647 passed, 1 skipped.
    • The isolated MCP harness against the rebuilt bundle passes all 18 checks, including the deferred tool being restored after resume.

@oxwen11

oxwen11 commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Independent review: blocked (changes requested)

  • Reviewed head: aff3ff76e2e349bd51790357ba6639a3bce13e77
  • Base: main @ db2acfaed28ff308297ffcbfda1c66f72951c771
  • Trusted rules: origin/main @ db2acfae (pull-requests.md, review.md, acceptance.md, toolchain.md, security.md; identical on the PR head)
  • PR state at review: open, draft, MERGEABLE

1. CI

All required checks passed for aff3ff76: Check, react-doctor, and Publish @getpie/cli preview (x2).

2. Review: blocking finding

The restore replaces the startup loadout with the transcript's. Tools from extensions loaded at resume but not active at the session's last run are now dropped.

_restoreToolsFromTranscript() calls _setActiveTools(current.toolsAdded). That replaces the default-plus-extension set built by _buildRuntime({ includeAllExtensionTools: true }). Pie always takes the _usesDefaultTools path, so the patch extends this replacement from deferred MCP tools to every tool registered at load time. Before the patch, Pie resumed with every tool those extensions activate on registration.

Concrete failure, reproduced on the built pie-pi-process bundle with an isolated PI_CODING_AGENT_DIR/PIE_HOME and the fake e2e provider:

  1. Create a Session and run one turn. Its loadout is read,bash,edit,write,grep,find.
  2. Add an extension to the agent dir that registers probe_tool. This stands in for a newly installed Pi package or extension, or a new Pie built-in extension tool in a later Pie release.
  3. Resume the way Pie does: --session-id <id> --approve --no-extensions.
Build Tools after resume probe_tool call
base db2acfae read,bash,edit,write,grep,find,probe_tool probe ok
head aff3ff76 read,bash,edit,write,grep,find Tool probe_tool not found
head, new Session ...,probe_tool,... n/a

The same comparison against the installed dist/cli.js gives the same result: unpatched resume includes probe_tool, patched resume drops it. A user who installs a tool-bearing package and returns to an existing Session silently loses the tool, and Pie has no UI to re-enable it. The same applies to any extension tool Pie adds in a future release, for every pre-existing Session. Tools newly added to the defaultTools setting are also no longer applied on resume. MCP tools that register asynchronously after startup are not affected, because _refreshToolRegistry still activates them on registration.

This trades one dropped-tool bug for another, and the new one affects a wider class of tools. Possible directions, which need a Developer decision because they change resume semantics:

  • Restore only additively on the default-tools path. Keep the startup set and add the transcript's tools as pending. This loses deliberate removals, which is what the new test asserts.
  • Or keep tools that activate on registration and that the transcript never declared or removed.

Either way, add a regression test for "an extension tool introduced after the Session was created is active after resume", alongside the existing removal test.

Non-blocking notes:

  • The hunk patches dist/core/agent-session.js only. dist/bundle/chunks/*, used by the pi bin and ./rpc-entry, keeps the old condition. Pie imports dist/index.js and the built pie-pi-process.js contains the patched condition (verified), so this matches the existing patch convention.
  • Upstream Pi 1.0.4 (latest) still has the unpatched condition.
  • The PR body says it is stacked on chore: upgrade pi coding agent to 1.0.2 #433, but the base is main and the PR is still a draft.

3. Independent verification

Run in a clean reviewer worktree pinned to aff3ff76, with pnpm install --frozen-lockfile and turbo run build --filter=@getpie/server.... The build picked up the patched hash b50aff90….

  • resume-tool-loadout.test.ts passes. Negative control: the same test against a copy of the package with the hunk reverted fails (bash comes back), so the test detects the bug.
  • Server suite: 89 files passed / 1 skipped; 647 tests passed / 1 skipped.
  • MCP stdio and HTTP+OAuth fixture harness on the built bundle, Pie-style resume, RESUME_WAIT_MS=12000: every check passes on the head, including "deferred tool loaded before resume is restored after reconnect". On the base db2acfae build the same check fails with Tool mcp__deferred_tools__weather_lookup not found. OAuth, credential migration and the non-blocking first prompt pass on the head.
  • The new-extension regression is shown in the table above.

Gaps: no real model or real MCP servers (fixtures only). The reconnect race the PR describes was not exercised. I did not run pnpm check locally; CI Check passed.

Re-review trigger

A new head that resolves the dropped-extension-tool regression, or an explicit Developer decision accepting transcript-replace semantics for resumed Sessions.

@oxwen11
oxwen11 force-pushed the fix/restore-deferred-tools-on-resume branch from aff3ff7 to 0183aa5 Compare October 6, 2026 08:42
@oxwen11
oxwen11 marked this pull request as ready for review October 6, 2026 08:43
@oxwen11

oxwen11 commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Addresses the blocking finding in #436 (comment). New head: 0183aa5, base main bd1af63.

  • Choice of fix: resume keeps extension tools that activate on registration and still restores the transcript loadout. This is the same policy Pi's reload() uses.
    • Fully additive restore was rejected because it would bring back built-in tools the session had turned off.
    • Treating tools the transcript never declared as new was rejected because compaction collapses the transcript into one system message, so removed built-ins would look undeclared and come back.
  • Test: the regression test now also asserts that an extension tool added after the session was created is active after resume.
  • Results:
    • the test fails on both earlier patch versions and passes at this head;
    • your probe shows probe_tool active after resume, returning probe ok;
    • the MCP harness passes all 18 checks;
    • pnpm check passes; server tests 647 passed, 1 skipped.
  • Draft state: I marked the PR ready for review.

This needs a fresh independent review of the new head.

@oxwen11

oxwen11 commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Independent re-review: ready

This follows my earlier record, #436 (comment), which blocked aff3ff76. That record stays as written; this comment supersedes its conclusion for the new head only.

  • Reviewed head: 0183aa5987e87a5a916309ef79cd75cb74d0713d
  • Base: main @ bd1af63aa8bec3c1bd8e8d1fd57004e5868ba4dd. The base moved since the earlier record, but db2acfae..bd1af63a only touches app UI, skills and CI, not the server, the Pi patch or the rules.
  • Trusted rules: origin/main @ bd1af63a. .agents/rules is unchanged from db2acfae.
  • PR state: open, ready for review (no longer a draft), MERGEABLE.

1. CI

After waiting for every required check to finish on 0183aa59, all of them passed: Check, react-doctor, and Publish @getpie/cli preview (x2).

2. Review

The new hunk makes the following change, only when the session uses the default tools:

  • Before: _restoreToolsFromTranscript() replaced the active tools with the transcript's set.
  • Now: it restores the transcript's set plus every active tool that activates on registration and is not a base tool (!_baseToolDefinitions.has(name) && _isActivatedOnRegistration(name)).

The new-session path (initialActiveToolNames === undefined) and explicit --tools / --no-tools are untouched.

  • Earlier blocker resolved. Extension tools present at startup are kept, so a tool from an extension installed after the session was created is active after resume. This matches the base behavior for every extension tool.

  • Base-named overrides follow the transcript. Pie's bash override and fff's grep/find share names with base tools, so they keep the session's choices. A bash the session dropped stays dropped.

  • Deferred MCP tools still come back. They are not activated on registration, so they come from the transcript as pending tools and are re-added when their server reconnects.

  • Policy and tradeoffs are acceptable. The PR body states them. Compared with the base, the resulting behavior:

    • (a) is the same for extension tools: one turned off within a session comes back on resume, as it always did before this PR;
    • (b) is better for transcript-declared deferred tools and for base tools the session turned off;
    • (c) narrows one case: a base tool newly added to defaultTools is not activated for older sessions. Pie does not expose defaultTools.

    None of these is a security or data-integrity concern. I agree with the author's reasons for rejecting the alternatives, since a fully additive restore would undo removals and compaction collapses the transcript's declarations.

  • Non-blocking: sessions created outside Pie. A Session created by plain Pi without fff, then resumed in Pie, keeps that session's built-in grep/find choice. This follows from (b) and is consistent with the policy.

  • Non-blocking: unpatched bundle copy. As before, only dist/core is patched, not dist/bundle. Pie imports dist/index.js, and the built pie-pi-process.js contains the new branch (verified).

  • Security. No trust boundary, input, credential or host-write change. The fixture runs only in tests.

3. Independent verification

Run in a clean reviewer worktree pinned to 0183aa59, with pnpm install --frozen-lockfile and turbo run build --filter=@getpie/server.... The patch hash is 445289b9…, and the server build re-ran for this revision.

  • resume-tool-loadout.test.ts passes. Negative control: with the new branch removed from a copy of the package, the same test fails. The earlier patch version already failed the probe_tool case in my previous probe.

  • Server suite: 89 files passed / 1 skipped; 647 tests passed / 1 skipped.

  • Reviewer probe on the built bundle (isolated PI_CODING_AGENT_DIR/PIE_HOME, fake e2e provider, Pie-style resume --session-id --approve --no-extensions, extension installed after the session ran a turn):

    Build Tools after resume probe_tool call
    head 0183aa59 read,bash,edit,write,grep,find,probe_tool probe ok
    base bd1af63a read,bash,edit,write,grep,find,probe_tool probe ok
    aff3ff76 (previous record) no probe_tool Tool probe_tool not found
  • MCP stdio + HTTP/OAuth fixture harness on the built head bundle (RESUME_WAIT_MS=12000): all 18 checks pass. That includes "deferred tool loaded before resume is restored after reconnect", which failed on the base build in the previous record. OAuth, credential migration and the non-blocking first prompt also pass.

Gaps:

  • No real model or real MCP servers (fixtures only).
  • The reconnect race the PR describes (a tool that re-registers after the first prompt is dropped) was not exercised; it is unchanged Pi behavior.
  • I did not run the compaction scenario or pnpm check locally; CI Check passed.

Re-review trigger

Any new head commit, a base change that touches the Pi patch or the server harness, or a change to the stated resume policy.

@oxwen11
oxwen11 merged commit e8ad3fa into main Oct 6, 2026
6 of 7 checks passed
@oxwen11
oxwen11 deleted the fix/restore-deferred-tools-on-resume branch October 6, 2026 08:58
@oxwen11

oxwen11 commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Merged the independently reviewed and verified head 0183aa5 (record: #436 (comment)) onto base bd1af63 by squash. Merge commit: e8ad3fa.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants