Skip to content

test(e2e): cover approval outcomes across an agent reload - #1561

Merged
TheGreatAxios merged 4 commits into
mainfrom
cl-10763-approved-tool-calls-stall-interactive-sessions-then-crash
Oct 25, 2026
Merged

TheGreatAxios merged 4 commits into
mainfrom
cl-10763-approved-tool-calls-stall-interactive-sessions-then-crash

Conversation

@bwachman

@bwachman bwachman commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • The approval reload e2e suite covers an approved, parked MCP call while an MCP tool-set change is pending: the agent rebuilds over the same toolset, as the TUI does, and the rebuilt agent has both the parked call's tool and the newly added one. The call runs once, its result survives the rebuild, the rebuilt agent continues, and passing the original approval deadline adds no second result or "approval timed out" text.
  • It also covers a rejected call and a call answered by the approval deadline, each with the agent reopened at the run's end, and an approved MCP call whose deferred tool-set rebuild fires at the run's end. Each leaves exactly one result for the parked call, a follow-up turn works, and no later deadline adds a result.
  • The TUI lifecycle tests cover an MCP server connecting while an approval prompt is live: the reload stays pending through the approval and rebuilds only after the resumed run reports its end.
  • The approval reload sessions pin their tool set at build, as the interactive TUI does.

A reload after a rejection or timeout but before the cycle commits loses the decision's result. That is product behavior outside this change and is tracked in CL-11433.

Verification

  • bun test e2e/reactor-approval-reload.test.ts src/tui/runner/exit.test.ts: 44 pass, 0 fail across 2 files
  • bun test e2e/reactor-approval-reload.test.ts --randomize --seed 7 and --seed 8: 6 pass, 0 fail each
  • bun run check (lint, typecheck, dead exports, build, and the guarded suite at --randomize --seed 424242): exit 0; 9455 pass, 0 fail, 36802 expect() calls across 618 files
  • Paths covered by the e2e: a reload before the cycle commits for approved shell and MCP calls (including during the approved call), and a reload at the run's end (the TUI's deferred-reload boundary) for an approved MCP call with a pending tool-set change, a rejected call, and a call answered by the approval deadline. The runner-level deferral gate is covered in src/tui/runner/exit.test.ts

Related to CL-10763

@linear-code

linear-code Bot commented Oct 23, 2026

Copy link
Copy Markdown

CL-10763

@bwachman
bwachman force-pushed the cl-10763-approved-tool-calls-stall-interactive-sessions-then-crash branch from 1e8eb64 to 4dd4fb4 Compare October 24, 2026 00:41

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fleet review verdict: APPROVE-WITH-NITS

PR: #1561 — test(tui): cover approved mcp calls across a deferred reload (CL-10763 family)

Convergence: swarm → defect / quality / solve convergence → converged approve-with-nits / approve-with-nits / approve.

Outcome

Solve lens: SOLVES — regression coverage added for approved MCP call across deferred reload.

Nits / credibility gaps (non-blocking)

  • Auditor F1–F4 dispositioned; QA flagged verification gaps (full-suite tally, e2e deferred-reload path).

Required changes

None blocking. Flagged for operator disposition.

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fleet review verdict: REQUEST CHANGES

PR: #1561 — test(tui): cover approved mcp calls across a deferred reload (CL-10763 family)

Operator disposition update: the prior "approve-with-nits" posture is withdrawn; this PR is converted to REQUEST CHANGES with the items carried.

Requested changes

  • N1 (qa): coverage gaps (e2e deferred-reload path; rejected/deadline-decision coverage).
  • N2 (auditor): F1–F4 dispositions (sleep-based flake addressed; deterministic; e2e race addressed; convention accepted).
  • N3 (qa): verification credibility gaps (full-suite tally; e2e path).

Required changes to merge

Address the requested changes above (or explicitly close each) before merge.

Why it matters

Non-blocking items remain; operator opted for a stricter review posture.

@bwachman

Copy link
Copy Markdown
Contributor Author

Coverage gaps (deferred reload, rejected and deadline decisions): fixed.
e2e/reactor-approval-reload.test.ts now settles the parked call three ways through one helper, decideThenReload (approve, reject, or let the deadline answer it). Three tests are new:

  • A rejected ask, with the agent reopened from the store at the run's end. The rebuilt agent holds exactly one result for the call ("denied by approver"), a follow-up turn works, and passing the original deadline adds nothing: no "approval timed out" and no reactor error.
  • An ask the reactor's own deadline answers before any decision, with the agent reopened at the run's end. The rebuilt agent holds exactly one "approval timed out" result, a follow-up turn works, and moving the clock two hours past the deadline adds no second result. That is past both the original deadline and a full approval window armed fresh at the reload.
  • The deferred-reload path for an approved MCP call. An MCP tool-set change lands while the approval prompt is live. The rebuild then runs in place at the run's end, and only while a tool has been added, which is the same boundary reloadIfIdle in src/tui/runner/exit.ts uses and one of the drifts it rebuilds for. After the rebuild the new tool is mounted, the drift is cleared, the MCP tool ran once, and the original deadline adds no second result.

The integration harness has no TUI runner, so the e2e does not call reloadIfIdle itself. It reproduces that function's trigger point for an added tool. The deferral gate itself, which holds the rebuild until the approval-resumed run ends, is covered at the runner level by the existing test in src/tui/runner/exit.test.ts.

One gap is left out on purpose. If the agent reloads after a reject or timeout but before the cycle commits, the decision's result is lost. The store keeps the pending approval, the rebuilt history holds the tool call with no result, and the next model request sends that tool call unanswered. No late "approval timed out" showed up within two hours of simulated time. This is product behavior outside this change, so it has no test here; it is tracked in CL-11433, along with the live case where an approval dropped by an interrupt gets a false "approval timed out" at its old deadline. The approved case at that same reload point is already covered and passes.

Auditor findings F1 to F4: closed, no change. These were dispositioned in the earlier round: the sleep-based flake was addressed, the tests are deterministic, the e2e race was addressed, and the convention point was accepted. This round changes nothing for them.

Verification credibility: fixed. Proposed Verification section for the description:

Verification

  • bun test e2e/reactor-approval-reload.test.ts src/tui/runner/exit.test.ts: 44 pass, 0 fail across 2 files.
  • bun test e2e/reactor-approval-reload.test.ts --randomize --seed 7 and --seed 8: 6 pass, 0 fail each.
  • bun run check (lint, typecheck, dead exports, build, and the guarded suite at --randomize --seed 424242): exit 0; 9455 pass, 0 fail, 36802 expect() calls across 618 files.
  • Paths covered by the e2e: a reload before the cycle commits for approved shell and MCP calls (including during the approved call), and a reload at the run's end (the TUI's deferred-reload boundary) for an approved MCP call with a pending tool-set change, a rejected call, and a call answered by the approval deadline. The runner-level deferral gate is covered in src/tui/runner/exit.test.ts.

@bwachman bwachman changed the title test(tui): cover approved mcp calls across a deferred reload test(e2e): cover approval outcomes across an agent reload Oct 24, 2026
@bwachman

Copy link
Copy Markdown
Contributor Author

Resolved findings at 9ca3fa36, with line citations

Finding Status Where
N1: coverage of the deferred-reload path Resolved The e2e test "an approved MCP call stays settled when the deferred tool-set rebuild fires at the run's end" (e2e/reactor-approval-reload.test.ts:442-495). The runner-level deferral gate is covered in src/tui/runner/exit.test.ts:1034-1058.
N1: rejected and deadline-decision coverage Resolved (at the run's end) Reject and expire cases, each asserting one result, the follow-up turn and no reactor.error (e2e/reactor-approval-reload.test.ts:502-563), through the decideThenReload helper (:236).
N3: verification credibility Resolved The PR description's Verification section gives the full-suite tally and the e2e paths. Re-run at this commit: bun test e2e/reactor-approval-reload.test.ts src/tui/runner/exit.test.ts gives 44 pass, 0 fail, and --randomize --seed 7 / --seed 8 give 6 pass each.

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed at 9ca3fa3. Rejected, timed-out, and rebuilt approvals are asserted, and the new tests do not reintroduce a sleep. Approving.

@corbits-triage

Copy link
Copy Markdown
Contributor

Triage: Pass

No changes needed from you. A maintainer will review this pull request.

@TheGreatAxios
TheGreatAxios merged commit 56013ae into main Oct 25, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants