fix(web-shell): follow an accepted approval answer to its terminal operation - #13609

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head b428c8c. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

_{Full-resolution recordings (.webm) are attached to the workflow run.}

— Qwen Code · web-shell visuals

✅ Qwen Triage finished — CI landed green on b428c8c and the deferred approval was posted. finalize run

✅ Qwen Triage 已完成 —— b428c8c 的 CI 全绿，延迟审批已提交。查看 finalize 运行

Thanks for the PR!

Template looks good ✓ — all nine sections present, and the "Not validated / out of scope" list is unusually honest about what this does not cover.

Problem: real, and I verified it in the base code rather than taking the description's word. use-managed-actions.ts adds the actionId to answered before the await, and that set is only ever cleared on a sessionId change or for ids recorded in endedAnswers (i.e. answers that failed with a structured action_expired/action_cancelled/action_already_resolved code). The displayed card is actions.find(entry => !answered.has(entry.actionId)). So a 202 whose operation is still pending/running resolves as success, and if delivery later fails the card stays hidden for the rest of the session while the Action is still requested server-side. The user has no way to act. That is an observable defect, not theoretical hardening. docs/design/2026-09-30-managed-agent-actions.md §6.2 backs the mechanism ("A worker forwards it to the Harness route and returns a failed attempt to pending with the dispatch backoff"; "While the Action stays requested, for example on a recovery-blocked Session, the operation stays running"), and #12867 lists D6 as an open slice.

Direction: aligned. This closes a documented gap in an active workstream instead of inventing one, and it is client-side only — /operations/query already exists (WebShellAgentController.queryOperation, SurfaceRegistry, and the generated schema all carry it), so no Java, contract or Harness surface moves. Nothing here touches auth, sandbox, model selection, telemetry, release or a public contract, so no direction escalation. CHANGELOG: no direct reference, but the managed-agent Actions area is clearly current (design docs 2026-09-30 / 2026-10-02 / 2026-10-07, #12867 and #12380 both open).

Size: the core-module gate does not apply — all three files sit under packages/web-shell/client/components/managed/, which is not a core path, and the change stays inside one package. For context the split is 68 production lines (+8 client, +60 provider) against 113 test lines, so the diff is mostly tests.

Approach: scope feels right and I would not cut anything. Reading the same operation back is the minimal way to learn the outcome, and refusing to re-send the answer is what keeps the idempotency key and the original attempt intact — a retry-loop that re-answered would have been the obvious wrong turn here. Related D6 items (fresh-attempt policy after a definitive failure, input previews, resuming across a reload) are explicitly left in the tracker rather than folded in, which is the right call. The one alternative I considered was settling over the existing event stream instead of polling; that avoids the poll budget entirely but needs stream correlation and is well past the bounded slice #12867 asked for, so polling is the better trade.

Risk: no elevated risk signals — none of the changed files match the high-revert-correlation paths.

Moving on to code review. 🔍

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at b428c8c7cb8c55d1627eebfbb26b9e26ad9924d1 · re-run with @qwen-code /triage}

Code review

I wrote down my own fix before reading the diff: add a queryOperation to the client, poll the operation the 202 returned until it is terminal, bound the budget, never re-send so the idempotency key survives, and reuse the terminal-status error path that already exists. That is what this PR does, so I have no competing approach to argue for. The details hold up too:

• The happy path costs nothing. The status check sits at the top of the loop body, so an admission response that is already terminal returns before any sleep or read. That is the easy thing to get wrong in a poll loop and it is right here.
• The budget matches the claim. Nine delays totalling 29,750 ms — "about 30 s" is accurate, not aspirational.
• The type guard is load-bearing, not decoration. /operations/query is declared to return WebShellOperation, which the generated schema defines as WebShellCwdOperation | WebShellCommandOperation, and the Java side returns Object branching on operation kind. So a cwd_change body is a real shape this call can produce. Rejecting read.type !== 'action_response' also gives TypeScript the discrimination it needs to narrow the union down to WebShellCommandOperation before the assignment to current.
• The status test covers the whole enum. !== 'pending' && !== 'running' is exactly the complement of the non-terminal members of "pending" | "running" | "completed" | "failed" | "cancelled" | "recovery_blocked", and all three failure states already route through the existing throw.
• The tests pin the claim that matters. ['/actions/respond', '/operations/query', '/operations/query'] proves the answer goes out exactly once, which is the whole point of reading back instead of retrying. The unconfirmed case additionally asserts every call after the first is a read. The eslint-disable vitest/valid-expect comments explain themselves.
• Reuse is good throughout: the new client method follows respondAction/queryActions exactly, goes through the existing post() helper, and types itself off the generated WebShellOperationRequest/WebShellOperation rather than declaring a parallel shape. JavaCommandOperation is derived with Awaited<ReturnType<...>> instead of restating the schema type.

Three things worth raising, none of them blocking:

1. A definitive end-state can now reach the user as "retry the same option", and the classifier that exists to prevent that will not catch it. §6.2 says an operation "completes … with its end state (action_expired, action_cancelled or action_already_resolved) otherwise, exposed as failure_code (failureCode on WebShell)". The provider throws a plain Error with that code only interpolated into the message (Managed Agent approval answer failed (action_expired)), while endedAction() in use-managed-actions.ts reads a structured failure.code — the shape the client produces for an HTTP 409. So the mismatch is not new, but this PR widens it a lot: before, the admission 202 itself had to come back terminal, which is rare because §6.3 routes a late answer to a 409 (structured code, classifier fires). Now any Action that expires, is cancelled, or is answered elsewhere during the ≤30 s poll lands here instead — the card comes back saying "could not be confirmed, retry the same option" for a retry that cannot succeed, and stays until an unrelated re-read fires (action_expired self-heals via the EXPIRY_GRACE_MS timer; action_already_resolved does not). Attaching code: result.failureCode to the thrown error would let the existing classifier do its job. Small, and arguably its own D6 item — flagging so it is a decision rather than an accident.

2. Nothing tests the action_response guard. Given the union above, a cwd_change read-back is a shape the endpoint can genuinely return, and the guard is the only thing standing between it and a bogus status interpretation. One test with a cwd_change body would pin it.

3. Abort can lag by up to 8 s. signal?.throwIfAborted() runs after the sleep, so an abort during the longest delay is not noticed until that delay elapses. The in-flight read does abort, since the signal is forwarded to queryOperation. Cosmetic for a UI component; only worth changing if a caller ever needs prompt teardown.

Nit: the pre-existing approval test that gained a third mock sits outside the new fake-timers describe, so it now pays a real 250 ms wall-clock wait before the first query. Harmless, just a little slower.

One thing I checked because it looked like a UX regression and turned out not to be: on a recovery-blocked Session the operation can legitimately stay running past the budget (§6.2), so every answer there will now hide the card, wait ~30 s, and return it with an error. I expected the generic failure string to misdescribe that, but managed.approval.failed already reads "The approval answer could not be confirmed. Retry the same option or refresh to check its status." — which is precisely right for an unconfirmed answer, and "retry the same option" is exactly the same-key replay the PR relies on. The new outcome lands in a slot whose wording was already correct for it. That is the same idiom the composer uses for an unconfirmed submission, so it is consistent rather than novel.

The reshaped handshake, since the "admitted ≠ applied" distinction is the whole change:

sequenceDiagram
    participant P1 as WebShell approval card
    participant P2 as provider respond
    participant P3 as Java service
    P1->>P2: user picks an option (card hides immediately)
    P2->>P3: POST actions respond
    P3-->>P2: 202 with operation op-1, status running
    Note over P2: admitted, not applied - the old code stopped here
    loop bounded reads, 9 attempts, about 30s total
        P2->>P3: POST operations query (same sessionId and operationId)
        P3-->>P2: latest durable status
    end
    alt completed
        P2-->>P1: resolve, card stays hidden
    else failed, cancelled or recovery_blocked
        P2-->>P1: reject, card returns with the reason
    else still pending or running at budget end
        P2-->>P1: reject as not confirmed yet, same key replays on retry
    end

Testing evidence

This is an unattended CI run, so I did not build or execute anything from this PR — the evidence below is the PR's own CI on the reviewed commit, read through the API.

Test (ubuntu-latest, Node 22.x) is green, and that is the suite the test plan names (java-managed-agent-provider.test.ts lives in the web-shell package it runs). Lint & Static, Integration Tests (no-AK, No Sandbox) and Capture web-shell visuals are green too. web-shell E2E Smoke was still in flight at the time of writing; Qwen Code CI has one pending run, so this table is not final and the finalize job will refresh it.

The two Desktop Shell jobs are red and I could not classify them — see the caveat below the table, it is the one open item in this review.

Final CI results for b428c8c (auto-updated by the triage finalize job after CI completed):

_{One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。}

Not verified: why Desktop Shell is red. Both jobs failed, and their logs and step details are not readable yet — gh run view --job 112863990457 --log-failed returns "run 37642027796 is still in progress; logs will be available when it is complete", and the jobs API returns an empty steps array for both. So I can offer the surrounding evidence but not the failure itself:

• That job compiles and tests the Tauri crate under packages/desktop (cargo test --manifest-path src-tauri/Cargo.toml, plus scripts/test-release.js). Its own change filter only trips on ^packages/desktop/, the two create-*-manifest.mjs scripts, ci.yml and desktop-release.yml. This PR changes none of those — three TypeScript files under packages/web-shell/client/components/managed/ — so on the workflow's own logic every compile step should have been skipped and the job should have concluded success. That the job reached a failure at all is the anomaly, and it points at the job's detection or environment rather than at this diff. There is no mechanism I can find by which a provider polling change breaks a Rust crate.
• It is not a repo-wide outage: the same two jobs are green on four concurrent PR runs (37642965340, 37642569789, 37642466887, 37641461342), and the job is skipped on main (PR-CI only).

I am deliberately not calling this pre-existing infra noise, because the concurrency evidence argues against that, and not calling it PR-caused, because the diff gives no mechanism. It needs a human eye on the log once the run finishes.

Sandboxed verification would settle the behavioural claim: @qwen-code /verify. Every test in this PR drives a mocked fetch, so what the suite proves is the shape of the polling — one answer, then reads — not that Java behaves the way the design assumes. Specifically unverified: that /operations/query for an action_response operationId really returns the command-operation shape rather than the cwd_change member of the union the guard defends against; that a delivery failure during the worker's dispatch backoff actually surfaces as terminal failed with a populated failureCode (the R1-5 path from #13163); and that ~30 s is a realistic budget against real Harness delivery rather than a guess that will mostly time out into "not confirmed yet". The PR itself states it was "Not exercised against a real Java stack" and "Not run locally" — that is the gap, and the author has write access, so /verify (or /tmux for the card's visible behaviour) can be triggered directly without a sponsored run.

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at b428c8c7cb8c55d1627eebfbb26b9e26ad9924d1 · re-run with @qwen-code /triage}

Confidence: 4/5 — the code is solid and I found no blocker; the missing fifth is a CI signal I could not read, not a doubt about the change.

Stepping back: this is what a well-scoped slice of a tracked workstream looks like. I went looking for the usual problems and did not find them. The diff is 68 production lines against 113 test lines, every one of which serves the stated goal — no drive-by refactor, no formatting churn, no opportunistic "while I was in here". The adjacent D6 items that would have been tempting to bundle (fresh-attempt policy, input previews, reload-resume) are explicitly left in #12867 instead. The new code reuses what exists rather than standing up a parallel mechanism: same client method shape as its two neighbours, same post() helper, types taken from the generated schema, and the terminal-status throw that was already there.

I also checked the part I was most suspicious of, because "poll until settled" is where a change like this usually goes wrong. It does not. The status check sits at the top of the loop body, so an already-terminal admission response returns without a single sleep — the common case pays nothing. The budget is nine reads totalling 29,750 ms, so "about 30 s" is a measured claim rather than a round number. And the thing that makes the whole approach safe — never re-sending the answer, so the original attempt and its idempotency key survive — is exactly what the tests pin, via a URL sequence that proves one answer and nothing but reads afterwards.

The bug itself is real, and I confirmed it in the base code rather than accepting the framing: answered is only reset on a session change or for ids that failed with a structured end-state code, so a 202 that is still running hides the card for the rest of the session even when the answer never applies. §6.2 of the Actions design confirms the operation legitimately stays running while the Action is requested. Nothing theoretical about it.

What keeps this at 4 rather than 5 is honest uncertainty, not a defect I am waving through:

• Two Desktop Shell checks are red and I could not classify them. Logs and step details are unavailable while the run is in progress. The evidence cuts both ways — the job's own path filter does not match any file this PR touches and there is no mechanism I can find from a TypeScript polling change to a Rust Tauri crate, but the same job is green on four concurrent PRs, so I cannot write it off as infra noise either. Someone should read that log.
• The behavioural claim rests entirely on mocked fetch. The suite proves the polling shape; it cannot prove Java returns the command-operation member of the union, that a backoff failure surfaces as terminal failed with a populated failureCode, or that ~30 s is realistic against a real Harness. The PR says so plainly, which I appreciate, but saying so is not the same as settling it — @qwen-code /verify would.
• Finding 1 in the review above is a real, newly-widened gap (a definitive action_expired/action_cancelled/action_already_resolved end-state arriving as a plain Error bypasses endedAction(), so the user is offered a retry that cannot succeed). It is small and reasonably a follow-up, but I would not want it silently dropped — worth a line in feat(managed-agent): Stage D follow-ups for durable lifecycle, Turns, Actions, durable admission and AgentDefinition #12867 either way.

Would I maintain this in six months without cursing the author? Yes. The polling budget is a named constant with a comment explaining why it is shaped that way, the doc comment on settleActionResponse records the reasoning and the tracker reference, and the failure modes are enumerated rather than discovered later. That is the part that usually rots, and it is handled here.

Verdict: approve, deferred until CI lands green on b428c8c7cb8c55d1627eebfbb26b9e26ad9924d1. One pull_request run is still in flight (Qwen Code CI, with web-shell E2E Smoke running) and two checks are already red, so I am not posting an approval in this run — approving now would attest to a result that does not exist yet. The marker below lets the finalize job post the same commit-pinned approval once every check on that SHA is green, and it withholds and flags if anything stays red or the head moves. Given the Desktop Shell reds, the likely outcome is that this needs a human to read that log rather than an automatic approval — which is the correct result, not a failure of the gate.

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at b428c8c7cb8c55d1627eebfbb26b9e26ad9924d1 · re-run with @qwen-code /triage}