From 7afb57c0222c6f41dcd6f239b2c4db7e94ca370a Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 6 Nov 2026 12:59:54 -0600 Subject: [PATCH] test(runner-e2e): repair restart-cell harness drift (#19538) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Runner Product E2E suite proves that agent work survives a control-plane restart > - A restart chaos campaign found three restart cells that fail in the test harness, not in the product > - A harness failure hides the real restart result, so the campaign cannot classify these cells > - This pull request repairs the harness drift in those cells and names the one failure the harness cannot prevent > - The benefit is that these cells now test restart behavior, or fail with a clear cause ## Linked Issues or Issue Description Refs #14811 (connector setup became one screen). Refs #19475 (completion-update restart fixture). **What happened?** 1. `lifecycle-baseline.runner-codex.local.tool-review-restart` clicked a "Save and continue" button. #14811 removed that button. The cell failed during setup. After that fix, the cell also failed after the restart: the expanded review card does not survive the post-restart reload, so "Approve & run" was not visible. 2. `local-session-integrity.runner-acpx-claude.local.structured-question-restart-resume` failed with `page.goto: net::ERR_ABORTED` after the controller restart. The old document's Vite client reloads when the replacement server starts, and this aborts the test navigation. The code tolerated only `TimeoutError`. 3. `completion-updates.runner-acpx-claude.local.handoff-completion-restart` waited for the source agent to run an instructed wait command. Claude replied without the command (2/2 attempts), so the restart never occurred. The cell then failed with a generic timeout. **Expected behavior** Each cell reaches its restart boundary and tests restart behavior. If the boundary cannot be reached, the cell names the cause. **Steps to reproduce** Run `pnpm test:e2e:runner -- --max-parallel 1 --max-automatic-retries 0 --id ` for each cell above on `master`. ## What Changed - `tests/runner-e2e/connection-reviews.ts`: accept the one-screen "Check link" step and the older "Save and continue" step. This uses the same regex approach as `connection-flow.ts`. - `tests/runner-e2e/runner.spec.ts` (tool review): click "Review request" after the restart reload, on the document that acts on the card. - `tests/runner-e2e/chat-restart.ts`: add `gotoAfterServerRestart`. It retries only `net::ERR_ABORTED`, within the caller's deadline and at most 5 attempts. It still accepts `TimeoutError`. It rethrows all other errors. The structured-question restart uses it. The existing sentinel and durable-state assertions still prove the new document. - `tests/runner-e2e/completion-updates.ts` and `completion-update-flow.ts`: add `completionRestartGateSkipped` and `completionRestartGateTimeoutDetail`. The gate poll stops at once with `restart boundary not reached (agent skipped gate)` when no completion turn is active and the turn that carries this task's Done facts (`chatCompletionUpdates`, as in `completionDelivery`) succeeded or published its reply before the gate reports `waiting`. A failed or cancelled turn is not blamed on the agent, because delivery can retry it; the timeout detail names it instead. The post-restart chat navigation now uses `gotoAfterServerRestart`. - A fixture-level gate is not possible without a product hook. The native run finalizer publishes the completion reply inside its own transaction, through the issue service closure. The isolated-server entrypoint can patch only class prototypes and HTTP responses, and the reply uses no tool call or HTTP request. - `tests/runner-e2e/README.md`: document the skipped-gate failure. - Docs: use `pnpm --filter @paperclipai/paperclip-runner run build:runner-binaries` in `tests/runner-e2e/README.md`, `tests/restart-chaos/README.md`, `tests/restart-chaos/main.ts` and `doc/DEVELOPING.md`. - No product code changes. ## Verification - Unit: `pnpm exec vitest run --config tests/runner-e2e/vitest.config.ts tests/runner-e2e/chat-restart.test.ts tests/runner-e2e/completion-updates.test.ts tests/runner-e2e/connection-reviews.test.ts` (36 passed). - Typecheck: `node scripts/typecheck-test-projects.mjs tests/runner-e2e` passed. - Live cells, local, `--max-parallel 1 --max-automatic-retries 0`, macOS, load average 35–65: - `lifecycle-baseline.runner-codex.local.tool-review-restart`: PASS (first run found the review-card reload drift, fixed in the second commit). - `local-session-integrity.runner-acpx-claude.local.structured-question-restart-resume`: PASS. A first attempt failed before the restart with `PRP command session.open timed out` at load ~36 (environment). - `completion-updates.runner-acpx-claude.local.handoff-completion-restart`: FAIL as designed, twice (before and after the review fix), with `restart boundary not reached (agent skipped gate): completion turn … ended succeeded without the reference gate reporting waiting`. Claude skipped the wait both times, so this cell still does not exercise a restart. ## Risks - Low risk. Test harness and docs only. - The navigation retry could hide a real abort. It is limited to `net::ERR_ABORTED`, 5 attempts and the deadline, and the test still asserts that a new document loaded. - The ACPX Claude completion restart cell still does not exercise a restart when the agent skips the gate. It now reports this cause by name. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, via Claude Code with tool use (shell, file edit, browser test runs). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- doc/DEVELOPING.md | 2 +- tests/restart-chaos/README.md | 2 +- tests/restart-chaos/main.ts | 2 +- tests/runner-e2e/README.md | 4 +- tests/runner-e2e/chat-restart.test.ts | 54 +++++++++++++++++++++ tests/runner-e2e/chat-restart.ts | 34 +++++++++++++ tests/runner-e2e/completion-update-flow.ts | 13 +++-- tests/runner-e2e/completion-updates.test.ts | 44 ++++++++++++++++- tests/runner-e2e/completion-updates.ts | 41 ++++++++++++++++ tests/runner-e2e/connection-reviews.test.ts | 19 ++++++++ tests/runner-e2e/connection-reviews.ts | 15 +++++- tests/runner-e2e/runner.spec.ts | 30 ++++++------ 12 files changed, 233 insertions(+), 27 deletions(-) create mode 100644 tests/runner-e2e/chat-restart.test.ts create mode 100644 tests/runner-e2e/connection-reviews.test.ts diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index 814cb9750d0..da29540ff9c 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -3425,7 +3425,7 @@ successful warm-session retention or admit another provider turn. Run the credential-free real-process restart suite with: ```sh -pnpm --filter @paperclipai/paperclip-runner build:runner-binaries +pnpm --filter @paperclipai/paperclip-runner run build:runner-binaries pnpm exec vitest run server/src/services/native-runtime/native-runner-restart-recovery.integration.test.ts pnpm --filter @paperclipai/paperclip-runner exec vitest run src/live/runnerd-codex-transport-adoption.test.ts -t 'adopts a live runner' ``` diff --git a/tests/restart-chaos/README.md b/tests/restart-chaos/README.md index 14207e23782..348747af498 100644 --- a/tests/restart-chaos/README.md +++ b/tests/restart-chaos/README.md @@ -12,7 +12,7 @@ T-oracle. ```sh pnpm install --frozen-lockfile -pnpm --filter @paperclipai/paperclip-runner build:runner-binaries # native runner (debug) +pnpm --filter @paperclipai/paperclip-runner run build:runner-binaries # native runner (debug) set -a; source ~/.secrets; set +a # provider keys, process env only node cli/node_modules/tsx/dist/cli.mjs tests/restart-chaos/main.ts --env local \ --adapters codex,claude,opencode --concurrency 4 --tasks 8 --faults K1,K2,K4 --seed 42 diff --git a/tests/restart-chaos/main.ts b/tests/restart-chaos/main.ts index 7fd6981e028..c17c08d6c03 100644 --- a/tests/restart-chaos/main.ts +++ b/tests/restart-chaos/main.ts @@ -53,7 +53,7 @@ function preflight(plan: WavePlan) { if (missing.length) throw new Error(`Missing credentials ${missing.join(", ")}. Run: set -a; source ~/.secrets; set +a`); const runner = process.env.PAPERCLIP_RUNNER_BINARY ?? path.join(repositoryRoot, "packages/paperclip-runner/runner/target/debug/paperclip-runnerd"); - if (!existsSync(runner)) throw new Error(`Runner binary missing at ${runner}. Run: pnpm --filter @paperclipai/paperclip-runner build:runner-binaries`); + if (!existsSync(runner)) throw new Error(`Runner binary missing at ${runner}. Run: pnpm --filter @paperclipai/paperclip-runner run build:runner-binaries`); // Same provider bundle preparation as tests/runner-e2e/verified-provider-bundles.setup.mjs. const build = spawnSync(process.execPath, [path.join(repositoryRoot, "packages/paperclip-runner/scripts/build-verified-provider-entrypoints.mjs")], { cwd: repositoryRoot, stdio: "inherit" }); diff --git a/tests/runner-e2e/README.md b/tests/runner-e2e/README.md index f6be16f3c1d..dcd913f3133 100644 --- a/tests/runner-e2e/README.md +++ b/tests/runner-e2e/README.md @@ -686,7 +686,7 @@ so a content mismatch cannot suppress the communication evidence. The source thread is observed for 120 seconds. The probe retains a later correction even if an earlier reply already passes delivery and access. A later clarification does not erase an earlier accessible delivery. -The busy cell holds a separate source reply open until the worker finishes; the multiple cell delegates two notes and requires one completion per task. The restart cell holds the source provider at a fixture reference gate, then restarts the server after durable Done but before publication and releases the gate. The gate makes the interruption boundary observable and prevents a fast successful reply from racing the restart assertion. The onboarding cell records its naturally occurring timing. +The busy cell holds a separate source reply open until the worker finishes; the multiple cell delegates two notes and requires one completion per task. The restart cell holds the source provider at a fixture reference gate, then restarts the server after durable Done but before publication and releases the gate. The gate makes the interruption boundary observable and prevents a fast successful reply from racing the restart assertion. Reaching the gate depends on the source agent running the instructed wait: the completion reply is published inside the run finalizer's own transaction, so the isolated-server entrypoint cannot hold it without a product hook. If no completion turn is still active and the turn that received this task's Done facts has succeeded or published its reply before the gate reports `waiting`, the cell fails at once with `restart boundary not reached (agent skipped gate)` rather than a generic timeout. That failure is agent compliance (the restart was never exercised), not restart robustness. A completion turn that fails or is cancelled before the gate is not blamed on the agent, because delivery can retry it; if the wait then times out, the timeout names that turn and its status as `restart boundary not reached`. After the restart, the chat navigation retries an abort caused by the old document's reload. The onboarding cell records its naturally occurring timing. The mechanical oracle requires a run-attributed source reply after durable completion, plus the actual saved output or a navigable task/output link. @@ -839,7 +839,7 @@ runner binaries: ```bash pnpm install pnpm exec playwright install chromium -pnpm --filter @paperclipai/paperclip-runner build:runner-binaries +pnpm --filter @paperclipai/paperclip-runner run build:runner-binaries ``` List cells without loading credentials or starting Paperclip: diff --git a/tests/runner-e2e/chat-restart.test.ts b/tests/runner-e2e/chat-restart.test.ts new file mode 100644 index 00000000000..c2e2e5544d2 --- /dev/null +++ b/tests/runner-e2e/chat-restart.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from "vitest"; +import { gotoAfterServerRestart, isAbortedNavigation } from "./chat-restart.js"; + +const aborted = () => new Error("page.goto: net::ERR_ABORTED at http://127.0.0.1:1/RUN/issues/RUN-1"); +const timeout = () => Object.assign(new Error("page.goto: Timeout 30000ms exceeded."), { name: "TimeoutError" }); +function page(outcomes: Array) { + const calls: string[] = []; + return { + calls, + goto: async (url: string) => { + calls.push(url); + const outcome = outcomes.shift(); + if (outcome) throw outcome; + return null; + }, + }; +} + +describe("navigation after a server restart", () => { + it("retries a navigation the old document's reload aborted", async () => { + const p = page([aborted(), aborted(), undefined]); + await gotoAfterServerRestart(p as never, "/issue", { deadlineAt: Date.now() + 10_000, retryDelayMs: 0 }); + expect(p.calls).toEqual(["/issue", "/issue", "/issue"]); + }); + + it("keeps tolerating an unsettled lifecycle timeout without retrying", async () => { + const p = page([timeout()]); + await gotoAfterServerRestart(p as never, "/issue", { deadlineAt: Date.now() + 10_000, retryDelayMs: 0 }); + expect(p.calls).toHaveLength(1); + }); + + it("rethrows other navigation errors immediately", async () => { + const p = page([new Error("page.goto: net::ERR_CONNECTION_REFUSED"), undefined]); + await expect(gotoAfterServerRestart(p as never, "/issue", { deadlineAt: Date.now() + 10_000, retryDelayMs: 0 })) + .rejects.toThrow("ERR_CONNECTION_REFUSED"); + expect(p.calls).toHaveLength(1); + }); + + it("stops retrying aborts at the attempt bound and at the deadline", async () => { + const bounded = page([aborted(), aborted(), aborted(), undefined]); + await expect(gotoAfterServerRestart(bounded as never, "/issue", { deadlineAt: Date.now() + 10_000, retryDelayMs: 0, maxAttempts: 3 })) + .rejects.toThrow("ERR_ABORTED"); + expect(bounded.calls).toHaveLength(3); + const late = page([aborted(), undefined]); + await expect(gotoAfterServerRestart(late as never, "/issue", { deadlineAt: 0, retryDelayMs: 0 })).rejects.toThrow("ERR_ABORTED"); + expect(late.calls).toHaveLength(1); + }); + + it("recognizes only Chromium's aborted-navigation error", () => { + expect(isAbortedNavigation(aborted())).toBe(true); + expect(isAbortedNavigation(new Error("net::ERR_ABORTED_SOMETHING"))).toBe(false); + expect(isAbortedNavigation("net::ERR_ABORTED")).toBe(false); + }); +}); diff --git a/tests/runner-e2e/chat-restart.ts b/tests/runner-e2e/chat-restart.ts index ef488ab3bd6..ed6bcfc0368 100644 --- a/tests/runner-e2e/chat-restart.ts +++ b/tests/runner-e2e/chat-restart.ts @@ -5,3 +5,37 @@ export async function restartChatServer(page: Page, restart: () => Promise await page.goto("about:blank", { waitUntil: "commit", timeout: 30_000 }); await restart(); } + +/** Chromium reports a navigation another navigation replaced as net::ERR_ABORTED. */ +export function isAbortedNavigation(error: unknown): boolean { + return error instanceof Error && /\bnet::ERR_ABORTED\b/.test(error.message); +} + +/** + * Open `url` on a page whose document predates a server restart. The old + * document's Vite client reloads itself when the replacement server comes up; + * if that reload lands while this navigation is in flight, Chromium aborts one + * of them with net::ERR_ABORTED. Retry only that abort, within the caller's + * deadline and a bounded attempt count, and rethrow every other error. A + * timeout still resolves: the replacement server can commit and render a fresh + * document while its lifecycle stays unsettled, so callers must assert the new + * document and durable state explicitly afterwards. + */ +export async function gotoAfterServerRestart( + page: Pick, + url: string, + input: { deadlineAt: number; maxAttempts?: number; retryDelayMs?: number; now?: () => number }, +): Promise { + const now = input.now ?? Date.now; + const maxAttempts = input.maxAttempts ?? 5; + for (let attempt = 1; ; attempt += 1) { + try { + await page.goto(url, { waitUntil: "commit" }); + return; + } catch (error) { + if (error instanceof Error && error.name === "TimeoutError") return; + if (!isAbortedNavigation(error) || attempt >= maxAttempts || now() >= input.deadlineAt) throw error; + } + await new Promise(resolve => setTimeout(resolve, input.retryDelayMs ?? 500)); + } +} diff --git a/tests/runner-e2e/completion-update-flow.ts b/tests/runner-e2e/completion-update-flow.ts index 9e6978ad53a..2b4bfbb423b 100644 --- a/tests/runner-e2e/completion-update-flow.ts +++ b/tests/runner-e2e/completion-update-flow.ts @@ -6,7 +6,8 @@ import { pollUntil, type RunnerApi } from "./api.js"; import { sendChatMessage, readChatOutputDocument, collectChatRunEvidence, type ChatFlowInput, type ChatRun } from "./chat-flow.js"; import { arm as armDocumentGate, clear as clearDocumentGate, release as releaseDocumentGate, waitUntilHeld } from "./context-comment-gate.js"; import { publishChatBrief, workspaceChatBrief } from "./chat-stories.js"; -import { completionDelivery, completionOutputUsesReleasedBrief, type CompletionObservation } from "./completion-updates.js"; +import { gotoAfterServerRestart } from "./chat-restart.js"; +import { completionDelivery, completionOutputUsesReleasedBrief, completionRestartGateSkipped, completionRestartGateTimeoutDetail, type CompletionObservation } from "./completion-updates.js"; type Row = Record; export async function observeCompletionUpdate(input: { @@ -232,10 +233,15 @@ export async function runChatCompletionUpdate(context: { load: async () => { const runs = await context.allRuns(); return { runs, ready: await readFile(completionWait!.at(taskDirectory(context.issue().id)).ready, "utf8").catch(() => ""), + comments: await api.get(`/api/issues/${context.issue().id}/comments?order=asc`), sourceCwds: runCwd(runs, context.issue().id) }; }, accept: state => state.ready === "waiting" && state.runs.some(r => r.contextSnapshot?.issueId === context.issue().id && - r.contextSnapshot?.wakeReason === "chat_task_completed" && r.status === "running") }); + r.contextSnapshot?.wakeReason === "chat_task_completed" && r.status === "running"), + // The gate depends on the agent running the instructed wait; name a + // skipped gate rather than timing out (see completionRestartGateSkipped). + reject: state => completionRestartGateSkipped({ sourceId: context.issue().id, workerId: task!.id, ...state }), + timeoutDetail: state => completionRestartGateTimeoutDetail(state && { sourceId: context.issue().id, workerId: task!.id, ...state }) }); const beforeRestart = await api.get(`/api/issues/${context.issue().id}/comments?order=asc`); const workerState = await api.get(`/api/issues/${task!.id}`); expect(beforeRestart.filter(c => c.authorAgentId && c.createdAt >= workerState.completedAt), "restart must precede completion publication").toEqual([]); @@ -245,7 +251,8 @@ export async function runChatCompletionUpdate(context: { await input.evidence("completion-restart-boundary.json", { worker: await api.get(`/api/issues/${task!.id}`), runs: reportingRun.runs, sourceReferenceGateReady: true }); await input.restart(); await publishBrief(completionWait!, context.issue().id, "The organizer is ready to receive the saved result. Report the completed work now."); - await page.goto(`/${f.company.issuePrefix}/chats/${f.agent.id}`, { waitUntil: "commit" }); + // The old document's Vite client can reload and abort this navigation. + await gotoAfterServerRestart(page, `/${f.company.issuePrefix}/chats/${f.agent.id}`, { deadlineAt: input.deadlineAt }); } for (const [index, item] of delegated.entries()) { await pollUntil({ label: "delegated note completed", deadlineAt: Date.now() + 180_000, intervalMs: 1000, diff --git a/tests/runner-e2e/completion-updates.test.ts b/tests/runner-e2e/completion-updates.test.ts index d06bbee5736..0900c9aa987 100644 --- a/tests/runner-e2e/completion-updates.test.ts +++ b/tests/runner-e2e/completion-updates.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { completionDelivery, completionOutputUsesReleasedBrief, type CompletionObservation } from "./completion-updates.js"; +import { COMPLETION_RESTART_GATE_SKIPPED, completionDelivery, completionOutputUsesReleasedBrief, completionRestartGateSkipped, completionRestartGateTimeoutDetail, type CompletionObservation } from "./completion-updates.js"; import { runnerMatrix } from "./catalog.js"; const output = "Welcome to our free Friday garden meetup for beginners. Join us at 10:30; reference GARDEN123."; @@ -82,3 +82,45 @@ describe("completion-update delivery oracle", () => { } }); }); + +describe("completion restart gate", () => { + const turn = (status: string, id = "turn", workerId: string | null = "worker") => ({ id, status, + contextSnapshot: { issueId: "source", wakeReason: "chat_task_completed", ...(workerId ? { chatCompletionUpdates: [{ id: workerId, status: "done" }] } : {}) } }); + const base = { sourceId: "source", workerId: "worker", runs: [] as Record[], comments: [] as Record[] }; + it("keeps waiting while the completion turn is pending or running at the gate", () => { + expect(completionRestartGateSkipped({ ...base, ready: "" })).toBeUndefined(); + expect(completionRestartGateSkipped({ ...base, ready: "", runs: [turn("queued", "turn", null)] })).toBeUndefined(); + expect(completionRestartGateSkipped({ ...base, ready: "", runs: [turn("running")] })).toBeUndefined(); + expect(completionRestartGateSkipped({ ...base, ready: "waiting", runs: [turn("succeeded")] })).toBeUndefined(); + }); + it("names a correlated completion turn that succeeded without reaching the gate", () => { + expect(completionRestartGateSkipped({ ...base, ready: "", runs: [turn("succeeded")] })) + .toBe(`${COMPLETION_RESTART_GATE_SKIPPED}: completion turn turn ended succeeded without the reference gate reporting waiting`); + }); + it("keeps waiting while a later completion turn is still active", () => { + expect(completionRestartGateSkipped({ ...base, ready: "", runs: [turn("succeeded", "first"), turn("queued", "retry", null)] })).toBeUndefined(); + }); + it("does not blame the agent for a turn that failed before the gate, and names it on timeout", () => { + const state = { ...base, ready: "", runs: [turn("failed")] }; + expect(completionRestartGateSkipped(state)).toBeUndefined(); + expect(completionRestartGateTimeoutDetail(state)) + .toBe("restart boundary not reached: completion turn(s) turn ended failed before the reference gate reported waiting"); + expect(completionRestartGateTimeoutDetail({ ...state, ready: "waiting" })).toBeUndefined(); + expect(completionRestartGateTimeoutDetail(undefined)).toBeUndefined(); + }); + it("names a correlated agent reply and ignores unrelated replies", () => { + const comments = [ + { id: "handoff", authorAgentId: "lead", createdByRunId: "initial" }, + { id: "user", authorUserId: "board" }, + ]; + expect(completionRestartGateSkipped({ ...base, ready: "", runs: [turn("failed")], comments })).toBeUndefined(); + expect(completionRestartGateSkipped({ ...base, ready: "", runs: [turn("failed")], + comments: [...comments, { id: "reply", authorAgentId: "lead", createdByRunId: "turn" }] })) + .toBe(`${COMPLETION_RESTART_GATE_SKIPPED}: agent reply reply was published without the reference gate reporting waiting`); + }); + it("ignores other conversations, other tasks and ordinary chat turns", () => { + const other = { id: "o", status: "succeeded", contextSnapshot: { issueId: "other", wakeReason: "chat_task_completed", chatCompletionUpdates: [{ id: "worker" }] } }; + const ordinary = { id: "u", status: "succeeded", contextSnapshot: { issueId: "source", wakeReason: "chat_message" } }; + expect(completionRestartGateSkipped({ ...base, ready: "", runs: [other, ordinary, turn("succeeded", "t2", "another-worker")] })).toBeUndefined(); + }); +}); diff --git a/tests/runner-e2e/completion-updates.ts b/tests/runner-e2e/completion-updates.ts index 2cb751ccf6a..1af507116b7 100644 --- a/tests/runner-e2e/completion-updates.ts +++ b/tests/runner-e2e/completion-updates.ts @@ -23,6 +23,47 @@ export const completionReviewRubric = [ "Does it avoid repeated completion announcements or redundant acknowledgements, while allowing corrections and updates about different tasks?", ] as const; +export const COMPLETION_RESTART_GATE_SKIPPED = "restart boundary not reached (agent skipped gate)"; + +type GateState = { sourceId: string; workerId: string; ready: string; runs: Row[]; comments: Row[] }; +const completionTurns = (state: GateState) => state.runs.filter(r => r.contextSnapshot?.issueId === state.sourceId && + r.contextSnapshot?.wakeReason === "chat_task_completed"); +/** Correlated as completionDelivery does: the turn received this task's Done facts. */ +const carriesWorker = (run: Row, workerId: string) => Array.isArray(run.contextSnapshot?.chatCompletionUpdates) && + run.contextSnapshot.chatCompletionUpdates.some((u: Row) => u.id === workerId); + +/** + * The restart cell's boundary is the source provider waiting at its reference + * gate. The completion reply is published inside the run finalizer's own + * transaction, which an isolated-server fixture cannot hold without a product + * hook, so reaching the boundary depends on the agent running the wait. Once + * no completion turn is active and this task's correlated turn has succeeded + * or published its reply without the gate ever reporting "waiting", the + * boundary can no longer be reached: report it by name instead of waiting for + * a generic timeout. A failed or cancelled turn is not evidence that the agent + * skipped the gate, and delivery may retry it; completionRestartGateTimeoutDetail + * names it if the wait then times out. + */ +export function completionRestartGateSkipped(state: GateState): string | undefined { + if (state.ready === "waiting") return undefined; + const turns = completionTurns(state); + if (turns.some(r => ["queued", "running"].includes(String(r.status)))) return undefined; + const correlated = turns.filter(r => carriesWorker(r, state.workerId)); + const reply = state.comments.find(c => c.authorAgentId && correlated.some(r => r.id === c.createdByRunId)); + if (reply) return `${COMPLETION_RESTART_GATE_SKIPPED}: agent reply ${reply.id} was published without the reference gate reporting waiting`; + const succeeded = correlated.find(r => r.status === "succeeded"); + if (succeeded) return `${COMPLETION_RESTART_GATE_SKIPPED}: completion turn ${succeeded.id} ended succeeded without the reference gate reporting waiting`; + return undefined; +} + +/** Names completion turns that ended unsuccessfully before the gate when the wait times out. */ +export function completionRestartGateTimeoutDetail(state: GateState | undefined): string | undefined { + if (!state || state.ready === "waiting") return undefined; + const ended = completionTurns(state).filter(r => !["queued", "running", "succeeded"].includes(String(r.status))); + if (!ended.length) return undefined; + return `restart boundary not reached: completion turn(s) ${ended.map(r => `${r.id} ended ${r.status}`).join(", ")} before the reference gate reported waiting`; +} + /** The start time exists only in the brief released after the source is idle. */ export function completionOutputUsesReleasedBrief(body: string): boolean { return /\b(?:10\s*[:.h]\s*30|ten[-\s]+thirty|half[\s-]+past[\s-]+(?:ten|10))\b/i.test(body); diff --git a/tests/runner-e2e/connection-reviews.test.ts b/tests/runner-e2e/connection-reviews.test.ts new file mode 100644 index 00000000000..81bb5917ac4 --- /dev/null +++ b/tests/runner-e2e/connection-reviews.test.ts @@ -0,0 +1,19 @@ +import { readFile } from "node:fs/promises"; +import { describe, expect, it } from "vitest"; +import { CONNECTOR_SETUP_NEXT_STEP } from "./connection-reviews.js"; + +describe("connection review setup", () => { + it("accepts the one-screen Check link step and the older Save and continue step", () => { + expect(CONNECTOR_SETUP_NEXT_STEP.test("Check link")).toBe(true); + expect(CONNECTOR_SETUP_NEXT_STEP.test("Save and continue")).toBe(true); + expect(CONNECTOR_SETUP_NEXT_STEP.test("Continue")).toBe(false); + expect(CONNECTOR_SETUP_NEXT_STEP.test("Checking…")).toBe(false); + }); + + it("matches a label the current connector setup actually renders", async () => { + const source = await readFile(new URL("../../ui/src/features/connections/ConnectionSetupFlow.tsx", import.meta.url), "utf8"); + const labels = [...source.matchAll(/"(Check link|Save and continue)"/g)].map(match => match[1]!); + expect(labels.length).toBeGreaterThan(0); + expect(labels.every(label => CONNECTOR_SETUP_NEXT_STEP.test(label))).toBe(true); + }); +}); diff --git a/tests/runner-e2e/connection-reviews.ts b/tests/runner-e2e/connection-reviews.ts index a92cfff2035..2962bdd0a8a 100644 --- a/tests/runner-e2e/connection-reviews.ts +++ b/tests/runner-e2e/connection-reviews.ts @@ -3,6 +3,10 @@ import { expect, type Page } from "@playwright/test"; import type { RunnerApi } from "./api.js"; import { startReviewProvider } from "../fixtures/connection-review-provider.js"; +/** The button after the URL step: Check link today, Save and continue before #14811. */ +export const CONNECTOR_SETUP_NEXT_STEP = /^(Save and continue|Check link)$/i; +const CHECK_LINK = /^Check link$/i; + /** Uses the same production connection and permission screens as the fixture browser suite. */ export async function setupConnectionReview(input: { page: Page; @@ -62,8 +66,15 @@ export async function setupConnectionReview(input: { .getByPlaceholder("https://example.com/actions") .fill(provider.url); await page.getByRole("button", { name: "Continue", exact: true }).click(); - await page.getByRole("button", { name: "Save and continue" }).click(); - await page.getByRole("button", { name: /Check link/i }).click(); + // #14811 made connector setup one screen: Continue now leads straight to + // Check link. Accept the older intermediate "Save and continue" step too, + // as connection-flow.ts does, so either layout reaches the link check. + const next = page + .getByRole("button", { name: CONNECTOR_SETUP_NEXT_STEP }) + .first(); + await expect(next).toBeVisible(); + if (!CHECK_LINK.test((await next.textContent()) ?? "")) await next.click(); + await page.getByRole("button", { name: CHECK_LINK }).click(); await expect( page.getByRole("heading", { name: /is ready/i }), ).toBeVisible(); diff --git a/tests/runner-e2e/runner.spec.ts b/tests/runner-e2e/runner.spec.ts index c0c4524b1a2..3113cf0ad9a 100644 --- a/tests/runner-e2e/runner.spec.ts +++ b/tests/runner-e2e/runner.spec.ts @@ -64,7 +64,7 @@ import { armResponseTiming } from "./response-timing.js"; import { projectFirstTaskResult, runFirstTaskFlow, setupFirstTaskFixtures } from "./first-task-flow.js"; import { runChatFlow } from "./chat-flow.js"; -import { restartChatServer } from "./chat-restart.js"; +import { gotoAfterServerRestart, restartChatServer } from "./chat-restart.js"; import { matchesRunCount, minimumRunCount } from "./run-count.js"; import { createHash, randomBytes } from "node:crypto"; import { mkdir, readFile, rename, writeFile } from "node:fs/promises"; @@ -1489,11 +1489,13 @@ for (const execution of executions) { await pollUntil({ label: "governed waiting turn", deadlineAt, load: loadTaskState, accept: state => state.taskRuns.length === 1 && state.taskRuns.every(run => TERMINAL_RUN_STATUSES.has(run.status)) }); await captureScreenshot("tool-review-pending", "Connection review awaiting a human", "tool-review-pending.png"); await page.getByRole("button", { name: "Dismiss Approve tool action" }).click(); - await page.getByRole("button", { name: "Review request", exact: true }).click(); if (execution.task.toolReviewDecision === "restart") { await restartIsolatedPaperclipServer({ api, requestId: `tool-review-${nonce}`, deadlineAt }); await page.reload(); } + // Open the review card on the document that will act on it: an + // expanded card does not survive the post-restart reload. + await page.getByRole("button", { name: "Review request", exact: true }).click(); if (execution.task.toolReviewDecision === "always") { await page.getByRole("button", { name: "Approval options", exact: true }).click(); await page.getByRole("menuitem", { name: "Always allow", exact: true }).click(); @@ -1602,20 +1604,16 @@ for (const execution of executions) { (key) => Reflect.set(window, key, true), documentSentinel, ); - try { - await page.goto( - `/${encodeURIComponent(issuePrefix)}/issues/${encodeURIComponent(issue.identifier ?? issue.id)}`, - // The replacement Vite server can commit and render a fresh - // document while its navigation lifecycle remains unsettled. - // The sentinel and explicit assertions below prove the new - // document and durable state even when Playwright times out. - { waitUntil: "commit" }, - ); - } catch (error) { - if (!(error instanceof Error) || error.name !== "TimeoutError") { - throw error; - } - } + // The replacement Vite server can commit and render a fresh document + // while its navigation lifecycle remains unsettled, and the old + // document's Vite client can reload and abort this navigation. The + // sentinel and explicit assertions below prove the new document and + // durable state either way. + await gotoAfterServerRestart( + page, + `/${encodeURIComponent(issuePrefix)}/issues/${encodeURIComponent(issue.identifier ?? issue.id)}`, + { deadlineAt }, + ); await expect( page .getByRole("radio", {