feat(ui): give driving the computer its own row, and say what it did - #1888
Conversation
a4c13b5 to
3486e6d
Compare
|
Pushed fixes for findings 5, 6, 8 and 9. Finding 7 is scoped out deliberately; reasoning below. Finding 5, the fixtures only exercised one path. Confirmed: all three were status completed, and ToolTrow short-circuits to the Astryx component when every item supports it, so ToolTrowRow was never mounted. The presentation test had the same problem from the other direction: its fixture declared an intent, which wins outright, so the Computer Use term below it was never evaluated. Added a row test that runs the interrupted, waiting_permission and sandbox-denied fixtures through the product row, and a presentation test that asserts its fixture carries no intent rather than assuming it. Finding 6, ToolActivityCard printed the bare noun. Confirmed at line 227. One correction to the finding: the card is not itself exported from components.tsx; it is module-private and reaches the public surface as the body of the exported ToolActivity. The conclusion stands and it is patched, with a render test that goes through ToolActivity. Finding 8, summary.activity had no reader. Confirmed, zero hits anywhere. The live line is already correct without it, both because presentation.ts now derives a Computer Use label and because summary.kind.computer reads "正在操作电脑 N 次". Removed. Its sibling thinkingActivity is dead in the same way but predates this branch, so I left it. Finding 9, the fourth copy of the kind list. Confirmed, and fixed by reading TOOL_ACTIVITY_KINDS instead of writing the list out again, which is what the protocol commit in this branch already did for the decoder. I also gave the enum value a producer: maka_computer now declares activityKind: 'computer' the way every other builtin does. Without that, computer was a value the wire accepted that nothing emitted, and every surface had to recognise Computer Use by tool name. Finding 7, the CLI transcript, is not fixed here and I want to be plain about why rather than quietly leaving it. The defect is real and identical: a ten-call turn is ten rows titled "Maka Computer". computerActionLabel is portable enough to fix it, it is pure TypeScript with no React or DOM, and its only non-type dependency is redactSecrets, which already lives in @maka/core. The blocker is the copy catalog it reads from, so the move is a real refactor of packages/ui, and packages/ui cannot be built or typechecked on this machine, so I would be pushing that unverified. It is a separate change with its own review. Negative controls. The packages/ui test suite cannot run here at all, so I did these against a bundle of the pure modules, which have no Astryx imports, driven by the same assertions. Restore the hand-written KNOWN_ACTIVITY_KINDS: trowActivityKind('some_other_name', 'computer') returns 'tool', expected 'computer'. Restored, all green: the module probe passes and the runtime suite is 71 pass, 0 fail. What I could not verify locally: @astryxdesign/core is not installed and cannot be fetched here, so packages/ui does not build. The three render assertions in this change, the two in the new row test and the one in the card test, are unrun. Everything asserted about presentation.ts, trow-summary.ts and copy.ts was verified through the module probe, and everything about the runtime tool definition through the normal suite. The probe also caught a mistake of my own before it was pushed. I had written an assertion that a whitespace-only intent falls through to the derived label; it does not, because formatUserVisibleToolText returns the whitespace and it is truthy. That is a real if minor defect, a blank intent renders a blank row, but it is not one of the findings and I did not widen the change to cover it. I dropped the assertion rather than assert something false. |
|
Second-round review addressed. Findings verified before changing anything; one is rejected with evidence. The renderer never saw the arguments every label was derived from. Confirmed by running the real projection into the real label function. maka_computer declares categoryHint 'computer_use', so ToolRuntime.executeTool substitutes computerUseApprovalSummary(...) before anything is emitted or persisted, and that is the only argument shape any renderer can receive:
On which projection to read: neither the one from #1952 nor a widened summary. computerUseModelCallArgs is built for the model surface and reduces value, text and coordinate to and , so it fixes element_id and nothing else, and it is not on main. Widening the persisted summary to carry the rest was the option I rejected: computer-use-privacy-boundary.test.ts asserts by name that a typed value and a coordinate reach neither the persisted tool_call, the tool_start event, nor the invocation record. Deleting that boundary to win a nicer sentence is not a trade worth making. The one field that is not screen content is element_id, the model's own choice of an index into an observation it already read, and it is exactly the field that separates two clicks in a turn. It is now carried, admitted only when it is a stable identifier so an accessibility label arriving under that key is dropped rather than persisted. The negative control for that guard is the sharp one: with the loose treatment app gets, the projection persists Customer SSN 123-45-6789. Every label whose input the projection withholds is now gone rather than left to render a lie, along with launch_app, scroll_element and element_sequence, which no producer emits and which the strict wire schema has no steps field for. Five tests were keeping those green by feeding the function action strings the schema rejects. The silent revert of upstream 51f0479 is restored. Those two Tailwind arbitrary values were the last in packages/ui and nothing compiles them, so the child rows of an expanded group rendered with a class matching no rule while the group header above kept its tint. The Pi TUI is fixed rather than scoped out. It printed Maka Computer for every call and spelled the approval summary out as the detail line, so two clicks truncated to identical rows of the host's own bookkeeping: copy.ts setValue no longer has a value branch, so the collision with type is gone by construction. Negative controls, each run: removing elementId from the projection reddens 8 of the label tests and the runtime one (actual ['ensure'...] is the other PR — here: actual is the summary without elementId, expected with it). Restoring the Tailwind classes reddens the new tone assertion with "The input did not match the regular expression /maka-tool-tone-warning/". Loosening the identifier guard reddens with actual 'Customer SSN 123-45-6789', expected undefined. Removing the CLI case reddens with "three rows that read differently: 2 !== 3" and prints the two identical truncated rows. Every fixture now goes through computerUseApprovalSummary rather than writing args by hand, and the runtime assertion sits beside the privacy test that produces the value. packages/ui 257/257, packages/core 697/697, packages/runtime CU suites green, packages/cli 468/468. |
4935b26 to
a1c96fa
Compare
|
Rebased onto origin/main at 623fcf5 and fixed the two findings. The leak is real and is now fixed. computerUseApprovalSummary admitted element_id through stableIdentifier alone, and that regex is [A-Za-z0-9._:-]{1,256}, which is also the shape of an API key. Probed against the built module before the change, a model emitting {action:'click_element', element_id:'sk-ant-api03-...'} got the key back verbatim as the persisted elementId, while the same string under observation_id came back [redacted]. Since ToolRuntime substitutes this projection at tool-runtime.ts before anything is persisted, that value landed in the persisted tool_call, on tool_start and in the row, and only summarizePersistedArgs scrubbed it — so it was clean in telemetry and kept in the transcript. elementId now takes the same boundedDisplay(redactSecrets(...)) pass observationId and app do. Re-running the probe after the fix, element_id and observation_id project identically for every input. Two assertions cover it. The unit one in packages/core asserts the parity directly, and a seam-level one in computer-use-privacy-boundary.test.ts drives the real ToolRuntime and asserts the persisted elementId is [redacted]. Negative controls: reverting the source line makes the core test fail with "true !== false", and makes the seam test fail with actual 'sk-ant-api03-AbCdEfGhIjKlMnOpQrStUvWxYz0123456789AbCdEfGh' against expected '[redacted]'. Both go green again on restore. On requireToolActivityKind: your read was right that reverting it failed nothing, so I added a test rather than leave it. protocol.test.ts now decodes a tool_start frame for every value in TOOL_ACTIVITY_KINDS and rejects four plausible non-values. Restoring the hand-written chain fails it with "Got unwanted exception: the wire declares computer, so the decoder must accept it". Two things worth flagging. First, the rebase was not clean, and not because of anything in this branch. Upstream 2da7a81 retired the second row renderer entirely: presentation.ts, trow-summary.ts, the tool-row-motion helpers, the ToolActivity card and the per-kind icon table are all gone, and every row now goes through Astryx ChatToolCalls. So this PR's icon entry and its trowActivityKind fix no longer have anything to attach to, and I dropped them; the label wiring moved to the single surviving row name in ToolTrow, which is where this branch's own last commit had already concluded it belonged. The tests moved with it: the ones that asserted deriveToolActivityPresentation, trowActivityKind, the maka-tool-tone classes and the ToolActivity card surface are gone or restated against the renderer that exists, and the recognise-by-kind claim is now asserted through isComputerTool, which still carries it. Net effect on the product is unchanged; the diff is smaller than it was. Second, a limitation to know about rather than fix here. The runtime-host tool_start frame carries no args at all — assertAllowedKeys lists nine keys and args is not among them. A client sitting behind the Session protocol therefore gets the new computer kind but has nothing to derive a label from. The desktop is unaffected because it receives full SessionEvents over IPC. Noting it so it is not found later as a surprise. One cross-PR note: #1891 changes the same persistedArgs line to persist the tool's own wire dialect (window_id, element_id) rather than this projection's (windowId, elementId). computerActionLabel reads the latter. The seam test here now asserts the exact key set with a comment naming that file, so whichever PR lands second gets a red test rather than a silent fallback to the vague labels. The redaction fix itself is compatible either way — it is strictly stronger than the stable-identifier-only rule #1891 states, not in tension with it. Verification: @maka/core 741 pass, @maka/ui 268 pass, @maka/runtime 2761 pass. The remaining @maka/runtime failures are three OpenAI-compatible streaming tests that fail inside a prebuilt @ai-sdk/provider-utils older than upstream ea51535, and the runtime-host and cli failures were load and environment: each passes in isolation, the path-containment ones need TMPDIR in its resolved /private/var form, and the two cli ones are FORCE_COLOR leaking into asserted stdout. biome check clean on all touched files, check-console passes. |
677c8db to
1e0626a
Compare
Computer Use's display name is a noun — "Maka Computer" — so a turn that
observed a window, clicked a button and observed again rendered three
identical rows. `computerActionLabel` derives a row label from the call's
own arguments instead: 观察「计算器」窗口, 点击元素 e7.
The label is derived, never declared. The model is not given an `intent`
field to write; every word it produces is owned by the runtime, and a free
text field would be one more place it can be wrong.
What the renderer can read is `computerUseApprovalSummary(...)`, which
`ToolRuntime.executeTool` substitutes for the wire call before anything is
persisted. `element_id` had to be carried on that projection for a row to
name its target at all, and this adds it — but a shape filter is not a
privacy boundary. `stableIdentifier` admits `[A-Za-z0-9._:-]{1,256}`, which
is also the shape of an API key, and arguments are not validated before the
projection runs, so a model that put a token under `element_id` wrote it
verbatim into the persisted `tool_call`, onto `tool_start` and into the row
— while the same string under `observation_id` came out `[redacted]`. It
now takes the same `redactSecrets` pass `observationId` and `app` do.
Also puts `'computer'` on the wire as a tool activity kind and has the
Session decoder read the one list rather than a second hand-written copy of
it, which had already fallen behind and rejected the new kind outright.
Note for clients behind the runtime-host Session protocol: the `tool_start`
frame carries no `args`, so such a client gets the kind but cannot derive
the label. The desktop receives full SessionEvents over IPC and is
unaffected.
…e now persisted in This branch was written when `ToolRuntime` persisted a Computer Use call as `computerUseApprovalSummary(...)`, which spells the target `windowId` and `elementId`. apache#1891 changed that line to `computerUseModelCallArgs(...)`, which spells the same two fields `window_id` and `element_id`, because the model reads its own history back through those args and was copying a dialect the tool rejects. Reading the old names off the new projection is not a crash and not a type error. Every element action falls back to the generic "点击该元素" and every window-scoped observe loses its window — which is the exact defect this branch exists to remove, reintroduced by the rebase and invisible from either renderer's own suite, because both build their fixtures by calling a projection and neither could tell which one the runtime uses. - `computer-action-label.ts` and `pi-transcript-tools.ts` read `window_id` and `element_id`. The label module's key type now filters the projection's index signature back out, so a stale name is a build error rather than an `undefined`; before, `keyof` widened to `string` and accepted anything. - Both renderers' fixtures go through `computerUseModelCallArgs`, and the seam test says in one place which object is the dialect and which two files read it. - The row refuses to print an element id that is not identifier-shaped. The persisted record keeps whatever the model sent, deliberately — it has to read back the call it made — but free text under that key is either a label copied off the screen or a mistake, and neither belongs in a sentence a person reads. The redaction this branch carries is unchanged in effect and now lives where the value actually lands: `computerUseModelCallArgs` already runs `element_id` through `redactSecrets`, and the seam test pins it — an `element_id` of `sk-ant-api03-…` is persisted as `[redacted]`, matching `app`. The addition of `elementId` to `ComputerUseApprovalSummary` is dropped: after apache#1891 that projection is neither persisted nor rendered, and its only remaining caller reads `approvalClass`, so the field would have had no producer and no reader.
1e0626a to
a8f1457
Compare
1 participant
Two things a Computer Use call could not do in a transcript
Be found
It rendered under the same gear icon as every other tool call. Driving the user's own machine has its own risk, its own approval classes, and is the one activity a person most wants to pick out at a glance. It gets its own activity kind and icon.
Be told apart from the call before it
The row falls back to the tool's display name when a backend supplies no intent. This tool's display name is "Maka Computer":
computerActionLabelderives the label from the call's own arguments. Only tools whose display name already reads as an action fall through to the name — this one is a noun.The live line
That noun also reached the line that says what the agent is doing, which is a verb phrase —
正在…/Working: …. "正在Maka Computer" was reported as reading wrong, and it was. A kind with an entry insummary.activitysupplies the verb instead.Working on old transcripts
trowActivityKindrecognises the tool by name as well as by declared activity kind, so rows recorded before the kind existed render correctly too.Verification
packages/corebuilds against current main.packages/uicannot be built on this machine — upstream's@astryxdesign/core@0.2.0does not resolve from this registry — so CI is the check for the UI package. The new label module and its test are self-contained; the three touched files take one line each.