fix(runtime-host,ui,cli): name live tool calls on compact and collapsed rows - #3376
fix(runtime-host,ui,cli): name live tool calls on compact and collapsed rows#3376me2seeks wants to merge 1 commit into
Conversation
c8d63d8 to
61391a6
Compare
|
Heads up — this is currently conflicting with One thing worth knowing: #3397 landed on 2026-08-22 and added ASF license headers across ~2685 files, so the rebase will touch more than you'd expect, and any file you add now needs a header ( Ping me once it's rebased and I'll pick it up. |
|
Independent review of [P2] A Bash row that was redacted while live shows the original value again once the turn settles
return `$ ${firstRealLine ?? command.split('\n')[0]!.trim()}`; // before the shared formatterTwo things combine. Reproduced with a placeholder secret: the live row renders Why this is worse than "not redacted at all": the row was already redacted in front of the user, and then un-redacted itself. Someone who watched it go by has no reason to look again, and anyone reading the scrollback later sees the raw value with nothing indicating it was ever meant to be hidden. A redactor that reverses itself breaks a promise it already made. Existing tests cover the live preview only — nothing exercises the live → durable reconcile, which is why the round trip passes. Minimal fix: keep the "first non-comment command line" presentation (it is genuinely the useful part), but route the final text through the shared redactor instead of returning early, plus a regression asserting a placeholder secret does not reappear across reconcile. [P2] A synthesized empty
|
61391a6 to
c05b843
Compare
|
Rebased onto current
Verified on the new head: full root build, runtime-host 1092/1092 (incl. handshake-compatibility), CLI 385/385, biome clean. UI suite is 218/219 — the one failure ( |
Astro-Han
left a comment
There was a problem hiding this comment.
Review at exact head c05b843952f1808417560c0a6e72b27c70c6c5df.
No P0–P2. The mechanism here is well chosen and the security-sensitive part is built the right way round.
The problem is real: live tool_start frames from the Runtime Host omit full args, so a compact row could only print ● Bash with nothing to say what ran until the durable transcript arrived at turn end. Rather than widen the wire to carry full args — a Write call would drag an entire file across — this adds a bounded, redacted argsPreview and has the display path read args ?? argsPreview, so durable replay keeps priority and never sees the preview.
Three details worth calling out as correct, because each is a place this kind of change usually goes wrong:
projectToolArgsPreviewuses an allowlist, not a denylist.ARGS_PREVIEW_SCALAR_KEYSenumerates the keysformatToolInvocationLinecan actually render; file contents, option payloads and provider blobs have no path into the preview because they are simply not on the list. A denylist here would have needed updating every time a tool grew a new field.- Redaction happens before truncation.
boundPreviewStringcallsredactSecrets(value)and then slices to 240 chars. The other order is the classic bug — slicing first can leave a partial secret that the redactor no longer recognizes as one, and the truncated remainder ships. - The bound is enforced at the decode seam, not just the encode seam.
SESSION_TOOL_ARGS_PREVIEW_MAX_BYTESis checked indecodeSessionToolEventalongside an explicitassertAllowedKeys, so a malformed or oversized preview is rejected on arrival rather than trusted because the sender was supposed to bound it.
The CLI-side change is the modest half and reads well: event.args ?? event.argsPreview in both the shell-poll and the general tool_start branch, plus suppressing the no output placeholder once the row can name the call. ● Bash $ git add -A (no output) really is worse than ● Bash $ git add -A.
[P3] — 'input' is the one broadly-named key on the scalar allowlist. For today's tools it resolves to something short, but unlike command / path / pattern it does not name a shape, so a future tool with an input field holding a payload would pass the allowlist and be bounded only by the 240-char cap and the redactor. Not a live defect; just the entry most likely to age badly. A comment noting which tools it exists for would keep the next reader from widening it further.
Not mergeable as-is: this head conflicts with current main in packages/runtime-host/src/protocol/index.ts (I confirmed by rebase, not just by the API flag). A rebase is needed before this can land; the conflict is in the export surface rather than in the new logic, so I don't expect it to disturb the review conclusions above.
Verification: exact-head test is completed/success. I did not run the Desktop or Playwright suites and am not claiming them.
7452d8a to
0b99e89
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at exact head 27d506e07a6b89b80aa7ad5d5ca467bc5e21c6ba. No P0-P2 on the code. One [P3] inline about the rebase (the epoch number this branch claims is now taken on main). Exact-head test is terminal green (ran 2026-08-23 16:38-16:53).
Context: this head moved after the 2026-08-23 review at c05b8439 - a rebase onto a newer base plus 27d506e07, which resolves conflict markers the rebase left in protocol/index.ts. I verified that resolution directly: the final file has no markers, keeps both epoch comments, and the PR-authored deltas in all 19 files are semantically identical to the reviewed head (the only textual differences in e.g. pi-transcript.ts are base changes from main, not this PR). The 08-23 review's conclusions carry; the still-open [P3] from it ('input' as the one broadly-named scalar allowlist key, worth a comment naming the tools it exists for) was not addressed by the rebase and remains open - I did not re-raise it.
Independent checks done on this head:
- Redaction order:
boundPreviewStringrunsredactSecretsbefore the 240-char slice, in bothprojectToolArgsPreviewand the newformatToolInvocationLinebranches - the truncation cannot ship a partial secret the redactor no longer recognizes. - Bounds enforced three times: core caps each string (240) and the whole preview (2,048, dropping lowest-priority fields with the highest-priority present field always surviving); the host re-checks 8 KB before emitting (
projectArgsPreviewForWire); the decoder re-checks at the seam (requireEncodedByteLimit) plusassertAllowedKeys. A formatter change cannot silently bloat frames. - Full args never cross the live wire: the coordinator test proves a 100 KB
contentfield produces a preview of{command}only, and'args' in event === false. - The 2 KB budget-drop loop terminates (droppable list shrinks each iteration) and protects the top-priority present field.
- Naming authority stays client-side, which answers the cross-layer question: the host ships data (
intentpass-through,argsPreviewprojected by the same@maka/corecode the clients use), never text; both Desktop and TUI render through the sharedformatToolInvocationLine. The surfaces can still differ by design - Desktop uses the UI locale and a 120-char first-line cap, the TUI formats inen- but they cannot each compute a different name from the same data, because there is one formatter and one data source per window (preview live, full args after reconcile, persisted args on replay).args ?? argsPreviewordering means durable args always win over the preview once they exist.
Gate: exact-head test green; the branch is CONFLICTING against main, which blocks merge but not review. Not mergeable as-is - see the inline for what the rebase must do.
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 42 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 43 as const; |
There was a problem hiding this comment.
[P3] The epoch this branch claims (43) is now taken on main - the rebase must move to 45, and one array needs a three-way union.
main's epoch 43 is shellRunRef on Session tool-start events (from #3569), and 44 is the Session last-used removal (#3619) - so after rebasing onto current main, this change needs epoch 45, with the comment rewritten to the new number (the current // 43: live tool_start frames may carry optional intent / argsPreview text would collide with main's actual 43).
The sharper edge: #3569 also edits the same tool_start allowed-keys array in decodeSessionToolEvent (shellRunRef added on main; intent + argsPreview added here). The rebase conflict in that array must resolve to the union of all three keys - taking either side alone would either break live shell-poll correlation or have the strict decoder tear down connections on the new keys. The epoch guard will catch a wrong epoch mechanically (it rejects both same-number and backward values), and #3569's protocol tests would catch a dropped shellRunRef, but the union is the one resolution to get right by hand.
Everything else about the epoch handling here is correct (the 27d506e07 conflict-marker resolution kept both comments and the bump), so this is purely a stale-base note, not a code defect.
…ed rows Rebase of apache#3376 onto current main as one clean change: live tool_start frames may carry optional intent / argsPreview keys so compact and collapsed rows can name the call before durable args arrive. The strict decoder's allowed-key union retains main's shellRunRef alongside them, and correlated hidden-shell polls keep publishing only their correlation ref. Compatibility epoch advances to 45. Generated-by: maka
27d506e to
d0be632
Compare
|
Rebased as a single clean change onto |
Closes #3336. Closes #3338.
Problem
Collapsed tool rows on Desktop and compact rows on the TUI rendered name-only —
● Bash,● Task Create— with no hint of what the call does. Two distinct causes, one shared root:tool_startframes omitargsentirely (lean-channel decision from feat(cli): add Runtime Host-backed TUI sessions #2308), and durable args arrive only with the turn-end transcript reconcile. So during the one window a user actually watches, neither surface can name the call.item.intent, whichdescribeToolIntentproduces solely forExploreAgent— every other tool rendered● Nameforever.What changes
@maka/coreformatToolInvocationLinegains per-tool lines fortask_create(first subject + count),task_update(subject /id → status),GoalSet(condition),AskUserQuestion(first question + count). NewprojectToolArgsPreview(toolName, args)builds a bounded, redacted, whitelist-shaped args subset for the live wire (never file bodies or option payloads; sensitive keys dropped structurally; every string throughredactSecrets; per-string 240 chars, whole preview 2 KB).tool_startframes carry optionalintent(pass-through, 512 B) andargsPreview(≤8 KB). The strict decoder accepts and bounds both.RUNTIME_HOST_COMPATIBILITY_EPOCHbumped 29 → 30: older clients reject unknown keys on this event and would tear the connection down.@maka/ui)intent ?? firstLine(formatToolInvocationLine(args ?? argsPreview)), hard-capped at 120 chars. Works live (preview), after settle (full args), and on history replay (persisted args) — history needs no wire change.packages/cli)argsPreviewwhile live (turn-end reconcile still replaces it with durable full args). The dim(no output)placeholder now appears only when the row cannot name the call —● Bash $ git add -Ano longer carries the disclaimer. Empty args objects no longer render asinput: {}noise.formatToolInvocationLinestays client-side, so each surface formats in its own locale; the host ships data, not text.Verification
@maka/core585/585 — incl. new invocation-line cases (task/goal/question/ScheduledTask) andprojectToolArgsPreview(whitelist shape, secret redaction, sensitive-key drop, bounds, count fidelity viatasksTotal, WriteStdininputPreviewshape).@maka/runtime-host1038/1038 — incl. livetool_startprojection (intent + bounded preview, never full args), strict-decoder accept/reject cases, client projector pass-through.@maka/ui189/189 — incl. collapsed-target suite: args-derived line, intent precedence, liveargsPreview, task subject, 120-char cap, redaction.packages/cli339/339 — incl. live quiet-Bash row from the preview,task_createsubject row,no outputkept only for un-nameable rows.tool-args-redaction-contract(secrets in command strings never reach the collapsed row or the wire preview).npm run typecheck(all workspaces),biome lint/format, andknip(desktop, ui) clean.Notes / follow-ups
(no output)annotation remains for rows that genuinely cannot name the call; counts (5 matches,3 lines, exit codes) are untouched.AI use
Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push.
Generated-by: Makatrailers are present on the branch commits.