diff --git a/docs/reviews/2026-10-28-cr6-native-review.md b/docs/reviews/2026-10-28-cr6-native-review.md index cc7fac8e..77c76551 100644 --- a/docs/reviews/2026-10-28-cr6-native-review.md +++ b/docs/reviews/2026-10-28-cr6-native-review.md @@ -195,10 +195,17 @@ Code Review → paste the PR link Val named → **Review with Claude** → read **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 -grep -E -- '--method (POST|PUT|PATCH|DELETE)' "$SMOKE/gh-argv.log" && echo WRITE-FOUND || echo no-github-write +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 -gh auth token | grep -rlF -f - "$SMOKE/home" "$SMOKE/claude-argv.log" "$SMOKE/electron-stderr.log" && echo TOKEN-FOUND || echo token-absent +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 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 index 9912ed18..d59c4128 100644 --- a/docs/reviews/2026-11-01-cr7-live-canary.md +++ b/docs/reviews/2026-11-01-cr7-live-canary.md @@ -89,12 +89,14 @@ proved. A step that fails is recorded as failed with its screenshot; the canary ## 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 file names only, never the token): -gh auth token | grep -rlF -f - "$CANARY/home" "$CANARY/electron-stderr.log" "$CANARY/steps.md" && echo TOKEN-FOUND || echo token-absent +# 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 diff --git a/docs/reviews/2026-11-01-cr8-acceptance-matrix.md b/docs/reviews/2026-11-01-cr8-acceptance-matrix.md index 04d917e1..9467fd7c 100644 --- a/docs/reviews/2026-11-01-cr8-acceptance-matrix.md +++ b/docs/reviews/2026-11-01-cr8-acceptance-matrix.md @@ -143,7 +143,7 @@ Decisions 2–4 (C21, C31, C35, C38, C43's +) were settled by Val on 2026-10-25 | #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. Re-checked in this branch's full run | +| #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) | @@ -181,7 +181,7 @@ Decisions 2–4 (C21, C31, C35, C38, C43's +) were settled by Val on 2026-10-25 | 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 | Met at fixture: every control is built, a recorded deviation, or a named open decision (table above). Seven built controls have no asserting spec (follow-ups below). | +| 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. | @@ -197,7 +197,7 @@ 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 and decisions 1, 5, 6, 7:** remove the switch (or +- **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 @@ -220,7 +220,8 @@ the destination is visible. A product rollback (removing a capability) is a code 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. **The `modex.codeReview` packaged-build switch:** when it goes (stages above). +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) @@ -230,7 +231,6 @@ the destination is visible. A product rollback (removing a capability) is a code 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. -- C58 (Attach) and C59's effort are left out with no numbered decision; Val may want them recorded as D-rows. - 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). 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 6de59f2d..ca8d45f9 100644 --- a/docs/status.md +++ b/docs/status.md @@ -233,7 +233,7 @@ The published application source is `6a97d23aaea315b29008890f25dde2d7ea07f1d0` ( ## In flight -- **Deferred Markdown for long threads (hosted cold-open budget).** Hosted CI's approved 500 ms cold-open gate failed on runner variance alone: 33 hosted runs on 2026-10-30 had a 377.7 ms median and a 781.9 ms failure on application code identical to a 436.1 ms pass. Val chose on 2026-10-30 to defer offscreen answers. Turns before the newest eight show their Markdown as written until it nears the view, then parse once; scroll anchoring keeps the reader's place. Locally the five-launch median fell from about 190 to 88 ms, and 3× CPU throttling cut it from 806–870 to 312–340 ms. The gate is unchanged. The first hosted run had a 129.9 ms median (481 desktop E2E passes). It landed through #463 (`32f1cb4`) on 2026-11-01 and is unreleased. [Evidence](reviews/2026-10-30-deferred-markdown.md). +- **Deferred Markdown for long threads (hosted cold-open budget).** Hosted CI's approved 500 ms cold-open gate failed on runner variance alone: 33 hosted runs on 2026-10-30 had a 377.7 ms median and a 781.9 ms failure on application code identical to a 436.1 ms pass. Val chose on 2026-10-30 to defer offscreen answers. Turns before the newest eight show their Markdown as written until it nears the view, then parse once; scroll anchoring keeps the reader's place. Locally the five-launch median fell from about 190 to 88 ms, and 3× CPU throttling cut it from 806–870 to 312–340 ms. The gate is unchanged. The first hosted run had a 129.9 ms median (481 desktop E2E passes). It landed through #463 (`32f1cb4`) on 2026-10-31 UTC and is unreleased. [Evidence](reviews/2026-10-30-deferred-markdown.md). Desktop CI memory diagnostics landed through #458 after #456’s cold transcript budget failure. The workflow reuses the existing resource sampler and retains its log as a separate CI artifact; it does not establish the cause of the earlier failure or change the 500 ms gate. The #458 queue passed desktop and site E2E and retained its memory log. See [the diagnostic review](reviews/2026-10-30-desktop-ci-memory.md). @@ -304,7 +304,7 @@ Desktop CI memory diagnostics landed through #458 after #456’s cold transcript overview and remote diff now in the panes, that the list, overview, diff and file tree scroll alone and keep focus at 1440×900 and 900×600 ([acceptance record](reviews/2026-10-28-cr3-acceptance.md)). On 2026-10-30 the automated half was re-run on `main` at `f51ef7e` and the read-only authenticated - navigation probe passed six of six there again ([resume record](reviews/2026-10-30-cr3-acceptance-resume.md), landed through #464, `3d72c6e`, on 2026-11-01); + navigation probe passed six of six there again ([resume record](reviews/2026-10-30-cr3-acceptance-resume.md), landed through #464, `3d72c6e`, on 2026-10-31 UTC); the journey spec's R1 and R4 captures, which had shown an empty Changes pane and no checks, now carry populated content for the comparison. Original reference comparison waits for Val to name the references (deferred on 2026-10-28); human @@ -319,6 +319,8 @@ Desktop CI memory diagnostics landed through #458 after #456’s cold transcript 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. The [integrated CR-8 review-flow fixture](reviews/2026-10-28-cr8-review-flow.md) connects a populated remote diff, synthetic native review, anchored draft, explicit submission and restart reconciliation, requiring exactly one provider run and one synthetic write. The extended fixture passed build/unit validation and full E2E (475 desktop passes, 7 skips and 7 site passes). All seven combined agent and journey tests passed after integrating #440; it landed through #441 (`eca2bd9`) and is in v0.4.0. Human, original-reference and live-service acceptance remain open.