chore(issues): reconcile 35 queued ledger requests - #1954
Conversation
Second serial transaction. 25 active mutations (13 done, 6 add, 6 update) plus 5 cancellation decisions. Ledger 106 open / 222 archived to 99 open / 235 archived; inbox 0 pending / 129 applied. Three of the closures queued in #1940 were cancelled by review, and the cancellations are right: #235, #237 and #238 each asked for visual or browser proof, and they were closed on executable evidence instead — proof shots, a real 320px browser pass, and product-overlay journeys are not satisfied by a docs table, a jsdom assertion, or generic Sheet unit coverage. Those three rows stay open. The other six closures applied. Verified zero live same-target collisions before applying. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017paT42ZVMf8jaLtkjFxdy5
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (37)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017paT42ZVMf8jaLtkjFxdy5
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af68b32719
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -242,6 +222,12 @@ removed after current-main verification; it is not missing recommended work. | |||
| | #328 | P2 | issue | A row can outlive its own completion — nothing closes a ledger row when its work merges | **Found during the 2026-08-12 yield review; re-confirmed on main 2026-08-13.** The then-#304 row described a ranking-snapshot freshness fuse due to trip around 2026-08-19 and sat in the recommended queue as time-critical, but its work had already landed as commit d182844 (PR #1876) — the snapshot's generatedAt and sourceRunId no longer matched anything the row said. Nothing closes a row when its work merges: `issues:done` is a manual call, and the session that ships the work is often not the session that owns the row. This is the mirror of #292, which covers duplication BEFORE work starts; this is staleness AFTER it finishes, and it is more dangerous because the row keeps advertising urgency to every session that reads the queue. **Next:** the cheapest useful guard is a periodic re-verification pass that re-measures each open row against current main and flags rows whose stated evidence no longer reproduces — several rows already carry a hand-written VERIFIED CORRECT stamp, which shows the need but does it manually and unevenly. A stronger version has the handoff skill close the row in the same commit that lands the work. **Stop:** do not auto-close on keyword match; a row can be partially delivered (#215, #231) and auto-closing those would lose real remaining work. | session 2026-08-12 ledger yield review; re-verified 2026-08-13 | 2026-08-13 | | |||
| | #329 | P2 | issue | All live mobile routes breach LCP; shared CSS delivery and JavaScript are the current bottleneck | PR #1927 is merged and deployed to Railway production at exact SHA f2abf5baf3f449a1803bedef9dc107f30b70db93. Three-sample live medians on that SHA are Documents 3374 ms, DSM 3961 ms, Forms 3507 ms, root 3819 ms, Therapy 3422 ms, and Services 3793 ms; desktop LCP is 580-679 ms and mobile CLS remains within the rule. The production CSS split is retained and reduced four canonical medians modestly, but every mobile route still breaches 2500 ms. Root trace attribution is now concrete: TTFB 283 ms, LCP render delay 3449 ms, the 46,724-byte transferred shared stylesheet completes at 3644 ms under the throttled critical-request contention, total main-thread work is 1785 ms, script evaluation is 1030 ms, and shared chunk 8322 alone consumes 870 ms CPU. This is separate from canonical #117, which continues to track the unresolved Therapy catalogue payload and per-field safety decision. Next: split the 4,251-line global stylesheet by route ownership and reduce the shared search-shell/root client boundary before repeating the same bounded live matrix. Therapy field safety review remains required for search/pathways. INP remains unverified because Lighthouse does not measure it and no usable CrUX result exists. Stop: do not strip clinical fields, weaken the Lighthouse budget, refresh a passing baseline to hide latency, or claim an INP pass. | PR #1927; Railway deployments 1224ed55-210d-443b-94e5-20f87475468c and 810cc8b3-e39a-493f-b18f-8c63d150d53f; live Web Vitals runs 31719448766 and 31719451951; PR #1933 review | 2026-08-13 | | |||
| | #330 | P2 | task | Re-land PR #1800 (fuzzy catalogue search), applying the #310 one-edit cap in the same commit | PR #1800 squash-merged as 022c83b on 2026-08-10 and its entire content is absent from main: git show origin/main:src/lib/catalog-search.ts \| grep -c typoDistanceLimit returns 0, eight of its 11 source and test files are byte-identical to their pre-#1800 state. The remaining three (`src/components/therapy-compass/data/select.ts`, `src/lib/formulation.ts`, and `tests/formulation.test.ts`) contain later unrelated changes, but the fuzzy-search hunks are absent from them too; preserve those newer changes during the re-land. Cause and evidence in the merge-loss detector row filed alongside this one. Consequence today is a MISSING FEATURE, not a live hazard: because the matcher is gone, the #310 cross-drug defect is not reachable on main. Do not close #310 on that basis, and do not re-land #1800 unchanged. RE-LAND WITH THE FIX: #310 measured that the tier term.length >= 8 -> 2 edits is the problem, because Damerau scores an adjacent transposition as one edit, so fluoxetine to duloxetine is distance 2 and both are ten characters. Re-run 2026-08-13 against the algorithm confirms it, and confirms prednisone to prednisolone as the second real cross-drug hit. Capping that tier at 1 edit removes both while preserving sertraline to sertralin style recovery. The row's other claims also held on re-run: citalopram and escitalopram do not fuzzy-match, because the substring guard fires first, and clozapine/clonazepam and quetiapine/olanzapine are correctly out of range. Next: cherry-pick 022c83b onto current main, change typoDistanceLimit's >= 8 tier from 2 to 1, and add a test over real catalogue drug names with both the exact and the near-match record present, asserting the wrong drug is excluded while the exact drug remains. Gate: focused Vitest on `tests/catalog-search.test.ts` plus the other four test files #1800 touched. Stop: this path is clinicalRisk true under classifyPullRequestFiles because catalog-search.ts feeds medications.ts and prescribing, so the PR needs a complete Clinical Governance Preflight and must not be bundled with unrelated chores. ragRanking is correctly false; this is catalogue ranking, not pgvector retrieval. | session 2026-08-13; 022c83b; origin/main at 63526ee; row #310; algorithm re-run locally against real drug-name pairs | 2026-08-13 | | |||
| | #331 | P2 | issue | check:medication-lexicon-report fails on 3 independent branches despite zero diff on the flagged file or its inputs | Reproduced identically across three independently-authored branches on 2026-08-14 (PR #1947 archive-backfill-scripts, PR #1949 visual-layout-polish, PR #1950 search-round-trip-budget) during otherwise-unrelated verify:pr-local runs. Each session confirmed via git diff origin/main --name-only that docs/medication-interaction-lexicon-review.md and its generator inputs (src/lib/medication-interaction-lexicon, the medication snapshot, the medication interaction index) were untouched on their branch, yet check:medication-lexicon-report still reported the file stale. This is a tooling/process finding distinct from #1bfaf0ef (the lexicon's clinical content has never been signed off) -- this row is about the staleness CHECK itself firing on unchanged files, which suggests a bug in how the generator's staleness comparison works (timestamp vs content hash, or a comparison against the wrong base) rather than a real content drift. Next: investigate scripts/medications-lexicon-report.mjs (or equivalent) staleness-detection logic directly against origin/main; if it is a comparison bug, fix it; if the report genuinely is stale on main independent of these branches, regenerate it. Stop: do not treat repeated non-fixes of this check across unrelated PRs as acceptable long-term -- three independent confirmations is enough to act on. | PR #1947, PR #1949, PR #1950 verify:pr-local runs, 2026-08-14 | 2026-08-14 | | |||
| | #332 | P3 | task | Three mode-nav icon glyphs sit at 17px, off the --spacing-icon-* scale, and no gate flags them | Split out of #275 rather than folded into its badge-box token. mode-nav/mode-nav.tsx:64 and :214 and mode-nav/nav-slot-ink.tsx:44 size their <Icon> with h-[1.0625rem] w-[1.0625rem] — 17px against an icon scale of 12/14/16/20/24 (--spacing-icon-xs..xl in the globals.css @theme block). #275 counted these among its five files because they share the badge's number, but they are a different role: the badge is a text-bearing box sized around its own --text-2xs numeral, these are glyphs. They are now the only consumers of that value, since the badge moved to --spacing-search-band-badge. Nothing gates this: check-icon-scale.mjs enforces only the retired 4.5 (18px) half-step and its header states it deliberately does NOT flag arbitrary h-[Nrem], because non-icon boxes legitimately use that form. So this is unguarded and will not self-report. Why it was not just fixed: snapping to size-icon-md (16px) or size-icon-lg (20px) visibly changes nav chrome at every breakpoint, and 17px is close enough to 16 that the choice looks arbitrary without seeing it rendered — a design call, not a token swap. Next: get a Chromium look at mode-nav at phone and desktop widths with the icon at 16 and at 20, pick one, then migrate all three together. If 17px turns out to be deliberate, say so in a comment at the call site and consider whether check:icon-scale should flag off-scale arbitrary icon sizes on <Icon>-typed elements specifically, which would have surfaced this. Stop: do not add a 17px step to --spacing-icon-* to make the problem go away — that token block's own comment argues against widening the scale off the 4px grid, and it would sanction the drift rather than resolve it. | session 2026-08-14; split from #275; check-icon-scale.mjs header | 2026-08-14 | | |||
| | #333 | P2 | issue | check:medication-lexicon-report has been failing on main for every local verify:pr-local, and no CI job runs it | Found 2026-08-14 while running the PR preflight for an unrelated design-token change. 'npm run check:medication-lexicon-report' reports 'docs/medication-interaction-lexicon-review.md is stale. Run npm run medications:lexicon-report and commit the result.' and exits 1. Two things make this worth a row rather than a quick fix in a passing PR. FIRST, it is on main, not on any branch: reproduced in a clean worktree checked out at pristine origin/main (both d47aa6d and, after a merge, 79b01b3), with a diff touching zero medication, lexicon or data/ files. SECOND, and this is the part that explains why it went unnoticed, NOTHING IN CI RUNS IT — a grep for medication-lexicon-report across .github/workflows/ returns nothing. It is reached only through the local verify:pr-local chain, where it is the LAST step, so it fails every local PR preflight while every CI run stays green. The failure mode is therefore self-concealing in the direction that matters: the gate is invisible to the required checks and visible only to whoever is about to hand off, who then has to decide whether an unrelated stale generated doc is theirs to fix. It was not fixed in the design-token PR that found it, deliberately: the report is a clinical-facing generated document and regenerating it inside a CSS-token PR would bundle a clinical-risk artefact with unrelated chores, which AGENTS.md PR bundling explicitly forbids. Next: run 'npm run medications:lexicon-report', read the resulting diff to confirm it is a pure regeneration and not a content change needing clinical review, and commit it in its own PR. Then decide the real question this exposes — either wire the check into CI so it cannot silently rot again, or move it out of verify:pr-local so it stops failing preflights it does not gate. A check in the local chain but not in CI is the worst of both. Stop: do not simply delete the check or drop it from verify:pr-local to get a green preflight; the staleness is real and the generated file is a clinical artefact. | session 2026-08-14; PR #1942 preflight; reproduced on pristine origin/main d47aa6d and 79b01b3; grep over .github/workflows | 2026-08-14 | | |||
There was a problem hiding this comment.
Consolidate the duplicate medication-report rows
When operators pick up this report-staleness work, this row duplicates #331 two lines above: both track the same check:medication-lexicon-report failure on an unchanged main tree—the failure also reproduces locally—but they create separate IDs and prescribe competing initial diagnoses/remedies. Keep one row and merge the CI-wiring and regeneration evidence so the same defect cannot be scheduled and closed twice.
AGENTS.md reference: AGENTS.md:L895-L902
Useful? React with 👍 / 👎.
| | #332 | P3 | task | Three mode-nav icon glyphs sit at 17px, off the --spacing-icon-* scale, and no gate flags them | Split out of #275 rather than folded into its badge-box token. mode-nav/mode-nav.tsx:64 and :214 and mode-nav/nav-slot-ink.tsx:44 size their <Icon> with h-[1.0625rem] w-[1.0625rem] — 17px against an icon scale of 12/14/16/20/24 (--spacing-icon-xs..xl in the globals.css @theme block). #275 counted these among its five files because they share the badge's number, but they are a different role: the badge is a text-bearing box sized around its own --text-2xs numeral, these are glyphs. They are now the only consumers of that value, since the badge moved to --spacing-search-band-badge. Nothing gates this: check-icon-scale.mjs enforces only the retired 4.5 (18px) half-step and its header states it deliberately does NOT flag arbitrary h-[Nrem], because non-icon boxes legitimately use that form. So this is unguarded and will not self-report. Why it was not just fixed: snapping to size-icon-md (16px) or size-icon-lg (20px) visibly changes nav chrome at every breakpoint, and 17px is close enough to 16 that the choice looks arbitrary without seeing it rendered — a design call, not a token swap. Next: get a Chromium look at mode-nav at phone and desktop widths with the icon at 16 and at 20, pick one, then migrate all three together. If 17px turns out to be deliberate, say so in a comment at the call site and consider whether check:icon-scale should flag off-scale arbitrary icon sizes on <Icon>-typed elements specifically, which would have surfaced this. Stop: do not add a 17px step to --spacing-icon-* to make the problem go away — that token block's own comment argues against widening the scale off the 4px grid, and it would sanction the drift rather than resolve it. | session 2026-08-14; split from #275; check-icon-scale.mjs header | 2026-08-14 | | ||
| | #333 | P2 | issue | check:medication-lexicon-report has been failing on main for every local verify:pr-local, and no CI job runs it | Found 2026-08-14 while running the PR preflight for an unrelated design-token change. 'npm run check:medication-lexicon-report' reports 'docs/medication-interaction-lexicon-review.md is stale. Run npm run medications:lexicon-report and commit the result.' and exits 1. Two things make this worth a row rather than a quick fix in a passing PR. FIRST, it is on main, not on any branch: reproduced in a clean worktree checked out at pristine origin/main (both d47aa6d and, after a merge, 79b01b3), with a diff touching zero medication, lexicon or data/ files. SECOND, and this is the part that explains why it went unnoticed, NOTHING IN CI RUNS IT — a grep for medication-lexicon-report across .github/workflows/ returns nothing. It is reached only through the local verify:pr-local chain, where it is the LAST step, so it fails every local PR preflight while every CI run stays green. The failure mode is therefore self-concealing in the direction that matters: the gate is invisible to the required checks and visible only to whoever is about to hand off, who then has to decide whether an unrelated stale generated doc is theirs to fix. It was not fixed in the design-token PR that found it, deliberately: the report is a clinical-facing generated document and regenerating it inside a CSS-token PR would bundle a clinical-risk artefact with unrelated chores, which AGENTS.md PR bundling explicitly forbids. Next: run 'npm run medications:lexicon-report', read the resulting diff to confirm it is a pure regeneration and not a content change needing clinical review, and commit it in its own PR. Then decide the real question this exposes — either wire the check into CI so it cannot silently rot again, or move it out of verify:pr-local so it stops failing preflights it does not gate. A check in the local chain but not in CI is the worst of both. Stop: do not simply delete the check or drop it from verify:pr-local to get a green preflight; the staleness is real and the generated file is a clinical artefact. | session 2026-08-14; PR #1942 preflight; reproduced on pristine origin/main d47aa6d and 79b01b3; grep over .github/workflows | 2026-08-14 | | ||
| | #334 | P3 | issue | Claude Code web containers can ship Node 22 with no node_modules, so npm ci fails engine-strict before any work starts | Hit 2026-08-14 at the start of a Claude Code on the web session, and it blocks a session completely until worked around, so it is worth recording even though the cause is the container image rather than this repo. The container provided /opt/node20, /opt/node21 and /opt/node22 with node22 on PATH, no nvm, and no node_modules in either the primary checkout or a fresh worktree. package.json requires node >=24.15.0 <25 with engine-strict, so 'npm ci --include=dev' aborts immediately with 'notsup Required: {node: >=24.15.0 <25, npm: 11.x} Actual: {npm: 10.9.7, node: v22.22.2}'. Nothing in the repo can fix this from inside, because the failure happens before any repo script can run — .nvmrc correctly says 24 and is simply not consulted, and there is no nvm for it to drive. Workaround used, which took about a minute and is safe: fetch the current 24.x from the nodejs.org dist index, untar to /opt/node24, and prefix subsequent commands with 'export PATH=/opt/node24/bin:/opt/node24/bin:/root/.local/bin:/root/.cargo/bin:/usr/local/go/bin:/opt/node22/bin:/opt/maven/bin:/opt/gradle/bin:/opt/rbenv/bin:/root/.bun/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin'. Everything downstream then behaved normally — npm ci, the full unit suite, build, and the Playwright-free gates all passed. Worth knowing that this is a DIFFERENT surface from the Codex Cloud provisioning path: scripts/setup-codex-cloud.sh and scripts/setup-codex-worktree.mjs cover Codex, and docs/codex-cloud.md is explicit that Cloud mirrors the tracked toolchain, but neither runs for a Claude Code web session, so that hardening does not carry over. Next: decide whether this deserves repo-side help at all. Options are a short note in the AGENTS.md or CLAUDE.md orientation telling an agent to install Node 24 to /opt/node24 and re-export PATH rather than concluding the environment is broken, or a small bootstrap script equivalent to the Codex ones that a web session can run first. Prefer the note: a bootstrap script that downloads a runtime is a bigger surface than the problem. Stop: do not relax the engines range, drop engine-strict, or pass --force to get npm ci through — the Node 24 floor is enforced deliberately in several places (preinstall, check:runtime, scripts/dev-free-port.mjs) and loosening it to accommodate a bad container would disable a real guard. | session 2026-08-14; Claude Code web container for PR #1942 | 2026-08-14 | | ||
| | #335 | P2 | rec | Merge-loss detection covers file-level reverts and inbox-request loss separately; neither covers the other, and the scheduled run is undecided | **Outcome:** one decision about how merge loss is detected on this repo, rather than two half-overlapping checks and an undecided schedule. **Detail.** Two detectors now exist for the same underlying hazard — content that reached main and then stopped being there — and they measure different things. (1) PR #1944 added scripts/audit-merge-loss.mjs (npm run audit:merge-loss): for every PR landing on origin/main in a bounded window it compares the ref's current blob for each file that landing changed against the blob at the landing's first parent, so it catches a landing whose CONTENT was reverted by a later merge resolution. Validated by independently rediscovering the acf78bf casualties (#1803 with 53 files, #1800, #1804, #1796, #1811). (2) PR #1937 filed a request about a queued inbox request that existed on a branch and never reached main through that branch's squash — a file that never landed at all, which detector (1) cannot see, because it only ever examines what a landing actually contributed. Conversely #1937's own cancel request warns that comparing all historical branch additions against the squash produces FALSE losses when a PR deliberately removes a file during review; detector (1) avoids that by construction (it diffs merge^1 against merge, not the branch's whole history), which is worth reusing rather than rediscovering. **Three things to decide, ideally together.** (a) Whether detector (1) gets a scheduled or post-merge run. PR #1944 deliberately shipped script-plus-test only: scheduling is an operational change needing its own PR and explicit approval, and joining verify:cheap:internal would force a matching static-pr step in ci.yml via check-gate-manifest. Until something runs it, it only helps whoever remembers to type it. (b) Whether the branch-versus-squash case becomes a second check or a mode of the same script. (c) What a positive costs a human: detector (1) is advisory and exits 0 on purpose, because a deliberate revert is byte-identical to an accidental one at blob level — a scheduled run therefore needs a named owner to triage it, or it becomes ignorable noise. **Next:** decide (a) first; it is the cheapest and it is what turns an existing script into an actual control. **Stop:** do not make either detector auto-fail without deciding (c) — an advisory check flipped to blocking on a signal that cannot distinguish intent will be silenced rather than triaged. | PR #1944 (scripts/audit-merge-loss.mjs); PR #1937 and its cancel request 63419f06; inbox request 829597d4; acf78bf; session 2026-08-14 | 2026-08-14 | |
There was a problem hiding this comment.
Remove the merge-loss work based on cancelled evidence
When planning merge-loss hardening, #335 treats PR #1937 as evidence that an inbox request vanished during squash and asks for a second detector mode, but this same reconciliation applies cancellation 63419f06-c12a-4a84-a684-6e177f527365, which establishes that the request was deliberately removed from the final PR head and that the historical-additions comparison was a false loss. Retain the independently supported scheduling/ownership question for the existing detector, but do not carry forward the branch-versus-squash work without a verified final-head-to-squash mismatch.
AGENTS.md reference: AGENTS.md:L895-L902
Useful? React with 👍 / 👎.
Summary
docs— the second serial reconciliation, draining the inbox that refilled after chore(issues): reconcile 75 queued ledger requests #1936. 35 pending requests: 25 active mutations (13done, 6add, 6update) plus 5 cancellation decisions.planRequestBatchdedupes on request UUID rather than target row id, so this check is the operator's job, not the tool's.Three of my own closures were cancelled by review, and the cancellations are correct
PR #1940 queued nine archives. Six applied. Three were cancelled, each with a reason that identifies a real error in how I verified them — all three rows asked for visual or browser proof, and I closed them on executable evidence:
#235ADOPTION.md§7.1's evidence table#237AccessibleTabledensities in a real 320px browser#238tests/sheet.dom.test.tsx#235is the sharpest of the three: the very section I cited as evidence opens with "This PR records executable evidence rather than committing image baselines." I quoted that sentence and still called the row delivered. All three rows stay open, correctly.Worth stating because it is the point of the cancellation mechanism: nothing here was lost or overwritten. The rejected closures survive as immutable records alongside the cancellations that overruled them, so the disagreement is auditable rather than silently resolved.
Verification
npm run verify:pr-localcheck:ledger-write-disciplineinitially refused to report at all, because it compares two committed refs and the reconciliation was still uncommitted — a pass there would have been meaningless. It was re-run after the commit; the line above is from that run.A
--dry-runpreceded the write and reported the same35 request(s) … 5 cancellation decision(s).UI verification not run: no UI, routing, styling, or browser-behaviour change in this diff.
Risk and rollout
checkIssuesbefore writing, and by every request being preserved underapplied/rather than deleted.git revertthe single commit — that restores both the canonical ledger and the pending inbox, since request files are moved rather than destroyed.docs/outstanding-issues-inbox/README.md:13).Notes
## Clinical Governance Preflightsection:classifyPullRequestFilesreturnsclinicalRisk: false,operationalRisk: false,ragRanking: false,ui: falsefordocs/outstanding-issues.mdanddocs/outstanding-issues-inbox/**.ISSUES-LIST.html) is not refreshed here — that runs from a Windows PowerShell script on the operator's machine. Per theissuesskill, a stale visual artifact must not invalidate a successful canonical transaction: the Markdown source is current, the HTML artifact is stale until refreshed locally.Generated by Claude Code