diff --git a/apps/desktop/e2e/code-review-agent.spec.ts b/apps/desktop/e2e/code-review-agent.spec.ts index 0f67ae6f..936bfb77 100644 --- a/apps/desktop/e2e/code-review-agent.spec.ts +++ b/apps/desktop/e2e/code-review-agent.spec.ts @@ -253,3 +253,84 @@ test("Stop ends a running review, and a crash mid-run reads as unknown after a r await expect(tid(page, "cr-agent-run")).toHaveCount(2); expect(runs()).toHaveLength(2); }); + + +test("integrated review: populated diff → native fixture finding → confirmed review → crash recovery never replays the provider or write", async () => { + const world = baseWorld(); + Object.assign(world.routes["repos/example-org/lending/pulls/73"] as object, { changed_files: 2, additions: 3, deletions: 1 }); + const reviews = "repos/example-org/lending/pulls/73/reviews"; + world.routes["repos/example-org/lending/issues/73/timeline"] = [{ event: "commented", id: 900, + user: { id: 1002, login: "grace-fixture", type: "User" }, + body: "Keep the hold in its queue position.", created_at: "2026-10-18T21:00:00Z" }]; + world.routes["repos/example-org/lending/deployments"] = []; + world.postHolds = { [reviews]: "integrated-review-answer" }; + h = await launchWithGh(world); + installClaude("reply"); + let page = await openLending(); + const activity = tid(page, "cr-activity"); + await expect(activity).toContainText("All activity loaded"); + const comment = activity.getByText("Keep the hold in its queue position.", { exact: true }); + await comment.scrollIntoViewIfNeeded(); + await expect(comment).toBeVisible(); + await fitWindow(h, 1766, 1057); + await tid(page, "code-review-split-toggle").click(); + await expect(tid(page, "code-review-workspace")).toHaveAttribute("data-panes", "overview changes"); + await page.getByRole("button", { name: "View remote diff for src/holds/move.ts" }).click(); + await expect(page.getByRole("button", { name: "New line 2", exact: true })).toBeVisible(); + await page.screenshot({ path: test.info().outputPath("populated-split.png"), animations: "disabled" }); + expect(runs()).toEqual([]); + expect(h.posts()).toEqual([]); + + await tid(page, "code-review-review").first().click(); + await panel(page).getByRole("button", { name: "Start review" }).click(); + const run = tid(page, "cr-agent-run").first(); + await expect(run).toHaveAttribute("data-run-state", "completed", { timeout: 20_000 }); + const finding = tid(run, "cr-agent-finding").first(); + await expect(finding).toHaveAttribute("data-reference", "anchored"); + await finding.getByRole("button", { name: "Add to review draft" }).click(); + await expect(finding.getByRole("status")).toContainText("Added to the review draft"); + const composer = page.getByRole("region", { name: "Review this pull request" }); + if (await composer.getByRole("textbox", { name: "Review summary" }).count() === 0) await composer.getByRole("button", { name: "Review", exact: true }).click(); + await expect(composer.getByRole("list", { name: "Line comments" })).toContainText("src/holds/move.ts · head 2"); + await composer.getByRole("textbox", { name: "Review summary" }).fill("Review the untested branch before merging."); + expect(runs()).toHaveLength(1); + expect(h.posts()).toEqual([]); + + await composer.getByRole("button", { name: "Preview review" }).click(); + const preview = page.getByRole("region", { name: "Preview review" }); + await expect(preview.getByTestId("cr-preview-destination")).toHaveText("As ada-fixture on example-org/lending #73"); + await expect(preview.getByTestId("cr-preview-revision")).toContainText("Head cccccccc into main"); + await expect(preview.getByRole("list", { name: "Line comments to submit" })).toContainText("src/holds/move.ts · head 2"); + expect(h.posts()).toEqual([]); + await page.screenshot({ path: test.info().outputPath("review-preview.png"), animations: "disabled" }); + await preview.getByRole("button", { name: "Submit review" }).click(); + await expect.poll(() => h.posts().length).toBe(1); + expect(h.posts()[0]).toMatchObject({ endpoint: reviews, body: { commit_id: HEAD, event: "COMMENT", + comments: [{ path: "src/holds/move.ts", side: "RIGHT", line: 2, body: "Untested branch\n\nLine 2 adds a path with no test." }] } }); + + // The synthetic server accepted the review but withholds the response. Restart the whole app, + // then reconcile by read-back; neither the native review nor its external action may replay. + await h.relaunch(); + h.release("integrated-review-answer"); + page = await openLending(); + const doubt = tid(page, "cr-doubt"); + await expect(doubt).toBeVisible(); + await expect(doubt).toContainText("review"); + await doubt.scrollIntoViewIfNeeded(); + await page.screenshot({ path: test.info().outputPath("restart-unknown-outcome.png"), animations: "disabled" }); + expect(runs()).toHaveLength(1); + expect(h.posts()).toHaveLength(1); + await doubt.getByRole("button", { name: "Check GitHub" }).click(); + await expect(doubt).toHaveCount(0); + expect(h.calls().filter(c => c.startsWith("POST "))).toEqual([`POST ${reviews}`]); + expect(h.posts()).toHaveLength(1); + expect(runs()).toHaveLength(1); + + // A later revision makes the recovered finding stale without starting another review or write. + h.world((w) => { (w.routes["repos/example-org/lending/pulls/73"] as { head: { sha: string } }).head.sha = "d".repeat(40); }); + page = await reopen(); + await expect(tid(page, "cr-agent-outdated").first()).toHaveText(`Outdated · made for head ${HEAD.slice(0, 8)}`); + await expect(tid(page, "cr-agent-finding").first().getByRole("button", { name: "Add to review draft" })).toBeDisabled(); + expect(runs()).toHaveLength(1); + expect(h.posts()).toHaveLength(1); +}); diff --git a/docs/reviews/2026-10-28-cr8-review-flow.md b/docs/reviews/2026-10-28-cr8-review-flow.md new file mode 100644 index 00000000..7e679d4d --- /dev/null +++ b/docs/reviews/2026-10-28-cr8-review-flow.md @@ -0,0 +1,42 @@ +# CR-8 integrated native-review and write-recovery fixture + +The CR-8 journey had no single test connecting populated remote diff content, a native review finding, the review draft, explicit submission and restart reconciliation. This addition extends the existing Code Review agent spec with the existing synthetic Claude CLI and GitHub transport. No product code, default capability or external-service configuration changes. + +The fixture declares a complete two-file diff. Through real Electron controls, it opens that diff, starts one synthetic Claude review, adds an anchored finding to the review draft, fills a summary and previews the account, repository, PR, head and inline destination. The synthetic GitHub server records the confirmed review and withholds its response. Restarting the full app must show an unknown outcome. Explicit Check GitHub resolves it from a read-back while both provider-start and POST counts remain exactly one. The submitted review pins the expected head and inline path/side/line. No actual provider inference or real GitHub mutation occurs. + +## Validation + +The working test was extended after its earlier case completed in session `49837`: it now visits a complete activity fixture, scrolls the unknown-outcome control into the capture viewport, and checks that a later head change disables promotion of the recovered finding. The extended focused run passed: one test in 8.0 seconds, with a 7.7-second test body. Its artifacts are preserved in `cr8-review-flow-extended-focused-passed/`, with SHA-256 hashes for the captures. The earlier results and first capture inspection below apply to the preserved pre-extension source (`cr8-review-flow-pre-extension.spec.ts` and its SHA-256 receipt), not to the new assertions. The extended full validation also passed: build, 2,242 unit passes with 26 skips and zero failures or cancellations, 475 desktop passes with 7 skips, and 7 site passes (exit 0). The integrated case passed in 8.1 seconds; the transcript latency median was 131.3 ms. Results and artifacts are in `cr8-review-flow-extended-validation.log` and `cr8-review-flow-extended-full-passed/`. + +Baseline product source is main `33a782039c0bbb49c622915520dc7ecf9b86a7b7`. Build passed. The first focused run passed: one integrated Electron test in 9.0 seconds, with an 8.7-second test body. Captures were then added at the populated split, submission preview and restarted unknown-outcome states for the full validation run. The full validation chain passed build and unit tests (2,242 passed, 26 skipped, zero failed or cancelled). The integrated test passed within the ongoing full E2E suite in 6.4 seconds; that earlier full E2E run completed with 475 desktop passes, 7 skips and 7 site passes (exit 0). Its transcript latency median was 114.5 ms. Those results apply to the pre-extension source. + +Logs and artifacts are preserved outside the worktree under `~/Documents/Codex/artifacts/modex-land-and-prune/`, including `cr8-review-flow-build.log`, `cr8-review-flow-focused-first.log` and `cr8-review-flow-focused-passed/`. + +After validation, the branch fast-forwarded to main `d1cc80a15925cdcad529e199bc5b7fc4e955a7f3` (#440). The incoming diff contains only the R2/R3 journey spec and documentation; application source and dependencies are unchanged. The validated working changes reapplied without conflicts. Both the full agent spec and updated journey spec passed together on this integrated tree: seven tests in 40.1 seconds (integrated review 8.5 seconds; R1–R4 journey 6.2 seconds). The result is preserved in `cr8-review-flow-main-integration-focused.log` and `cr8-review-flow-main-integration-passed/`. The pre-integration stash `13084a91263dbc2c0cdb12993d6ab284a57e9eae` remains preserved. + +## Reproduction environment + +The current environment is Mac17,6, Apple M5 Max (18 cores), macOS 27.0.1, Node 24.22.0, pnpm 10.34.5, Electron 44.4.5 and Playwright 1.63.0. Installed versions and the extended spec SHA-256 are preserved in `cr8-review-flow-environment.json`. The synthetic Claude version response is 2.1.312; it does not identify an installed live provider. From the owned worktree, run `pnpm build && pnpm test` and `pnpm test:e2e`. For a focused rerun after building, use `pnpm --filter @modex/desktop exec playwright test e2e/code-review-agent.spec.ts -g "integrated review:"`. Run only one local desktop E2E process at a time. + +## Capture inspection + +The populated split capture visibly shows the remote text patch and binary-file entry. The submission preview visibly shows the fixture account, repository, pinned head, inline finding and explicit Submit review action. After restart, the captured viewport shows the persisted completed review and finding; the unknown-outcome control is outside that viewport, so this image alone does not establish its presentation. The test separately asserts that control is visible and resolves it through Check GitHub. Missing activity, stack, checks and deployment fixtures render unavailable states; these captures do not establish those surfaces. The three PNGs were copied and SHA-256 verified under `cr8-review-flow-captures/manifest.json` in the external artifact directory. + +The extended focused capture was also inspected: the unknown-outcome warning, pinned head, Check GitHub and Stop tracking controls are now visible. Activity and deployments are refreshing in that restart capture; their loading state is not treated as a completed activity snapshot. + +## Mapping to the CR-8 contract + +This is a scoped supplement to the historical `2026-10-22-cr8-acceptance-matrix.md`, checked against the live #327 requirements. Its evidence is from real Electron over production main/IPC services with synthetic GitHub and a synthetic native CLI. It supplies no new authenticated-read, live-provider, live-write or human acceptance evidence. + +| Requirement or control | Evidence supplied here | Remaining boundary | +| --- | --- | --- | +| C27 native review entry | Explicit Review with Claude and Start review dispatch one synthetic CLI run; an anchored finding reaches the local draft. | Other provider choices and live inference retain their separate evidence gates. | +| C64 file header; C68 diff lines | The remote file is selected and New line 2 is visible before review; the captured split shows the populated text patch. | This test does not exercise context expansion, every diff control or original-reference fidelity. | +| #327 complete fixture journey | Inbox row → overview/activity → populated split/file → native CLI fixture → current anchored finding → draft → preview → confirmed mocked review → full restart → read-back reconciliation → later head change and disabled stale finding. | The stale transition occurs after reconciliation; this test does not claim a stale finding was submitted or exercise every context-selection control. | +| #327 AC3 recovery | The server records acceptance while withholding its response; restart exposes an unknown outcome; explicit reconciliation finishes with one provider start and one POST. The POST includes the pinned head and exact inline destination/body. | This covers one lost-response path, not every identity, injection, pagination or transport adversary. | +| #327 AC4 existing regressions | Build, unit and the full E2E suite passed for the extended test. | All seven combined agent and journey tests passed after integrating #440; hosted validation remains pending. | +| #327 AC6 evidence separation | Product baseline, focused/full-run distinction, synthetic transports and external capture hashes are recorded above. | No current authenticated, live-provider, disposable-target-write or human result is claimed. | + +## Acceptance limits + +This supplies integrated fixture evidence across CR-5, CR-6 and CR-7 for #327. It does not replace the separate adversarial tests or prove authenticated/native/live-write acceptance. R1–R4 original-reference comparison, human VoiceOver and any unrun live gates remain open. The synthetic CLI is a native child-process fixture, not a real model or paid provider. The server accepts a review before withholding the reply; it does not simulate every transport or remote failure mode. diff --git a/docs/status.md b/docs/status.md index 99a59c43..d11921b9 100644 --- a/docs/status.md +++ b/docs/status.md @@ -302,7 +302,9 @@ PR #422's CR-6 review found two blockers: unconfirmed child exit was reported as CR-8 (#327) keeps the [acceptance matrix](reviews/2026-10-22-cr8-acceptance-matrix.md). It has one row per control C01–C82 and per AC of #321–#326, with fixture, authenticated, live-provider, live-write and human evidence kept apart. It adds Electron journeys and cross-slice regressions through the production read path with a synthetic `gh`: inbox → R1 → R4, identity, head moves, incomplete data, hostile links, no dispatch, and local Discard. It also adds keyboard/contrast checks and stress measurements, and a [VoiceOver checklist](reviews/2026-10-22-cr8-voiceover/checklist.md) for Val. The read-only authenticated service smoke and the navigation probe were re-run on `main` at `0e674e7`, and both passed. That historical matrix predates the implemented overview, remote diffs and review instructions. The original-reference comparison remains blocked because Val confirmed the original R1–R4 files are gone; the human VoiceOver pass and remaining integrated acceptance gates stay open. - The [CR-8 journey refresh](reviews/2026-10-28-cr8-journey-refresh.md) adds R2 activity scrolling and R3 instructions focus/Escape coverage to the existing production-read fixture journey, with captures and no provider starts or external writes. Focused validation and the full local suite passed (474 desktop, seven skipped, seven site); hosted validation remains pending. Original-reference, human and live-provider/write acceptance remain open. + The [CR-8 journey refresh](reviews/2026-10-28-cr8-journey-refresh.md) adds R2 activity scrolling and R3 instructions focus/Escape coverage to the existing production-read fixture journey, with captures and no provider starts or external writes. Focused validation and the full local suite passed (474 desktop, seven skipped, seven site); PR #440 and its merge queue passed hosted validation (477 desktop, four skipped, seven site), and the change is on main. Original-reference, human and live-provider/write acceptance remain open. + + The [integrated CR-8 review-flow fixture](reviews/2026-10-28-cr8-review-flow.md) connects a populated remote diff, synthetic native review, anchored draft, explicit submission and restart reconciliation, requiring exactly one provider run and one synthetic write. The extended fixture passed build/unit validation and full E2E (475 desktop passes, 7 skips and 7 site passes). All seven combined agent and journey tests passed after integrating #440; hosted validation remains pending. Human, original-reference and live-service acceptance remain open. - **Mac App Store edition.** A sandboxed Store front end with a separately installed signed host, as Val chose. [Investigation and parity matrix](mac-app-store.md).