refactor(runtime): extract tool-call argument projections

[Adarsh-Me]

Summary

Re-scopes #4908's call-data extraction to the current production path after the rebase onto c87651e23. No intended public API, event, message, or durable-format change.

• Extract existing argument snapshotting, declared-schema validation, and permission/persisted projection into tool-call-snapshot.ts.
• Keep synchronous snapshotting and admission registration before the first await. Preserve direct-only rejection, deferred sandbox validation, and projection-error handling.
• Remove buildToolCallCommonFields and its input/output and mirrored activity-identity types. The rebased parent already has one event recipe; restore that recipe in executeTool() rather than introduce a single-use wrapper.
• Remove the unused modelFacingArgs alias. Persistence and model replay share persistedArgs; transcript messages are derived downstream, not independently assembled here.

The earlier description's two-record consolidation and failing stored-message regression claims do not apply to this rebased tree and are withdrawn.

Measurements

Physical lines, including comments and blank lines, against the actual rebased parent c87651e23:

Measurement	Parent	Revised PR
Common call-field recipes	1 inline	1 inline, unchanged
Argument projection construction	1 inline region	1 named operation
tool-runtime.ts	3,566	3,487
executeTool() region	822	814
New internal module	0	232
Net affected production lines	—	+153

This is an extraction, not a net line reduction. Test files are unchanged.

Verification

• Rebuilt @maka/core, @maka/storage, @maka/mcp, then @maka/runtime. Stale dependency declarations initially caused local type errors; rebuilding resolved them.
• npm run build --workspace=@maka/runtime — passed.
• npm run typecheck --workspace=@maka/runtime — passed.
• npx biome check packages/runtime/src/tool-call-snapshot.ts packages/runtime/src/tool-runtime.ts — passed.
• Compiled focused tests: tool-runtime-*.test.js, tool-args-violation, computer-use-args-violation, computer-use-privacy-boundary, model-history-timeline, and core computer-use-model-call-args — 115 passed, 0 failed. Includes real SQLite persistence/replay and argument-owner isolation.
• Re-ran tool-runtime-argument-ownership.test.js independently — 1 passed.
• Full npm run test:dist --workspace=@maka/runtime was attempted on Windows. It reported failures including ShellRunProcessManager PTY cases and stopped making progress; the run was terminated. The full suite is not claimed passing, and these failures have not been baseline-classified.
• Full desktop/E2E gates were not run locally.

AI use

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: ZCode assisted with the refactor, review-response edits, verification, and this description. Independent human review is still required.

Checklist

• Existing behavior coverage retained; focused argument ownership, settlement, persistence and replay tests pass.
• Changed-file lint/format, runtime build and typecheck pass locally.
• Full local runtime suite passes (see limitation above).

Does this PR entail a change in behavior?

• No intended behavior change.

[Adarsh-Me]

The CI failure on f312477 was real and mine — root-caused and fixed on c840671.

The shared common-field recipe emitted toolUseId as its own key. The tool_start event carries that field, but the persisted tool_call message names the same value id, and storage validates every appended message against an exact-shape schema that rejects unknown keys (hasExactShape in @maka/core/session). appendMessages therefore threw Invalid stored message schema on the first tool call, the turn-level error path generalized it to Operation failed (the redaction fallback), and the turn died before reaching the fixture tool. Found by temporarily un-generalizing the model-failure path and rerunning the integration test locally: the raw stack pointed straight at the append-time decode.

The fix keeps the shared recipe but stops it from emitting the call id: the recipe's input still takes toolUseId (both records need it), and each record names it at its own site — toolUseId on the event, id on the message — exactly as the two handwritten recipes did. Documented in the interface so the next consolidation cannot reintroduce it.

Re-verified locally: the previously failing tui-mcp-remote-integration test passes, and the four suites named in the issue are green (argument-ownership 1, settlement 12, model-loop 3, privacy-boundary 5 — 21/21). Line counts move by two: tool-runtime.ts 4,078, tool-call-snapshot.ts 310.

[hqhq1025]

复核 c84067161d553509646afbc885571fd0bdf97ce4，未发现有充分证据支持的 P0–P3 问题。技术检查通过；是否接受这次重构的代码量与边界取舍，仍需维护者决定。

实际改动是把参数快照、声明式校验和权限/模型参数投影提取到 tool-call-snapshot.ts，并把事件与存储消息的公共字段构造从两处合并为一处。原始参数仍在首次等待前冻结并完成步骤准入登记（tool-runtime.ts:1123）；直接调用限制、普通步骤拒绝和不可用沙箱界面的校验例外保持在原有控制路径（tool-runtime.ts:1133、tool-call-snapshot.ts:212）。Computer Use 的模型投影仍调用原来的转换函数（tool-call-snapshot.ts:230）。

公共字段构造器每次独立克隆 args 和 providerOptions（tool-call-snapshot.ts:291），事件和消息分别调用它（tool-runtime.ts:1235、:1256）。事件独有的 toolUseId 没有混入持久化消息，后者继续使用 id，符合 packages/core/src/session.ts:1191、:1489 的严格解码形状。数据库 schema 未改变，不涉及 migration。

实测规模为 tool-runtime.ts 4192→4078 行，executeTool 1096→1049 行，新文件 310 行，两文件合计净增 196 行。可验证的收益是消除一份重复字段构造，并提取已有数据边界；这不是净删代码，也没有证明性能收益。

验证通过：干净安装与 build:test、Runtime typecheck、两文件 lint/format、diff 检查，以及 123 项相关回归。回归包含真实 SQLite 提交/读取、持久化拒绝路径、参数校验、沙箱边界和 Computer Use 模型/隐私路径。另外通过生产 settleToolCall 入口，用可控 Promise 暂停验证了校验期间调用者修改原参数、执行期间修改投影，以及消息/事件互改参数和提供方元数据后的隔离。

边界说明：displayName: '' 现在会被保留，原代码会省略；持久化解码允许空串，已检查的正常生产工具生成器不会产生该值，主 UI 仍回退到工具名，因此未将其定为可达产品缺陷。此次没有运行原生 Computer Use 桌面操作或全仓测试；不能把上述结果扩展为任意外部自定义工具的全面兼容证明。对当前 main 492ff80f0f4abf3a917ce8f2c41b32fa9e3fc60c 的 Git 合并树检查无冲突，但未在合并树上运行测试。提交审查前该 head 的 hosted test 为 SUCCESS；没有可沿用的既有 review。

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[hqhq1025]

Reviewed exact head cab8050ccd757acc22d2592652a0d985e83668ef. This is a technical NO-GO because the rebase removed the duplicated production authority that the new common-field abstraction is meant to consolidate; see the inline P2.

The argument snapshot/validation extraction otherwise preserved the exercised behavior. Clean Node 24.18.1 install, build:test, full workspace typecheck, the full Runtime suite (3,490 passed / 13 skipped), 269 focused Runtime/CLI tests, Biome, ASF header checks, and git diff --check passed. A conflict-free synthetic merge onto current main c980b93a37ad38fb78f255b6fffcf844e125f64c also passed build:test, full typecheck, and the same 269 focused tests. Hosted test is green.

I did not run a live provider request or native Computer Use interaction.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

[hqhq1025]

[P2] Re-scope this helper to the current single-authority path

After the rebase onto c87651e2, executeTool() no longer constructs or appends a ToolCallMessage; tool_start is mapped to the RuntimeEvent ledger and the transcript message is derived later. Consequently this helper now has one invocation (tool-runtime.ts:1197), not two, toolUseId in its input is never read, and modelFacingArgs is assigned at tool-runtime.ts:1120 but never consumed. The exact parent already has one call-field recipe, so this adds a 310-line module (+196 net production lines) without consolidating the advertised duplicate; it also reintroduces the redundant model-facing alias removed by #4971. The prior commit b1b4684f passes the claimed failing tui-mcp-remote-integration on this rebased tree, consistent with there being no stored-message construction here. Please remove the stale event/message contract and redundant view, or re-scope the extraction around an actual current production owner.

[Adarsh-Me]

ed8605106 took this: the module is now 232 lines (was 307), tool-runtime.ts is −105/+26, modelFacingArgs is gone from this path, and toolUseId only feeds toolCallId. Your "redundant model-facing alias" and "two callers" points no longer apply at this head.

What is still true: one production caller, and no direct test of the module. Two ways forward — say which you want: keep the extraction as the permission/persist boundary (and I add direct tests for it), or fold it back into tool-runtime.ts so the PR is a pure move with no new module.

[Astro-Han]

Reviewed the current revision ed8605106add6e1aef5bdd312379c64cfb1c2262: a new packages/runtime/src/tool-call-snapshot.ts (+232) and packages/runtime/src/tool-runtime.ts (+26/−105). It moves the call-argument construction out of executeTool() into a named module — the frozen argument snapshot, the declared-schema validation, the permission projection, and the Computer Use persisted projection.

No P0–P2 found. Two inline notes and two scope/coverage observations, all minor.

The earlier NO-GO does not survive at this head. Its three factual claims were: two callers, an unread toolUseId, and a modelFacingArgs assigned but never consumed. At this head there is one production caller (tool-runtime.ts:1099), toolUseId flows into the projection context's toolCallId and is read there, and tool-runtime.ts no longer computes a modelFacingArgs at all. What is left is the scope question, which the author has already raised; see the third observation.

What I checked

• Fidelity of the move, statement by statement against the merge base. The directOnlyFailure === undefined guard, the deferred validation for an unavailable sandbox-boundary surface, the private mutable clone handed to permissionArgs, the re-snapshot of its result, and the Computer Use persisted projection are all preserved. The one statement that moved outside the try — sandboxBoundaryUnavailable (tool-runtime.ts:1095-1098) — is inert: interactionRun() returns a field (tool-runtime.ts:2746-2748), so nothing that used to become permissionArgsError can now escape as a throw instead.
• The move is complete. snapshotToolArgs is still used for the entry snapshot (tool-runtime.ts:1082), and the merge base had no other consumer of snapshotJsonValue or validateDeclaredToolArgs; in the merged tree the two old private helpers are gone and nothing references them.
• Checks. test is green on this head (run 35212800271). Its log shows the affected-surface planner selecting packages/runtime and every selected workspace passing with zero failures. That run is from 2026-09-17.
• Behavioural coverage of the new module — which has no unit test of its own — is nonetheless real and end-to-end. packages/runtime/src/__tests__/computer-use-privacy-boundary.test.ts asserts that the persisted tool_start args carry the tool's own argument names (window_id, text) rather than the approval-summary projection, that the transcript tool_call args equal the persisted args, and that a throwing permissionArgs routes to refusal while the persisted view still records the model's own input under the privacy rule (~lines 175-235 and 420-470). tool-args-violation.test.ts:191 covers declared-schema rejection, and loop-gate.test.ts:232 and tool-runtime-settlement.test.ts:548 exercise permissionArgs through ToolRuntime. So "no direct test of the module" does not mean these paths are untested, and I would not treat it as a gap on its own.
• Merge state. This head is 158 commits behind main, and main has changed this file since the merge base — a five-line union widening at MakaTool.providerTool.kind (#5533, around line 193), far from the edited region. A synthetic merge of this head into today's main is clean and keeps both changes, and the module's only cross-package dependency, computerUseModelCallArgs, is unchanged on main (packages/core/src/computer-use.ts has not been touched since the merge base). GitHub reports the PR mergeable.

Minor observations (P3, none blocking)

1. Inline at tool-call-snapshot.ts:119 and tool-runtime.ts:1114-1116: three parts of the module's declared surface have no reader or writer.
2. Inline at tool-runtime.ts:1104-1106: a comment naming a variable that does not exist in this tree.
3. Scope. The module is a pure function of ten inputs with one caller, and the two guards that decide whether construction happens — directOnlyRejected and validationDeferred — are still computed by the caller and passed in as booleans. So it owns construction only, exactly as its docstring says. There is no duplicate authority left to consolidate, which is what the earlier review objected to, so what remains is whether naming and isolating this region justifies a module plus its input and output types — including the unused pieces in observation 1. If the choice is fold-back versus keep, I would keep it: the permission view against the persisted view is the part of executeTool() most worth naming, and it is covered end-to-end. That is an opinion for whoever decides, not a request — and it is the author's open question from 2026-09-25, so it needs an answer from someone with the merge decision rather than another round of inline discussion.

What I could not judge

• I did not compile or run the merged tree locally. My merge check is textual, plus confirming the two edited regions are disjoint from main's change to the same file and that the single cross-package dependency is unchanged. The green test on this head comes from a 2026-09-17 merge with a far older main.
• No live provider request and no native Computer Use interaction.
• Whether a named boundary pays for itself over time is a judgement about future maintenance, not something I can measure here.

I did not approve, request changes, or merge.

---

Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

[Astro-Han]

toolName is declared here and passed by the one caller (tool-runtime.ts:1100), but buildToolCallArgs never reads it — grep the module: the name appears only on this line. The same is true of the returned executionArgs: the caller destructures three fields and not that one (tool-runtime.ts:1114-1116), and validateDeclaredToolArgs is exported at line 61 while its only consumer is this module itself (line 207).

None of these change behaviour, and I am not asking for a new consumer. The reason to fix them is that this module's stated purpose is to name a boundary and say who owns what, so a field that nothing reads is the one thing it cannot afford to carry — a reader has to decide whether toolName is part of the contract or was left over. Either drop them, or say in the docstring why they are there.

[Astro-Han]

rawExecutionArgs does not exist anywhere in this tree, or in the merge base — the local is executionArgs, snapshotted at line 1082 and passed unchanged here. A reader cannot tell what was renamed, when, or whether the comment is still load-bearing. The rest of the comment (the value passes through unchanged and the entry snapshot already holds it) is worth keeping; the rename sentence is not.

[me2seeks]

Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.

Summary

Extracts tool-call argument projections into a dedicated tool-call-snapshot.ts: the call-data boundary (the argument views one tool execution produces) moves out of tool-runtime.ts behind the same call sites, a faithful extraction with no behavior change intended. test lane passes.

Findings

1. [P3] Only the test lane appears in the rollup on this head — for a refactor of the tool execution boundary (hot path for every tool call), typecheck and the full gate should run green before merge to prove the extraction is faithful.

Verdict

merge-ready — clean boundary extraction; request full CI as housekeeping.