diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 13e4b1469f..045467909f 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -252,5 +252,6 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-07-30 | PR #1477 | 26d713922006c1af8187994edfa76669dc14cd46 | PR #1477 fork-safe Codex autofix routing | Fixed fork routing to the PR head repository, added fail-closed metadata handling, reconciled current main, and found no remaining actionable defects. | check:codex-autofix-workflow; check:github-actions; check:pr-policy; check:outstanding-issues; check:branch-review-ledger; docs:check-inventory; docs:check-links; docs:check-scripts; typecheck; focused Vitest 53 passed; Prettier | | 2026-07-30 | PR #1477 | 20f795da2d9d0adafa6cb3117429ab3665129c0d | PR #1477 fork-safe Codex autofix routing | Refreshed onto current main after #1465; issue and ledger reconciliation remained clean and no new actionable defects were introduced. | check:outstanding-issues; check:branch-review-ledger; check:codex-autofix-workflow; focused Vitest 53 passed | | 2026-07-30 | PR #1480 | 6c1e76f53aee87be8408cebc295744fbdce05367 | PR #1480 bounded outstanding reliability fixes | Fixed both review findings: documented the dark accent role and added partial favourites retry without hiding valid counts; no other actionable defects found. | focused Vitest 119 passed; docs index; issue and ledger guards; Actions and Codex workflow guards; Prettier; diff check; typecheck coordinator-blocked | +| 2026-07-30 | claude/global-search-mockups-mrgmzl | e79e499839e56bff66aecc37d1c915ac3127e995 | prlanded | merged and verified by content | verify:cheap exit 0 (437 files, 4576 passed); verify:pr-local exit 0; CI pr-required green on a6f2281; squash e79e499 content diff vs branch tip empty; late aria-live/role=alert commit confirmed present on main | | 2026-07-30 | PR-1469 | 02108d5424f8a3ab50f45808a6cc3cbd872e7555 | PR #1469 component state matrix coverage | PASS after current-main sync; tests execute enabled and disabled popup transitions plus document search loading, empty, and fault states | focused Vitest 2 files, 10 tests passed; outstanding-issues and branch-review-ledger guards passed; no unresolved review threads | | 2026-07-30 | codex/close-pr1480-issues | bf8ac88b024642eb45d1fead86f4ee30fce3f98d | archive PR 1480 issue resolutions | approved: five resolved rows moved intact to archive | check:outstanding-issues; prettier check; diff check | diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 23b0ea40fa..c1cef43d3f 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -157,7 +157,7 @@ removed after current-main verification; it is not missing recommended work. | #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 | | #145 | P3 | issue | A branch sync during in-flight CI cancels a near-complete run, and nothing warns | **Outcome:** a behind-but-passing PR is not re-synced into a CI restart loop. **Observed 2026-07-30 on PR #1454:** three `Update-branch` merges in ~20 minutes (`d49ac0f`, `6de2bf5`, `d9598b7`) each landed while that head's CI was mid-flight, so `cancel-in-progress` cancelled `Build` + `Unit coverage` three times and restarted a full cycle each time; no run finished, so armed auto-merge never fired until the queue quietened. Every check that got time to complete passed, twice over — the PR was never broken. **Gap:** AGENTS.md's anti-churn rule ("assemble every commit before the first push, or wait for the current run to settle") addresses _your own_ pushes only. It says nothing about an operator/`update-branch` sync landing on a head whose CI is already running, which costs the same cancelled cycle and is the case that actually recurs — `#1406`'s ~40%-cancellation measurement is the same phenomenon from the push side. Distinct from `#095` (which is the _reporting_ of cancelled jobs, and worked correctly here: `PR required` said "CANCELLED with no failing job … look for a newer run on the PR's current head") and from `#129`/`#116` (genuine-vs-apparent conflict). **Next:** extend the anti-churn guidance to cover syncs — before an `update-branch`/`git merge origin/main` on an open PR, check whether that head has a run in flight and let it settle first unless the branch is genuinely blocking-behind; prefer syncing once, late, over repeatedly. **Stop:** do not "fix" this by disabling `cancel-in-progress` — superseded runs should still be cancelled; the cost is in _when_ the sync lands, not in the cancellation policy. | session 2026-07-30; PR #1454 runs `30546286820`, `30546667904`, `30547100273`; AGENTS.md § Anti-conflict and CI-speed operating procedure | 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. | PR #1427 run `30534158395`; 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 | | #148 | P2 | issue | Rows can be deleted from the issues ledger and no gate notices | **Outcome:** a row cannot leave `docs/outstanding-issues.md` without being archived, and a gate catches it if one does. **Observed 2026-07-30 on `main`:** commit `d8b034c8` (PR #1441, "guard upload limits in production builds") deleted **three** rows from the Open items table and added **zero** rows to the archive — `#085` (genuinely resolved by that same PR, which added `scripts/check-upload-limit-parity.mjs`), `#119` (a duplicate of `#122`), and `#105` (**open and unresolved** — its `LoadingPanel` half was and is unverified). Two of the three were dispositionable; none was dispositioned, and the third should not have been touched at all. The file's own Conventions section says of the archive: _"Move resolved rows here with the resolution date and a one-line outcome. Keep them — do not delete."_ There is precedent for doing it properly even for duplicates — `#140` was archived as "CLOSED as a duplicate, superseded by `#133`" rather than removed. **Why nothing caught it:** `npm run check:outstanding-issues` validates duplicate ids, an id present in both tables, a marker at or below the highest id, malformed rows, and missing headings — every shape the 2026-07-29 triple collision took (`#112`). It does **not** compare against the previous revision, so a row that simply disappears is invisible to it, in `verify:cheap` and in `static-pr` alike. **Likely mechanism, and why this file is the one it happens to:** `#133` deliberately left this file without a `merge=union` driver so overlapping edits conflict loudly instead of being silently concatenated. That is the right design, but it means every sync is a hand-resolution, and taking one side wholesale silently drops the other side's rows — exactly what AGENTS.md forbids for this file. This branch hit five consecutive genuine conflicts on it, and `#105` only survived because each resolution was done row by row and diffed with column padding normalised away afterwards. **Next:** add a deletion check — compare the id set against the merge base (or `origin/main`) and fail when an id present there is absent from both tables, unless the same commit archives it. That is a few lines in `scripts/check-outstanding-issues.mjs` and closes the one shape `#112`'s guard left open. **Stop:** do not fix this by adding a union merge driver — `#133` removed that deliberately and union merge would reintroduce the duplicate-row damage it was removed to stop. The fix is detection, not merge policy. | `main` commit `d8b034c8` (PR #1441); `scripts/check-outstanding-issues.mjs`; `#112`, `#133` | 2026-07-30 |