Skip to content

[CR-8][P1] Verify Code Review fidelity, accessibility, recovery and end-to-end safety #327

Open

Description

@BunsDev

Outcome

Provide reproducible evidence that the integrated Code Review workspace matches R1–R4 and behaves correctly through navigation, remote diffs, native review, drafts and explicit actions. A passing image comparison alone is not completion; a mock run is not live GitHub/native-provider evidence.

Parent: #319. Start dependency: #320's accepted reference/fixture contract; test design can begin immediately after it. Completion dependencies: accepted implementations/evidence from #321, #322, #323, #324, #325 and #326. Every implementation issue still owns its own tests; this issue integrates and independently audits them rather than postponing testing until the end. Do not create a dependency from those producers back to this issue.

Context / entry points

Read current AGENTS.md, the live #319 graph, apps/desktop/package.json, existing Playwright configuration/layout tests and docs/status.md. Audit baseline cb33db5df00e235f68d36b5fac556dc30e0851f2 already has ui:capture/ui:compare scripts and substantial conversation-fixture work under #281. Extend appropriate existing harnesses; do not create a second browser, transcript or test-reporting platform.

Proposed additions: a focused apps/desktop/e2e/code-review-acceptance.spec.ts or several small specs beside existing e2e tests, approved reference/golden assets in the existing convention, and a dated docs/reviews/ evidence record. Confirm exact existing paths in #320's source map. Do not publish original private screenshots or real PR payloads without approval.

Items to do

  • Build an acceptance matrix mapping every R1–R4 region/control and each producer AC to a test/evidence owner. Mark native window chrome, voice, proprietary review integrations and other explicitly deferred controls as documented deviations, not secretly implemented functionality.
  • Make deterministic captures through the actual Electron application and main/IPC fixtures: fixed synthetic PR data, timestamps, locale, theme, approved fonts, viewport and DPR. Calibrate original image crop/scale with [CR-1][P1] Establish the Code Review reference contract and deterministic fixtures #320. Browser-only component snapshots must be labeled as such.
  • Reach R1 by navigation, R2 by scrolling, R3 by opening the instructions popover and R4 through split/Changes controls. Compare panel boundaries, text hierarchy, card density, independent scrolling and composer clearance. Use screenshot assertions plus behavioral assertions and human review of meaningful differences; never mask the main content or auto-accept a new golden to hide a defect.
  • Exercise a complete fixture journey: inbox → overview/activity → file/context selection → explicit native-CLI fixture review → inspect stale/current findings → draft → confirmation → mocked external write → reconciliation/restart. Assert zero actual external writes and zero paid inference in the default suite.
  • Add integrated adversarial/race cases: wrong-account delayed responses, two repositories sharing a PR number, revoked access, head changes, incomplete pagination/diffs, unsafe Markdown/URLs/paths, injected review instructions, provider stop/unknown completion and lost responses to writes. Assert no secret leakage, local Discard routing, duplicate provider start or replayed mutation.
  • Verify keyboard navigation, visible focus, accessible labels/status, popover Escape/focus return, resizable panes, reduced motion and readable contrast. Capture 900×600, 1440×900 and the calibrated reference conditions. Record a separate human screen-reader pass; automated checks cannot stand in for it.
  • Measure representative and stress fixtures on a named environment, including a large inbox, 1,000 changed files and a 3 MiB textual patch. Record actual request count, mounted content/memory and navigation/scroll responsiveness. Agree numerical regression budgets from measured baseline before enabling them as gates; distinguish these product fixtures from provider API limits.
  • Run existing local Changes/discard, shell/tab, conversation, native approval and recovery regressions. Route defects to their existing owner/child with a minimal reproducer; avoid a broad final-stage rewrite or duplicate follow-up issue when a matching owner already exists.
  • Produce an evidence matrix with separate columns for Node/model tests, main/IPC integration, real Electron, authenticated read-only GitHub, authorized live native provider, explicitly authorized disposable-target writes and human accessibility review. Missing authorization/environment leaves a named open gate, never a fabricated pass.
  • Document staged enablement: read-only navigation/overview/diffs first; native review and GitHub writes have independent capability gates. Document a rollback that disables new capabilities safely without deleting private drafts, replaying actions or claiming to cancel an already accepted remote write. Activation/release needs its own authorization.

Acceptance criteria

  • AC1: R1–R4 are reproduced by an interactive test journey in real Electron, with approved visual differences and reference provenance recorded.
  • AC2: every in-scope visible control has behavioral evidence or an explicit unavailable state; no static fake-working controls or unrelated private screenshot data ship.
  • AC3: identity/revision isolation, injection resistance, incomplete-data handling and at-most-one local dispatch/unknown-outcome recovery pass integrated regression tests.
  • AC4: local Changes, navigation and native conversation/approval behavior remain intact; unresolved regressions block readiness.
  • AC5: measured performance and keyboard/contrast checks have reproducible evidence. The human screen-reader gate is completed or explicitly remains open.
  • AC6: the final matrix separates fixture, authenticated, live-provider and live-write evidence and ties each result to its actual SHA/environment. No unrun test is reported as passing.
  • AC7: a reviewer can reproduce the result from the documented setup/commands and identify exactly which capabilities are ready, blocked or deliberately deferred. No merge/deployment is performed by this acceptance task without separate authority.

Validation / handoff

Run focused e2e specs, then repository-required pnpm build && pnpm test and pnpm test:e2e. Inspect the existing ui:capture and ui:compare scripts before using them for this feature; do not assume they automatically cover new views. Preserve final screenshots/traces outside directories overwritten by later Playwright runs. Record full SHA, OS/Electron/browser/native-CLI versions, fixture revision, viewport/DPR, exact commands/results, approved deviations, risks and remaining manual gates in docs/reviews/ and docs/status.md.

Sources: #319; Playwright screenshot comparisons, Electron security, and the existing #281 acceptance work. Follow current official documentation for concrete test APIs.

Long-running goal prompt

Complete CR-8 as the independent integration and acceptance pass for #319 in TypeSafeAI/modex. Read the live issue graph, #320's accepted reference contract, all producer PRs, current source and scoped AGENTS.md. Verify your owned worktree and the exact build SHA. Reuse existing Electron/Playwright/native-CLI fixtures. Exercise R1–R4 through actual controls and verify behavior as well as pixels. Test account/head races, incomplete data, hostile content, stop/restart, draft isolation and uncertain external writes without real paid requests or mutations in the default suite. Recheck local Changes and conversation/approval regressions. Inspect screenshots, keyboard/narrow layouts and measured stress behavior; never auto-accept goldens or call mocks live evidence. Record separate authenticated, provider, write and human accessibility gates; use existing authority for canaries only when explicit. Fix only bounded defects within your ownership, otherwise hand the reproducer to the right child. Checkpoint SHA, commands/results, evidence paths, risks and one next action. Finish with a reproducible readiness matrix and signed reviewable changes, not an unsupported all-done claim. Do not activate, merge or release.

Follow-up / resume prompt

Resume CR-8 by reading current producer heads, the evidence matrix and the latest checkpoint. Verify worktree ownership and rebuild/rerun a focused smoke test; invalidate evidence whose source, fixture or environment changed. Continue the smallest remaining integrated gate rather than repeating the entire plan. Do not overwrite screenshots/traces before preserving final evidence, and do not rerun uncertain provider/write canaries automatically. Keep unavailable live and human checks explicit. Update the exact readiness/blocked/deferred matrix and one next action; no automatic golden acceptance, merge or release.

Independent review prompt

Audit the final evidence matrix against the actual current SHA and interactive R1–R4 flows. Reproduce representative identity/head races, hostile input, incomplete diffs and restart-after-unknown-write cases. Inspect meaningful visual differences and keyboard/narrow behavior yourself. Verify local regressions and that mock, Electron, authenticated, native and human evidence are not conflated. Report remaining blockers and precise capability readiness; do not approve from the implementer's summary alone.

Activity

  1. BunsDev commented on Oct 20, 2026

    @BunsDev
    ContributorAuthor

    The CR-1 contract is on main as 43d5b9c (#334). Bind to these files at that commit:

    • apps/desktop/src/shared/code-review.ts (the contract)
    • apps/desktop/test/fixtures/code-review.ts (synthetic fixtures)
    • docs/design/code-review.md (source map, references, inventory, open decisions)

    For CR-8:

    • The design page holds the reference manifest: checksums, scale (R1 is 1×, R2–R4 are 2×), crop offsets, measured geometry and typography.
    • A contract test keeps the manifest and the inventory in step with the fixtures.
    • The references stay off-repo.

    Coven-Harness: Claude Code / claude-opus-5-5

  2. self-assigned this
    on Oct 20, 2026
  3. mariscate commented on Oct 22, 2026

    @mariscate

    Refreshed goal prompts — 2026-10-22 (main at 0e674e7)

    Where it stands: no acceptance spec yet. The harness pieces exist: e2e/code-review-navigation.spec.ts, e2e/code-review-ipc.spec.ts, the ui:capture/ui:compare scripts, and the reference manifest in docs/design/code-review.md (R1 at 1×, R2–R4 at 2×; the references themselves stay off-repo). Evidence already recorded:

    Not started: #323, #324, #325, #326.

    Long-running goal prompt

    Own CR-8 (#327) acceptance for the Code Review workspace in TypeSafeAI/modex. Read AGENTS.md, #319–#326, docs/design/code-review.md and the existing CR review docs in docs/reviews, then verify main's SHA. Create your own worktree. Start now, in parallel with the implementation slices: write a dated acceptance matrix in docs/reviews. Give it one row per in-scope control (C01–C82) and per AC across #321–#326, with columns for fixture, authenticated, live-provider and live-write evidence, each tied to a SHA and environment. Fill in the rows that existing evidence already proves; mark the rest pending with their owners. Add the integrated Electron journeys as small specs beside the existing code-review e2e specs, inbox → R1 → R2 → R4 → (R3 once #325 lands), using the existing harness and ui:compare, not a new framework. Write cross-slice regressions for identity and revision isolation, hostile content, incomplete data, at-most-once dispatch, and the local Changes/Discard separation. Collect keyboard and contrast checks, plus measured performance on a named machine. Prepare the human VoiceOver checklist for Val. Never report an unrun check as passed, and never commit private screenshots or real PR payloads. Update the matrix as each slice lands, and update docs/status.md. No merges or releases on this task's authority.
    

    Follow-up / resume prompt

    Resume CR-8. Fetch main and list which CR slices have landed since the matrix's last SHA. Re-run the integrated specs on current main and update only the rows backed by fresh evidence; mark rows whose evidence is older than the code they cover as stale. Add journeys for newly landed slices. File regressions on the owning slice's issue, not here. Post a checkpoint: SHA, matrix delta, blocked rows with owners, and one next action.
    
  4. BunsDev commented on Oct 22, 2026

    @BunsDev
    ContributorAuthor

    CR-8 checkpoint, 2026-10-22

    Coven-Harness: Claude Code / claude-opus-5-5

  5. BunsDev commented on Oct 22, 2026

    @BunsDev
    ContributorAuthor

    CR-8 update: Val authorized a read-only authenticated re-run on main (0e674e7) on 2026-10-22. The CR-2 service smoke against #367 (head 0866265) passed: 17 GET requests, 7 of them 304s; stack unavailable as designed; the foreign-account and unknown-repository keys were refused. CR-3's authenticated navigation probe passed all six checks. It is recorded as A-main in the matrix (#372 at 1a3aa18). Still open: the R1–R4 reference comparison (waiting on Val for the references' new location), the human VoiceOver pass, and R2/R3 (#323, #325).

    Coven-Harness: Claude Code / claude-opus-5-5

  6. BunsDev commented on Oct 23, 2026

    @BunsDev
    ContributorAuthor

    Refreshed goal prompts — 2026-10-23 (main at 203a350)

    On main already:

    • the earlier CR evidence: the CR-2 read smoke, Electron IPC and cache denial, and the CR-3 read integration;
    • the ui:capture/ui:compare scripts;
    • the reference manifest in docs/design/code-review.md.

    The acceptance matrix itself has not landed.

    In review: #372 (cr8-acceptance, head 92ce082). Tests and docs only. All checks pass and it merges cleanly. It adds:

    Still open:

    Long-running goal prompt

    Own CR-8 (#327) acceptance for the Code Review workspace in TypeSafeAI/modex. The matrix, journeys and regressions are in PR #372 (branch cr8-acceptance); build on them, don't start over.
    
    Read AGENTS.md, #319–#326, docs/design/code-review.md and #372's matrix, then verify main's SHA. Work in cr8-acceptance only if you own it (scripts/worktree.sh claim, then check).
    
    1. Keep #372 mergeable with current main, and land it on Val's go-ahead.
    2. Run the journey against #393's head and add the R2 step. Mark the evidence as candidate evidence until it is on main.
    3. When CR-5 opens a PR, add the populated R4 diff step and its regressions to the journey, including the Changes/Discard separation and head changes during a files page.
    4. After each slice lands, re-run the integrated specs on main and update only the rows backed by fresh evidence. Mark rows whose evidence is older than the code they cover as stale.
    5. Ask Val where the R1–R4 references are now. Run ui:compare against them only when they are found; never substitute synthetic captures.
    6. Give Val the VoiceOver checklist and record the result.
    
    Write red tests first for new regressions. Run pnpm build && pnpm test && pnpm test:e2e, with CI as the clean run of record. Never report an unrun check as passed. Never commit private screenshots or real PR payloads. Update docs/status.md. Use signed commits, and don't merge or release on this task's authority.
    

    Follow-up / resume prompt

    Resume CR-8.
    1. Fetch main and list which CR slices and PRs (#372, #393, CR-5) have landed since the matrix's last SHA.
    2. Re-run the integrated specs on current main.
    3. Update only the rows backed by fresh evidence, and mark stale rows.
    4. Add journeys for newly landed slices.
    File regressions on the owning slice's issue, not here. Post a checkpoint: SHA, matrix delta, blocked rows with owners, and one next action.
    
  7. BunsDev commented on Nov 1, 2026

    @BunsDev
    ContributorAuthor

    CR-8 checkpoint, 2026-11-01 (main at 0a8c513)

    PR: #472 (cr-gates-lane-c). Docs and evidence only. It adds the refreshed matrix.

    Commands and results (Mac17,6, macOS 27.2, load average about 40):

    • Code Review unit tests: 224 tests, 223 passed, 1 skipped (Windows only).

    • MODEX_CR6_NATIVE=1 native denial: 6 of 6 passed on Claude Code 2.1.316 and codex-cli 0.166.0.

    • Focused Playwright run, 51 of 51 passed:

      Spec Cases
      journey 1
      regressions 9
      a11y 5
      measure 2
      agent 6
      actions 14
      navigation 12
      scroll 2

      The journey's R1–R4 captures carry CR-3 acceptance re-run on main: populated journey captures and evidence (#322) #464's populated checks and diff. They are kept outside the repository.

    • pnpm build: passed. pnpm test: exit 0, 2,277 passed, 26 skipped. pnpm test:e2e: exit 1: desktop 477 passed, 7 skipped, 1 failed. The failure is app.spec.ts:729 (accessibility), a strict-mode violation with two "Chat details" dialogs, at a load average near 100. It is not a Code Review spec, and it passed 1/1 alone. The chain stopped before the site stage, which passed 7/7 when run separately. This is not a green local full run. The hosted unit + e2e (macOS) check passed on c1f74c7.

    Evidence by kind:

    • Fixture: everything above.
    • Authenticated: the earlier read-only service and navigation probes, nothing new. CR-4's and CR-5's authenticated evidence is service-level, on candidates before they merged.
    • Live provider / live write: none.
    • Human: none.

    The refresh also adds:

    • staged enablement and rollback text for the modex.codeReview switch;
    • a list of follow-ups:
      • C22 still reads the repository and number from the row or link (D6);
      • the a11y spec still requires only CR-3's 12 controls;
      • seven built controls have no asserting spec;
      • C58 and C59's effort selector are left out with no numbered decision.

    Waiting on Val: VoiceOver, the R1–R4 references, the two repositories, decisions 1, 5, 6 and 7, and the switch. The batched ask is on #319.

    Next action: fold the a11y spec's required set over the CR-4 to CR-7 controls (C76), in a test-only PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions