Skip to content

fix(runtime): preserve client capability tool outcomes - #5458

Open
testikun wants to merge 35 commits into
apache:mainfrom
testikun:codex/fix-5449-capability-outcome
Open

testikun wants to merge 35 commits into
apache:mainfrom
testikun:codex/fix-5449-capability-outcome

Conversation

@testikun

@testikun testikun commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Computer Use refusals and explicit stops could return model-visible text without a call-failure fact. Desktop's Client Capability projection then dropped even existing result status, so Host settlement, T2, telemetry, and activity views could record a failure as success. This PR carries an explicit success | error | aborted outcome from CU/native projection through the Client Capability wire and Host tool settlement, then through durable events, continuity, UI, CLI, and ACP replay. Ordinary MCP and business JSON results retain their existing semantics; failed process observations remain successful tool calls.

Client Capability results now require the outcome. After merging current main through epoch 197, the Client Capability outcome and interrupted Session Trace changes occupy epoch 198. Old stored records without the field remain readable; this does not promise that an older binary can read newly written records. No old records are rewritten. The change stays within the bounded outcome contract, without a general ToolResult/plugin migration.

Outcome is now persisted for every tool result, rather than only Client Capability and Computer Use calls. Consequently, cancelled terminal, subagent, and swarm results render as interrupted instead of errored.

Fixes #5449
Refs #4909

Verification

  • Current head 14b172d30 contains the review repair 85d357cf9, merges main 49dacdf13, and retains protocol epoch 198.
  • Persisted UI and CLI replay bind each result to the latest preceding call with the same opaque ID. A retained orphan result cannot attach to a later reused call, while cross-Turn ShellRun ownership remains supported.
  • Session Trace represents an aborted tool as interrupted; the Inspector keeps the neutral tool row and the authoritative turn_aborted failure.
  • element_sequence maps a retired pre-dispatch action to duplicate_action instead of outcome_unknown, using the same public binding mapping as a single action.
  • A structured Bash cancellation (exit code 130) now persists and publishes outcome: aborted even when the Turn abort signal itself was not set.
  • Current main's interrupted-tool settlement contract remains intact: unknown effects are explicit non-retryable outcome_unknown failures, never inferred successes.
  • Current-head Runtime build and full Runtime suite pass (3673 passed, 14 platform skips), as do the 108 focused Computer Use/settlement tests, Biome, git diff --check, the protocol epoch guard, and Windows inventory.
  • The ablation removed each new classification independently and reproduced both reported failures; restoring the classifications returns the focused suite to green.
  • Hosted CI run 36386601454 passes on the exact head, including Runtime Host, Desktop E2E, Storybook smoke, transcript geometry, and installed CLI validation. Windows ACP run 36386601429 also passes. No real OS permission or external-provider acceptance was performed in this repair.

AI use

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

Tool(s) and scope: OpenAI Codex implemented the bounded result contract, cross-layer propagation, regression tests, review repairs, and verification. The commits carry a Generated-by trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Carry explicit CU and Client Capability success, error, and aborted outcomes through transport, durable events, continuity, and UI projections. Keep legacy tool result semantics and old records readable.

Generated-by: OpenAI Codex
@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 17, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The #5449 fix verifies end to end: projectToolResult computes outcome before projecting, CU refusals carry it via settleComputerResult (user_stopped→aborted, other error→error), the wire requires a legal tri-state with controlled rejection (bad frame → connection-level failure; old peers are refused at the epoch handshake anyway), settlement consumes the wire outcome instead of re-inferring, both decoders enforce outcome ↔ isError consistency, old records without the field stay readable, and business-JSON / failed-process-observation semantics are preserved. Explicit outcome wins over inference for migrated tools, and isError remains the compatibility bit rather than the authority.

One P2 inline, P3s below.

P3s:

  • Derived aborted for tools without a declared resultOutcome still never reaches the continuity wire — the ...(tool.resultOutcome ? { outcome } : {}) gate means cancelled subagent/terminal/swarm results travel as errored to remote subscribers until transcript reconciliation. The mechanism now exists; dropping the gate closes the local/remote divergence.
  • The CC result envelope leaks into durable content: coerceResultContent({outcome, content, structuredContent}) stores the whole envelope as {kind:'json'}, so the outcome protocol key shows up in transcript/UI raw views (model-facing projection is clean).
  • aborted is not pinned end-to-end on the real wire — only error is (UDS test); the tri-state is assembled from per-boundary unit tests. One wire-level aborted e2e or a session-projector status↔outcome unit test would close it.
  • Non-migrated tools still infer via deriveToolResultStatus — declared scope boundary, but the same text-only-refusal hole remains open outside CU/client-capability.
  • Diagnostic surfaces (session-trace-projection, recap) still collapse aborted→failed — pre-existing vocabulary.
  • Unrelated hunks to split or explain: a permissionMode 'bypass'→'ask' change in session-catalog-two-client-uds.test.ts and whitespace-only model-picker-internals.tsx deletions.
  • Doc nit: body says epoch 161→162; the code bumps 162→163 (163 is correctly attributed).

Comment thread packages/runtime/src/tool-runtime.ts Outdated
@testikun

testikun commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor Author

Review follow-up (9c9553e): the P2 thrown-abort path is fixed and its inline thread resolved. The derived-cancelled P3 is also fixed by persisting/publishing the already computed outcome for all tools. I corrected the PR body to epoch 162→163. The Client Capability envelope in raw JSON and missing wire-level aborted integration pin remain explicit non-blocking cleanup/coverage, not claimed done. The current hosted run passed the Runtime Host step but failed an unrelated Desktop browser-login port race (46961 vs 46960); the prior run failed the unchanged PTY integrity timeout. I cannot rerun Actions without repository admin rights. The permission-mode fixture change makes the concurrent CAS race deterministic; the model-picker whitespace is merge residue, not part of this fix.

Generated-by: OpenAI Codex
…bility-outcome

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 927a47e. Found one P2 issue in shared transcript replay; details are inline.

Comment thread packages/runtime-host/src/server/session-continuity-coordinator.ts
…bility-outcome

# Conflicts:
#	packages/cli/src/acp/tool-event-mapper.ts
#	packages/runtime-host/protocol-compatible-changes/external-session-cwd-limit-authority.json
#	packages/runtime-host/src/protocol/index.ts
@testikun

Copy link
Copy Markdown
Contributor Author

Resolved the current-main conflicts in a27b54e on September 20, 2026. The Client Capability outcome protocol now advances the latest main epoch 166 to 167; the CLI mapper preserves both the PR outcome type and main interaction snapshot imports. GitHub reports MERGEABLE and fresh CI is running. diff --check and the protocol epoch guard pass. Please review the updated head.

@testikun

Copy link
Copy Markdown
Contributor Author

Follow-up on the earlier P3 review notes, pushed in b5dc546 on September 20, 2026:\n\n- Fixed the Client Capability envelope leak: a tool-declared top-level outcome is now classified first and removed before durable business content is coerced. The canonical function-response outcome remains authoritative, while transcript/raw JSON retains only the provider content and structured content. The tri-state durable-boundary regression pins this for success/error/aborted.\n- Added a real UDS Client Capability aborted result alongside success/error, so all three wire outcomes are now exercised through the provider connection.\n- Removed the unrelated model-picker-internals.tsx whitespace hunk.\n\nValidation: Biome; Runtime and Runtime Host builds; durable-boundary plus Client Capability UDS tests (25/25).\n\nI did not broaden this PR into the pre-existing inference behavior of tools without a declared resultOutcome, or rename diagnostic failed/interrupted vocabularies; those are separate cross-tool/read-model changes rather than gaps in this Client Capability protocol fix.

Merge apache/maka main at 0dc1142. Preserve the removal of turn.regenerate and assign Client Capability result outcomes compatibility epoch 168; re-pin the compatible cwd declaration.

Generated-by: OpenAI Codex
# Conflicts:
#	packages/runtime-host/src/__tests__/client-capability-channel.test.ts
#	packages/runtime-host/src/protocol/index.ts
@testikun

testikun commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor Author

@Astro-Han @me2seeks Review follow-up on the current head 2ac5b51be (September 21, 2026):

  • The previously reported thrown-abort and shared-transcript replay issues remain fixed, and both inline threads are resolved. Client Capability outcomes continue to preserve success / error / aborted through the durable boundary and continuity projection without leaking the protocol envelope into business content.
  • Merged the latest main and resolved the protocol collision by advancing the Runtime Host compatibility epoch from main's 169 to 170. The merged progress tests now retain main's monotonic/filtering coverage while asserting the required success outcome.
  • Fixed one additional merge-only test expectation in the CLI MCP progress path: progress forwarding and the tri-state success result are now asserted together.
  • Ablation: removed the stale re-pin of the already-landed external-session compatible-change declaration; it is back to main's historical value and no longer appears in the PR diff.

Validation passed: protocol epoch guard (169 -> 170) and its 17 tests; full workspace typecheck; Biome on the resolved files; Runtime Host focused suites (104 tests); Runtime focused suites (194 tests); UI live-turn projection (43 tests); CLI/ACP/MCP focused suites (38 tests); git diff --check. The branch is conflict-free and ready for another review.

Hosted CI run 35550660225 completed successfully on this head.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-verification on head a7d3f5e12: the follow-ups all landed correctly.

  • 9c9553e6f — thrown aborts now carry aborted through the durable record, telemetry status, and the terminal-failure path (tool-runtime.ts:1769,1807); the resultOutcome gate is gone so derived outcomes persist for every tool (closes the derived-cancelled P3).
  • b5dc546c8 — stripDeclaredToolOutcome removes the envelope outcome before durable coercion only when it matches the declared outcome, so business content keeps a same-named field that disagrees. The boundary test pins the stripped shape.

The remaining P3s stand as declared scope: aborted is still assembled from per-boundary units rather than one wire-level end-to-end, non-migrated tools keep deriveToolResultStatus, and diagnostic surfaces still collapse aborted→failed.

The branch is currently CONFLICTING with main — packages/core/src/session.ts and packages/runtime-host/src/protocol/index.ts (the epoch file; main is at 176, this head carries 173). On rebase the wire change needs to move to the next free epoch. CI on this head is green.

Advance the wire contract to epoch 179 and update the new Session-scoped capability fixtures to carry the required success outcome.

Generated-by: OpenAI Codex
@testikun

testikun commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

@Astro-Han Thanks for re-verifying the follow-ups. On current head cae0e1e2e, your conflict note has been addressed against the latest main. Main occupies epochs 179–182, so the Client Capability tri-state wire change is at epoch 183. The merge had only this protocol-index conflict; the original inline P2 threads remain resolved.

The remaining P3 items are still explicitly scoped as follow-up work: non-migrated tools' inference and diagnostic vocabulary are not claimed fixed here. The Client Capability UDS coverage exercises success/error/aborted through the provider connection; the distinction you raised about broader wire-level end-to-end coverage remains useful for follow-up.

Local validation passed: protocol guard (182 -> 183) and its 17 tests, full workspace build/typecheck, 80 focused Client Capability/continuity/durable-boundary tests, and git diff --check. Hosted CI also passed on this exact head, including Desktop E2E and Storybook. The PR body has been corrected to the current head and epoch. Please re-review.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head cae0e1e2e432d91b41d026c6fc6344d2444cf01e.

P2: persisted transcript replay can attach a later turn's result and outcome to an earlier tool call when a provider reuses an opaque tool-call ID. packages/ui/src/materialize.ts:195-206 builds one session-wide result map keyed only by toolUseId; packages/cli/src/pi-transcript.ts:1070-1076,1111-1116 does the same. The persisted contract carries turnId on both rows and does not require session-global uniqueness (packages/core/src/session.ts:868-872,899-908); the runtime backfill path correctly scopes messages to the current turn before matching (packages/runtime/src/runtime-event-backfill.ts:97-115,560-566). On this head, two turns using call_1, with the first result success and the second aborted, materialize as two interrupted rows both containing the second turn's payload. Keying by (turnId, toolUseId) in both projections would preserve the result identity this PR adds.

P3: Session Inspector still renders an explicit aborted tool outcome as a tool failure. packages/runtime/src/session-trace-projection.ts:364-377 ignores response.outcome and maps isError: true to failed; attributeTurnFailure then prefers that step and emits tool_failed (:425-452). TraceToolStep cannot represent interruption (packages/core/src/session-trace.ts:146-157), and the Desktop inspector marks the row failed (apps/desktop/src/renderer/application/contracts/session-inspector/session-inspector-panel-model.ts:129-164). A minimal trace containing an aborted function response followed by terminal status: 'aborted' returns step.status === 'failed' and failure.code === 'tool_failed', hiding the correct turn_aborted reason.

The remaining production path is consistent: Desktop/native and Client Capability results carry the tri-state through the strict epoch-183 wire, trusted Host adapter, ToolRuntime durable event, shared transcript, continuity, and UI/CLI status conversion. The earlier thrown-abort, envelope-leak, and shared-transcript-loss issues are fixed.

Validation on Node 24.18.1 included clean install, build:test, workspace typecheck, lint, format check, protocol epoch guard and its 17 tests, focused Runtime/Runtime Host/Core/Storage/UI/CLI/Desktop suites, the production Client Capability composition test, the 18 Session Inspector projection tests, both executable counterexamples above, and git diff --check. Hosted test is green on this exact head. The merge tree against current main c7d205a42dc32073edbb74f0954489e9388568c1 is clean. No database schema migration is needed because the new persisted JSON field is optional and legacy rows remain readable.

Residual coverage: I did not exercise a real macOS/Windows permission prompt, mixed-epoch binaries, or one process-level path spanning Desktop → UDS → Host T1/T2 → SQLite reopen → Inspector/UI.

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.

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 506da7dd4cfbcb4844ee176752cc199a7cb0e0a6.

Current-head result: no P0-P3 findings.

The P2 from my review on 56ed84dc is fixed. Persisted CLI replay now keeps exact (turnId, toolUseId) matching as the first choice and falls back across Turns only when the opaque call ID has exactly one owning Turn (packages/cli/src/pi-transcript.ts:1070-1129), matching the UI projection. The updated PTY regression stores the WriteStdin result under a different Turn, verifies that the operation settles as done, and advances its Bash parent from revision 1 to revision 2 (packages/cli/src/__tests__/pi-transcript.test.ts:2035-2112). Removing the fallback makes that regression fail at the stale revision-1 assertion.

I also exercised the real transcript replacement entry with four boundary shapes: a unique cross-Turn result is accepted, an exact-Turn result wins over a conflicting cross-Turn candidate, repeated call IDs owned by multiple Turns do not use the fallback, and input order does not change the unique match. A SQLite close/reopen probe preserved the cross-Turn messages and the CLI projected the restored tool as completed. The trailing 506da7dd CI retry commit has the same tree as the functional fix commit a9264cdc.

Validation on Node 24.18.1: build:test, full workspace typecheck, lint, format check, ASF headers, focused CLI/UI projection tests, and the full CLI suite (1166 passed, 3 platform skips). git diff --check passes; current main 99cfeb7e94728dfece75e7a47677c82c7506bb39 is an ancestor (24 commits ahead, 0 behind), and merge-tree 9fd10261bc5de365adbe69a2def1e43e2dba4d22 is clean. GitHub's exact-head test run completed successfully.

No database schema migration is required because this delta only changes CLI projection and its regression. Remaining unexercised scope is native macOS/Windows behavior, mixed-epoch binaries, and a packaged Desktop-to-UDS-to-SQLite-reopen-to-Inspector journey.

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.

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Carries an explicit success | error | aborted outcome from Computer Use/native tools through the Client Capability wire, Host settlement, durable events/messages, continuity, UI, CLI, and ACP. The issue is real: base Desktop projectToolResult (apps/desktop/src/main/runtime-host-native-capabilities.ts:727 on main) dropped all status, so CU refusals without an error field settled as success, and user_stopped/aborts surfaced as errored (toolResultActivityStatus, packages/core/src/tool-result-status.ts:79-88 main) — especially with contentOmitted. Direction is sound and bounded: decoders stay backward-compatible (outcome optional, with a strict (outcome !== 'success') === isError invariant in packages/core/src/session.ts:1671 and runtime-event.ts:1113), while the wire now requires it under a new compatibility epoch.

Findings

  1. [P1] Process, not code: PR metadata reports mergeable: CONFLICTING and CI (run 36097923317) passed on an older head. The PR already renumbered the epoch once (packages/runtime-host/src/protocol/index.ts:106, 187→189); main may have taken new epochs since. Must rebase, re-resolve the epoch ledger, and re-run CI before merge.
  2. [P3] Duplicated outcome authority for CU: settleComputerResult (packages/runtime/src/computer-use-tools.ts:325-338) derives outcome from error, but the real-model policy (apps/desktop/src/main/computer-use-real-model-policy.ts:112-160) hand-sets outcome alongside error in five refusal returns because it composes outside the settle wrapper. A future policy refusal that forgets outcome will throw requireToolCallOutcome (apps/desktop/src/main/runtime-host-native-capabilities.ts:736) and surface as a generic tool failure rather than the intended refusal. Loud, not silent — acceptable, but a shared helper would remove the twin classification points.
  3. Verified safe paths (no finding): CLI MCP results set outcome: 'success' unconditionally, but McpClientManager.callTool throws on isError results (packages/mcp/src/index.ts:1074), so only true successes reach it; stripDeclaredToolOutcome (packages/runtime/src/tool-runtime.ts:3313) keeps the envelope out of model-visible content; abort-racing thrown errors classify as aborted (tool-runtime.ts:1781), consistent with pre-existing practice (main tool-runtime.ts:2188).
  4. Tests are mutation-effective: decoder invariant throws on outcome/isError mismatch, T2-ledger round-trip survives store reopen (sqlite-runtime-store.test.ts), repeated opaque call IDs are turn-scoped in both UI materialize and CLI transcript, and the ACP digest change is covered by the "only outcome changes" test. The original misclassification (contentOmitted + aborted) has a direct regression test in live-turn-projection.test.ts.

Verdict

needs-changes — code is merge-quality, but the PR conflicts with main and CI is stale; rebase (re-checking epoch 188/189 uniqueness) and a green CI run are required pre-merge.

@testikun

testikun commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor Author

@Astro-Han 感谢这次独立复核。当前 head 14b172d30 已处理本轮意见,并合入最新 main 49dacdf13:

  • element_sequence 的意见成立。序列在真正派发前遇到 retired_action / invalid_binding 时,现在复用单动作的公开错误映射,分别返回 duplicate_action / stale_frame,不再错误标成 outcome_unknown。新增回归先制造“执行器明确未派发并退休该动作”,再以 sequence 重试,确认没有第二次 dispatch。
  • Bash 取消意见也成立。catch 路径会读取已经规范化的 terminal 状态;结构化 code 130 / cancelled 即使没有同步设置 Turn abort signal,也会持久化、发布并记录为 aborted。
  • PR 描述已补充行为变化:outcome 现在为所有工具结果持久化,因此 cancelled terminal/subagent/swarm 在回放中显示为 interrupted;旧的 188/189 已更正为当前 epoch 198。
  • 关于 feat(desktop): hand off live terminals for private user input #5732 同样占用 198 的 merge-order 提醒已确认。按当前约定本轮不修改 feat(desktop): hand off live terminals for private user input #5732;两者中后合入的分支必须在合并前基于届时 main 重新分配 epoch。

验证:Runtime 完整套件 3673 passed / 14 platform skips / 0 failed;Computer Use 与 settlement 聚焦测试 108/108;Biome、git diff --check、protocol epoch guard、Windows inventory 均通过。消融时分别移除两个新分类,稳定复现 outcome_unknown 和 error,恢复后通过。exact-head CI run 36386601454 已通过,包括 Runtime Host、Desktop E2E、Storybook smoke、transcript geometry 与 installed CLI validation;Windows ACP run 36386601429 也已通过。

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head e19ec01b1a342398e7b09ae6b23d10cda961484e.

Current-head result: one P2 finding, described inline.

The Client Capability outcome propagation remains intact, and the prior CLI/UI repeated-ID fix is present. However, its cross-Turn fallback still treats uniqueness within the currently materialized message window as global uniqueness. Because Desktop retains and evicts transcript rows by Turn while this PR intentionally supports results stored under a different Turn, a result can remain after its original call Turn has left the resident tail. A later provider call reusing the same opaque ID is then the only visible call and receives the stale result/outcome.

Validation on Node 24.18.1: clean npm ci, build:test, git diff --check, UI materialization tests (22/22), and CLI transcript tests (114 passed, 3 platform skips). Two direct exact-head probes reproduced the window-boundary failure in both projections: an old tool_result plus a new tool_call sharing provider-call-1 makes the new call errored with old failure. Existing repeated-ID tests pass because both owning calls are present at once, so the fallback is disabled. Hosted test and windows_acp passed. Against fresh main 0fd7540831ebc4c38f6af9b96c8a510d7d087f1, the PR is 26 commits ahead and 4 behind; merge-tree 913cdd79a72b77753fd3349fa9220aaa1c892bea is conflict-free.

No schema migration is required. I did not exercise native macOS/Windows permission prompts, mixed-epoch binaries, or a packaged Desktop-to-UDS-to-SQLite-reopen-to-Inspector journey.

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.

Comment thread packages/ui/src/materialize.ts Outdated

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head d21039d2f3e3a5b32b416920ce695c68591b2269.

Current-head result: no P0-P3 findings.

The P2 from my review on e19ec01b is fixed. Both persisted transcript projections now pair each result with the latest preceding call object carrying the same opaque ID, so an orphan result retained before a later reused call remains unmatched instead of being attached to the new call (packages/ui/src/materialize.ts:189-214, packages/cli/src/pi-transcript.ts:1070-1085,1119-1124). The new UI and CLI regressions exercise the exact [old result, new call] history boundary and keep the new call unfinished rather than displaying the stale failure (packages/ui/src/__tests__/materialize.test.ts:414-439, packages/cli/src/__tests__/pi-transcript.test.ts:1474-1501). Existing repeated-ID and cross-Turn ShellRun regressions continue to pass.

I also checked the ordering contract around this fix. Stored transcript rows are read in durable append order; Runtime replay already treats a result before a later reused call as an orphan; and Runtime Host admits only one active root Turn per Session. Within an invocation, duplicate provider tool-call IDs are diagnosed by the tool ledger. I did not find a reachable production path where a valid earlier result can arrive after a later same-ID call and defeat the interval pairing.

The functional delta from e19ec01b is limited to four UI/CLI projection and test files (+75/-33). The trailing merge from current main does not alter those files; its conflict resolution only advances the Runtime Host protocol epoch with the upstream queue changes.

Validation on Node 24.18.1: build:test, full workspace typecheck, lint, format check, ASF headers, focused UI/CLI transcript suites (141 total: 138 passed, 3 platform skips), and git diff --check passed. GitHub's exact-head test and windows_acp checks are green. Current main is 8b104db12b70814b33f25b111021e2e8ec325d1f; the PR is 28 commits ahead and 1 behind, with conflict-free merge tree 88cb2693ec06d303be43b12e30828d9c92a6d48a.

No database schema migration is required. Remaining unexercised scope is native macOS/Windows permission prompts, mixed-epoch binaries, and one packaged Desktop-to-UDS-to-SQLite-reopen-to-UI journey.

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.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A second, independent review of head d21039d2 (Claude lineage), alongside the hqhq1025 review 5334268047. It found no P0–P2.

Confirmed fixed and checked independently:

  • The earlier P2 (a reused call ID across turns). materialize.ts:189-214 and pi-transcript.ts:1070-1085,1123 now bind each result to the nearest preceding call with the same ID, in append order. ShellRun refs from other turns still merge into their Bash rows.
  • Epoch 197 → 198. This passes protocol-epoch-check.mjs.
  • Storage. outcome is optional, older records decode unchanged, and every write site keeps it consistent with isError.

One behaviour change worth a line in the description: outcome is now stored for every tool result, not only for Client Capability and Computer Use calls. As a result, cancelled terminal, subagent and swarm calls now render as interrupted rather than errored.

P3:

  • Wrong error code for element_sequence rejections (computer-use-tools.ts:1987-1990). Rejections before dispatch (retired_action, invalid_binding) are coded outcome_unknown, but the action definitely did not run. The single-action path uses duplicate_action / stale_frame in the same situation.
  • A cancelled Bash can show as an error (tool-runtime.ts:1781). The failure path checks only the turn's abort signal. If Bash throws code 130 ("cancelled") while the turn isn't aborted, the result is recorded as error.
  • Stale epoch numbers in the PR description. It still mentions 188/189; the code uses 198.

Merge-order note: #5732 also claims epoch 198. Whichever merges second has to move to 199.

Tests: build:test passes, and the touched suites pass in core, runtime, ui, cli, storage and desktop main. runtime-host passes 307/308. The one failure is a sandbox Bash test that fails the same way on an unrelated worktree on this machine. Not run: the full suite, Desktop E2E, and real macOS Computer Use permissions.


Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; please verify before acting.

@testikun

Copy link
Copy Markdown
Contributor Author

@Astro-Han @hqhq1025 Thank you for the current-head reviews. Follow-up on head 14b172d30:

  • Resolved — behavior description: the PR now states that outcome is persisted for every tool result, so cancelled terminal, subagent, and swarm results replay as interrupted rather than errored.
  • Resolved — element_sequence pre-dispatch classification: retired_action and invalid_binding now use the same public mapping as single actions (duplicate_action / stale_frame) instead of outcome_unknown. The regression proves no second dispatch occurs.
  • Resolved — thrown Bash cancellation: a normalized terminal result with code 130 / status cancelled now persists and publishes outcome: aborted even when the Turn abort signal was not set.
  • Resolved — stale PR metadata: the description now identifies compatibility epoch 198 and the current head.
  • Acknowledged merge-order dependency: fix(runtime): preserve client capability tool outcomes #5458 and feat(desktop): hand off live terminals for private user input #5732 both currently claim epoch 198. Per the current decision, feat(desktop): hand off live terminals for private user input #5732 is not modified here; whichever PR merges second must rebase and advance to the next epoch.

The mutation tests reproduce both reported bugs when the new classifications are removed. Runtime passed 3673 tests with 14 platform skips, the focused suite passed 108/108, and hosted CI run 36386601454 plus Windows ACP run 36386601429 pass on the exact head. There are no unresolved review threads.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental re-review at 14b172d3, relative to d21039d2, which our earlier review found clean. Apart from a merge of main, the only new commit is 85d357cf9 (4 files, +94/−7). Verdict: no P0–P2 in the delta.

  • computer-use-tools.ts:969-980: publicBindingFailureCode maps retired_action to duplicate_action and invalid_binding to stale_frame. The element-sequence stop path (:1992) now uses it instead of falling through to outcome_unknown, which is the right outcome class.
  • tool-runtime.ts:1788-1792: a terminal failure whose content status is cancelled is now classified aborted rather than error, unless the outcome is uncertain. The ordering is correct, because terminalFailure is now computed before the classification.
  • The new tests cover both cases.

P3, also noted in the other review: in the sequence path the structured error is public, but the modelText headline can still surface the internal retired_action wording.

The protocol epoch is 198, which is also claimed by #4751, #5709 and #5670. Whichever merges later must renumber.

Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 14b172d30db8323484d955d8d79f5438441c9e68.

The latest Runtime changes correctly classify Bash exit code 130 as an aborted outcome and add structured public error codes to Computer Use results. I found one non-blocking model-facing regression in the element_sequence pre-dispatch refusal path; see the inline P3.

Validation completed on Node 24.18.1: clean npm ci, npm run build:test, full Runtime (3,659 passed / 13 skipped), focused Runtime/UI/CLI/Inspector tests, workspace typecheck, lint, format check, ASF headers, protocol guard/tests, and git diff --check. Hosted test and windows_acp are green for this exact head. The PR is 30 commits ahead and 4 behind current main 5735554b6fa99ab2029054c142f746c0a8fa0e71; the merge tree is conflict-free.

Not independently validated: packaged Electron, native macOS/Windows permission flows, real Computer Use providers, or mixed-version protocol peers. Protocol epoch 198 also remains subject to merge ordering with other unmerged PRs using the same epoch.

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.

Comment thread packages/runtime/src/computer-use-tools.ts Outdated
@testikun

Copy link
Copy Markdown
Contributor Author

@hqhq1025 Review/conflict follow-up on current head 98a663ffe:

  • Resolved - model-facing sequence refusal: the finding was valid. element_sequence now builds its headline from the same public binding mapping as structured error, adds the reason-specific recovery guidance, and never exposes internal retired_action through modelText. The regression now calls toModelOutput, requires duplicate_action plus recovery text, rejects retired_action, and still proves no second dispatch.
  • Merged current apache/main at 2f3220552. Main owns epoch 198 for executor restore/readiness states; this PR's Client Capability outcome/continuity shape advances the combined protocol to epoch 199.
  • Validation on Node 24: full workspace build:test; focused Computer Use, Client Capability, tool outcome, CLI/UI replay, and protocol tests (487 passed, 3 platform skips); all 17 protocol-guard tests; merge-result epoch guard (198 -> 199); Biome; and git diff --check pass.
  • Ablation by removing the recovery guidance made the new model-output assertion fail; restoring it returned the test to green. The inline thread is resolved. Hosted CI is running for the refreshed head.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 98a663ffee751d5989ed59e2614f812ee80b01bd. I did not find any remaining P0-P3 issues.

The prior sequence-output issue is fixed at the model boundary: element_sequence now derives one public failure code for the structured error, headline, and modelText, and appends the matching recovery guidance. The regression exercises toModelOutput() and verifies that duplicate_action plus actionable recovery is visible while the internal retired_action reason is absent. I also rechecked that the earlier UI/CLI cross-turn tool-result ownership fixes remain intact.

Validation on Node 24.18.1: npm run build:test; all 94 computer-use-tools tests; 8 focused UI/CLI transcript ownership and ShellRun tests; git diff --check; hosted test and windows_acp. Current main is 03237142ddbb8145c80ec541f54f6292cbe714a9; the PR is 32 commits ahead / 2 behind and git merge-tree --write-tree completed without conflicts (21b460ab20939622bad2f4ea718bbb876737a853). The protocol history is contiguous at epoch 199 over main's epoch 198.

I did not validate packaged Electron, native macOS/Windows permission flows, a real provider, or mixed-version peers.

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.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We checked the increment since 14b172d3 at 98a663ff: the merge of main plus fix(runtime): keep sequence failures model-safe. The element_sequence headline, modelText and the structured error now share the public binding-failure code and its recovery hint, so the model no longer sees the internal retired_action. The merge only renumbers the compatibility epoch to 199 on top of main's 198, with the ledger comment updated, and we found no other resolution changes in the PR's paths. There are no new findings.

Other open PRs also claim epoch 199, so whichever of them lands after this one will need to renumber.

Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
@testikun

Copy link
Copy Markdown
Contributor Author

Merged current main at 064997019 into the branch in 65096abf9. The storage-usage contract remains compatibility epoch 199 and this PR's tri-state Client Capability/session-continuity contract moves to epoch 200, so the sibling epoch collision is resolved. Local build:test, the 267 focused Client Capability/continuity tests, the merge-result epoch guard, Biome/ASF/diff checks, hosted CI run 36668405044, and Windows ACP run 36668404973 all pass. The PR is conflict-free against current main.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed commit 65096ab, focusing on the increment since the prior reviewed head 98a663f. This commit merges main. Its manual protocol resolution advances RUNTIME_HOST_COMPATIBILITY_EPOCH to 200 (packages/runtime-host/src/protocol/index.ts:107-113), preserving both the PR's tri-state Client Capability result change and main's epoch-199 storage-usage operations; the two compatible-change declarations are re-keyed to 200. I found no substantiated new P0–P3 issue in this increment. The earlier review covers the feature paths, not this merge automatically.

The protocol epoch guard passes against fresh main 0aa2707 (199 -> 200), git merge-tree --write-tree and git diff --check are clean, and this head's hosted test and windows_acp checks pass. I did not independently repeat the full local suite, packaged Electron, native permission paths, real provider calls, or mixed-version peers. This is a technical review, not merge approval.

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.

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the current merge commit and the retained Client Capability outcome paths. The merge keeps the tri-state success | error | aborted result contract while moving its compatibility epoch to 201 after main’s Agent Graph epoch 200 (packages/runtime-host/src/protocol/index.ts:107-112). The preceding-call pairing in UI materialization (packages/ui/src/materialize.ts:189-214) and the public Computer Use binding-error mapping remain in the effective diff. I found no substantiated new P0–P3 issue in the inspected increment.

The current-head hosted test and windows_acp checks pass; a fresh-main merge-tree and diff check are clean. I did not rerun the full local suites, packaged Electron, native permission flows, or mixed-version peers. The PR description still describes an older protocol epoch and should be updated before merge; this review does not approve or merge the PR.

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 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head cc62c1884fc1c76fa42a3f278c098338c1bcc80a. This is a main merge. Its only manual resolution changes the ACP Plan/real-Host subtest timeout from the branch's 60 seconds to main's 45 seconds (packages/cli/src/__tests__/acp-goal-plan-child-process.test.ts:584-586); the client-capability tri-state and model-safe tool-result paths reviewed on prior heads remain in place. I found no new substantiated P0–P3 code issue in this increment.

This head is not merge-ready. The fresh main 9b089f58 merge-tree conflicts at packages/runtime-host/src/protocol/index.ts:107-110: this PR uses compatibility epoch 201 for tri-state Client Capability results, while main independently uses epoch 201 for archived session-catalog projections. The merge must preserve main's 201 declaration and assign a later epoch to the Client Capability change, then rerun the epoch guard and checks. GitHub reports CONFLICTING despite current-head test and windows_acp being green. Node 24's protocol-epoch guard against this head's first parent and its 17 unit tests passed; git diff --check passed. I did not rerun the full local suite, packaged Desktop, or mixed-version Client/Host peers.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[A06] Computer Use failures appear successful across result boundaries

4 participants