diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 17ecf55d33..57b8696039 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -269,3 +269,5 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-07-30 | PR-1494 | 807a3a09f5afc12e8db4f9158abe09d9c7b336c9 | PR #1494 pre-commit fail-open review | FIXED P2: legacy worktrees may skip a genuinely absent generator, while a staged deletion or rename now fails closed | docs-inventory Vitest 5 passed; shell syntax passed; Prettier test check passed; diff check passed | | 2026-07-30 | claude/x3-rag-coverage-gate-qx9j7d (PR #1463, squashed as dba7356f) | dba7356fc8dc926d951d6de6f019d5b8e000be21 | X3/#101 per-request hydration extraction from rag.ts into rag-hydration.ts | clean and landed — byte-identical move verified against pre-merge main, rag.ts 4780->4543, budget ratcheted to 4543, no back-edge (cluster referenced zero rag.ts-local symbols), both public re-exports preserved; squash captured 100% of branch content | typecheck, lint, check:knip, check:maintainability-budgets 4543/4543, focused vitest 83/83 incl rag-query-concurrency, eval:rag:offline 572/572 36 golden, format:check, verify:cheap, verify:pr-local build+bundle-scan, post-merge content verification on main | | 2026-07-30 | dba7356fc8dc926d951d6de6f019d5b8e000be21 | dba7356fc8dc926d951d6de6f019d5b8e000be21 | X3 hydration unit: per-request hydration extraction from rag.ts into rag-hydration.ts (PR #1463) | clean and landed — byte-identical move verified against pre-merge main, rag.ts 4780->4543, budget ratcheted to 4543, no back-edge, both public re-exports preserved. Supersedes the earlier row for this HEAD, which was keyed only to the slash-form branch token and so returned NOT REVIEWED on a landed-SHA lookup; it also mislabelled the unit as #101, which is the unrelated open canary-gated retrieval-parallelisation recommendation | typecheck, lint, check:knip, check:maintainability-budgets 4543/4543, focused vitest 83/83, eval:rag:offline 572/572 36 golden, format:check, verify:cheap 442 files 4625 passed, verify:pr-local, post-merge content verification on main | +| 2026-07-30 | codex/coverage-scope-policy | 94f97cdb1d0543724de408f19e79d64e61c8b31a | issue 139 coverage scope policy | approved: workflow coverage breadth is deliberate and test-pinned; docs-like skills remain static-only | check:ci-scope; check:gate-manifest; check:outstanding-issues; prettier; diff check | +| 2026-07-30 | codex/coverage-scope-policy | 4da2a003bc2254507662d1b8b6e9768e94371abd | issue 139 coverage scope policy post-sync | approved: late main sync preserves deliberate workflow coverage and static-only skill policy | check:ci-scope; check:outstanding-issues; check:branch-review-ledger; diff check | diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 1d6210bd7c..327ef06c9f 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -149,7 +149,6 @@ removed after current-main verification; it is not missing recommended work. | #142 | P3 | task | Four loose dated docs need source and migration edits before they can be filed | **Outcome:** every dated point-in-time doc lives in `docs/audit/` or `docs/archive/` as `docs/README.md` requires, not loose at the `docs/` top level. **Detail:** PR #1436 filed the five that were docs-only moves. These four are referenced from outside `docs/`, so relocating them means editing source, tests and migration SQL comments — a different risk class than a docs tidy, and not worth bundling into one: `capacity-review.md` (`scripts/soak-test.ts`), `tenancy-defense-in-depth-review.md` (`src/lib/owner-scope.ts`, `tests/owner-scope-guard.test.ts`, two migrations, `SECURITY.md`, `.claude/agents/clinical-governance-reviewer.md`), `operator-apply-july8-batch.md` (three migrations plus `supabase/schema.sql`), `scale-readiness-review.md` (one migration). Also note `forward-codify-retrieval-rpcs-workorder.md` is indexed as a completed workorder but live is still ahead of the repo on those RPCs, so archiving it would misrepresent open operator work. **Next:** treat as low priority — the docs are correctly indexed and reachable where they are; only file them if a pass is already editing those migrations. Editing applied migration SQL is subject to `npm run check:migration-role`'s immutability pin. | PR #1436; session 2026-07-30 | 2026-07-30 | | #135 | P2 | rec | UI scope overfires: `ui_changed` matches every `src/app` path | **Outcome:** a change that cannot alter a rendered journey stops paying the longest job in CI. **Detail:** `uiPatterns` in `scripts/ci-change-scope.mjs` matches all of `src/app`, so an edit confined to `src/app/api/**` sets `ui_changed` and runs the full Chromium gate — measured 2026-07-30 at 15m26 of an 18m36 run, the entire critical path. **Next:** decide whether API-only diffs can be excluded. **Not done blind, deliberately:** the journeys exercise a production build that serves those routes, so a naive exclusion can hide a real regression; this needs a decision plus a compensating check, not a quieter filter. Sharding `ui-critical` (PR #1427) cut the cost of over-firing but did not remove it. **VALIDATED 2026-07-30, and it is already implemented:** `isUiChangedPath` in `scripts/ci-change-scope.mjs` already excludes `src/app/api/**`. A naive grep suggested a coverage hole — 16 of 17 API route groups appear in Playwright specs — but 92 of those references are `page.route(` / `context.route(` **stubs**, not real calls: the journeys mock the API layer, so an API handler change genuinely cannot move them. The exclusion is correct and the measurement now says so. **Stop:** do not re-open on a bare grep for `/api/` in specs; check whether the reference is an interception before concluding anything. | `scripts/ci-change-scope.mjs`; CI runs 30520443076 / 30519912667; session 2026-07-30 | 2026-07-30 | | #138 | P3 | task | CI Triage ships inert pending a repo variable | **Outcome:** a PR red that is really a main-side regression is labelled as such instead of costing an author a debugging session. **Detail:** `.github/workflows/ci-triage.yml` is complete and self-tested (`check:ci-triage` runs in `verify:cheap` and `static-pr`) but every run short-circuits on `vars.CI_TRIAGE_ENABLED == 'true'`, which is unset. Its purpose is the failure mode this doc records repeatedly: CI merges the PR branch with current `main`, so a main regression surfaces on every open PR. **Next:** operator sets the repository variable, then confirm one triage comment posts. Reads job metadata from a trusted default-branch checkout only; never runs PR code. | `.github/workflows/ci-triage.yml`; session 2026-07-30 | 2026-07-30 | -| #139 | P3 | rec | `changes` job computes outputs nothing consumes, and over-triggers coverage | **Outcome:** the change-scope contract says what it means. **Detail:** `changes` exports `source_changed`, `workflow_changed`, `changed_files` and `rag_eval_changed` and **no job reads any of them** — only `rag_eval_changed` is documented as intentionally advisory (`ci-change-scope.mjs`); the other three read as live wiring. Separately `coverage_changed` is derived as _any non-doc file_, so a workflow-only edit runs the ~4 min coverage job. **Next:** delete or document the dead outputs; decide whether coverage should narrow. Low value alone — bundle with the next `ci.yml` change rather than minting a PR. **DONE 2026-07-30:** `source_changed`, `workflow_changed` and `changed_files` are no longer job outputs. The script still computes all three — `docs_only` derives from the first two — they were simply exported and read by nothing. The coverage-breadth half is untouched and stays open. | `.github/workflows/ci.yml`; `scripts/ci-change-scope.mjs`; session 2026-07-30 | 2026-07-30 | | #146 | P2 | issue | `ui-phone-scroll` Services result anchor jumps on viewport shrink under CI load | **Outcome:** the required `Production UI` job stops failing on a test the PR did not touch. **Evidence 2026-07-30 (PR #1427 run `30534158395`, job `90844138237`, shard 1 of 3):** `tests/ui-phone-scroll.spec.ts:973` "Services results keep a continuous browser viewport after shared chrome releases" failed at `:1133` — `afterViewportResize.anchorTop` did not stay close to `hidden.anchorTop` across the viewport shrink. `120 passed (6.5m)`, classified "needs investigation". **Intermittent on unchanged code:** the spec is byte-identical across heads `79dd28f`, `76f80f3` and `17c8cb9`; shard 1 passed, passed, failed, then passed again on re-run — four data points, one failure. **Not #127 and not fixed by #1427:** this head already carried #1427's runway poll and travel assertion, and the failing assertion is result-content anchor stability across a resize, not the chrome hide. #127 is the reduced-motion hide on the document route; this is the Services result canvas. **A narrowing claimed here was WITHDRAWN 2026-07-30 (Codex, PR #1446):** an earlier version of this row argued that the sibling `documentScrollTop` assertion did not fail, so the scroll position must have held. That inference is invalid — Playwright aborts a test at the first failing `expect`, so once the `anchorTop` assertion threw, the `documentScrollTop` assertion never executed. Its absence from the output shows nothing, and scroll-restoration causes are NOT ruled out. **Next:** capture `anchorTop` and `documentScrollTop` either side of the resize from a CI-load run, and check whether the chrome release and the viewport change can interleave. **Stop:** do not add to `tests/flake-ledger.json` — that ledger takes reproduced `@quarantine` entries only, and this reproduces once in four. Do not loosen the `toBeCloseTo` tolerance; the anchor stability is the assertion's whole point. **Two further data points 2026-07-30 (PR #1457):** failed once more on head `c739340` (run `30551018893`, shard 1) with `anchorTop` expected `-138`, received `-7`, difference `131`, `120 passed`; then passed on heads `9da02d9` and `a6f2281` across all three shards with the diff byte-identical. That is six data points, two failures, still shard 1 only — and both failures landed on a PR touching nothing but `src/app/mockups/**` plus one mockup test, which strengthens the unchanged-code reading. The magnitude is worth noting for the capture suggested above: `131` is roughly 2x the 64px viewport shrink, not a sub-pixel drift. | PR #1427 run `30534158395`; session 2026-07-30 | 2026-07-30 | | #147 | P2 | rec | Mobile CLS breaches four routes, and it reproduces exactly offline | **Outcome:** mobile CLS is under 0.1 on every measured route, or each remaining breach has a recorded reason. **Measured 2026-07-30 with the LOCAL OFFLINE production harness** (`CHROME_PATH=/opt/pw-browsers/chromium npm run verify:lighthouse -- --keep`; Lighthouse 12.8.2, Chromium 141.0.7390.37, demo corpus, inert loopback Supabase). Mobile CLS: `/dsm` 0.363, `/documents/search` 0.220, `/therapy-compass` 0.142, `/` 0.023 — `/forms` did not measure locally (Lighthouse `NO_NAVSTART`, its own "run again" transient), but the live dispatch put it at 0.212. Desktop passes everywhere: 0.016–0.097. **The finding that matters is that these reproduce the live production dispatch EXACTLY.** Against run `30548662649` on `psychiatry.tools`, all four locally measurable mobile routes match to three decimals — 0.023 / 0.142 / 0.220 / 0.363 on both — and the desktop range matches at both endpoints, 0.016–0.097. So CLS on these routes is deterministic layout structure, not network, latency, or production data. Three consequences: (a) the CLS half of `#017` is debuggable and gate-able locally at zero provider cost; (b) `#017`'s recorded blocker "prod server hard-requires Supabase secrets" is stale — `scripts/run-lighthouse-budget.mjs` builds and serves an offline production app precisely to sidestep that, and its earlier note that dev-mode CLS looked "excellent at 0.00–0.04" was measuring DEV, which does not reproduce production; (c) re-dispatching the live workflow to re-measure CLS buys nothing. **LCP does NOT reproduce and must not be read from local runs:** local mobile LCP is 2014–2051 ms against 3639–4716 ms live, because the loopback server has no network latency. The LCP _ranking_ does survive — `/therapy-compass` is the outlier both locally (4978 ms, ~2.4x every other route) and live — which corroborates `#117` rather than replacing it. Mobile TBT, local: `/documents/search` 581 ms, `/dsm` 448, `/therapy-compass` 379, `/` 338. **Ranking by measured contribution, which is what `#017` asked for:** 1. `/dsm` CLS 0.363 (3.6x the threshold, the worst single number anywhere); 2. `/documents/search` 0.220; 3. `/forms` 0.212 (live only); 4. `/therapy-compass` 0.142, and separately the sole LCP outlier via `#117`; 5. `/` 0.023, which passes and needs nothing. **Next:** attribute the shifts to elements. Lighthouse's `layout-shift-elements` audit returned **zero items on every route** — the `cumulative-layout-shift` audit carries only `debugdata` — so attribution needs a driven Chromium session with a `PerformanceObserver` on `layout-shift`, reading `entry.sources[].node` and `entry.value`, against the same offline production build. Start with `/dsm`. **Gate — `#118` owns it, and this row supplies a constraint it was missing.** `#118` already tracks flipping `lighthouse-budget.json` to `enforce: true` and says not to do it "before `#117` or the baseline pins a known-slow route". These numbers make that concrete: pinning a baseline today would bake CLS 0.363 on `/dsm` in as the accepted value, so the CLS fixes must land first or the gate ratifies the breach. Separately, no baseline could have been committed from this run anyway — the grader correctly refused it as incomplete evidence because `/forms` produced no report, and that refusal was not overridden. **Reproduction trap:** without `CHROME_PATH`, `chrome-launcher` cannot find a browser in this container and every route fails; the harness reads `CHROME_PATH`/`PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH` (`run-lighthouse-budget.mjs:291`) but nothing sets either, even though `PLAYWRIGHT_BROWSERS_PATH` is set. Related to `#121`. Also seen: a truncated `progress` package in the npx cache broke Lighthouse startup entirely; clearing the npx cache entry fixed it. **Stop:** do not re-dispatch the live Web-Vitals workflow to re-measure CLS — it is reproducible offline for free. Do not commit a budget baseline from a run with a failed route. Do not treat local LCP numbers as production LCP. | `#017` live run `30548662649`; local `verify:lighthouse` 2026-07-30; `lighthouse-budget.json` | 2026-07-30 | | #149 | P2 | issue | `check:installed-lock-parity` only compares seven top-level packages, so transitive drift passes | **Outcome:** a session cannot run local gates against a `node_modules` that silently disagrees with the lockfile. **Evidence 2026-07-30:** the `SessionStart` hook reported "node_modules matches the lockfile, skipping install" while `brace-expansion` was installed at **1.1.16** and the lockfile pinned **1.1.18** — the CVE-2026-14257 patch. Every local gate run in that window was weaker than it appeared, including several `verify:cheap` runs reported as green during PR #1430/#1444 work. It surfaced only because `tests/installed-lock-parity.test.ts` asserts the patched behaviour directly (`maxLength` honoured); its failure was initially mistaken for a pre-existing repo defect, since it also failed on clean `main` **in the same stale container**. `npm ci --include=dev` fixed it. **CORRECTION 2026-07-30 (PR #1496 review, Codex):** an earlier version of this row said the parity check lives only in `verify:ui` and proposed adding it to `verify:cheap`. **Both were wrong** — `verify:cheap:internal` already runs `check:installed-lock-parity`, so that follow-up was a no-op, and it ran green throughout the stale window. The real gap is its **scope**: `scripts/check-installed-lock-parity.mjs` compares only seven top-level packages (`criticalInstalledPackages` = next, react, react-dom, eslint, playwright, typescript, vitest), so a stale **transitive** dependency is invisible to it. Proved by faking `node_modules/brace-expansion` back to `1.1.16` against the `1.1.18` lockfile pin: `npm run check:installed-lock-parity` **exits 0**. That also explains why the hook and the gate both reported parity while the tree was stale — neither was looking at the package that drifted. **Next:** broaden the check beyond the seven names — validate the full installed tree against the lockfile, or stamp the install with the lockfile hash and compare that — so transitive drift is caught rather than assumed absent. **Stop:** do not treat a green `check:installed-lock-parity` as proof the install is lockfile-current; today it means seven packages match. | `scripts/check-installed-lock-parity.mjs`; `tests/installed-lock-parity.test.ts`; `package.json` | 2026-07-30 | @@ -175,6 +174,7 @@ Move resolved rows here with the resolution date and a one-line outcome. Keep th | #148 | issue | Rows could be deleted from the issues ledger without detection | Resolved 2026-07-30. `check:outstanding-issues` now compares the current ID set with the event base SHA in CI (or the local `HEAD`/`origin/main` merge base), fails when an allocation disappears from both open and archive tables, accepts a proper archive move, and fails closed when an explicitly required base cannot be read. Static PR checkout now fetches history so the base blob is available. | 2026-07-30 | | #145 | issue | Branch syncs could repeatedly cancel healthy in-flight CI | Resolved 2026-07-30. The operator helper now queries Actions for the current head and skips `update-branch` when the required CI workflow is queued or in progress; focused tests cover queued, running, completed, and unrelated workflows. The open-PR sync, CI-speed, and canonical Run PR procedures all require one late sync after review/fix work is assembled. `cancel-in-progress` remains enabled. | 2026-07-30 | | #137 | rec | Advisory UI ran mockup tests for unrelated UI changes | Resolved 2026-07-30. `advisory_ui_changed` now runs the lane only for mockup-surface changes or when the flake ledger contains a quarantine. The scope self-test covers both directions, unreadable-ledger fail-open behavior, and parity for all five advisory specs; current-main `check:ci-scope` and `check:gate-manifest` pass. | 2026-07-30 | +| #139 | rec | Change scope exposed dead outputs and appeared to over-trigger coverage | Resolved 2026-07-30. Unconsumed `source_changed`, `workflow_changed`, and `changed_files` job outputs were removed. The remaining coverage breadth is now an explicit fail-safe policy: every non-documentation change, including workflow-only edits, runs unit coverage because those files can alter test setup or the gate itself. Self-tests pin both cases; narrower signals still scope build, UI, database, and container work. | 2026-07-30 | | ID | Type | Summary | Outcome | Resolved | | ---- | ----- | ----------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------- | diff --git a/scripts/ci-change-scope.mjs b/scripts/ci-change-scope.mjs index cd72d08340..741c6d9223 100644 --- a/scripts/ci-change-scope.mjs +++ b/scripts/ci-change-scope.mjs @@ -243,7 +243,10 @@ function classify(files, { readLedger = readFlakeLedger } = {}) { const sourceChanged = normalized.some((file) => pathMatches(file, [...sourcePatterns, ...staticConfigPatterns])); // Preserve the pre-consolidation unit gate for every non-documentation // change. Narrower signals still scope build/UI/database work, but must not - // leave runtime, worker, or configuration changes without unit coverage. + // leave runtime, worker, workflow, or configuration changes without unit + // coverage. A workflow-only edit can change test setup or the + // coverage gate itself, so its ~4 minute proof is deliberate rather than an + // accidental over-trigger (#139). const coverageChanged = normalized.some((file) => !pathMatches(file, docPatterns)); const uiChanged = normalized.some((file) => isUiChangedPath(file)); const advisoryUiChanged = @@ -682,6 +685,7 @@ function selfTest() { ); assertScope("workflow", [".github/workflows/ci.yml", "docs/process-hardening.md"], { workflow_changed: true, + coverage_changed: true, docs_only: false, build_changed: false, }); @@ -694,6 +698,9 @@ function selfTest() { assertScope("repo-skill", [".agents/skills/database-flightplan/SKILL.md"], { workflow_changed: true, source_changed: false, + // Skill Markdown is documentation-like: static policy checks still run, + // but unit coverage has no executable product surface to measure. + coverage_changed: false, docs_only: false, build_changed: false, });