diff --git a/docs/reviews/2026-10-22-cr8-acceptance-matrix.md b/docs/reviews/2026-10-22-cr8-acceptance-matrix.md index b760c20e..f07683d3 100644 --- a/docs/reviews/2026-10-22-cr8-acceptance-matrix.md +++ b/docs/reviews/2026-10-22-cr8-acceptance-matrix.md @@ -1,5 +1,7 @@ # Code Review acceptance matrix (CR-8), 2026-10-22 +**Superseded for current `main`:** the [2026-11-01 matrix refresh](2026-11-01-cr8-acceptance-matrix.md) maps every control and AC to evidence on `main` `0a8c513`. This page stays as the 2026-10-22 baseline; its "not started" and "Gate A is not complete" lines describe product `0e674e7` only. + **Later fixture refresh:** The [2026-10-28 journey receipt](2026-10-28-cr8-journey-refresh.md) adds real Electron R2 activity and R3 instructions interactions through the production read service with synthetic GitHub responses. It supersedes this historical snapshot's claim that those journey states are waiting for unbuilt producers. Original-reference comparison, human accessibility and the remaining integrated/live gates stay open. **Historical baseline:** This matrix records the CR-8 candidate based on product `0e674e7`. Its “not started” entries describe that snapshot. Later overview, remote-diff and unwired action-foundation implementations have separate receipts; the integration candidate must reconcile their automated evidence without claiming missing original-reference or human acceptance. diff --git a/docs/reviews/2026-10-22-cr8-voiceover/checklist.md b/docs/reviews/2026-10-22-cr8-voiceover/checklist.md index 6e91b3cc..b50f4874 100644 --- a/docs/reviews/2026-10-22-cr8-voiceover/checklist.md +++ b/docs/reviews/2026-10-22-cr8-voiceover/checklist.md @@ -14,7 +14,8 @@ listed at the end for their own issues' acceptance and are not part of #322 AC4. **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 +([resume record](../2026-10-30-cr3-acceptance-resume.md)). On 2026-11-01 it was re-run on `0a8c513`, the seed was +rebuilt there and its launch line opened the app ([matrix refresh](../2026-11-01-cr8-acceptance-matrix.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) @@ -28,7 +29,9 @@ node ../../docs/reviews/2026-10-22-cr8-voiceover/seed.mjs /tmp/modex-cr8-voiceov ``` The script writes the home, the synthetic `gh` and its world (the journey's three authored PRs and one to -review, all made up), and prints an `env … npx electron .` line. Run that line, then: +review, all made up), and prints an `env … npx electron .` line. Since 2026-11-01 it also puts recorder `claude` +and `codex` executables first on PATH: they log to `provider-calls.log` and exit 1, so nothing in this home reaches +your own Claude login or a provider. Run that line, then: 1. Open DevTools (View → Toggle Developer Tools) and run `localStorage.setItem("modex.codeReview", "on")`. Then reload (Command-R). This turns on the unfinished destination in a built app. 2. Make the window about 1440×900. @@ -44,8 +47,8 @@ review, all made up), and prints an `env … npx electron .` line. Run that line | A1 | Tab to the rail's **Code Review** button and press Return | The inbox opens: "Code Review", **Inbox actions**, the search field, then "Authored by me" and "Needs my review" | | | A2 | Tab into the list, then use Down, Up, Home and End | One Tab stop for the whole list; the arrow keys move between rows; each row shows a focus ring | | | A3 | Press Return on "Move holds between branches…" | The PR opens: the PR tab and the **Changes +8,912 −47** tab appear in the title bar; the header reads "Open", "lending #73" | | -| A4 | Keep pressing Tab | Pin, Copy link, Open in GitHub, Review with Codex, Review instructions and Merge, then the title bar's PR tab, its close, Changes and its close, New tab, and the split button. **Note:** the title-bar tabs come after the header, though they are drawn above it. Record whether that order is acceptable. | | -| A5 | On **Review with Codex**, **Review instructions**, **Merge** and **New tab**, press Return and Space | Nothing happens; each one says why it isn't available (tooltip). Focus stays put. | | +| A4 | Keep pressing Tab | Pin, Copy link, Open in GitHub, Review with Claude, Review instructions and Merge, then the title bar's PR tab, its close, Changes and its close, New tab, and the split button. **Note:** the title-bar tabs come after the header, though they are drawn above it. Record whether that order is acceptable. | | +| A5 | On **New tab**, press Return and Space. Then on **Review with Claude**, **Review instructions** and **Merge**, press Return | New tab does nothing and says why it isn't available (tooltip); focus stays put. The other three open their panel or popover (they have landed, #325 and #326), and Close or Escape returns focus to the button. Don't press Start or confirm anything here; those rows belong to #325 and #326 below | | | A6 | Tab to the split button and press Return; then Tab to the divider between the panes and press Left/Right (Shift for larger steps) | Changes opens beside the PR. The divider moves and its value changes. It stops at each pane's minimum. | | | A7 | Inbox actions → Return → Escape | The menu opens with **Refresh** focused; Escape closes it and returns focus to **Inbox actions** | | | A8 | Open the second PR, then use the title bar's Back and Forward | Each PR returns as you left it (split or not), without a loading spinner | | @@ -60,7 +63,7 @@ review, all made up), and prints an `env … npx electron .` line. Run that line | --- | --- | --- | --- | | B1 | VO-Right from the rail into the inbox | "Pull request inbox, complementary"; "Code Review, heading level 1"; "Inbox actions, menu pop-up button"; "Search or paste a PR link, search text field"; "Authored by me, group" | | | B2 | VO-Right over a row | The title, then "example-org/lending #73, ada-fixture ·" and its age. A selected row adds "current". A row that may be part of a stack adds "May be part of a stack (inferred from its base branch)". | | -| B3 | Open a PR and VO-Right through the header | "Open"; "lending #73"; while it loads, "Loading the pull request"; "Pin, toggle button"; "Copy link"; "Open in GitHub, link"; "Review with Codex, dimmed" with the reason as its description; the same for Review instructions and Merge | | +| B3 | Open a PR and VO-Right through the header | "Open"; "lending #73"; while it loads, "Loading the pull request"; "Pin, toggle button"; "Copy link"; "Open in GitHub, link"; "Review with Claude, button"; "Review instructions"; "Merge" (record what each one says while the PR loads and after) | | | B4 | Activate **Copy link** | "Link copied" is announced (a polite live region) | | | B5 | VO-Right through the title bar's tabs | "…, tab, selected, 1 of 2" for the PR tab; "Changes, plus 8,912 minus 47, tab" (note how the totals are read); "Close Changes, button" | | | B6 | Move to a divider | "Resize the pull request pane, splitter" or similar, with its value; VO-Shift-Arrow or Left/Right changes it | | diff --git a/docs/reviews/2026-10-22-cr8-voiceover/seed.mjs b/docs/reviews/2026-10-22-cr8-voiceover/seed.mjs index dbcae3e1..7c71fa4a 100644 --- a/docs/reviews/2026-10-22-cr8-voiceover/seed.mjs +++ b/docs/reviews/2026-10-22-cr8-voiceover/seed.mjs @@ -1,6 +1,8 @@ // Seeds a disposable Modex home for the Code Review keyboard and VoiceOver review: the CR-8 journeys' synthetic // GitHub world (apps/desktop/test/fixtures/code-review-gh-world.ts, built to dist/test), a synthetic `gh` first on // PATH, and a throwaway gh configuration. No token is set and the person's own gh login is never read. +// `claude` and `codex` on PATH are recorders that log their argv and exit 1, so pressing Start in Review with +// Claude (#325) shows a failure and never reaches the person's own login or a provider. // Prints the command that launches the app against it. // Usage, from apps/desktop after `pnpm build`: node ../../docs/reviews/2026-10-22-cr8-voiceover/seed.mjs import { execFileSync } from "node:child_process"; @@ -29,6 +31,9 @@ fs.writeFileSync(path.join(home, "app", "state.json"), JSON.stringify({ settings: { default_backend: "mock", default_mode: "chat", default_model: { codex: "", claude: "", mock: "mock" }, routing: { jev_transport: "http" } }, })); fs.writeFileSync(path.join(bin, "gh"), WORLD_GH, { mode: 0o755 }); +const recorder = `#!/bin/sh\nprintf '%s %s\\n' "$(basename "$0")" "$*" >> '${path.join(root, "provider-calls.log").replace(/'/g, "'\\''")}'\nexit 1\n`; +for (const cli of ["claude", "codex"]) fs.writeFileSync(path.join(bin, cli), recorder, { mode: 0o755 }); +fs.writeFileSync(path.join(root, "provider-calls.log"), ""); fs.writeFileSync(path.join(root, "gh-world.json"), JSON.stringify(baseWorld())); fs.writeFileSync(path.join(root, "gh-calls.log"), ""); fs.writeFileSync(path.join(config, "hosts.yml"), "github.com:\n user: ada-fixture\n git_protocol: https\n"); diff --git a/docs/reviews/2026-10-25-cr7-merge.md b/docs/reviews/2026-10-25-cr7-merge.md index 65ab5c63..3a802a30 100644 --- a/docs/reviews/2026-10-25-cr7-merge.md +++ b/docs/reviews/2026-10-25-cr7-merge.md @@ -63,7 +63,9 @@ Everything below is mock or synthetic `gh`. **No live merge was run**, and no re ## Open -- A live merge against a disposable repository Val names. +- A live merge against a disposable repository Val names (step 9 of the + [canary procedure](2026-11-01-cr7-live-canary.md); not run). - A merge queue set by classic branch protection is invisible to the rules read. GitHub refuses the direct merge, which shows as a refusal. -- Decisions 2–4 (increment 3). +- ~~Decisions 2–4 (increment 3).~~ Landed with this increment through #416 (`b3aad0d`, 2026-10-27); + see [2026-10-25-cr7-pr-actions.md](2026-10-25-cr7-pr-actions.md). (Corrected 2026-11-01.) diff --git a/docs/reviews/2026-10-25-cr7-pr-actions.md b/docs/reviews/2026-10-25-cr7-pr-actions.md index a9b7dc1c..1f152f22 100644 --- a/docs/reviews/2026-10-25-cr7-pr-actions.md +++ b/docs/reviews/2026-10-25-cr7-pr-actions.md @@ -72,7 +72,8 @@ Everything below is mock or synthetic `gh`. **No live action was run** on any re ## Open -- A live run against a disposable repository Val names. +- A live run against a disposable repository Val names (steps 3–8 of the + [canary procedure](2026-11-01-cr7-live-canary.md); not run). - GitHub has no precondition on a PATCH, so an edit made on GitHub in the moment between main's check and its PATCH would still be overwritten. - Reviewer requests can't be withdrawn, and team reviewers aren't offered. diff --git a/docs/reviews/2026-10-25-cr7-write-submit.md b/docs/reviews/2026-10-25-cr7-write-submit.md index 6e2fa494..723cc4c1 100644 --- a/docs/reviews/2026-10-25-cr7-write-submit.md +++ b/docs/reviews/2026-10-25-cr7-write-submit.md @@ -87,7 +87,10 @@ repository authorized by Val yet, and no real PR, comment or review was created. ## Open -- A live write against a disposable repository Val names. -- Merge (increment 2) and decisions 2–4 (increment 3). +- A live write against a disposable repository Val names. The procedure is + [2026-11-01-cr7-live-canary.md](2026-11-01-cr7-live-canary.md); it has not run. +- ~~Merge (increment 2) and decisions 2–4 (increment 3).~~ Both landed with this increment through #416 + (`b3aad0d`, 2026-10-27): see [2026-10-25-cr7-merge.md](2026-10-25-cr7-merge.md) and + [2026-10-25-cr7-pr-actions.md](2026-10-25-cr7-pr-actions.md). (Corrected 2026-11-01.) - The intent marker is visible in the Markdown source of the posted text. - A read-back matches a review's body, author and head, not each of its line comments. diff --git a/docs/reviews/2026-10-26-cr7-write-repair.md b/docs/reviews/2026-10-26-cr7-write-repair.md index 547aefd0..f7ae35ad 100644 --- a/docs/reviews/2026-10-26-cr7-write-repair.md +++ b/docs/reviews/2026-10-26-cr7-write-repair.md @@ -31,3 +31,15 @@ The follow-up build, renderer typecheck and full unit command passed (2,165 pass Complete first-failure results are preserved externally in `cr7-graphql-first-e2e-results`, with extracted `cr7-panel-pointer-events.json`. The passive trace records off-script zero-button pointer moves interleaved with the scripted held-button drags; the sidebar never establishes capture, and the workspace loses capture on a zero-button move before its drag completes. The resize component and test are unchanged from the earlier published candidate. This supports investigating input interference without identifying its source or proving absence of a source defect. A single unchanged focused diagnostic passed 1/1 in 8.2 seconds with retries disabled; its trace is preserved separately in `cr7-panel-diagnostic-results`. Independent review confirmed the conflicting delivered input without identifying its origin or establishing a source defect. No timeout, assertion or product behavior was altered. The full E2E command remains failed; the site stage and fresh hosted candidate checks are separate evidence. The separately executed site E2E stage passed 7/7 in 14.0 seconds. The follow-up is published for fresh hosted validation with the failed aggregate result, narrow diagnostic pass and independent review all retained. No green aggregate local E2E claim or protected-merge readiness is made from these results. + +## Delivery (recorded 2026-11-01) + +The signed candidate and protected delivery that this page said "remain to be recorded" have landed: PR #416 +merged through the merge queue on 2026-10-27 as `b3aad0d85b91f479d5415abdec15d9f74a5dd61c`, carrying #412's +preflight and the stacked #413, #414 and #415 (all four closed unmerged; #416 superseded them) together with +this repair and the GraphQL follow-up. The required `unit + e2e (macOS)` check passed on the PR head `fffd07e` +(run `40415729228`) and in the merge queue on top of `main` (`merge_group` run `40420085521`, job `121912060965`). +The local `panel-resize.spec.ts:65` failure above was not reproduced there and stays recorded as diagnosed input +interference, not a fix. The post-merge push run on `main` (`40423890925`) failed its unit step (job +`121924751474`) and the run was then cancelled; that failure is not diagnosed here and is not counted as a pass. It shipped in v0.4.0 and v0.4.1. No authenticated live write has been +performed; the [live canary procedure](2026-11-01-cr7-live-canary.md) waits for a disposable repository Val names. diff --git a/docs/reviews/2026-10-28-cr6-native-review.md b/docs/reviews/2026-10-28-cr6-native-review.md index 24f0854c..77c76551 100644 --- a/docs/reviews/2026-10-28-cr6-native-review.md +++ b/docs/reviews/2026-10-28-cr6-native-review.md @@ -122,3 +122,93 @@ The second full desktop E2E run finished with 468 passed, seven skipped and two On main-based repair `ba23f5c195e41ab91b345e43df775af9de82ae9c`, build, unit tests (2,196 passed, 23 skipped) and typecheck passed. Full desktop E2E finished with 469 passed, seven skipped and one failed (21.0 minutes); the site stage did not run. Both the repaired crash case and unchanged Canvas case passed. The IPC case lost its page during the inbox request after a successful connection; the trace proves cleanup began afterward, but contains no process-exit or renderer-crash evidence. Its trace is preserved in `cr6-main-e2e-failure/`. The fixture now attaches bounded lifecycle events and stderr before cleanup without changing assertions or timeouts. Independent review found no blocking issue in that diagnostic change; three isolated repetitions passed (14.5 seconds). These passes do not establish the cause of the original closure or clear the failed full-suite result. Raw diagnostic stderr must be inspected before any public sharing. On diagnostic source `4c5eaee2472e1192c8030ead32b920ab276ad12f`, the fresh root `pnpm test:e2e` run exited zero: 470 desktop tests passed, seven skipped (23.6 minutes), followed by all seven site tests passing (12.8 seconds). The crash, Canvas and IPC cases passed. The complete log is preserved as `cr6-diagnostics-full-e2e.log` outside the worktree. This is passing full-suite evidence for that source; it does not establish the causes of the earlier Canvas and IPC failures or replace their preserved evidence. Production source is unchanged from the build, unit and typecheck checkpoint above. Hosted checks, protected merge and live-provider/human acceptance remain separate gates. + +## Re-run on Claude Code 2.1.316 (2026-11-01) + +The evidence above was taken on Claude Code 2.1.312. On `main` at `0a8c513` (Mac with macOS 27.2, Darwin +27.2.0, Node v24.22.0, gh 2.102.0), with the installed **Claude Code 2.1.316** and **codex-cli 0.166.0**: + +- `pnpm -s run build:main`, then `node --test --test-force-exit dist/test/code-review-*.test.js + dist/test/claude-backend.test.js dist/test/desktop-service-drain.test.js`: 224 tests, 223 passed, 0 failed, + 1 skipped (a Windows-only admission case). +- `MODEX_CR6_NATIVE=1 node --test --test-force-exit test/code-review-isolation.native.test.mjs`: **6 of 6 + passed, 0 skipped**, every case on `2.1.316 (Claude Code)` and `codex-cli 0.166.0`. The production-argv case + (`REVIEW_CONFINEMENT_ARGS` from the build) again advertised exactly `["Read"]`; the injected `Bash`, `Write` + and `WebFetch` answered "No such tool available … disabled for this session" and `mcp__sentinel__write` "No + such tool available"; the outside file and the symlink were refused by `--restricted`; the selected file + read; no write, hook or MCP marker; two provider turns on the loopback fixture. The hook and MCP positive + controls still fire without the candidate flags. Codex's read-only profile still permits shell, outside reads + and the inherited synthetic token, so Codex stays out of reviews. +- `e2e/code-review-agent.spec.ts`: 6 of 6 passed, inside a focused 51 of 51 Code Review run; see the + [2026-11-01 matrix refresh](2026-11-01-cr8-acceptance-matrix.md#environments-added-since-the-baseline). + +This is still fixture evidence: a scripted local provider, a synthetic key, a disposable home. It says what the +2.1.316 CLI enforces for these argv; it is not a live run. Logs: `~/.claude/task-evidence/lane-c-2026-11-01/` +(`cr-unit.log`, `cr6-native-2.1.316.log`, `versions.txt`). + +## Live smoke procedure (#325 AC6; not run) + +Runs only after Val names a repository and a pull request on #325 and says go. It uses Val's own `claude` +login and Val's own `gh` login read-only; it makes no GitHub write and runs no Codex. Pick a small PR (a few +files) so the provider cost stays small. + +**Setup**, from `apps/desktop` in a worktree at the SHA under test, after `pnpm build`: + +```sh +SMOKE=~/.claude/task-evidence/cr6-live-smoke-$(date -u +%Y-%m-%d); mkdir -p "$SMOKE/bin" "$SMOKE/home" +REAL_GH=$(command -v gh); REAL_CLAUDE=$(command -v claude) +# Pass-through wrappers. They log argv only (never the environment) and the snapshot's file list, then run the +# real CLI. Paths are written in, because the confined environment strips MODEX_* variables. +cat > "$SMOKE/bin/gh" <> "$SMOKE/gh-argv.log" +exec "$REAL_GH" "\$@" +SH +cat > "$SMOKE/bin/claude" <> "$SMOKE/claude-argv.log" +case "\$1" in -p) find . -type f | sort > "$SMOKE/snapshot-files-\$\$.txt" ;; esac +exec "$REAL_CLAUDE" "\$@" +SH +chmod 755 "$SMOKE/bin/gh" "$SMOKE/bin/claude" +gh api user --jq .login > "$SMOKE/login-before.txt"; claude --version > "$SMOKE/claude-version.txt" +env -u MODEX_E2E -u MODEX_DEMO MODEX_HOME="$SMOKE/home" MODEX_NO_LOGIN_PATH=1 \ + PATH="$SMOKE/bin:$PATH" npx electron . 2> "$SMOKE/electron-stderr.log" +``` + +In the app: DevTools → `localStorage.setItem("modex.codeReview", "on")` → reload (this throwaway home only); +Code Review → paste the PR link Val named → **Review with Claude** → read the panel's provider disclosure → +**Start** once. Wait for the reply. Then open the instructions gear and note the version shown. + +**Capture** into `$SMOKE/receipt.md`: + +1. The argv the app used: the `-p` line in `claude-argv.log`. It must contain `--safe-mode --restricted --tools + Read --strict-mcp-config --mcp-config {"mcpServers":{}} --disable-slash-commands --setting-sources + --permission-mode dontAsk --permission-prompts none --no-session-persistence --max-turns 60` and + `--append-system-prompt`, its `cwd` must be under `$SMOKE/home/review-agent/runs//snapshot`, and there + must be no `--permission-prompt-tool`, `--resume`, `--add-dir` or MCP server. +2. The snapshot's file list (`snapshot-files-*.txt`), compared with the PR's changed files + (`gh api repos///pulls//files --paginate --jq '.[].filename'`), and the omissions the panel lists. +3. The reply as shown, the findings with their anchors ("In the diff" or unverified), and the run record from + `$SMOKE/home/review-agent/state.json`: head SHA, instruction version, outcome. +4. That the snapshot directory is gone after the run (`ls "$SMOKE/home/review-agent/runs"`). + +**Closing checks:** + +Run from the same `apps/desktop` directory. The token check prints only a status: exit 0 means the token was absent from all required evidence, exit 1 means it was found, and exit 2 means the check was incomplete. Failed authentication, missing or unreadable evidence, and symlinks never count as a clean scan. Resolve an incomplete check before recording acceptance. + +```sh +if grep -qE -- '--method (POST|PUT|PATCH|DELETE)' "$SMOKE/gh-argv.log"; then + echo WRITE-FOUND +else + scan_status=$? + if [ "$scan_status" -eq 1 ]; then echo no-github-write; else echo write-check-incomplete; fi +fi +gh api user --jq .login | diff - "$SMOKE/login-before.txt" && echo login-unchanged +python3 ../../docs/reviews/code-review-token-check.py "$SMOKE/home" "$SMOKE/claude-argv.log" "$SMOKE/electron-stderr.log" +``` + +Do not press **Add to review draft** → submit: findings reach GitHub only through CR-7's previewed draft, and +that belongs to the [CR-7 canary](2026-11-01-cr7-live-canary.md) on a disposable repository, never here. The +receipt (argv, file list, reply, findings, record, checks) goes into a dated `docs/reviews/` page; strip the +PR's private content first if the repository is private. diff --git a/docs/reviews/2026-11-01-cr-gate-corrections.md b/docs/reviews/2026-11-01-cr-gate-corrections.md new file mode 100644 index 00000000..92c92183 --- /dev/null +++ b/docs/reviews/2026-11-01-cr-gate-corrections.md @@ -0,0 +1,9 @@ +# Code Review acceptance and canary corrections + +This candidate follows PR #472 at `245be00b2976e0c58b7fe036515feb2206a2160b` and preserves its full-suite failure, isolated retry, hosted pass and seed-launch evidence. It marks AC2 partial because attachment C58 and reasoning effort C59 lack an implementation or approved deviation, and includes those omissions in the enablement and decision gates. The existing current-run acceptance row now distinguishes the failed local E2E run from separate evidence. + +The CR-6 and CR-7 procedures previously piped `gh auth token` into `grep` and treated any nonzero result as absence. Synthetic probes reproduced a false clean result for failed authentication and missing evidence. Both procedures now invoke a shared status-only scanner: exit 0 means absent, 1 means found, and 2 means incomplete. It refuses failed authentication, empty or malformed token output, missing or unreadable paths, and symlinks. The CR-6 write-trace check also distinguishes no matching write from a failed scan. + +Run `python3 docs/reviews/code-review-token-check.test.py` from the repository root. All nine synthetic tests passed: clean evidence, a nested token match, failed authentication, empty token, missing evidence, symlink, unreadable file, missing gh and no paths. The fixture replaces PATH with a synthetic gh executable and asserts that no token text is printed. No real credential, provider call, live canary or GitHub write from the app was used. + +Build passed. Independent review found no blockers and confirmed status-only scanner output, failure handling, documented invocation paths, and the unresolved C58/C59 acceptance gates. Full unit validation passed: 2,277 passes, 26 skips, zero failures. The human and named-repository gates remain open. diff --git a/docs/reviews/2026-11-01-cr7-live-canary.md b/docs/reviews/2026-11-01-cr7-live-canary.md new file mode 100644 index 00000000..d59c4128 --- /dev/null +++ b/docs/reviews/2026-11-01-cr7-live-canary.md @@ -0,0 +1,113 @@ +# CR-7 live-write canary: procedure (not run), 2026-11-01 + +Issue: [#326](https://github.com/TypeSafeAI/modex/issues/326) AC6, under [#319](https://github.com/TypeSafeAI/modex/issues/319). +Product: `main` at `0a8c513` or later. Writes, Merge and decisions 2–4 landed through #410 (`deb373a`) and #416 +(`b3aad0d`, 2026-10-27). + +**Status: not run.** Every write so far went to a synthetic `gh`. This page is the procedure for the one live +canary that the [2026-10-25 approval in AGENTS.md](../../AGENTS.md) allows: through the person's own `gh` login, +against a **disposable repository Val names**, and only after Val says go on #326. No repository has been named. +Never run it against `TypeSafeAI/modex` or any real pull request. + +## What the canary may touch + +Only the fixed endpoints main builds (AGENTS.md, 2026-10-25): + +| Step | Endpoint main sends | +| --- | --- | +| Top-level comment | `POST repos/{o}/{r}/issues/{n}/comments` | +| Submitted review with one line comment | `POST repos/{o}/{r}/pulls/{n}/reviews` (`commit_id` = reviewed head) | +| Close / reopen | `PATCH repos/{o}/{r}/pulls/{n}` with only `state` | +| Convert to draft / ready for review | `POST graphql`, GitHub's `convertPullRequestToDraft` / `markPullRequestReadyForReview`, node id only | +| Edit title | `PATCH repos/{o}/{r}/pulls/{n}` with only `title` (refused if GitHub's title changed) | +| Request a reviewer | `POST repos/{o}/{r}/pulls/{n}/requested_reviewers` with only `reviewers` | +| Squash-merge | `PUT repos/{o}/{r}/pulls/{n}/merge` with `merge_method: squash` and `sha` = reviewed head | + +No DELETE, no other GraphQL, no automatic resend. Anything else in the argv log below fails the canary. + +## Preconditions (Val) + +1. Name the disposable repository on #326, `/`, and say go. It should be private, hold + nothing of value, have no required checks or merge queue, and allow squash merges. +2. Create a throwaway PR there yourself (this is your own `gh`, outside the app): + ```sh + gh repo clone / /tmp/cr7-canary-repo && cd /tmp/cr7-canary-repo + git switch -c canary && printf 'one\ntwo\nthree\n' > canary.txt && git add canary.txt + git commit -m "canary" && git push -u origin canary + gh pr create --title "CR-7 canary" --body "Disposable PR for the Modex CR-7 canary." + ``` +3. For the reviewer step, a second account with read access to the repository. Without one, skip that step + and record it as not run. + +## Setup (agent or Val, about 5 minutes) + +From `apps/desktop` in a worktree at the SHA under test, after `pnpm build`: + +```sh +SHA=$(git rev-parse HEAD) +CANARY=~/.claude/task-evidence/cr7-canary-$(date -u +%Y-%m-%d); mkdir -p "$CANARY/bin" "$CANARY/home" +REAL_GH=$(command -v gh) +# A pass-through gh that logs each argv (no environment, no token) and then runs the real gh. +cat > "$CANARY/bin/gh" <> "$CANARY/gh-argv.log" +exec "$REAL_GH" "\$@" +SH +chmod 755 "$CANARY/bin/gh" +gh api user --jq .login > "$CANARY/login-before.txt"; gh auth status > "$CANARY/auth-before.txt" 2>&1 +env -u MODEX_E2E -u MODEX_DEMO MODEX_HOME="$CANARY/home" MODEX_NO_LOGIN_PATH=1 \ + PATH="$CANARY/bin:$PATH" npx electron . 2> "$CANARY/electron-stderr.log" +``` + +In the app: `localStorage["modex.codeReview"] = "on"` from the developer tools of this throwaway home only +(it is a per-home setting, so the person's real home is unchanged), then Code Review → paste the throwaway +PR's link. + +## Steps + +For **each** step, record four things in `$CANARY/steps.md`: the preview text (screenshot of the confirmation: +account, destination, head and frozen body), main's re-check (the step's intent in +`$CANARY/home/review-actions/state.json` moving to sent or confirmed), exactly one new mutating line in +`gh-argv.log`, and the read-back (the app's state after it reads the PR again, plus one independent +`gh api` GET from a terminal). + +| # | Action in the app | Independent read-back | +| --- | --- | --- | +| 1 | Comment composer → "Canary comment 1" → preview → send | `gh api repos///issues//comments --jq '.[].body'` | +| 2 | Changes → line 2 of `canary.txt` → line comment → submit review as Comment | `gh api repos///pulls//reviews --jq '.[] \| [.state,.commit_id]'` and `…/pulls//comments --jq '.[] \| [.path,.line]'` | +| 3 | State menu → Close | `gh api repos///pulls/ --jq .state` → `closed` | +| 4 | State menu → Reopen | → `open` | +| 5 | State menu → Convert to draft | `--jq .draft` → `true` | +| 6 | State menu → Ready for review | `--jq .draft` → `false` | +| 7 | Edit pull request → title "CR-7 canary (edited)" | `--jq .title` | +| 8 | Reviewers + → the second account (or record "not run") | `gh api repos///pulls//requested_reviewers --jq '.users[].login'` | +| 9 | Merge → Squash → preview names the head → confirm | `--jq '[.merged, .merge_commit_sha, .head.sha]'`; `.head.sha` equals the previewed head | + +Double-click each confirm button once on purpose (step 1 and step 9 at least): `gh-argv.log` must still show +one send. If any outcome reads unknown, do not send again: use *Check GitHub*, and record what the read-back +proved. A step that fails is recorded as failed with its screenshot; the canary stops there. + +## Closing checks + +Run from the same `apps/desktop` directory. The token check prints only a status: exit 0 means the token was absent from all required evidence, exit 1 means it was found, and exit 2 means the check was incomplete. Failed authentication, missing or unreadable evidence, and symlinks never count as a clean scan. Resolve an incomplete check before recording acceptance. + +```sh +gh api user --jq .login | diff - "$CANARY/login-before.txt" && echo login-unchanged +grep -E -- '--method (POST|PUT|PATCH|DELETE)' "$CANARY/gh-argv.log" # main's argv form; expect 9 lines (8 if step 8 was skipped), each matching the table above +grep -cE -- '--method DELETE' "$CANARY/gh-argv.log" # must be 0 +# The token must appear nowhere Modex wrote (prints status only, never the token): +python3 ../../docs/reviews/code-review-token-check.py "$CANARY/home" "$CANARY/electron-stderr.log" "$CANARY/steps.md" +``` + +Then copy `$CANARY/steps.md`, the screenshots and the argv log (they contain only the disposable repository's +text) into a dated receipt under `docs/reviews/`, record the SHA, `gh --version`, macOS version, and mark #326 +AC6 and the matrix's live-write cells with what actually ran. Delete the disposable repository afterwards if +Val wants (that is Val's own action; Modex has no DELETE). + +## Limits this canary does not remove + +- The intent marker stays visible in the posted Markdown. +- A review's read-back matches its body, author and head, not each line comment. +- GitHub has no precondition on PATCH: an edit made on GitHub between main's check and its PATCH is overwritten. +- A merge queue set by classic branch protection is invisible to the rules read; the disposable repository + should have none. diff --git a/docs/reviews/2026-11-01-cr8-acceptance-matrix.md b/docs/reviews/2026-11-01-cr8-acceptance-matrix.md new file mode 100644 index 00000000..9467fd7c --- /dev/null +++ b/docs/reviews/2026-11-01-cr8-acceptance-matrix.md @@ -0,0 +1,260 @@ +# Code Review acceptance matrix (CR-8), refreshed 2026-11-01 + +CR-8 ([#327](https://github.com/TypeSafeAI/modex/issues/327)) for the Code Review workspace +([#319](https://github.com/TypeSafeAI/modex/issues/319)). This page refreshes the +[2026-10-22 baseline](2026-10-22-cr8-acceptance-matrix.md) to `main` at `0a8c513`. The baseline stays as written +for product `0e674e7`; its environments (`U`, `F-nav`, `F-ipc`, `F-prod`, `CI`, `A-main`, `A-321`, `A-349`, +`A-322`), measurements, contrast table and E1 description still apply to that SHA and are not repeated here. + +**Code is on `main` for every slice; acceptance is not complete.** CR-2 #366; CR-3 #367 `af49881`, #438 +`afec85b`; CR-4 #393 `6c40547`; CR-5 #404 `e3527f4`; CR-6 #396 `03ce71a`, #422 `b643617`, #426 `d0482a0`; CR-7 +#410 `deb373a`, #416 `b3aad0d`; CR-8 #440 `d1cc80a`, #441 `eca2bd9`; #464 `3d72c6e`. All of it shipped in v0.4.0 +and v0.4.1 except #464, which is evidence only. Nothing has been run against a live provider, written to GitHub +live, compared with the original references, or checked by a person with VoiceOver. No row below is "not +started". A cell that names no run says **open** and who owns it. No cell says "pass" for a check that didn't run. + +Columns as in the baseline: **Fixture** (Node tests, or real Electron with synthetic `gh`/`claude`), +**Authenticated** (read-only through a real `gh` login), **Live provider / write** (an authorized native-CLI turn, +#325; an authorized write on a disposable target, #326), **Human** (a person, keyboard/VoiceOver or references). + +## Environments added since the baseline + +| ID | What ran | SHA | Result | +| --- | --- | --- | --- | +| `A-323` | CR-4 authenticated **service** smoke (`CodeReviewService` + `ghTransport`), BunsDev, TypeSafeAI/modex PR #334 head `5106c0d` ([cr4-overview](2026-10-23-cr4-overview.md)) | candidate `485391a` (branch `cr4-overview`, before #393 merged) | overview, activity (35 objects, complete) and 13 checks ready. One current-head deployment read through the real deployment/status endpoints. `stack` unavailable (the PR is closed). Not rendered in Electron | +| `A-324` | CR-5 authenticated **service** smokes ([cr5-remote-diffs](2026-10-23-cr5-remote-diffs.md), [cr5-merge-base](2026-10-24-cr5-merge-base.md)) on PR #393 head `52375d4` | CR-5 integration candidate (before #404 merged) | (1) 15/15 files, 15/15 patches parsed complete, an exact-head anchor, first context line matched. (2) 2026-10-24 merge-base probe: target `2824ed7`, base `68860a0`. Deleted line, base context and anchor matched the merge base. Not rendered in Electron | +| `A-main-2` | #464's re-run of the read-only navigation probe (`live-navigation.mjs`), BunsDev, gh 2.102.0 ([cr3-acceptance-resume](2026-10-30-cr3-acceptance-resume.md)) | main `f51ef7e` | 6/6: inbox and repo search; #349 and #348 resolved with overview totals; back/forward, Changes and split; pin after reload; missing-PR error; signed-out | +| `U-1101` | `node --test --test-force-exit dist/test/code-review-*.test.js dist/test/claude-backend.test.js dist/test/desktop-service-drain.test.js` after `pnpm -s run build:main` | `main` `0a8c513` | 224 tests: 223 passed, 0 failed, 1 skipped (Windows-only) | +| `N` | `MODEX_CR6_NATIVE=1 node --test --test-force-exit test/code-review-isolation.native.test.mjs`: installed CLIs in a disposable home under `sandbox-exec`, scripted loopback provider, synthetic key; no account, no real inference | `main` `0a8c513`; **Claude Code 2.1.316**, **codex-cli 0.166.0** | 6 of 6 passed, 0 skipped. Production argv advertises only `Read`; Bash, Write, WebFetch and MCP calls refused; outside and symlink reads refused by `--restricted`. Codex read-only still allows shell and outside reads (why it is not offered). [Record](2026-10-28-cr6-native-review.md#re-run-on-claude-code-21316-2026-11-01) | +| `F-1101` | `pnpm exec playwright test` on the journey, regressions, a11y, measure, agent, actions, navigation and scroll specs: real Electron, production preload/IPC/services, synthetic `gh` and `claude` | `main` `0a8c513` | 51 of 51 passed in 6.2 min (journey 1, regressions 9, a11y 5, measure 2, agent 6, actions 14, navigation 12, scroll 2). Journey captures R1–R4, split-1440 and narrow-900 with populated R1 checks and R4 diff | +| `S-1101` | `pnpm build && pnpm test && pnpm test:e2e` | `main` `0a8c513` + this page's docs | `pnpm build` passed. `pnpm test` exit 0: 2,277 passed, 26 skipped, 0 failed (worktree 120, core 16, desktop 2,094 + 20 skipped, desktop scripts 21 + 6 skipped, site 10, Store 16). `pnpm test:e2e` **exit 1**: desktop 477 passed, 7 skipped, **1 failed**: `app.spec.ts:729` accessibility hit a strict-mode violation (two "Chat details" dialogs) while the load average was near 100. It passed 1 of 1 alone. The chain stopped before the site stage, which then passed 7 of 7 on its own. This is **not** a green full run. The hosted required `unit + e2e (macOS)` check passed on this page's commit `c1f74c7` (run `41308827432`) | + +**E2** (2026-11-01): Mac17,6 (Apple M5 Max), macOS 27.2 (Darwin 27.2.0), Node v24.22.0, Electron 44.4.5, +Playwright 1.63.0, gh 2.102.0. Other sessions were running: load average about 37–43, so no timing here is a +budget. Logs and captures are outside the repository at `~/.claude/task-evidence/lane-c-2026-11-01/`. + +"journey" is `code-review-journey.spec.ts` (R1 → R2 → R3 → R4; R2/R3 from #440, R1 checks and R4 diff from +#464). "review-flow" is the `code-review-agent.spec.ts` case from #441 (populated diff → synthetic native +finding → anchored draft → explicit submit → crash recovery, exactly one provider run and one synthetic write). +The baseline's `F-prod` regressions test named "dispatch" no longer exists; its no-dispatch loop now sits in +"hostile content" and covers the New tab button only, because Review, instructions and Merge are built. + +## Controls C01–C82 + +Status values: *built*, *unavailable*, *deviation Dn*, *needs decision N*, *unchanged*, or *not built* where the +inventory left a control out. "unverified" in a Fixture cell means the control is in the code but no spec was +found that asserts it; it is not a pass. + +| ID | Control | Owner | Status | Fixture | Authenticated | Live provider / write | Human | +| --- | --- | --- | --- | --- | --- | --- | --- | +| C01 | Traffic lights, window drag | native chrome | deviation D5 | — (macOS) | — | — | — | +| C02 | Back / forward | #322 | built | `code-review-navigation.spec.ts` "back and forward…"; journey (no re-read) | `A-322`, `A-main`, `A-main-2` | — | open (VoiceOver) | +| C03 | Inbox pane toggle | #322 | built | `code-review-navigation.spec.ts` back/forward test | — | — | open (VoiceOver) | +| C04 | Top-right buttons | shell | deviation D9 (only the split toggle applies) | `code-review-navigation.spec.ts`; journey (split toggle) | `A-322`, `A-main`, `A-main-2` (split) | — | open (VoiceOver) | +| C05 | PR tab: icon, title, close | #322 | built | `code-review-navigation.spec.ts`; journey; `code-review-a11y.spec.ts` | — | — | open (VoiceOver) | +| C06 | Changes tab with +/− | #322 | built | `code-review-navigation.spec.ts`; journey (totals from the production overview) | `A-322`, `A-main`, `A-main-2` (overview totals) | — | open (VoiceOver) | +| C07 | New tab (+) | needs decision 6 | unavailable | `code-review-navigation.spec.ts` (aria-disabled); `code-review-regressions.spec.ts` "hostile content" (double-click, Enter and Space dispatch nothing) | — | — | open (VoiceOver) | +| C08 | Code Review rail button | #322 | built | `code-review-navigation.spec.ts`; journey (`aria-current`) | `A-322` | — | open (VoiceOver) | +| C09 | Other rail icons | shell | unchanged | full `pnpm test:e2e` | — | — | — | +| C10 | Account monogram | shell | unchanged | full `pnpm test:e2e` | — | — | — | +| C11 | Inbox title and ⋯ menu (Refresh) | #322 | built | `code-review-navigation.spec.ts` (Escape returns focus); `code-review-a11y.spec.ts` | — | — | open (VoiceOver) | +| C12 | Search or paste a PR link | #322 | built | `code-review-contract.test.ts` (`parsePrLocator`); `code-review-navigation.spec.ts`; `code-review-regressions.spec.ts` "hostile content" | `A-322`, `A-main`, `A-main-2` (search, links, missing PR) | — | open (VoiceOver) | +| C13 | "Authored by me" group | #322 | built | contract test; `code-review-navigation.spec.ts`; `code-review-regressions.spec.ts` "incomplete data" | `A-322`, `A-main`, `A-main-2` (inbox) | — | open (VoiceOver) | +| C14 | "Needs my review" group | #322 | built | as C13 | `A-322` | — | open (VoiceOver) | +| C15 | PR row: title, author, age | #322 | built | `code-review-navigation.spec.ts`; journey | `A-322` | — | open: reference comparison | +| C16 | Row avatar and badge | needs decision 1 | needs decision 1 (not built; D1 login only) | — | — | — | — | +| C17 | Row stack icon | #322 | built | `code-review-navigation.spec.ts` (only for `stackHint: true`, named "inferred") | — | — | open (VoiceOver) | +| C18 | Selected and hover rows | #322 | built | `code-review-navigation.spec.ts`, journey (`aria-current`, R1/R4 captures) | — | — | open: reference comparison | +| C19 | Show more | #322 | built | contract test (`knownCount`, `mergePages`, #438 same-total rule); `code-review-navigation.spec.ts` pagination ×3; `code-review-measure.spec.ts` | — | — | open (VoiceOver) | +| C20 | Independent inbox scroll | #322 | built | `code-review-navigation.spec.ts` keyboard test; `code-review-scroll.spec.ts` ×2 (#438 `afec85b`) | — | — | — | +| C21 | State pill "Open ▾" | #326 (decision 2 settled 2026-10-25) | built (#416 `b3aad0d`); D7: no menu until main answers, none for merged | `code-review-actions.spec.ts` "the state menu closes and reopens a PR, and converts it to draft…", "a state change GitHub may have made stays in doubt…"; `code-review-writes.test.ts`, `code-review-write-ipc.test.ts`; `code-review-a11y.spec.ts` (pill contrast) | — | open (needs disposable repo Val names, #326) | open (VoiceOver) | +| C22 | Repository and number | #323 | deviation D6 (interim): the header still reads `pr.locator` from the row or resolved link (`PrPane.tsx` `PrHeader`), not `overview.locator` | `code-review-navigation.spec.ts` (`title` gives `owner/repo`) | — | — | open (VoiceOver) | +| C23 | Loading spinner | #322 | built | `code-review-navigation.spec.ts` (reduced motion); `code-review-regressions.spec.ts` "identity" | — | — | — | +| C24 | Pin | #322 | built | `code-review-navigation.spec.ts` (local, per account, survives reload) | `A-322`, `A-main`, `A-main-2` (pin after reload) | — | open (VoiceOver) | +| C25 | Copy link | #322 | built | contract test (`canonicalPrUrl`); `code-review-navigation.spec.ts` (clipboard) | — | — | open (VoiceOver) | +| C26 | Open in GitHub | #322 | built | `code-review-navigation.spec.ts` (canonical link only) | — | — | open (VoiceOver) | +| C27 | Review with **Claude** (no longer "with Codex"; Codex is not offered) | #325 | built (#422 `b643617`, #426 `d0482a0`) | `code-review-agent.spec.ts` (5 cases + review-flow #441 `eca2bd9`); `code-review-regressions.spec.ts` "hostile content" (opening the panel starts nothing); `code-review-agent.test.ts`; opt-in `code-review-isolation.native.test.mjs` (`N`, 2.1.316) | — | open (needs repo Val names, #325) | open (VoiceOver) | +| C28 | Review settings (gear) | #325 | built (#422 `b643617`) | `code-review-agent.spec.ts` "the instructions popover states its scope…"; journey R3 (#440 `d1cc80a`); `code-review-regressions.spec.ts` | — | open (needs repo Val names, #325) | open (VoiceOver) | +| C29 | Merge | #326 | built (#416 `b3aad0d`); D10: one button and a panel, no chevron | `code-review-actions.spec.ts` ×4 ("Merge offers only the repository's methods…", "…rules block it…", "a head that moves between preview and merge…", "a crash during a merge…"); `code-review-merge-preflight.test.ts`; `code-review-writes.test.ts`; `code-review-navigation.spec.ts` (disabled under `MODEX_E2E`) | — | open (needs disposable repo Val names, #326) | open (VoiceOver) | +| C30 | Title | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` "R1 metadata and R2 pagination…", "hostile Markdown is inert and narrow content remains reachable"; journey | `A-323` (service read only) | — | open: reference comparison | +| C31 | Edit title | #326 (decision 3 settled) | built (#416 `b3aad0d`) | `code-review-actions.spec.ts` "an edit saves only the changed field, and never overwrites a change made on GitHub meanwhile"; `code-review-writes.test.ts` | — | open (needs disposable repo Val names, #326) | open (VoiceOver) | +| C32 | Author, age, head → base | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` "R1 metadata…"; journey | `A-323` (service read only) | — | open: reference comparison | +| C33 | Description Markdown | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` "hostile Markdown is inert…"; `code-review-regressions.spec.ts` "hostile content" | `A-323` (service read only) | — | open (VoiceOver); open: reference comparison | +| C34 | Reaction under the description | deferred | deviation D3 | — | — | — | — | +| C35 | Edit description | #326 (decision 3 settled) | built (#416 `b3aad0d`) | as C31 | — | open (needs disposable repo Val names, #326) | open (VoiceOver) | +| C36 | Summary card: files and totals | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts`; journey (R4) | `A-323` (service read only) | — | open: reference comparison | +| C37 | Summary card: comments | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` | `A-323` (service read only) | — | open: reference comparison | +| C38 | Summary card: Request review | #326 (decision 4 settled) | built (#416 `b3aad0d`) | `code-review-actions.spec.ts` "reviewer requests go to others, need triage, and show on the PR…"; `code-review-writes.test.ts` | — | open (needs disposable repo Val names, #326) | open (VoiceOver) | +| C39 | Summary card: passing checks | #323 | built (#393 `6c40547`) | contract test (`summarizeChecks`); `code-review-overview.spec.ts`; journey R1 checks (#464 `3d72c6e`) | `A-323` (13 checks, service) | — | open: reference comparison | +| C40 | Sidebar: Changes | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts`; journey | `A-323` (service read only) | — | open: reference comparison | +| C41 | Sidebar: Stack | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` ("Inferred"); `code-review-overview.test.ts` "stack duplicates and invalid positions never invent an order…"; journey shows "not found" (no branches in that world) | `A-323` (closed PR: `stack` unavailable, as designed) | — | open (VoiceOver) | +| C42 | Sidebar: comment count | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` "stale checks and unavailable collections do not become empty…" | — | — | open (VoiceOver) | +| C43 | Sidebar: reviewers and + | #323 list; + #326 (decision 4 settled) | built: list #393 `6c40547`, + #416 `b3aad0d` | `code-review-overview.spec.ts`; `code-review-actions.spec.ts` "reviewer requests…" | — | +: open (needs disposable repo Val names, #326) | open (VoiceOver) | +| C44 | Checks with elapsed clock | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` ("Elapsed at read"); journey R1 (#464) | `A-main` (13/13 read, pre-render), `A-323` (13 ready) | — | open (VoiceOver) | +| C45 | Check row states | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` (pending state, keyboard expand); contract test | `A-323` (service read only) | — | open (VoiceOver) | +| C46 | Show more checks | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` ("Show all checks") | — | — | open (VoiceOver) | +| C47 | Independent sidebar scroll | #323 | built (#393 `6c40547`) | `code-review-scroll.spec.ts` (#438 `afec85b`, where the sidebar is its own scroller) | — | — | — | +| C48 | Activity heading and filter | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` "refresh does not mix fresh and pending cached pages…" (filter keeps focus) | — | — | open (VoiceOver) | +| C49 | Commit groups | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` ("7 commits"); `code-review-overview.test.ts` (dedup) | `A-main` (activity 36, read), `A-323` (35, complete) | — | open (VoiceOver) | +| C50 | "opened this pull request" | #323 | built (#393 `6c40547`; rendered in `PrOverview.tsx`) | unverified: no spec found that asserts the text | `A-323` (opening enrichment read) | — | open: reference comparison | +| C51 | Bot comment card (⋯ menu: Copy comment link, Add to chat) | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` (cards); the ⋯ menu's Copy comment link is unverified (no spec found); its Add to chat is covered under C54 | — | — | open (VoiceOver) | +| C52 | Deployment table and preview links | #323 | built (#393 `6c40547`) | `code-review-overview.test.ts` "preview destinations reject credentials…"; `code-review-read-service.test.ts`; `code-review-overview.spec.ts` | `A-323` (one real deployment and status) | — | open (VoiceOver) | +| C53 | Third-party bot button | #323 | deviation D4 (inert) | `code-review-overview.spec.ts` "hostile Markdown is inert…"; `code-review-regressions.spec.ts` "hostile content" | — | — | — | +| C54 | Reaction and Add to chat | #325 (reactions D3) | built: Add to chat (#422 `b643617`); reactions deviation D3 | `code-review-agent.spec.ts` "a question is answered by a run of its own, with 'Add to chat' quoting a comment into it" | — | open (needs repo Val names, #325) | open (VoiceOver) | +| C55 | Comment composer | #326 | built (#416 `b3aad0d`) | `code-review-actions.spec.ts` "a comment stays a local draft across a reload…", "navigating before the draft debounce…", "a denial and a moved head…", "a crash mid-send…", "a send GitHub may or may not have received…"; `code-review-writes.test.ts`, `code-review-write-ipc.test.ts`, `code-review-draft-buffer.test.ts`, `code-review-intents.test.ts` | — | open (needs disposable repo Val names, #326) | open (VoiceOver) | +| C56 | Review instructions popover (R3) | #325 | built (#422 `b643617`) | `code-review-agent.spec.ts` instructions case; journey R3 (#440 `d1cc80a`: focus, Escape, scroll kept) | — | open (needs repo Val names, #325) | open (VoiceOver) | +| C57 | Ask about this pull request | #325 | built (#422 `b643617`); an inline field in the overview's review section, not a floating composer | `code-review-agent.spec.ts` question case; `code-review-agent.test.ts` | — | open (needs repo Val names, #325) | open (VoiceOver) | +| C58 | Attach (+) | #325 | not built (left out; no numbered decision covers it) | — | — | — | — | +| C59 | Model and reasoning effort | #325 | built: model select only, effort left out (#422 `b643617`) | `code-review-agent.spec.ts` (runs with `--model`); `code-review-agent.test.ts` (unknown model refused) | — | open (needs repo Val names, #325) | open (VoiceOver) | +| C60 | Voice input | deferred | deviation D5 | — | — | — | — | +| C61 | Composer clearance | #323 / #324 | built at fixture (#393 `6c40547`): bottom clearance; final Comment in viewport at 900×600 | `code-review-overview.spec.ts` "hostile Markdown is inert and narrow content remains reachable"; `code-review-scroll.spec.ts`. Unverified against CR-6's inline Ask/review section | — | — | open (VoiceOver) | +| C62 | Header repeated over Changes | #322 | built; now repeats the live C21 and C27–C29 | `code-review-navigation.spec.ts`; journey (R4); `code-review-a11y.spec.ts` | — | — | open (VoiceOver) | +| C63 | Changes ⋯ menu and view toggles | #324 | built in part (#404 `e3527f4`): "Wrap lines" and "Highlight changes" toggles. The ⋯ menu and the tree/image/layout toggles are left out, as the inventory allows | unverified: no spec found that toggles them (they show in the diff spec and journey captures) | — | — | open (VoiceOver); open: reference comparison | +| C64 | File header | #324 | built (#404 `e3527f4`) | `code-review-diff.spec.ts` "grouped diffs, both gutters, hierarchy, filter…"; review-flow (#441) | `A-324` (service: 15/15 files) | — | open (VoiceOver); open: reference comparison | +| C65 | Viewed toggle | needs decision 5 | needs decision 5 (not built) | — | — | — | — | +| C66 | File ⋯ menu (never Discard) | #324 | built (#404 `e3527f4`): Copy path and Open in GitHub only | `code-review-regressions.spec.ts` "local Changes stay local"; `code-review-diff.spec.ts` "…use remote data only". The menu items themselves are unverified (no Code Review spec found that opens them) | — | — | open (VoiceOver) | +| C67 | Folded lines, expand | #324 | built (#404 `e3527f4`) | `code-review-diff.spec.ts` "expanding context asks the exact remote revision…", "review: context expands adjacent to both ends…", "context text budget is shared…"; `code-review-diff.test.ts` "context windows reveal both ends…" | `A-main` (context read), `A-324` (base context matched the merge base) | — | open (VoiceOver) | +| C68 | Line numbers, add/remove rows | #324 | built (#404 `e3527f4`) | `code-review-diff.test.ts` ×9; `code-review-diff.spec.ts` "line anchors have one keyboard entry per file…", "old-line anchors name the merge base…"; review-flow | `A-324` (15/15 patches parsed; anchor) | — | open (VoiceOver) | +| C69 | "More unchanged context…" | #324 | built (#404 `e3527f4`; `ContextFold` when `count` is null) | unverified: no spec found that asserts the text (`code-review-diff.spec.ts` clicks `/Expand.*unchanged/`) | — | — | open (VoiceOver) | +| C70 | Collapsed file sections | #324 | built (#404 `e3527f4`) | `code-review-diff.spec.ts` (Collapse a file; "1,000 files keep… mounted patch bodies bounded") | — | — | open (VoiceOver) | +| C71 | Horizontal scroll per file | #324 | built (#404 `e3527f4`; `.cr-patch { overflow-x: auto }` with wrap off) | unverified: no spec found | — | — | — | +| C72 | Filter files | #324 | built (#404 `e3527f4`) | `code-review-diff.spec.ts` (Filter remote files) | — | — | open (VoiceOver) | +| C73 | Folder tree, selected file | #324 | built (#404 `e3527f4`) | `code-review-diff.spec.ts` "…hierarchy, filter and keyboard selection…", "narrow tree selection returns to the diff…"; `code-review-scroll.spec.ts`; journey R4 populated (#464 `3d72c6e`) | — | — | open (VoiceOver); open: reference comparison | +| C74 | Overview / Changes divider | #322 | built | `code-review-navigation.spec.ts` (keyboard resize persists across reload) | — | — | open (VoiceOver) | +| C75 | Diff / tree divider | #322 | built; pointer and keyboard resize still not asserted | `code-review-a11y.spec.ts` (reached by Tab as a separator); `code-review-navigation.spec.ts` (tree folds when narrow) | — | — | open (VoiceOver) | +| C76 | Keyboard path, 900×600 and 1440×900 | #327 | partial: the a11y spec still requires only CR-3's 12 controls | `code-review-a11y.spec.ts`; `code-review-navigation.spec.ts` keyboard test; `code-review-scroll.spec.ts` (focus kept); per-slice keyboard cases in the overview, diff, agent and actions specs | — | — | **open: [VoiceOver checklist](2026-10-22-cr8-voiceover/checklist.md) parts A–D for Val (result row empty)** | +| C77 | Overview column scroll | #323 | built (#393 `6c40547`) | `code-review-scroll.spec.ts` (#438 `afec85b`); journey R2 (overview moves, inbox doesn't) | — | — | — | +| C78 | Diff pane scroll | #324 | built (#404 `e3527f4`) | `code-review-scroll.spec.ts`; `code-review-diff.spec.ts` "returning from Overview preserves… overall scroll position" | — | — | — | +| C79 | File tree scroll | #324 | built (#404 `e3527f4`) | `code-review-scroll.spec.ts` (#438 `afec85b`) | — | — | — | +| C80 | Closing Changes / leaving the split | #322 | built | `code-review-navigation.spec.ts` back/forward test | — | — | open (VoiceOver) | +| C81 | Timeline and composer avatars | needs decision 1 | needs decision 1 (not built; D1 login only) | — | — | — | — | +| C82 | Access-mode indicator | needs decision 7 | needs decision 7 (not built; the review panel states the confinement in words) | — | — | — | — | + +`A-322` keeps its baseline meaning: CR-3's authenticated navigation smoke on candidate `65e86dd` + `4dd5bb5`. +Decisions 2–4 (C21, C31, C35, C38, C43's +) were settled by Val on 2026-10-25 and built in #416. Decisions 1 +(C16, C81), 5 (C65), 6 (C07) and 7 (C82) are still open. + +## Acceptance criteria #321–#326 + +| AC | Criterion (short) | Fixture | Authenticated | Live provider / write | Status | +| --- | --- | --- | --- | --- | --- | +| #321 AC1 | List → detail → files over real main/IPC, isolated, no local mutation | `U`, `F-ipc`, `F-prod` journey (checkout unchanged) | `A-321` | — | met (issue closed) | +| #321 AC2 | Same PR number across repositories and accounts; account switch drops a delayed result | `U`; `F-prod` regressions "identity" ×2 | — (no second real account) | — | met at fixture level. A real `gh auth switch` mid-read was never run live | +| #321 AC3 | Revoked, denied, malformed, timeout, rate limit, partial, omitted patch, no secret leakage | `U`; `F-prod` "incomplete data" (gh stderr sentinel never reaches the renderer or a reply) | `A-349` | — | met | +| #321 AC4 | Head changes invalidate or label old diff/check data | `U`; `F-prod` "revision" (before the page: `head-changed`, files list never requested; during the page: `stale head-changed`) | — | — | met. The UI labelling landed with #324 (#404 `e3527f4`; `code-review-diff.spec.ts` "a new head cannot be overwritten by a delayed file page") | +| #321 AC5 | Authenticated read-only smoke, kept separate | — | `A-321`, `A-349`, `A-main` | — | met, and re-run on main | +| #321 AC6 | Existing Changes, connection and login tests green; no new permissions or writes | full suites (see Repository suites) | — | — | met at merge. The current local full-suite re-check failed in E2E; the isolated retry and earlier hosted pass are recorded separately (`S-1101`) | +| #322 AC1 | R1 and R4 through real controls; PR and tab identity correct | `F-nav`; `F-prod` journey (rail → row → split, with captures at the references' CSS sizes and geometry within 2 px of CR-3's table) | `A-322`, `A-main`, `A-main-2` | — | met for the host; R1 checks and R4 diff populated since #464 (`3d72c6e`). **Original-reference comparison is open** (see Visual comparison) | +| #322 AC2 | Rapid switching shows no stale reply; back/forward don't replay | `F-nav` ×3; `F-prod` "identity" ×2, journey (no re-read) | `A-322` (back/forward) | — | met | +| #322 AC3 | Scroll regions independent; resize and reopen keep controls in the window | `F-nav` keyboard test; `code-review-scroll.spec.ts` ×2 (#438 `afec85b`) | — | — | met at fixture: list, overview, diff and tree scroll alone and keep focus at 1440×900 and 900×600 (#438) | +| #322 AC4 | Keyboard-only at 900×600 and 1440×900; focus returns; reference DPR | `F-nav`; `F-prod` a11y | — | — | automated checks met; **human VoiceOver open** | +| #322 AC5 | Local Changes, threads and tabs pass; no remote PR reaches Discard | `F-nav`; `F-prod` "local Changes stay local" (a dirty file at the PR's own path is untouched; `changes:revert` never called) | — | — | met at fixture level; the full-suite result is under Repository suites | +| #322 AC6 | Fixture and authenticated smoke reported separately | this table | `A-322`, `A-main`, `A-main-2` | — | met, and re-run on main | +| --- | --- | --- | --- | --- | --- | +| #323 AC1 | R1 and R2 by selecting and scrolling the same PR; real adapter or labelled fixtures | `code-review-overview.spec.ts` "R1 metadata and R2 pagination…" (replaced fixture handler); journey R1→R2 through the production service with synthetic gh (#440 `d1cc80a`; R1 checks #464 `3d72c6e`). Evidence: [cr4-overview](2026-10-23-cr4-overview.md), [cr8-journey-refresh](2026-10-28-cr8-journey-refresh.md) | `A-323` (service, PR #334) | — | met at fixture (#393 `6c40547`). **Open:** original R1/R2 comparison (originals gone, Val) | +| #323 AC2 | Pagination and refresh: no duplicates, lost scroll or cross-PR bleed; partial never zero | `code-review-overview.spec.ts` ×8 (failed pages, refresh, delayed bleed, partial first page, retry cursor); `code-review-overview.test.ts`; [cr4-overview](2026-10-23-cr4-overview.md) | `A-323` (activity complete) | — | met at fixture (#393 `6c40547`). A live account switch was not exercised | +| #323 AC3 | Stale checks, inferred stack and comment-derived previews kept distinct | `code-review-overview.spec.ts` "stale checks and unavailable collections…"; `code-review-overview.test.ts` (stack, preview destinations, stale retention); `code-review-read-service.test.ts` | `A-323` (deployment; stack unavailable) | — | met at fixture (#393 `6c40547`) | +| #323 AC4 | Hostile Markdown/HTML, unsafe schemes, misleading previews inert | `code-review-overview.spec.ts` "hostile Markdown is inert…"; `code-review-regressions.spec.ts` "hostile content"; `code-review-overview.test.ts` "preview destinations reject…" | — | — | met at fixture (#393 `6c40547`). No live untrusted PR content was run | +| #323 AC5 | Long titles, large descriptions, narrow windows readable; composer clearance | `code-review-overview.spec.ts` (900×600, final Comment in viewport); `code-review-scroll.spec.ts` (#438 `afec85b`) | — | — | met at fixture (#393). Clearance next to CR-6's inline review/Ask section is unverified. **Open:** VoiceOver (Val) | +| #323 AC6 | Every visible write control is connected to a verified CR-7 capability or is unavailable; no mutation by #323 | At #393 all writes were unavailable ([cr4-overview](2026-10-23-cr4-overview.md)). Since #416 `b3aad0d` they are connected through `code-review:write` (`code-review-actions.spec.ts`) | — | see #326 AC6 | met at fixture | +| #324 AC1 | R4 overview-plus-diff, tree, folds, selected file through real controls; match approved reference geometry | `code-review-diff.spec.ts` "captures populated R4 diff geometry and narrow keyboard file selection"; journey R4 populated (#464 `3d72c6e`); review-flow (#441 `eca2bd9`). Evidence: [cr5-remote-diffs](2026-10-23-cr5-remote-diffs.md) | — | — | met at fixture for the controls (#404 `e3527f4`). **Open:** approved-reference geometry, since the original R4 is missing (Val) | +| #324 AC2 | Old/new line references for additions, deletions, renames, no final newline; context keeps them | `code-review-diff.test.ts` ×9; `code-review-diff.spec.ts` "review: deleted ranges and renamed paths retain base anchors…", "old-line anchors name the merge base…"; [cr5-merge-base](2026-10-24-cr5-merge-base.md) | `A-324` (anchor; merge-base match) | — | met at fixture (#404 `e3527f4`), plus an authenticated service probe | +| #324 AC3 | PR, account or head change during a delayed patch can't overwrite; old anchors outdated | `code-review-diff.spec.ts` "head change cancels delayed context…", "a new head cannot be overwritten by a delayed file page", "sign-out during a delayed patch page…", "review: changing PR during a delayed patch request…", "payload mismatch is rejected…"; `code-review-regressions.spec.ts` "revision" | — | — | met at fixture (#404 `e3527f4`) | +| #324 AC4 | No Discard, arbitrary workspace read or code execution; local Changes tests pass | `code-review-diff.spec.ts` "expanding context asks the exact remote revision and never a local file"; `code-review-regressions.spec.ts` "local Changes stay local"; full suites | — | — | met at fixture (#404 `e3527f4`) | +| #324 AC5 | Binary, omitted, oversized and truncated disclosed; no invented code or false zeros | `code-review-diff.spec.ts` "binary, omitted, oversized and truncated patches disclose…", "review: closed omitted and oversized fifth files…", "a 3 MiB textual patch…"; `code-review-diff.test.ts` | `A-324` (15/15 complete) | — | met at fixture (#404 `e3527f4`) | +| #324 AC6 | Keyboard and narrow navigation; measured stress on a named environment; any agreed budget enforced | `code-review-diff.spec.ts` "1,000 files keep requests and mounted patch bodies bounded", "accepted token-dense patches…", "line anchors have one keyboard entry…", "narrow tree selection…"; `code-review-measure.spec.ts`. The named environment is in [cr5-remote-diffs](2026-10-23-cr5-remote-diffs.md) | — | — | met at fixture for the product bounds (#404). **Open:** VoiceOver (Val). Whether #327 has agreed a performance budget is unverified | +| #325 AC1 | R3 popover accessible, states scope, persists only the intended settings, records the version per run | `code-review-agent.spec.ts` "the instructions popover states its scope…"; journey R3 (#440 `d1cc80a`); `code-review-agent.test.ts` "instructions are versioned…". Evidence: [cr6-native-review](2026-10-28-cr6-native-review.md) | — | — | met at fixture (#422 `b643617`). **Open:** VoiceOver (Val) | +| #325 AC2 | No wrong-account or wrong-repository context; findings bound to the revision; new head marks results outdated | `code-review-agent.test.ts` "a start is refused when the account, head or target differ…"; `code-review-agent.spec.ts` "a scripted reply shows its findings bound to the head…"; review-flow (a later head disables promotion) | — | — | met at fixture (#422 `b643617`) | +| #325 AC3 | Adversarial content stopped by the real capability/isolation boundary | Opt-in `code-review-isolation.native.test.mjs` (6/6 on Claude 2.1.312, and again 6/6 on **2.1.316** with codex-cli 0.166.0 on 2026-11-01, `N`; scripted SSE provider, shipped `REVIEW_CONFINEMENT_ARGS`); `claude-backend.test.ts` (argv, `confinedEnvironment`); [cr6-isolation](2026-10-23-cr6-isolation.md) (#396 `03ce71a`; Codex refused, so not offered) | — | — | met at fixture (#396, #422). The confinement is the CLI's own flags with no OS sandbox. The native suite runs only with `MODEX_CR6_NATIVE=1` | +| #325 AC4 | Explicit start, streaming, stop, failure, unknown and restart; no duplicate inference | `code-review-agent.spec.ts` "a review starts only on Start…", "Stop ends a running review, and a crash mid-run reads as unknown…"; review-flow; `code-review-agent.test.ts` (unconfirmed exit, #426 `d0482a0`) | — | — | met at fixture (#422 `b643617`, #426 `d0482a0`) | +| #325 AC5 | Omitted files and unsupported references visible; no promotion to evidence, approval or merge | `code-review-agent.spec.ts` (coverage "file list cut short"; "Not found in the diff"; Add to review draft only for an anchored current-head finding); `code-review-agent.test.ts` `planSnapshot`/`bindFindings` | — | — | met at fixture (#422 `b643617`) | +| #325 AC6 | Native-CLI fixture tests through the real app; one authorized live smoke | `code-review-agent.spec.ts` (synthetic `claude`), native fixture as AC3 | — | open (needs repo Val names, #325). None is recorded ([cr6-native-review](2026-10-28-cr6-native-review.md) "Live evidence: None") | fixture half met (#422). **Open:** live smoke (Val names the repo) | +| #326 AC1 | Local until an explicit submit; main checks permission and identity; denials surfaced | `code-review-actions.spec.ts` "a comment stays a local draft…", "a denial and a moved head are shown as such…"; `code-review-writes.test.ts`; `code-review-write-ipc.test.ts`. Evidence: [cr7-write-submit](2026-10-25-cr7-write-submit.md), [cr7-write-repair](2026-10-26-cr7-write-repair.md) | — | — | met at fixture (#416 `b3aad0d`) | +| #326 AC2 | A double click sends at most one request; an ambiguous restart sends none and shows reconciliation | `code-review-actions.spec.ts` (double clicks for comment, merge and close; "a crash mid-send restarts in doubt…", "a send GitHub may or may not have received is never resent…"); review-flow (#441 `eca2bd9`); `code-review-intents.test.ts` (SIGKILL) | — | — | met at fixture (#416 `b3aad0d`) | +| #326 AC3 | Wrong account or repository, stale head, changed target, revoked permission and outdated anchor rejected or reconfirmed | `code-review-writes.test.ts` (rechecks, login change, withdrawn permission); `code-review-actions.spec.ts` "a head that moves between preview and merge…", "a denial and a moved head…"; inline-anchor validation in [cr7-write-repair](2026-10-26-cr7-write-repair.md) | — | — | met at fixture (#416 `b3aad0d`) | +| #326 AC4 | Merge passes the reviewed head, offers only supported methods, ignores checks and findings as permission; safe fallback | `code-review-merge-preflight.test.ts`; `code-review-actions.spec.ts` merge ×4; `code-review-write-ipc.test.ts` (one PUT carrying the intent's head). Evidence: [cr7-merge-preflight](2026-10-24-cr7-merge-preflight.md), [cr7-merge](2026-10-25-cr7-merge.md) | — | — | met at fixture (#416 `b3aad0d`). Limit: a queue set by classic branch protection is invisible to the rules read, so it falls back to GitHub's refusal | +| #326 AC5 | Drafts survive errors, stay isolated, aren't overwritten by stale responses; no credentials in output | `code-review-intents.test.ts` (CAS, isolation); `code-review-draft-buffer.test.ts`; `code-review-actions.spec.ts` "navigating before the draft debounce preserves the latest text"; `code-review-writes.test.ts` ("no token text in any reply") | — | — | met at fixture (#416 `b3aad0d`) | +| #326 AC6 | Mock main/IPC and Electron cover success, rejection, timeout, crash, reconciliation; live canary only on an authorized disposable target | `code-review-writes.test.ts`, `code-review-write-ipc.test.ts`, `code-review-actions.spec.ts` (14 cases) | — | open (needs disposable repo Val names, #326). No live write, merge or action has run | mock half met (#416). **Open:** disposable-target canary (Val names the repo) | + +## #327's own criteria + +| AC | Where it stands on `0a8c513` | +| --- | --- | +| AC1 R1–R4 in real Electron | All four reached through real controls over production reads with synthetic `gh` (`F-1101` journey). **Open (Val):** comparison with the original references, which are gone; needs re-captured references or Val's decision to accept re-captures. | +| AC2 every visible control has behaviour or an explicit unavailable state | Partial: C58 attachment and C59 reasoning effort remain omitted without an approved deviation or numbered decision. The other controls are mapped above; seven built controls also lack an asserting spec (follow-ups below). | +| AC3 isolation, injection, incomplete data, at-most-once | Met at fixture: read regressions, native denial on 2.1.316 (`N`), agent stop/unknown/restart, write at-most-once and reconciliation (`F-1101`). **Open:** the live smoke (#325) and live canary (#326). | +| AC4 local Changes, navigation, conversation and approvals intact | `S-1101`: unit green; the local E2E had one non-Code-Review failure that passed alone, so it is not counted as green. The hosted required check passed on `c1f74c7`. | +| AC5 performance, keyboard/contrast; human screen-reader gate | Baseline measurements and contrast stand for `0e674e7`; `F-1101` re-ran measure and a11y green. **Open (Val):** the [VoiceOver checklist](2026-10-22-cr8-voiceover/checklist.md), Result row empty. | +| AC6 columns kept separate, tied to SHA and environment | This page. | +| AC7 reproducible, readiness stated, no merge by this task | Commands below. **Readiness:** the whole workspace (inbox, overview, remote diff, Claude review and questions, writes and merge) is built and fixture-proven; packaged builds still hide it behind `modex.codeReview`. Not ready to show by default until the live and human gates are run or Val accepts them as open. | + +## Staged enablement and rollback + +Today the only switch is the per-home `localStorage["modex.codeReview"]` (`renderer/code-review/feature.ts`): +development builds show the destination, packaged builds hide it unless the value is `"on"`. Nothing else reads +it, and main's channels answer regardless of whether the destination is visible. + +- **Stage 0, now:** hidden in packaged builds. Val (or a tester) turns it on per home from DevTools. +- **Stage 1, after the live smoke and canary pass on repositories Val names:** still hidden by default; testers + use it on their own repositories. Writes stay preview-bound and at-most-once. +- **Stage 2, after VoiceOver and Val's call on the references, decisions 1, 5, 6, 7, and the C58/C59 omissions:** remove the switch (or + invert it) in a product PR that Val approves. That PR updates `docs/status.md` and this matrix. + +**Rollback:** setting the value to anything but `"on"` (or clearing site data for that home) hides the +destination on the next launch. That hides the surface; it does not cancel a write already sent. An in-doubt +write keeps its record in `/review-actions/state.json` and an unconfirmed review run in +`/review-agent/state.json`; both stay until a read-back or the person settles them, whether or not +the destination is visible. A product rollback (removing a capability) is a code change that waits for Val. + +## Hand-off to Val, in one list + +1. **VoiceOver (AC5, #322 AC4, #325/#326 rows):** from `apps/desktop` on `0a8c513` or later after `pnpm build`: + `node ../../docs/reviews/2026-10-22-cr8-voiceover/seed.mjs /tmp/modex-cr8-voiceover`, run the printed + `env … npx electron .` line, then follow the [checklist](2026-10-22-cr8-voiceover/checklist.md) and fill its + Result row. Since 2026-11-01 the seed puts recorder `claude`/`codex` on PATH, so nothing reaches your login. +2. **R1–R4 references (AC1, #322 AC1, #323 AC1, #324 AC1):** a folder with `R1.png`–`R4.png`, or your decision + to accept re-captures. Then `pnpm exec playwright test e2e/code-review-journey.spec.ts` and + `pnpm run ui:compare -- --captures test-results/ui-code-review `. +3. **CR-6 live smoke (#325 AC6):** name a repository and PR on #325 and say go; + [procedure](2026-10-28-cr6-native-review.md#live-smoke-procedure-325-ac6-not-run). +4. **CR-7 live canary (#326 AC6):** name a disposable repository on #326 and say go; + [procedure](2026-11-01-cr7-live-canary.md). +5. **Decisions 1 (avatars C16/C81), 5 (Viewed C65), 6 (new tab C07), 7 (access mode C82).** +6. **C58 attachment and C59 reasoning effort:** decide whether to implement each omission or record an approved deviation; neither currently has a numbered decision. +7. **The `modex.codeReview` packaged-build switch:** when it goes (stages above). + +## Follow-ups found in this refresh (agent work, not gates) + +- C22 still shows the repository and number from the inbox row or resolved link (`PrPane.tsx` reads + `pr.locator`); D6 was meant to end when #323 bound `overview.locator`. Either bind it or record D6 as permanent. +- `code-review-a11y.spec.ts` still requires only CR-3's 12 controls; the overview, diff, review and write + controls are not in its required Tab set (C76). +- No asserting spec for C50's text, C51's Copy comment link, C63's toggles, C66's menu items, C69's text, C71's + horizontal scroll or C75's resize. +- C61's clearance was planned for a floating composer; CR-6 built Ask inline and clearance was not re-measured. +- CR-4's and CR-5's authenticated evidence is service-level on pre-merge candidates (`A-323`, `A-324`); an + authenticated Electron read of the overview and diff on `main` needs Val's authorization (GET only). + +## Reproducing + +From `apps/desktop` in a worktree at the SHA under test: + +```sh +pnpm run build +node --test --test-force-exit dist/test/code-review-*.test.js dist/test/claude-backend.test.js dist/test/desktop-service-drain.test.js +MODEX_CR6_NATIVE=1 node --test --test-force-exit test/code-review-isolation.native.test.mjs +pnpm exec playwright test e2e/code-review-journey.spec.ts e2e/code-review-regressions.spec.ts e2e/code-review-a11y.spec.ts \ + e2e/code-review-measure.spec.ts e2e/code-review-agent.spec.ts e2e/code-review-actions.spec.ts \ + e2e/code-review-navigation.spec.ts e2e/code-review-scroll.spec.ts +``` + +Then `pnpm build && pnpm test && pnpm test:e2e` from the repository root. Copy `test-results/` elsewhere first; +Playwright wipes it. + +## Checkpoint + +- **SHA:** `main` `0a8c513`. This page changes docs only. +- **VoiceOver seed on `0a8c513`:** `seed.mjs` ran, and its printed launch line opened the app against the seeded + home (user data under `/home/electron`). The app's provider probe reached only the recorder + (`provider-calls.log`: `claude --version`), and `gh-calls.log` stayed empty (Code Review was not opened in this launch check). +- **Next action:** Val names the two repositories (#325, #326) and answers decisions 1, 5, 6 and 7 on #319. diff --git a/docs/reviews/code-review-token-check.py b/docs/reviews/code-review-token-check.py new file mode 100644 index 00000000..ca7c0f30 --- /dev/null +++ b/docs/reviews/code-review-token-check.py @@ -0,0 +1,43 @@ +import os +from pathlib import Path +import subprocess +import sys + + +def incomplete(): + print("token-check-incomplete", file=sys.stderr) + raise SystemExit(2) + + +try: + result = subprocess.run(["gh", "auth", "token", "--hostname", "github.com"], stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, check=False) + token = result.stdout.strip() + if result.returncode or not token or b"\n" in token or b"\r" in token or not sys.argv[1:]: + incomplete() + files = [] + for value in sys.argv[1:]: + root = Path(value) + if root.is_symlink() or not root.exists(): + incomplete() + if root.is_dir(): + def walk_error(error): + raise error + for directory, directories, names in os.walk(root, onerror=walk_error): + for name in directories + names: + if (Path(directory) / name).is_symlink(): + incomplete() + files.extend(Path(directory) / name for name in names) + elif root.is_file(): + files.append(root) + else: + incomplete() + found = False + for path in files: + if not path.is_file(): + incomplete() + found = token in path.read_bytes() or found +except OSError: + incomplete() + +print("TOKEN-FOUND" if found else "token-absent") +raise SystemExit(1 if found else 0) diff --git a/docs/reviews/code-review-token-check.test.py b/docs/reviews/code-review-token-check.test.py new file mode 100644 index 00000000..95cdfd9d --- /dev/null +++ b/docs/reviews/code-review-token-check.test.py @@ -0,0 +1,79 @@ +"""Synthetic-only regression checks; never invoke the signed-in gh executable.""" +import os +from pathlib import Path +import subprocess +import sys +import tempfile +import unittest + +SCRIPT = Path(__file__).with_name("code-review-token-check.py") +TOKEN = "synthetic-review-token-not-a-credential" + + +class TokenCheck(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory() + self.addCleanup(self.temp.cleanup) + self.root = Path(self.temp.name) + self.bin = self.root / "bin" + self.bin.mkdir() + self.gh = self.bin / "gh" + self.gh.write_text('#!/bin/sh\n[ "$1 $2 $3 $4" = "auth token --hostname github.com" ] || exit 9\n[ "$FAKE_AUTH_FAIL" = 1 ] && exit 1\nprintf "%s" "$FAKE_TOKEN"\n') + self.gh.chmod(0o700) + self.evidence = self.root / "evidence" + self.evidence.mkdir() + (self.evidence / "result.txt").write_text("clean fixture result") + self.env = {**os.environ, "PATH": str(self.bin), "FAKE_TOKEN": TOKEN, "FAKE_AUTH_FAIL": "0"} + + def check(self, expected, *paths): + result = subprocess.run([sys.executable, str(SCRIPT), *map(str, paths)], env=self.env, capture_output=True, text=True) + self.assertEqual(result.returncode, expected, result.stderr) + self.assertNotIn(TOKEN, result.stdout + result.stderr) + if expected == 2: + self.assertEqual(result.stdout, "") + self.assertEqual(result.stderr.strip(), "token-check-incomplete") + else: + self.assertEqual(result.stdout.strip(), "TOKEN-FOUND" if expected else "token-absent") + + def test_clean_complete_evidence(self): + self.check(0, self.evidence) + + def test_token_in_nested_evidence(self): + nested = self.evidence / "nested" + nested.mkdir() + (nested / "log").write_text("prefix " + TOKEN + " suffix") + self.check(1, self.evidence) + + def test_failed_authentication(self): + self.env["FAKE_AUTH_FAIL"] = "1" + self.check(2, self.evidence) + + def test_empty_token(self): + self.env["FAKE_TOKEN"] = "" + self.check(2, self.evidence) + + def test_missing_required_evidence(self): + self.check(2, self.evidence, self.root / "missing") + + def test_symlink_is_not_clean_evidence(self): + (self.evidence / "link").symlink_to(self.evidence / "result.txt") + self.check(2, self.evidence) + + def test_unreadable_file(self): + file = self.evidence / "result.txt" + file.chmod(0) + try: + self.check(2, self.evidence) + finally: + file.chmod(0o600) + + def test_missing_gh(self): + self.gh.unlink() + self.check(2, self.evidence) + + def test_no_required_paths(self): + self.check(2) + + +if __name__ == "__main__": + unittest.main() diff --git a/docs/status.md b/docs/status.md index 9a009b00..902a9c8a 100644 --- a/docs/status.md +++ b/docs/status.md @@ -313,13 +313,15 @@ Desktop CI memory diagnostics landed through #458 after #456’s cold transcript 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. +- **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. On 2026-11-01 the unit and opt-in native denial tests were re-run on the installed Claude Code 2.1.316 and codex-cli 0.166.0 (6/6 native cases passed), and the live-smoke procedure was written down; it waits for a repository Val names. -- **CR-7 Code Review writes** (#326). Val decided on 2026-10-25: writes go through the person's `gh` login, and decisions 2–4 are in scope. The foundation stores scoped drafts and revision-bound intents, consumes confirmations once after durable admission, and recovers interrupted dispatches as unknown without resend. The first write increment adds main's separate `code-review:write` channel for top-level comments and submitted reviews (line comments from Changes included). Each is previewed with its exact destination, account and head, re-checked in main before sending, sent at most once, and reconciled by read-back when its outcome is unclear ([evidence](reviews/2026-10-25-cr7-write-submit.md)). Mock and synthetic-`gh` only: the live write waits for a disposable repository Val names. A second increment adds Merge: main reads push permission, the repository's allowed methods, GitHub's merge state and the target branch's rules fresh. It offers only what `mergePreflight` ([preflight evidence](reviews/2026-10-24-cr7-merge-preflight.md)) allows, sends the reviewed head as the merge's `sha`, and reconciles an unclear merge from the PR's own record ([evidence](reviews/2026-10-25-cr7-merge.md)). A third increment adds decisions 2–4 the same way: the state menu (close, reopen, convert to draft, ready for review), title and description edits that are refused if GitHub's text changed, and reviewer requests ([evidence](reviews/2026-10-25-cr7-pr-actions.md)). CR-6 isolation and C82 remain unchanged. See [the preparation evidence](reviews/2026-10-23-cr7-intents.md). A follow-up binds the model to CR-5's shared `LineAnchor`, diff commit included, and records the check of CR-4's action slots ([evidence](reviews/2026-10-24-cr7-anchors.md)). The integration repair fences queued and in-flight submits against document invalidation, verifies inline destinations against fresh complete patch evidence, preserves pending composer edits across navigation, and shows frozen inline details before submission ([repair evidence](reviews/2026-10-26-cr7-write-repair.md)). +- **CR-7 Code Review writes** (#326). Val decided on 2026-10-25: writes go through the person's `gh` login, and decisions 2–4 are in scope. The foundation stores scoped drafts and revision-bound intents, consumes confirmations once after durable admission, and recovers interrupted dispatches as unknown without resend. The first write increment adds main's separate `code-review:write` channel for top-level comments and submitted reviews (line comments from Changes included). Each is previewed with its exact destination, account and head, re-checked in main before sending, sent at most once, and reconciled by read-back when its outcome is unclear ([evidence](reviews/2026-10-25-cr7-write-submit.md)). Mock and synthetic-`gh` only: the live write waits for a disposable repository Val names. A second increment adds Merge: main reads push permission, the repository's allowed methods, GitHub's merge state and the target branch's rules fresh. It offers only what `mergePreflight` ([preflight evidence](reviews/2026-10-24-cr7-merge-preflight.md)) allows, sends the reviewed head as the merge's `sha`, and reconciles an unclear merge from the PR's own record ([evidence](reviews/2026-10-25-cr7-merge.md)). A third increment adds decisions 2–4 the same way: the state menu (close, reopen, convert to draft, ready for review), title and description edits that are refused if GitHub's text changed, and reviewer requests ([evidence](reviews/2026-10-25-cr7-pr-actions.md)). CR-6 isolation and C82 remain unchanged. See [the preparation evidence](reviews/2026-10-23-cr7-intents.md). A follow-up binds the model to CR-5's shared `LineAnchor`, diff commit included, and records the check of CR-4's action slots ([evidence](reviews/2026-10-24-cr7-anchors.md)). The integration repair fences queued and in-flight submits against document invalidation, verifies inline destinations against fresh complete patch evidence, preserves pending composer edits across navigation, and shows frozen inline details before submission ([repair evidence](reviews/2026-10-26-cr7-write-repair.md)). All of it landed through #416 (`b3aad0d`, 2026-10-27). The live-write [canary procedure](reviews/2026-11-01-cr7-live-canary.md) (comment, review, state, edit, reviewer, squash-merge on a throwaway PR) is written and not run; it waits for a disposable repository Val names. -- **Code Review workspace** ([tracker #319](https://github.com/TypeSafeAI/modex/issues/319)). A PR inbox, an overview with activity and checks, and a revision-bound remote diff in Modex desktop, rebuilt from four reference screenshots. Gate A, the read-only workspace (#320–#324), is under way. CR-1 (#320) defines the account- and revision-scoped read contract and synthetic fixtures, and its [design page](design/code-review.md) maps every reference control to an owner. CR-2 (#321) adds main's read service on the `code-review:read` channel: GET-only reads through the person's `gh` login, keyed by account, repository id and revision. The separate `cr2-cache-denial` follow-up invalidates retained PR data after visibility loss, including reads already in flight; it covers pending inbox searches, sibling file slices and denial during identity confirmation, while preserving unrelated repositories and ordinary offline/rate-limit fallback. [AC1 Electron evidence](reviews/2026-10-21-cr2-electron-ipc.md) adds inbox → overview → files coverage through the production preload/IPC/service with a synthetic GitHub executable. The CR-3 development flag exposes the integrated inbox and workspace host. CR-4 (#323) adds the PR description, paginated activity, checks, inferred branch stack and provider/comment preview provenance through that host; validation and delivery evidence is recorded in [the CR-4 review](reviews/2026-10-23-cr4-overview.md). Remote diff content, AI review (#325) and GitHub writes (#326) retain their own gates. Original-reference comparison and human accessibility remain open. +- **Code Review workspace** ([tracker #319](https://github.com/TypeSafeAI/modex/issues/319)). A PR inbox, an overview with activity and checks, and a revision-bound remote diff in Modex desktop, rebuilt from four reference screenshots. Every slice's code is on `main` and shipped in v0.4.0 and v0.4.1 (CR-4 `6c40547`, CR-5 `e3527f4`, CR-6 `b643617`/`d0482a0`, CR-7 `b3aad0d`); what remains is acceptance, and no live, human or original-reference gate has run. CR-1 (#320) defines the account- and revision-scoped read contract and synthetic fixtures, and its [design page](design/code-review.md) maps every reference control to an owner. CR-2 (#321) adds main's read service on the `code-review:read` channel: GET-only reads through the person's `gh` login, keyed by account, repository id and revision. The separate `cr2-cache-denial` follow-up invalidates retained PR data after visibility loss, including reads already in flight; it covers pending inbox searches, sibling file slices and denial during identity confirmation, while preserving unrelated repositories and ordinary offline/rate-limit fallback. [AC1 Electron evidence](reviews/2026-10-21-cr2-electron-ipc.md) adds inbox → overview → files coverage through the production preload/IPC/service with a synthetic GitHub executable. The CR-3 development flag exposes the integrated inbox and workspace host. CR-4 (#323) adds the PR description, paginated activity, checks, inferred branch stack and provider/comment preview provenance through that host; validation and delivery evidence is recorded in [the CR-4 review](reviews/2026-10-23-cr4-overview.md). Remote diffs (#324), agent review (#325) and GitHub writes (#326) are built; their live and human gates are listed below. Original-reference comparison and human accessibility remain open. Packaged builds still hide the destination unless `localStorage["modex.codeReview"]` is `"on"`; when that switch goes is Val's call. - 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. + 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 2026-10-22 matrix is a historical baseline; the [2026-11-01 refresh](reviews/2026-11-01-cr8-acceptance-matrix.md) maps every control and AC of #321–#326 to evidence on `main` `0a8c513`, with no "not started" rows, and lists Val's hand-off in one place. 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. + + **Code Review gate corrections.** A candidate marks the C58/C59 omissions as unresolved acceptance gates and replaces the CR-6/CR-7 false-clean token pipelines with a fail-closed scanner. Nine synthetic regressions and build pass; independent review found no blockers. Full unit validation passed: 2,277 passes, 26 skips, zero failures. [Evidence](reviews/2026-11-01-cr-gate-corrections.md). 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.