From fce6ccc5de122081b1effabba140c51a749fbd83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Val=20Alexander=20=F0=9F=91=91?= Date: Fri, 30 Oct 2026 22:23:25 -0500 Subject: [PATCH] test(code-review): populate the journey's R1 checks and R4 diff, record CR-3 acceptance re-run (#322) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The journey spec's synthetic world served no files for field-guide #91 and no check runs for lending #73's head, so its R1 capture showed "Checks · not found" and its R4 capture an empty Changes pane and file tree. The reference comparison (#322 Step 3) needs populated panes, so the world now carries four files for #91 (+11 −4) and four check runs plus one commit status for #73. Geometry assertions are unchanged. Records the 2026-10-30 re-run on main f51ef7e: focused Code Review specs 24/24, Step 1 unit files 75/75, full build/typecheck/unit/e2e green, and the read-only authenticated navigation probe 6/6. Points the VoiceOver checklist at the landed build. The R1–R4 comparison still waits for Val's references and the human keyboard/VoiceOver result. Co-Authored-By: Claude Fable 5.1 --- apps/desktop/e2e/code-review-journey.spec.ts | 17 +++ docs/design/code-review.md | 6 +- .../2026-10-22-cr8-voiceover/checklist.md | 7 +- .../2026-10-30-cr3-acceptance-resume.md | 117 ++++++++++++++++++ docs/status.md | 9 +- 5 files changed, 150 insertions(+), 6 deletions(-) create mode 100644 docs/reviews/2026-10-30-cr3-acceptance-resume.md diff --git a/apps/desktop/e2e/code-review-journey.spec.ts b/apps/desktop/e2e/code-review-journey.spec.ts index 40d02a31..32d38c1b 100644 --- a/apps/desktop/e2e/code-review-journey.spec.ts +++ b/apps/desktop/e2e/code-review-journey.spec.ts @@ -4,6 +4,7 @@ import fs from "node:fs"; import path from "node:path"; import { appDir, tid } from "./support.js"; import { baseWorld, fitWindow, launchWithGh, prRow, READ_ENDPOINT, type Harness } from "./code-review-support.js"; +import { ghFile } from "../test/fixtures/code-review-gh-world.js"; /** * CR-8 (#327): the integrated Code Review journey in real Electron, through main's production read service @@ -48,6 +49,22 @@ test("inbox → R1 → R2 → R3 → R4 through the production read service, wit body: `Journey activity ${i + 1}: keep the hold's place when moving between branches.`, })); world.routes["repos/example-org/lending/deployments"] = []; + // R1's sidebar shows checks for the head, and R4's Changes pane shows grouped diffs and a file tree: give the + // world lending #73's check runs and a commit status, and field-guide #91's four files (+11 −4, as its PR says), + // so the captures hold populated content for the reference comparison (#322 Step 3). All made up. + const head = `repos/example-org/lending/commits/${"c".repeat(40)}`; + const checkRun = (id: number, name: string, conclusion: string | null, minutes: number) => ({ + id, name, head_sha: "c".repeat(40), status: conclusion ? "completed" : "in_progress", conclusion, app: { slug: "github-actions" }, + started_at: "2026-10-18T20:31:00Z", completed_at: conclusion ? `2026-10-18T20:${31 + minutes}:00Z` : null, html_url: `https://github.com/example-org/lending/runs/${id}`, + }); + world.routes[`${head}/check-runs`] = { total_count: 4, check_runs: [checkRun(8101, "unit + e2e (macOS)", "success", 6), checkRun(8102, "typecheck", "success", 2), checkRun(8103, "lint", "failure", 1), checkRun(8104, "package", null, 0)] }; + world.routes[`${head}/status`] = { total_count: 1, statuses: [{ id: 8201, context: "preview/deploy", state: "pending", creator: { id: 1003, login: "deploy-bot", type: "User" }, created_at: "2026-10-18T20:32:00Z", updated_at: "2026-10-18T20:32:00Z", target_url: "https://example.test/preview/73" }] }; + world.routes["repos/example-org/field-guide/pulls/91/files"] = [ + ghFile("src/branches/regions.ts", "@@ -1,4 +1,7 @@\n import { Branch } from \"./branch\";\n-export const regions = [\"north\", \"south\"];\n-export function groupByRegion(branches: Branch[]) { return branches; }\n+export const regions = [\"north\", \"south\", \"east\", \"west\"] as const;\n+export type Region = (typeof regions)[number];\n+export function groupByRegion(branches: Branch[]): Map {\n+ return new Map(regions.map((region) => [region, branches.filter((b) => b.region === region)]));\n+}\n export const DEFAULT_REGION = \"north\";", 5, 2), + ghFile("src/branches/index.ts", "@@ -2,3 +2,5 @@\n export { Branch } from \"./branch\";\n-export { regions } from \"./regions\";\n+export { regions, groupByRegion } from \"./regions\";\n+export type { Region } from \"./regions\";\n+export { BranchList } from \"../pages/branch-list\";\n export { openingHours } from \"./hours\";", 3, 1), + ghFile("src/pages/branch-list.tsx", "@@ -8,4 +8,5 @@ export function BranchList({ branches }: Props) {\n return (\n-
    {branches.map((b) =>
  • {b.name}
    • )}
\n+ <>{[...groupByRegion(branches)].map(([region, list]) =>

{region}

\n+
    {list.map((b) =>
  • {b.name}
    • )}
)}\n );\n }", 2, 1), + ghFile("docs/branches.md", "@@ -1,2 +1,3 @@\n # Branches\n+Branch pages are grouped by region: north, south, east and west.\n Each branch page lists its opening hours.", 1, 0), + ]; const deploymentRead = `repos/example-org/lending/deployments?sha=${"c".repeat(40)}&per_page=20&page=1`; h = await launchWithGh(world); const { page, repo } = h; diff --git a/docs/design/code-review.md b/docs/design/code-review.md index fa998098..f596543d 100644 --- a/docs/design/code-review.md +++ b/docs/design/code-review.md @@ -596,7 +596,11 @@ With the overview (#323) and the remote diff (#324) in the panes, `e2e/code-revi proves AC3 and the scroll half of AC4 at 1440×900 and 900×600: the inbox list, the overview (its main column, or its body where the pane stacks), the diff and the file tree each scroll alone by wheel and by PageDown, nothing else moves, the focused control keeps focus, and each position is restored on -return ([acceptance record](../reviews/2026-10-28-cr3-acceptance.md)). Original R1–R4 reference +return ([acceptance record](../reviews/2026-10-28-cr3-acceptance.md)). On 2026-10-30 the automated half +and the authenticated probe were re-run on `main` at `f51ef7e`, and the journey spec's world was given +files for field-guide #91 and checks for lending #73's head, so its R1 and R4 captures show a populated +diff, file tree and checks sidebar rather than empty panes +([resume record](../reviews/2026-10-30-cr3-acceptance-resume.md)). Original R1–R4 reference comparison and human keyboard/VoiceOver acceptance remain open gates. diff --git a/docs/reviews/2026-10-22-cr8-voiceover/checklist.md b/docs/reviews/2026-10-22-cr8-voiceover/checklist.md index c6dc05e5..6e91b3cc 100644 --- a/docs/reviews/2026-10-22-cr8-voiceover/checklist.md +++ b/docs/reviews/2026-10-22-cr8-voiceover/checklist.md @@ -12,9 +12,10 @@ and, since the overview (#323) and the diff and file tree (#324) landed, parts C The review panel and instructions popover (#325) and the GitHub writes (#326) have landed too; their rows stay listed at the end for their own issues' acceptance and are not part of #322 AC4. -**Build:** `main` at `7a01df2` or later (the CR-3 acceptance PR's head once it merges), built with `pnpm build`. -Record the SHA you used. The automated half of this gate at that head is in -[the CR-3 acceptance record](../2026-10-28-cr3-acceptance.md). +**Build:** `main` at `afec85b` (#438, the CR-3 acceptance follow-up) or later, built with `pnpm build`. On +2026-10-30 `main` was `f51ef7e`; the automated half of this gate was re-run there +([resume record](../2026-10-30-cr3-acceptance-resume.md)). Record the SHA you used. The automated half's first run +is in [the CR-3 acceptance record](../2026-10-28-cr3-acceptance.md). ## Setup (about 3 minutes) diff --git a/docs/reviews/2026-10-30-cr3-acceptance-resume.md b/docs/reviews/2026-10-30-cr3-acceptance-resume.md new file mode 100644 index 00000000..4d225ea9 --- /dev/null +++ b/docs/reviews/2026-10-30-cr3-acceptance-resume.md @@ -0,0 +1,117 @@ +# CR-3 acceptance resumed on `main`, 2026-10-30 + +Evidence for [#322](https://github.com/TypeSafeAI/modex/issues/322) after the acceptance follow-up (#438, +`afec85b`) landed. It re-runs the automated half of the acceptance on current `main`, re-runs the read-only +authenticated navigation probe there, and hands Val the two gates that remain human: the R1–R4 reference +comparison and the keyboard/VoiceOver pass. It does not close the issue. + +## Candidate + +- `main` at `f51ef7e8caa17e79c9029be0d696f5bb40b3dcf2` (#453). The worktree is `cr3-resume`; it changes no + product code. Besides this record, the checklist and `docs/status.md`, it changes one spec, + `e2e/code-review-journey.spec.ts`, so that its R1 and R4 captures hold populated content (below). +- Evidence outside the repository: `~/Documents/Codex/artifacts/modex-cr3-acceptance/2026-10-30/` (logs, the + probe and its result, journey captures and the reference-less `compare.html`). `environment.txt` has the + versions: macOS 27.0, Node 24, pnpm as pinned, gh 2.102.0. + +## What is already on `main` + +| Step | Landed | Where | +| --- | --- | --- | +| 1. #349's re-review items: `mergePages` requires every page to report the same total; a `SIGN_IN_CHANGED` read is retried once by the renderer `Reader`, within the same continuation so a newer read or cancellation wins | #438 `afec85b` (2026-10-28) | `src/shared/code-review.ts`, `src/renderer/code-review/reader.ts`; tests in `code-review-contract.test.ts`, `code-review-workspace.test.ts`, `e2e/code-review-regressions.spec.ts` | +| 2. Independent scrolling and focus with the overview (#393) and the remote diff (#404) in the panes, at 1440×900 and 900×600 | #438 | `e2e/code-review-scroll.spec.ts` | +| 4. The human keyboard/VoiceOver checklist, parts A–D | #438 | [`2026-10-22-cr8-voiceover/checklist.md`](2026-10-22-cr8-voiceover/checklist.md) | + +CR-4 (#393, 2026-10-24) and CR-5 (#404, 2026-10-24) have both landed, so Step 2's precondition holds. + +## Re-run on `main` at `f51ef7e` + +Run from the worktree on this Mac, each exiting 0: + +| What | Command | Result | +| --- | --- | --- | +| Build | `pnpm install --frozen-lockfile && pnpm build` | passed | +| Step 1 unit files | `node --test --test-force-exit dist/test/code-review-{contract,workspace,read-service}.test.js` | 75 passed, 0 failed | +| Focused Code Review E2E (the resume prompt's two specs plus the scroll and regression specs) | `pnpm exec playwright test e2e/code-review-navigation.spec.ts e2e/code-review-ipc.spec.ts e2e/code-review-scroll.spec.ts e2e/code-review-regressions.spec.ts` | 24 passed, 0 failed, 1.9 min | +| Typecheck | `pnpm typecheck` | passed | +| Unit | `pnpm test` | worktree scripts 120/120; core 16/16; desktop 2,089 passed, 20 skipped, 0 failed; desktop native suites 21 passed, 6 skipped; site 10/10; store-desktop 16/16 | +| E2E | `pnpm test:e2e` | desktop 476 passed, 7 skipped (opt-in live cases), 0 failed, 40.3 min on a loaded machine; site 7/7 | + +The focused run includes the #349 regressions (the one-retry read through the production read service and a +rewritten `hosts.yml`; the `gh auth switch` refusal that is not retried) and both scroll cases. + +## Authenticated read-only navigation probe, re-run on `main` + +The same probe Val authorized for `main` on 2026-10-22 (`A-main` in the +[acceptance matrix](2026-10-22-cr8-acceptance-matrix.md)), `live-navigation.mjs`, was run again against this +build with the Mac's own `gh` login (account BunsDev, id 68980965, gh 2.102.0), a disposable Modex home and +no `MODEX_E2E` or demo flag. It exited 0 with all six checks passed: + +1. authenticated inbox and `repo:TypeSafeAI/modex` search, every row in that repository; +2. public PRs #349 and #348 resolved from their links with overview-derived totals and no overview error; +3. Back, Forward, Changes and the split through their real controls; +4. a pin retained and resolved after renderer reload; +5. an explicit resolve error for a nonexistent public PR; +6. a separate disposable home with an empty `GH_CONFIG_DIR` and no token variables shows signed-out and a + disabled search field. + +Only GET reads of public PRs were made. No GitHub write, coding turn, account switch or private content +export took place; `gh auth status` showed the same active account afterwards. The sign-in retry path was +not exercised live (it would require switching the Mac's gh account), so it stays synthetic-`gh` evidence. + +## Step 3: R1–R4 comparison + +Still not run. The originals were confirmed gone on 2026-10-22 and were not found on this Mac on 2026-10-28, +and Val decided that day to leave the comparison open rather than re-capture or substitute. Nothing has +changed since: no reference directory has been named. No synthetic capture was substituted for a reference. + +**The captures were not populated.** Reviewing the journey spec's R1 and R4 captures from the full E2E run on +this `main` (kept under `journey-captures/`), and the 2026-10-28 ones, showed the same two gaps: R4's Changes +pane read "0 of 4 files loaded" with an empty file tree, because the synthetic world serves no files for +field-guide #91, and R1's Checks section read "not found", because the world serves no check runs for +lending #73's head. The references show grouped diffs with a file tree (R4) and a checks sidebar (R1), so a +comparison from those captures would have measured empty panes. The journey spec's world now carries four +files for #91 (+11 −4, as its PR says) and four check runs and one commit status for #73's head; its +geometry assertions are unchanged and it passes. The populated R1–R4 captures at the references' CSS sizes, +plus 1440×900 and 900×600, are kept under `journey-captures-populated/` with their manifest and a +reference-less `compare.html`. The Stack section still reads "not found" (the world declares no branch +relationships; `stack` was unavailable as designed in `A-main` too). When Val names a directory holding +`R1.png`–`R4.png` (originals, or re-captures with their own calibration row), the comparison is: + +```sh +cd apps/desktop && pnpm exec playwright test e2e/code-review-journey.spec.ts +pnpm run ui:compare -- --captures test-results/ui-code-review +``` + +Each reviewed deviation then gets a D-row in the design page's Workspace section. + +## Step 4: the human keyboard/VoiceOver checklist + +[`2026-10-22-cr8-voiceover/checklist.md`](2026-10-22-cr8-voiceover/checklist.md) now names the landed build +(`afec85b` or later; this run used `f51ef7e`). To run it, from `apps/desktop` in a worktree at the SHA under +test after `pnpm build`: + +```sh +node ../../docs/reviews/2026-10-22-cr8-voiceover/seed.mjs /tmp/modex-cr8-voiceover +``` + +then the printed `env … npx electron .` line, `localStorage.setItem("modex.codeReview", "on")` in DevTools +and a reload. Parts A and B cover CR-3's inbox, tabs, header and dividers; C and D the populated overview, +diff and tree; A1–A6, C1–C3 and D1–D4 are repeated at about 900×600. The result row at the end waits for Val. + +## Acceptance criteria, by evidence + +| AC | Fixtures / synthetic `gh` (this run) | Authenticated (this run) | Human | +| --- | --- | --- | --- | +| AC1 R1/R4 arrangements through real controls | `code-review-navigation.spec.ts` (R1 test, R1/R4 captures) | probe check 3 (Changes and split) | geometry comparison with the originals: open, deferred by Val | +| AC2 stale replies, back/forward | navigation (identical numbers, slow search, account switch, back/forward) and regressions (identity ×3, revision) | probe check 3 (back/forward) | — | +| AC3 independent scrolling, restored geometry | `code-review-scroll.spec.ts` ×2; navigation keyboard test (persisted panes) | — | — | +| AC4 keyboard at 900×600 and 1440×900 | navigation keyboard test; scroll spec (focus kept) | — | keyboard/VoiceOver checklist: open | +| AC5 local Changes, no remote Discard | regressions "local Changes stay local"; navigation pin/copy test | — | — | +| AC6 fixtures and authenticated smoke reported separately | this table's first column | probe 6/6 on `f51ef7e` | — | + +## Still open on #322 + +- The R1–R4 comparison against originals or re-captures Val names (Step 3). +- The human keyboard/VoiceOver result (Step 4). +- A live run of the sign-in retry is not claimed. diff --git a/docs/status.md b/docs/status.md index 57be5545..3b82d95f 100644 --- a/docs/status.md +++ b/docs/status.md @@ -311,8 +311,13 @@ Desktop CI memory diagnostics landed through #458 after #456’s cold transcript cancellation remain authoritative across attempts) and proves, with the overview and remote diff now in the panes, that the list, overview, diff and file tree scroll alone and keep focus at 1440×900 and 900×600 ([acceptance record](reviews/2026-10-28-cr3-acceptance.md)). - Original reference comparison waits for Val's re-captured references; human keyboard/VoiceOver - acceptance waits for Val's pass with the [checklist](reviews/2026-10-22-cr8-voiceover/checklist.md). + On 2026-10-30 the automated half was re-run on `main` at `f51ef7e` and the read-only authenticated + navigation probe passed six of six there again ([resume record](reviews/2026-10-30-cr3-acceptance-resume.md)); + the journey spec's R1 and R4 captures, which had shown an empty Changes pane and no checks, now carry + populated content for the comparison. + Original reference comparison waits for Val to name the references (deferred on 2026-10-28); human + keyboard/VoiceOver acceptance waits for Val's pass with the + [checklist](reviews/2026-10-22-cr8-voiceover/checklist.md). - **CR-6 agent review and questions** (#325). Phase 1's native proof ([isolation evidence](reviews/2026-10-23-cr6-isolation.md)) showed Claude Code's restricted Read-only palette rejects injected shell, write, fetch, MCP and outside reads while Codex's read-only sandbox does not. Phase 2 builds on it: main's local `code-review:agent` channel starts the installed Claude Code CLI, under the person's own login, on a snapshot of the PR's diff that main reads itself through `gh`, with the shipped argv `--safe-mode --restricted --tools Read` and no permission tool, and an environment stripped of `gh`, GitHub and Modex variables. Each run is recorded before it starts, bound to the head it read, marked outdated when the head moves, stopped explicitly, and read as unknown after a crash; a finding reaches GitHub only through CR-7's review draft. Review instructions are one versioned text per Mac whose scope the popover states, and each run records the version it used. Unit, native-fixture (opt-in, production argv) and E2E evidence over a synthetic `claude` and `gh` are in [the review evidence](reviews/2026-10-28-cr6-native-review.md); an authenticated live run and C82 remain open gates.