docs: record PR #1649 ledger row and capture its cross-tool enforcement gap - #1670
Merged
Conversation
…nt gap Two append-only docs items deliberately held back from PR #1649 so a second commit could not cancel that branch's in-flight CI. - `docs/branch-review-ledger.md`: the missing review record for the pr-handoff-stop hook. Recorded against the squash-merge commit 76169eb rather than the branch tip 2ad32de, which is unreachable now that the merged branch is deleted and would fail the ledger's head-resolution guard. - `docs/outstanding-issues.md` `#258`: the hook is registered in `.claude/settings.json`, so its deny list binds Claude Code only. Codex and Cursor sessions get the AGENTS.md prose and no gate — which is what was already in force, and already insufficient, before #1649. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xg42EYSHXXyAXUJewzYxHA
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughThe changes update review and issue-tracking documentation. They add branch review and merge records for multiple pull requests, document PR ChangesReview documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/branch-review-ledger.md`:
- Line 688: Update the ledger row for PR `#1649` to name all six repository gate
scripts and their results, explicitly record the installed Playwright version
mismatch, and state that UI verification was not run. Also state whether any
provider-backed checks ran; preserve the existing verification details while
making the record clearly incomplete rather than clean.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b9e87b2-70df-435c-8af5-5d4a53679185
📒 Files selected for processing (2)
docs/branch-review-ledger.mddocs/outstanding-issues.md
BigSimmo
enabled auto-merge (squash)
August 7, 2026 06:28
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
CodeRabbit asked that the PR #1649 late ledger checks cell list the six named repo scripts and explicitly record incomplete handoff coverage (verify:pr-local / verify:ui not run; no provider-backed checks) instead of the opaque "6 repo gate scripts green" summary. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…s, and a second lock-parity reproduction Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xg42EYSHXXyAXUJewzYxHA
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Owner
Author
PR #1670 review-and-fix summaryTip: Findings
Threads
Required CI (this tip) — green
Local gates (decisive lines)
Residual risks
Merge left to you. |
…w sweep Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xg42EYSHXXyAXUJewzYxHA
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/branch-review-ledger.md`:
- Line 700: Update the verification summary in the ledger entry for PR `#1670` to
record the skipped test explicitly, replacing the shorthand “519 files/5493
passed” with the exact captured gate output or “519 test files passed; 5,493
tests passed; 1 skipped.”
In `@docs/outstanding-issues.md`:
- Around line 299-303: Update the recommended execution queue in
docs/outstanding-issues.md to include open issues `#258` and `#260` after `#257`,
preserving the queue’s ordered-row invariant and recording each issue’s acuity,
capability, trigger, and stopping condition. Alternatively, explicitly revise
the queue invariant so newly added open ledger rows cannot be omitted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90594f6e-ade7-4ce5-8216-5821fbb5046f
📒 Files selected for processing (2)
docs/branch-review-ledger.mddocs/outstanding-issues.md
| | 2026-08-06 | cursor/grok-quick-wins-a2c0 (PR #1651) | 5d625d3e64752df0655c071b061642cfbbe4ea5f | Run PR sweep: CI fix + threads + drift | before: Sentry+Devin threads (double-zoom, issues:done, TOKENS px) + stale queue renumber; after: fixed in 38b7f8e4+5d625d3e; 5 threads resolved; CI queued on runners; merge-tree clean | vitest gestures+hide-on-scroll 35 passed; outstanding-issues gate+writer self-test passed; no provider-backed checks run | | ||
| | 2026-08-07 | cursor/grok-quick-wins-a2c0 (PR #1651) | 2563b4adf4f0c0580ca371acfee94f9ae1ca3d0f | prlanded | MERGED; squash tip empty vs branch tip 4655643770dba7ca4078f1586c0461fd91657bf9; Grok-safe quick wins (tokens, a11y, perf, gates) | content tree empty vs squash; no provider-backed checks run | | ||
| | 2026-08-07 | cursor/viewer-phase0-gesture-a11y-1db8 (PR #1660) | 097dfd245f798f8105eeb6c1cf4fc077f969496f | prlanded | MERGED; squash tip empty vs branch tip 810cfc9b4a1c476a0dcc995bffb00d7329a85686; Phase 0 gesture INP, preview a11y, image decode | content tree empty vs squash; no provider-backed checks run | | ||
| | 2026-08-07 | claude/pr-handoff-loop-prevention-54y5zr (PR #1670) | dfe2946110e0ff93bd4acc571ae79c26b79a7a85 | PR #1670 heavy review-and-fix | synced origin/main (behind-but-clean DIRTY cleared); fixed CodeRabbit checks-cell to name exact #1649 gates + incomplete verify:pr-local/ui + no provider checks; Bugbot none; no P0/P1; #258/#ledger delta accurate; merge-tree clean; threads cleared | verify:cheap 519 files/5493 passed; verify:pr-local docs scope (format+docs+ledger+outstanding-issues); check:branch-review-ledger; check:outstanding-issues; no provider gates | |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Record the skipped test and decisive verification output.
Line 700 reports 519 files/5493 passed, but the verification result also included one skipped test. Replace this shorthand with the exact captured gate output, or record 519 test files passed; 5,493 tests passed; 1 skipped. This prevents the ledger from appearing to record a complete pass.
As per coding guidelines, verification records must include the decisive gate output line. The PR verification summary records one skipped test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/branch-review-ledger.md` at line 700, Update the verification summary in
the ledger entry for PR `#1670` to record the skipped test explicitly, replacing
the shorthand “519 files/5493 passed” with the exact captured gate output or
“519 test files passed; 5,493 tests passed; 1 skipped.”
Source: Coding guidelines
Comment on lines
+299
to
+303
| | #255 | P2 | issue | Remote/Cloud containers cannot run any browser gate: Playwright lock drift plus a missing Chromium build | In the 2026-08-05 remote container, npm run verify:pr-local failed closed at its first precondition — 'check:installed-lock-parity ... playwright: installed 1.62.0 does not match locked 1.62.1' — and Chromium could not launch at all: 'Executable doesn't exist at /opt/pw-browsers/chromium_headless_shell-1234/...' while the image only ships chromium-1194 and chromium_headless_shell-1194. So verify:ui, verify:phone-chrome and verify:release are all unrunnable in a remote session regardless of the diff, and verify:pr-local cannot even reach its selected gates. This is not a one-off 'run npm ci': the installed Playwright disagrees with the lockfile, and installing the locked 1.62.1 would request a browser revision the image also does not carry, so the fix is an image/pin alignment rather than an install. Consequence worth writing down: PRs authored from remote sessions can only produce static and unit evidence locally and must delegate browser proof to CI's Production UI jobs, which is what #1620/#1621/#1622 did — #1621's Chromium evidence came entirely from CI (0 failed of 17, all three Production UI shards). Next action: align the container image's Playwright browser builds with the lockfile pin, or document the delegation explicitly in docs/testing.md so a remote session does not read a fail-closed parity error as a defect in its own change. Related but distinct from #149, which is about the parity checker only comparing seven top-level packages rather than about an actual blocking drift. Stop: do not work around it by setting PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD or pointing executablePath at the mismatched 1194 build to force a green browser run — a browser gate run against the wrong revision is not evidence. Renumbered from this PR's original #252 → #255 because main claimed #249–#251 via PR #1624. **Second reproduction 2026-08-07 (remote container, PR #1670):** identical parity failure, same versions — 'playwright: installed 1.62.0 does not match locked 1.62.1' — two days later on a different image instance, so this is a standing property of the remote image rather than one bad container. New detail: the diff was docs-only (two append-only ledger files), and parity still halted `verify:pr-local` at step 2 of a 10-step plan that contained no browser gate at all. So the blast radius is wider than browser proof: a remote session cannot complete the PR-local gate for ANY scope, and has to re-run the selected steps by hand to get evidence (done there: the remaining eight all passed). That strengthens the documentation half of the next action — docs/testing.md should say a remote session may see this on a diff that never touches a browser. | session 2026-08-05 open-PR review sweep; verify:pr-local and verify:phone-chrome attempts on PRs #1620/#1621 | 2026-08-05 | | ||
| | #256 | P2 | issue | Declared information-page section sets whose target ids nothing renders | A set in informationPageSectionDefinitions claims its route before the mode branch, then AvailableInformationPageNavigation drops every item whose targetIds are absent from the DOM and returns null, so the page shows no navigation at all. Proven live on PR #1647: specifierSections/formulationSections declared ten ids (specifier-overview, formulation-five-ps, and eight more) that no component rendered; invisible while those pages carried their own subnav, a dead end the moment it moved to the shared bar. Fixed for those two by wiring the anchors in. Suspected remaining: form-overview and the differential-presentation-* set returned no literal id= match. Next: verify each declared set at route level and either render the anchors or delete the set. Do NOT audit by grepping for id=: several live sections are rendered through a sectionId prop instead (source-evidence), so a source scan reports them dead and would have to be muzzled to pass — assert against the rendered DOM per route. | session 2026-08-06; PR #1647; Devin review finding | 2026-08-06 | | ||
| | #257 | P3 | issue | Single unreproduced ui-formulation flake: keeps specifier and formulation route families clinically separate | Observed once on 2026-08-06 at PR #1647 head f5833acc, running tests/ui-formulation.spec.ts + tests/ui-specifiers.spec.ts together against local Chromium (1 failed, 11 passed). Did NOT reproduce: passed in isolation with --grep, and passed again on a full-file re-run (7/7). Recorded only so a second sighting is recognisable as a second rather than looking like a first. Per docs/testing.md this is one reproduction of three — do NOT quarantine, and do not weaken the assertion. Next: no action unless it recurs; if a second reproduction lands on the same SHA, note it here, and only on a third open a tests/flake-ledger.json entry with @quarantine and a <=30-day expiry. | session 2026-08-06; PR #1647 | 2026-08-06 | | ||
| | #258 | P2 | rec | The PR-handoff stop rule is enforced for Claude Code only; Codex and Cursor get prose with no gate | **Outcome:** a session that opens a PR stops following it in every agent this repo supports, not just Claude Code. **Detail:** PR #1649 added `.claude/hooks/pr-handoff-stop.sh` plus the AGENTS.md "Stop when the pull request is open" section. The hook is registered in `.claude/settings.json`, which only Claude Code reads, so the PostToolUse marker and the PreToolUse denials (shell `gh pr checks/status/view/run watch`, GitHub MCP tools named pull_request/workflow_run/workflow_job/check_run/check_suite/job_log/update_branch, and Monitor/ScheduleWakeup/CronCreate) simply do not exist for Codex or Cursor sessions. Those agents get the AGENTS.md prose and nothing else — and prose alone is exactly what was already in force, and already insufficient, before #1649. Cost is the same long tail of post-handoff CI polling the hook was built to cut, just relocated to whichever agent lacks the gate; a cloud Codex session is the worst case because nothing naturally ends it. **Next:** cheapest first — check whether Codex and Cursor expose any pre-tool interception this repo can register (Codex plugin hooks under `plugins/clinical-kb/`, Cursor rules under `.cursor/`); if neither offers a deny path, the fallback is a shared marker file plus a wrapper the agent is told to route `gh` through, which is weaker but still detectable. If no mechanism exists at all, record that explicitly here so the gap is a known limit rather than an open task. **Stop:** do not weaken the Claude Code hook to make the tools symmetric, and do not add a second copy of the deny list — one script, multiple registrations. | PR #1649; .claude/hooks/pr-handoff-stop.sh; .claude/settings.json; AGENTS.md "Stop when the pull request is open"; session 2026-08-07 | 2026-08-07 | | ||
| | #260 | P2 | task | Two unpushed Sentry commits are stranded on a Windows-only branch and will be lost with that machine | **Outcome:** the Sentry setup/logging-hardening work is either shipped or consciously discarded, not left sitting in one machine's reflog. **Detail:** `claude/cloud-pr-loop-prevention-bc052b` carries two commits — `c3c9d6a31` and `abbcdc8e9`, ~389 lines across `src/sentry.*.config.ts`, `src/lib/env.ts`, `src/lib/supabase/client.tsx`, `src/components/ui-primitives.tsx` — that were never pushed and are not the authoring session's own work. The branch does not exist on the remote, so the commits are unreachable from any cloud or remote container; a 2026-08-07 remote session could not inspect, verify, or ship them and could only record their existence. The same worktree (`.claude/worktrees/pensive-borg-6be2f0`) still holds the same four files uncommitted. Two Sentry branches DO exist on origin — `claude/sentry-nextjs-sdk-setup-2v24q5` and `cursor/sentry-nextjs-sdk-7cee` — but whether either already carries this change is unconfirmed: a three-dot diff against `origin/main` from the remote container returned empty for both, which is not trustworthy as proof either way and was not pursued further. Note this touches `src/lib/env.ts` and `src/lib/supabase/client.tsx`, so it is not a docs-class change and needs a real gate whenever it does ship. **Next:** from the Windows machine, diff those two commits against the two remote Sentry branches to decide whether the work is already represented. If it is, delete the branch; if it is not, push it and open a PR rather than leaving it local. **Stop:** do not discard the commits blind, and do not assume the remote Sentry branches supersede them without a content diff — nothing has yet compared them. | session 2026-08-07 remote container; handoff notes from the PR #1649 session; origin branches claude/sentry-nextjs-sdk-setup-2v24q5 and cursor/sentry-nextjs-sdk-7cee | 2026-08-07 | |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add the new open issues to the recommended execution queue.
The queue is defined as the ordered view of every open ledger row. It currently ends at #257, but this change adds open issues #258 and #260. Add both rows to the queue with their acuity, capability, trigger, and stopping condition, or revise the queue invariant. Otherwise, readers can miss newly recorded P2 work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/outstanding-issues.md` around lines 299 - 303, Update the recommended
execution queue in docs/outstanding-issues.md to include open issues `#258` and
`#260` after `#257`, preserving the queue’s ordered-row invariant and recording each
issue’s acuity, capability, trigger, and stopping condition. Alternatively,
explicitly revise the queue invariant so newly added open ledger rows cannot be
omitted.
12 tasks
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two append-only docs items that were deliberately held back from PR #1649, because a second commit pushed to that branch would have cancelled its in-flight CI. #1649 has since squash-merged as
76169eb, so they can land together here as their own low-risk PR — the bundling case AGENTS.md describes for queued ledger/outstanding-issues appends.docs/branch-review-ledger.md— the missing review record for thepr-handoff-stophook branch. It is recorded against the squash-merge commit76169ebcea48ca5efd9859b9a1f8c579dcc8b834rather than the original branch tip2ad32de9b: the merged branch is deleted, so that tip is no longer resolvable and would fail the ledger's head-resolution guard. The row says so explicitly, along with what was and was not verified on that branch (verify:pr-localwas not run there — nonode_modulesin that worktree).docs/outstanding-issues.md#258(P2, rec) — the hook is registered in.claude/settings.json, which only Claude Code reads. Its deny list therefore binds Claude Code alone; Codex and Cursor sessions get the AGENTS.md "Stop when the pull request is open" prose and no gate, which is what was already in force and already insufficient before feat(hooks): stop a session following its own PR after handoff #1649. The entry records the outcome to aim for, the cheapest next step (look for a pre-tool interception point in the Codex plugin or.cursor/config; fall back to a shared marker plus a wrappedgh; failing both, record it as a known limit rather than an open task), and a stop rule against weakening the Claude Code hook for symmetry or duplicating the deny list.No source, no schema, no workflow files. Both files were written with their own writers (
npm run ledger:append,scripts/outstanding-issues.mjs), not by hand.Verification
npm run verify:pr-localRan with the docs-only route it selected. One step fails for an environment reason unrelated to this diff — reported rather than worked around:
That halted the runner at step 2, so the remaining planned steps were run individually and all passed:
npm run formatwas run whole-tree before committing and changed nothing beyond these two files. The Playwright parity mismatch is a container condition, not something this diff can cause or fix; no Playwright gate is in scope for a docs-only change.Risk and rollout
ledger:lookupon the old branch name will still report NOT REVIEWED and re-review rather than silently skip.git revertthis commit; both files are append-only and neither is read by application code.Clinical Governance Preflight
Not applicable — this change touches no ingestion, answer generation, search/ranking, source rendering, document access, privacy, production environment, or clinical output path. Both changed files are documentation ledgers.
RAG impact: no retrieval behaviour change — the diff is confined to
docs/branch-review-ledger.mdanddocs/outstanding-issues.md, and touches no retrieval, ranking, selection, eval-harness, or fixture surface.Notes
c3c9d6a31,abbcdc8e9) onclaude/cloud-pr-loop-prevention-bc052b. That branch was never pushed and is unreachable from this container, so nothing about it could be verified or shipped from this session. Two Sentry branches do exist on the remote (claude/sentry-nextjs-sdk-setup-2v24q5,cursor/sentry-nextjs-sdk-7cee); whether they already carry that work is unconfirmed and remains a decision for a session with access to that machine.Generated by Claude Code
Summary by CodeRabbit