fix(pi-fff): drop stale mode tool names on reload (#855) - #856
Conversation
Pi carries the active tool list across /reload, and registerPendingTools only added names, so the previous mode's search tools stayed active. Prune the names the final mode did not register: FFF names unconditionally (only this extension registers them), override names only when the session actually ran in override mode, so a user-enabled builtin grep/find is kept. Closes #855
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesReload tool reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Reloading after a mode switch now removes stale search tools while retaining applicable user-enabled builtin tools. The covered behavior is ready to merge. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant prepareSession
participant sessionModes
participant registerPendingTools
participant ActiveTools
prepareSession->>sessionModes: Read persisted fff-mode entries
sessionModes-->>prepareSession: Return modes in order
prepareSession->>registerPendingTools: Pass stale FFF and override names
registerPendingTools->>ActiveTools: Remove stale names and append registered names
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Closes #855
Root cause
registerPendingToolswas purely additive (packages/pi-fff/src/index.ts:659-665), while pi hands the pre-reload active list straight back to the rebuilt tool registry (dist/core/agent-session.js:2063:activeToolNames: this.getActiveToolNames()). A/reloadafter a mode switch therefore keeps the previous mode's names active, and both directions are affected onmain.Relevant detail: pi's default active set is
["read", "bash", "edit", "write"](dist/core/sdk.js:132) — builtingrep/findare registered but inactive. A leftovergrep/findin a FFF-named mode is genuinely the extension's stale activation.Fix
registerPendingToolsnow takes the names the final mode did not register and drops them from the active set. FFF names (ffgrep/fffind/fff-multi-grep) are pruned unconditionally — no one else registers them. Override names (grep/find/multi_grep) are pruned only when this session actually ran in override mode, determined from the startup-resolved mode plus every persisted/fff-modeentry, so a builtingrep/findthe user enabled viadefaultToolssurvives.Steps to reproduce
On pre-fix
main:Expected active tools after override ->
tools-and-ui+/reload:["read","bash","edit","write","ffgrep","fffind"].Actual on
main:["read","bash","edit","write","grep","find","ffgrep","fffind"]— four search tools, duplicated in pairs.Manual equivalent (TUI):
How verified
Three tests added modelling pi's real cross-reload behaviour (active list and session entries survive, extension instance does not): both switch directions, plus a guard that a user-enabled builtin
grep/findis not pruned when override was never used.typecheckstill reports the two pre-existingTS7006insrc/index.tsand the unbuilt@ff-labs/fff-noderesolution errors — identical onmain.@dmtrKovalenko one judgment call worth your eyes: telling the extension's
grep/findfrom pi's builtins is impossible by name, so the prune leans on mode history as the evidence. If you would rather have hard provenance, the alternative is persisting the activated names in a session entry.Overlaps #854, which touches the same function — the prune there becomes redundant with this one.
Automated triage via Gustav. Honk-Honk 🪿
Summary by CodeRabbit
/reloadmode switches so tools from a previous mode no longer remain active.