diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6717955398..644637bdcf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -141,6 +141,7 @@ jobs: - name: Checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: + fetch-depth: 0 persist-credentials: false # GitHub cron fires are best-effort, and the canary's own failure-issue step only reacts @@ -219,6 +220,8 @@ jobs: run: npm run check:branch-review-ledger - name: Outstanding-issues ledger integrity + env: + OUTSTANDING_ISSUES_BASE_SHA: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha || github.event.before }} run: npm run check:outstanding-issues - name: PR mergeability workflow contract diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 80239137fb..173741f869 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -261,3 +261,4 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-07-30 | PR-1475 | 6de5c321beac55860cc4b6fc7d26ef5a7e088f38 | PR #1475 ingestion behavioral extraction | PASS after current-main reconciliation; extracted decisions preserve entrypoint behavior and replace the matching source-grep assertion with executable coverage | focused Vitest 3 files, 27 tests passed; typecheck passed; outstanding-issues and branch-review-ledger guards passed; provider-backed ingestion not run | | 2026-07-30 | PR-1458 | 8c1975b178c67e4c54acffc395d85e38c43d39f5 | PR #1458 superseded root-gate reconciliation | PASS: retained only unique documentation corrections after PR #1480 landed the stronger tracked-root gate; archived resolved shared-hook issue #143 | docs index and links passed; outstanding-issues and branch-review-ledger guards passed; diff check passed | | 2026-07-30 | PR-1448 | 8ece7f345e93170c6bd242701eaff05f5504d98b | PR #1448 authenticated live workflow | PASS after review repair: protected-main-only checkout, explicit bounded mutations, scoped secrets, and static dispatch confirmation; no live provider workflow dispatched | GitHub Actions and PR-policy guards passed; focused Vitest 3 passed; docs links and scripts, issue and ledger guards, Prettier and diff checks passed | +| 2026-07-30 | codex/outstanding-deletion-guard | da1bed4bcb05f2b975823c76bab0913278cfaea6 | issue 148 deletion guard | approved: no P0-P2 findings; base comparison is fail-closed in CI and preserves archive moves | verify:cheap; deletion self-test; unreadable-base proof; CI scope; GitHub Actions pin guard | diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 760a044001..808e58dd45 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -155,7 +155,6 @@ removed after current-main verification; it is not missing recommended work. | #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. **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 | ## Resolved / archive @@ -174,6 +173,7 @@ Move resolved rows here with the resolution date and a one-line outcome. Keep th | #134 | issue | Ledger merge driver could be absent when install was skipped | Resolved by PR #1480. The branch-review-ledger guard now fails loudly when merge=ledger is declared but merge.ledger.driver is not configured, and its self-test covers the missing-driver case. | 2026-07-30 | | #141 | issue | Production --clinical-accent-strong token was undefined | Resolved by PR #1480. The token is defined for light, dark, and forced-colors themes, documented in the permanent colour direction, and protected by focused design-token tests. | 2026-07-30 | | #144 | issue | check:docs-index lacked repository-root directory coverage | Resolved by PR #1480. The checker now discovers tracked top-level directories, applies an explicit infrastructure/output allowlist, verifies both orientation maps, and reports all 49 covered roots/modules/routes plus schema tables. | 2026-07-30 | +| #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 | | ID | Type | Summary | Outcome | Resolved | | ---- | ----- | ----------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------- | diff --git a/scripts/check-outstanding-issues.mjs b/scripts/check-outstanding-issues.mjs index 72e0f20b02..cbee9982b4 100644 --- a/scripts/check-outstanding-issues.mjs +++ b/scripts/check-outstanding-issues.mjs @@ -17,6 +17,7 @@ // - an id used twice is a merge that kept both sides' rows under one number // - an id above the marker is a merge that kept a row and lost the bump // - an id in both tables is an archive move that copied instead of moving +// - an id absent from both tables relative to the base is a row deletion // - a malformed row is usually a hand-edit that broke the column count // - a row outside any table is a blank line that silently ended the table // @@ -337,6 +338,61 @@ export function checkIssues(markdown) { return problems; } +/** + * IDs that existed at the comparison base but disappeared from the current + * ledger entirely. Moving a row from open to archive keeps its allocation and + * therefore passes; deleting it from both tables does not. + */ +export function missingIssueIds(baseMarkdown, currentMarkdown) { + const numbers = (markdown) => + new Set( + parseIssues(markdown) + .rows.filter((row) => row.number !== null) + .map((row) => row.number), + ); + const baseIds = numbers(baseMarkdown); + const currentIds = numbers(currentMarkdown); + return [...baseIds].filter((number) => !currentIds.has(number)).sort((left, right) => left - right); +} + +function argumentValue(name) { + const index = process.argv.indexOf(name); + return index >= 0 ? (process.argv[index + 1] ?? "") : ""; +} + +function gitOutput(args) { + return execFileSync("git", args, { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }).trim(); +} + +/** + * Resolve the comparison commit without network access. CI supplies the event + * base explicitly; local feature branches compare with their merge base + * against the already-fetched origin/main. A checkout with neither source + * still receives all structural checks, but an explicit unreadable base fails + * closed instead of silently dropping deletion protection. + */ +function issueBaseRevision() { + const requested = argumentValue("--base-ref") || process.env.OUTSTANDING_ISSUES_BASE_SHA || ""; + if (requested && !/^0{40}$/.test(requested)) return { ref: requested, required: true }; + try { + const main = gitOutput(["rev-parse", "--verify", "refs/remotes/origin/main^{commit}"]); + return { ref: gitOutput(["merge-base", "HEAD", main]), required: false }; + } catch { + return null; + } +} + +function readIssuesAtRevision(ref) { + const commit = gitOutput(["rev-parse", "--verify", `${ref}^{commit}`]); + return execFileSync("git", ["show", `${commit}:${ISSUES_PATH}`], { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); +} + function selfTest() { const good = [ "", @@ -404,6 +460,25 @@ function selfTest() { } } if (failures > 0) process.exit(1); + + const deletionCases = [ + ["an unchanged id set", good, []], + ["an open row deleted from both tables", good.replace("| #001 | P2 | a |\n", ""), [1]], + [ + "an open row moved to the archive", + good.replace("| #001 | P2 | a |\n", "").replace("| #002 | b |", "| #002 | b |\n| #001 | a |"), + [], + ], + ["a newly allocated row", good.replace("| #001 | P2 | a |", "| #001 | P2 | a |\n| #003 | P3 | c |"), []], + ]; + for (const [name, current, expected] of deletionCases) { + const actual = missingIssueIds(good, current); + if (JSON.stringify(actual) !== JSON.stringify(expected)) { + failures += 1; + console.error(`self-test FAILED: ${name} — expected missing ${expected}, got ${actual}`); + } + } + if (failures > 0) process.exit(1); console.log("outstanding-issues self-test passed."); } @@ -452,6 +527,27 @@ function main() { } const markdown = readFileSync(ISSUES_PATH, "utf8"); const problems = checkIssues(markdown); + const base = issueBaseRevision(); + let checkedBase = null; + if (base) { + try { + const missing = missingIssueIds(readIssuesAtRevision(base.ref), markdown); + checkedBase = base.ref; + for (const number of missing) { + problems.push( + `${canonicalId(number)} existed at base ${base.ref.slice(0, 12)} but is absent from both open and archive tables — ` + + "move resolved or superseded rows to the archive; never delete their allocation", + ); + } + } catch (error) { + if (base.required) { + problems.push( + `could not read ${ISSUES_PATH} at required base ${JSON.stringify(base.ref)} — ` + + `deletion protection cannot run (${error instanceof Error ? error.message.split("\n")[0] : String(error)})`, + ); + } + } + } // A merge driver on this file is a regression, not an improvement: union // concatenated conflicting hunks and duplicated the whole table rather than // failing (#133, and four times on PR #1430). Honest conflicts are the @@ -471,7 +567,8 @@ function main() { const open = rows.filter((row) => row.table === "open").length; console.log( `Outstanding-issues guard passed: ${rows.length} rows (${open} open, ${rows.length - open} archived), ` + - `unique ids, next-id=${nextId} above the highest, no merge driver.`, + `unique ids, next-id=${nextId} above the highest, no merge driver` + + `${checkedBase ? `, no ids deleted from base ${checkedBase.slice(0, 12)}` : ", deletion baseline unavailable"}.`, ); }