pi-model-tools 0.6.0: hook-based tool interception + generic sandbox compatibility

[theGiallo]

(Also fixes README.md about Super Power Mode being off by default after 0.5.2.)

I've changed the approach on tool wrapping. What this extension does can be done with just the callback that can modify parameters and block the tool call, without the need to remove and re-define all tools.

I need this for an extension that I'm developing that runs the tools inside a VM, so I need to re-declare them, and this would make the two extensions incompatible.

I've also added the possibility to handle the case in which an apply_patch tool already exist or the case in which some extension declares that the tools are in a vm, not adding the apply_tool for this extension because it would circumvent the vm. Probably it could be possible to implement it as a call to the bash tool.

[bacnh85]

Review: pi-model-tools 0.6.0 — hook-based tool interception

Thanks for this — I verified every load-bearing SDK claim against the installed pi 0.84.2 internals (agent-session.js, runner.js, agent-loop.js, pi-ai/validation.js) and ran the full test suite + typecheck in a clean worktree: 217/217 tests pass, tsc --noEmit clean. Great work on the harness in index.test.ts — driving the extension through a stub ExtensionAPI is exactly the right shape.

Verified sound ✅

• First-registration-per-name is real — ExtensionRunner.getAllRegisteredTools() (runner.js:281) documents "first registration per name wins", and custom tools override builtins in _refreshToolRegistry. The PR's core motivation (wrapper re-registration clobbering/conflicting load-order-dependently) is accurate.
• Hook contracts — tool_call mutable-input + block (types.d.ts:688–691), tool_result content replacement (types.d.ts:796–801). In-place event.input mutation is consumed by the executor (same object reference).
• Sandbox auto-detection — getAllTools() returns the deduped registry with sourceInfo; builtins carry source: "builtin". Logic checks out.
• Edit repair is strictly stronger — pre-flight fixes all uniquely trim-matching edits before execution vs the old single-failing-edit retry, and now composes with a sandbox-owned edit. Same ambiguity guard (count !== 1 → leave untouched).
• Read-defaults drop — base read already applies offset ?? 1 (read.js:35). Correct removal.
• Sandbox gate — fail-safe, load-order independent (per-session snapshot).
• Super Power "off by default" doc fix matches guidance.ts:171.

Findings

HIGH — Argument repair for schema-invalid args is dead in production (6/10 repair kinds never run)

pi-agent-core/agent-loop.js:402-405 order is: prepareToolCallArguments → validateToolArguments → tool_call hook. validateToolArguments (pi-ai/utils/validation.js:280-307) throws on schema-invalid args; the throw is caught in prepareToolCall and becomes an immediate error result — the tool_call hook never fires for that call.

The old wrapper's prepareArguments ran before validation, so it could rescue schema-invalid input. These repair kinds can now never execute:

• top-level-json-string (the documented GLM fix), json-string, bare-string-array, empty-object-array, json-object-wrapped-array, truncated-json-closed

Only schema-valid repairs survive (path-markdown-autolink; optional-null is now redundant — the host's normalizeOptionalNulls covers it). Practical impact: DeepSeek/GLM emitting stringified-JSON args burn a turn on "Validation failed" + recovery hint instead of being silently repaired.

Why the tests don't catch it: index.test.ts drives handlers directly through the stub, bypassing the agent loop's validation stage entirely — and the one repair test covers exactly the kind that still works.

There's no pre-validation hook in the SDK, so this can't be fully recovered hook-side. Minimum for merge: document the regression in README ("argument repair applies only to schema-valid arguments") + CHANGELOG, and add a test pinning that schema-invalid input surfaces as a validation error (not a silent repair). Worth an upstream SDK issue for a pre-validation mutation point.

MED — README contradicts itself and the SDK about collision resolution

pi-model-tools/README.md "Why one package" says "pi.registerTool resolves name collisions by last-wins" — wrong. It's first-registration-per-name among extensions (runner.js:281), custom-over-builtin vs the host. Your own CHANGELOG and Sandbox section say it correctly; please fix the "Why one package" paragraph — the whole design rationale sits on it.

LOW — dead code: computeRetryEdit is now unused by the extension (only its tests import it); computePreflightEdits supersedes it. Delete it + its describe block, or keep with a ponytail: note.

LOW — inaccurate comment on the tool_result handler: "error categorization below still sees the original error text" — tool_execution_end fires with the finalized result (agent-loop.js:357-358), which includes the appended nearest-region text. No behavior bug (enrichment only appends, mismatch regexes still match the prefix) — just fix the comment.

LOW — isOwnToolSource substring false-positive: a foreign extension under a path containing pi-model-tools (e.g. a fork dir pi-model-tools-vm) is treated as ours → its VM-routed apply_patch gets blocked. Fail-safe direction, contrived trigger — acceptable, but the exact-path check could run first and short-circuit.

NIT — repair runs after edit pre-flight: a markdown-autolink path on edit gets repaired after pre-flight already tried (and failed) to read the contaminated path. Swap the two blocks.

Test gaps

• No test that schema-invalid args (e.g. edits as a JSON string) surface as validation errors with the hint injected — the HIGH regression above is unpinned.
• No test for readFileForRetry returning null mid-hook (file deleted between calls).

---

➡️ Please rebase against main

GitHub reports this PR as CONFLICTING with main (mergeable: CONFLICTING, mergeStateStatus: DIRTY) — likely from the recent 0.6.0-era churn on pi-model-tools/index.ts (ds-anchor work landed after your branch point). Please rebase and re-push:

git fetch origin
git rebase origin/main
# resolve conflicts in pi-model-tools/extensions/index.ts (keep both: your hook architecture + the ds-anchor/model_select + turn_end promotion changes)
npm test && npm run typecheck   # in pi-model-tools/
git push --force-with-lease

Once rebased, I'm happy to re-run the suite and re-review the conflict resolution. Verdict: approve-with-changes — architecture verified correct; merge blocked on the HIGH being documented or consciously accepted (plus the one-line README fix).

[bacnh85]

Review: pi-model-tools 0.6.0 — hook-based tool interception

Thanks for this — I verified every load-bearing SDK claim against the installed pi 0.84.2 internals (agent-session.js, runner.js, agent-loop.js, pi-ai/validation.js) and ran the full test suite + typecheck in a clean worktree: 217/217 tests pass, tsc --noEmit clean. Great work on the harness in index.test.ts — driving the extension through a stub ExtensionAPI is exactly the right shape.

Verified sound ✅

• First-registration-per-name is real — ExtensionRunner.getAllRegisteredTools() (runner.js:281) documents "first registration per name wins", and custom tools override builtins in _refreshToolRegistry. The PR's core motivation (wrapper re-registration clobbering/conflicting load-order-dependently) is accurate.
• Hook contracts — tool_call mutable-input + block (types.d.ts:688–691), tool_result content replacement (types.d.ts:796–801). In-place event.input mutation is consumed by the executor (same object reference).
• Sandbox auto-detection — getAllTools() returns the deduped registry with sourceInfo; builtins carry source: "builtin". Logic checks out.
• Edit repair is strictly stronger — pre-flight fixes all uniquely trim-matching edits before execution vs the old single-failing-edit retry, and now composes with a sandbox-owned edit. Same ambiguity guard (count !== 1 → leave untouched).
• Read-defaults drop — base read already applies offset ?? 1 (read.js:35). Correct removal.
• Sandbox gate — fail-safe, load-order independent (per-session snapshot).
• Super Power "off by default" doc fix matches guidance.ts:171.

Findings

HIGH — Argument repair for schema-invalid args is dead in production (6/10 repair kinds never run)

pi-agent-core/agent-loop.js:402-405 order is: prepareToolCallArguments → validateToolArguments → tool_call hook. validateToolArguments (pi-ai/utils/validation.js:280-307) throws on schema-invalid args; the throw is caught in prepareToolCall and becomes an immediate error result — the tool_call hook never fires for that call.

The old wrapper's prepareArguments ran before validation, so it could rescue schema-invalid input. These repair kinds can now never execute:

• top-level-json-string (the documented GLM fix), json-string, bare-string-array, empty-object-array, json-object-wrapped-array, truncated-json-closed

Only schema-valid repairs survive (path-markdown-autolink; optional-null is now redundant — the host's normalizeOptionalNulls covers it). Practical impact: DeepSeek/GLM emitting stringified-JSON args burn a turn on "Validation failed" + recovery hint instead of being silently repaired.

Why the tests don't catch it: index.test.ts drives handlers directly through the stub, bypassing the agent loop's validation stage entirely — and the one repair test covers exactly the kind that still works.

There's no pre-validation hook in the SDK, so this can't be fully recovered hook-side. Minimum for merge: document the regression in README ("argument repair applies only to schema-valid arguments") + CHANGELOG, and add a test pinning that schema-invalid input surfaces as a validation error (not a silent repair). Worth an upstream SDK issue for a pre-validation mutation point.

MED — README contradicts itself and the SDK about collision resolution

pi-model-tools/README.md "Why one package" says "pi.registerTool resolves name collisions by last-wins" — wrong. It's first-registration-per-name among extensions (runner.js:281), custom-over-builtin vs the host. Your own CHANGELOG and Sandbox section say it correctly; please fix the "Why one package" paragraph — the whole design rationale sits on it.

LOW — dead code: computeRetryEdit is now unused by the extension (only its tests import it); computePreflightEdits supersedes it. Delete it + its describe block, or keep with a ponytail: note.

LOW — inaccurate comment on the tool_result handler: "error categorization below still sees the original error text" — tool_execution_end fires with the finalized result (agent-loop.js:357-358), which includes the appended nearest-region text. No behavior bug (enrichment only appends, mismatch regexes still match the prefix) — just fix the comment.

LOW — isOwnToolSource substring false-positive: a foreign extension under a path containing pi-model-tools (e.g. a fork dir pi-model-tools-vm) is treated as ours → its VM-routed apply_patch gets blocked. Fail-safe direction, contrived trigger — acceptable, but the exact-path check could run first and short-circuit.

NIT — repair runs after edit pre-flight: a markdown-autolink path on edit gets repaired after pre-flight already tried (and failed) to read the contaminated path. Swap the two blocks.

Test gaps

• No test that schema-invalid args (e.g. edits as a JSON string) surface as validation errors with the hint injected — the HIGH regression above is unpinned.
• No test for readFileForRetry returning null mid-hook (file deleted between calls).

---

➡️ Please rebase against main

GitHub reports this PR as CONFLICTING with main (mergeable: CONFLICTING, mergeStateStatus: DIRTY) — likely from the recent 0.6.0-era churn on pi-model-tools/index.ts (ds-anchor work landed after your branch point). Please rebase and re-push:

git fetch origin
git rebase origin/main
# resolve conflicts in pi-model-tools/extensions/index.ts (keep both: your hook architecture + the ds-anchor/model_select + turn_end promotion changes)
npm test && npm run typecheck   # in pi-model-tools/
git push --force-with-lease

Once rebased, I'm happy to re-run the suite and re-review the conflict resolution. Verdict: approve-with-changes — architecture verified correct; merge blocked on the HIGH being documented or consciously accepted (plus the one-line README fix).