fix(web-shell): restore the Managed approval retry budget on reload - #13294

APPROVE at 972aec31. The production change is five lines and it completes a policy the adjacent branch already documented rather than introducing one. 0 threads were filed before this review; CI is 14 pass / 0 fail with one check queued.

The inconsistency this closes was already named in the file

The branch immediately above (:113-117) resets the budget when the reader is withdrawn, with the reasoning spelled out:

A withdrawn reader is not a failed read: a restored reader gets the whole retry budget back instead of a single attempt with no ladder.

enabled === undefined is the same kind of early return — the hook's own doc comment says so at :59-61: "enabled is undefined while the Session summary is unknown, for example during a reload: the shown approval stays and can be answered, and reads resume once the capability is known." And it is a genuinely separate branch, not a subset of the first: reader = enabled === false ? undefined : provider.actions (:70), so with enabled undefined the reader is still present and !reader does not fire. Two early returns that both mean "nothing failed here", only one of which restored the budget.

The failure chain follows mechanically. loadFailures is a useRef (:84) that indexes the ladder at :164 (LOAD_RETRY_DELAYS_MS[loadFailures.current]), and when the index runs past the array delay is undefined so no timer is scheduled and retries stop for good. So: an earlier outage exhausts the ladder → the Session summary becomes unknown on reload → the effect re-runs and returns early without resetting → the summary resolves, enabled becomes true, the effect re-runs → listPending fails once → LOAD_RETRY_DELAYS_MS[N] is undefined → no further attempt. The user reloaded precisely to recover and gets one shot with no ladder, which is the exact outcome the neighbouring comment calls wrong. The manual Retry button (:264-267) does reset, so this was not permanently unrecoverable — but reload is the gesture a user actually makes, and it silently did nothing.

No unbounded-retry hazard, for two independent reasons

Resetting a budget invites the question of whether it can now retry forever, so I checked rather than assumed:

1. The reset sits on a branch that returns without issuing a read, so it cannot itself start any attempt. Restoring the ladder only matters once enabled resolves to true, and enabled is driven by the Session summary becoming known — a one-shot resolution per reload, not a poll this effect feeds.
2. The case the ladder exists to protect against is handled independently of the budget: if (isNonRetryableClientError(failure)) return; at :163 short-circuits a deterministic 4xx before any delay is computed, with the comment naming why ("retrying it only burns requests, and the Retry button would keep offering an attempt that cannot succeed"). So a backend that answers the same way every time still gets no retries however often the budget is restored.

The test fails if the fix is reverted

restores the retry budget after a reload of the Session summary exhausts the ladder first (toHaveBeenCalledTimes(4) — one attempt plus three retries), then hook.rerender({ enabled: undefined }) to model the reload, then asserts the count reaches 5 and 6 with loadError undefined at the end. That last step is the load-bearing one: without the reset at :122 the counter is still past the array, so the post-reload read fails once (call 5), LOAD_RETRY_DELAYS_MS[4] is undefined, and call 6 never happens. Asserting the count rather than the internal counter is also the right choice — it observes the retry actually being scheduled instead of the ref's value.

The two companions (ignores a call with the same ID in a later Turn, drops an approval another client already answered) match the second commit's stated purpose of pinning paths where mutants survived, and at +91/-1 the test weight is proportionate to a five-line production change.

Vote effect

All three files are under packages/web-shell/client/components/managed/, which no CODEOWNERS rule covers, and reviewRequests is empty — so unlike a /packages/core/ change this one has no outstanding request pinning the decision. required_approving_review_count is 1.

One state note, since it is visible on the PR: @qwen-code-ci-bot approved at 14:03, yet reviewDecision was still REVIEW_REQUIRED twenty minutes later with no request outstanding. I have not determined whether that is a stale decision awaiting recomputation or that approval not counting toward the requirement, so I am not claiming this review is redundant — with the count at 1 and no request pinning it, it should be sufficient on its own either way.

Code review — round 1

No blocking findings. Approval blockers: none.

Scope

Reviewed: use-managed-actions.ts production fix (+6/-1) and all three new tests (+85 lines across two test files). No generated files, lockfiles, or docs in this diff.

Execution: working tree unavailable, so mutation probes not run locally. The PR body supplies wenshao's third-party mutation run (N1, A3, A8 killed). That evidence covers the environment it was run on (macOS, wenshao's stack); Linux/Windows behavior of the retry timer not independently exercised here.

Production code

use-managed-actions.ts — the enabled === undefined branch now resets loadFailures.current = 0.

The fix is structurally symmetric with the reset that already exists six lines above it in the !reader || !sessionId branch. The docblock and inline comment correctly describe the invariant: enabled is undefined while the Session summary is unknown (e.g., during a page Refresh), and returning from that state is treated the same way as the reader being restored — the hook gets a fresh retry ladder rather than resuming from an exhausted one.

Traced the causal chain end-to-end:

1. reader is provider.actions when enabled !== false, so the !reader guard fires only when enabled === false, not undefined.
2. With the retry ladder exhausted (loadFailures.current = 3, all of LOAD_RETRY_DELAYS_MS consumed), re-entering with enabled === true after a reload would reach the .catch path, compute LOAD_RETRY_DELAYS_MS[3] === undefined, and schedule no retry — confirmed by the delay table in the source.
3. The new loadFailures.current = 0 assignment in the enabled === undefined branch intercepts the reload cycle before step 2 fires.

No other path appears to miss a reset: the sessionId-change useEffect resets the counter on session switch, the retry() callback resets it on user-initiated retry, and the success branch zeroes it on a clean read.

Tests

use-managed-actions.test.tsx — restores the retry budget after a reload of the Session summary

Valid efficacy probe. The test mock returns five failures followed by one success. After four calls (the three retries plus the initial), the ladder is exhausted. The test then cycles enabled through undefined → true, triggering the fix, and asserts a fifth call fires immediately and a sixth (the success) fires after 2 000 ms. Without the fix the hook stops at four calls, which the PR description confirms with an explicit expected-vs-actual count. The delay sequence [0, 2 000, 5 000, 10 000, 60 000] ms correctly lines up with the LOAD_RETRY_DELAYS_MS = [2_000, 5_000, 10_000] ladder.

use-managed-actions.test.tsx — drops an approval another client already answered

Tests the endedAction(failure) branch in respond: a 409 with code: 'action_already_resolved' must hide the card (action === undefined), leave no answerError, and trigger a re-read. The ENDED_ACTION_CODES set already includes 'action_already_resolved' at this head; the test pins that the code path is exercised and does not regress if the code is altered.

managed-approval.test.ts — ignores a call with the same ID in a later Turn

Tests both message-key forms. For call-ID-keyed rows, findManagedApprovalTool uses the exact ${turnId}:${callId} match, so turn-3:call-1 cannot match an action whose exact = 'turn-2:call-1'. For item-ID-keyed rows, the guard tool.callId.startsWith(${action.turnId}:) eliminates the turn-3 item when the action's turnId is turn-2. Both cases are covered and the assertions are specific.

Unreviewed dimensions

• Linux / Windows retry-timer behavior: no execution rung run locally; the PR author explicitly marks Windows as ⚠️ and Linux as CI-only. The timer behavior is pure JS (setTimeout), and the test environment (jsdom + Vitest fake timers) is platform-independent, so this is a low-risk gap.
• No terminal-dependent (OS filesystem, platform branch) changes in this diff; rung 3 not applicable.

Reviewed with AI assistance.