docs(issues): capture CI/merge findings and a merged-PR gate gap from PR babysitting - #1421
Conversation
#117: this repo's CI (on: pull_request with default types) doesn't retrigger on the draft-to-ready transition, only on opened/synchronize/reopened — a marked-ready PR can sit with a minimal check set until an actual new commit lands. #118: GitHub's update-branch API doesn't honor the merge=union .gitattributes driver on docs/branch-review-ledger.md, so it can 422 with a false conflict that a local git merge resolves cleanly. Both observed today on PR #1406. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Re9ERtQwJ82ErbAnahAhsa
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe outstanding-issues ledger advances its next-ID marker to 119 and adds open items ChangesOutstanding issues ledger
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:c0ceee291d
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
# Conflicts: # docs/outstanding-issues.md
PR #1396 repeatedly declared physical-device Safari/PWA acceptance (docs/phone-chrome-physical-acceptance.md) as required before merge because headless Chromium cannot certify Safari chrome minimisation or cold-launch PWA paint, then merged with the checklist still blank. Also notes a related missing pre-paint/cold-load hydration test the same PR's review flagged but never filed.
# Conflicts: # docs/outstanding-issues.md
npx prettier --write after merging main (9e2fe44) — a table cell width shifted during the merge and format:check would have caught it.
Uh oh!
There was an error while loading. Please reload this page.
Sixth conflict today, and the first that auto-merged *wrongly*: git's text merge concatenated both tables, duplicating all 63 open rows. PR #1421 had landed on main using #128/#129/#130 — the exact id collision #112 describes — so both sides had those ids with different content and the merge kept both. `npm run check:outstanding-issues` caught it and stated the correct resolution verbatim: renumber the incoming rows above the marker and bump it, rather than taking one side wholesale and dropping the other's rows. Done exactly that — main's table is authoritative, this branch's four rows renumber to #131/#132/#133/#134, marker to 135. Verified both sides' rows survive: main's #128-#130 and mine are all present and distinct. Worth noting main's new #129 (`update-branch` API does not honour the `merge=ledger` driver) is the server-side twin of my #134 (the driver is absent wherever `npm install` was skipped). Same root cause from two directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
…indings (#1424) * docs(ledger): record PR #1400 closeout and capture three unrecorded findings Documentation only — two ledger files, no code. **Review closeout for PR #1400** appended with `ledger:append` (never hand-written), recording the 17 findings fixed, the verification behind each, and the post-merge check that all 8 commits are ancestors of main with the 4 changed files byte-identical. **Three findings from that session that nothing else records:** - `#125` — `@codex fix` produced 11 commits across a branch named `work`, none fetchable, the same finding rewritten four times. It reads as success while the branch is unchanged, which is the actual hazard. - `#126` — both client-side push guards are inert for agent pushes: `gh` absent makes the auto-merge sentinel fail open, and `core.hooksPath` is set only by a local install. They protect the environment least likely to need them. - `#127` — this ledger's fixed-width padding makes one row's edit re-pad all 59, so it conflicts on nearly every main advance; each conflict silently stopped all CI on #1400 via `#116`. Records that `merge=union` is the wrong fix, with the evidence. CircleCI was deliberately not filed — already captured as `#122`. Checked before writing rather than after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * docs(issues): correct unsafe pull_request_target advice in #129 Review caught a real problem in the guidance I filed, not in code: #129's next-action suggested moving *both* push guards server-side into a `pull_request_target` job. That context carries secrets and a write token, and a format check must execute PR-head code — including the dynamic `prettier.config.*` this very PR taught the guard to load. That is the classic privileged-context vector, and `.github/workflows/pr-policy.yml` already avoids it deliberately by checking out only `github.workflow_sha`. Corrected, and the row now records why the whole idea was unnecessary: formatting is already enforced server-side by `Static PR checks` running `format:check` on ordinary `pull_request` CI, so the guard's only unique value is failing fast before the push. Only the metadata-only auto-merge sentinel could safely live in a target job. Bad advice in a durable ledger is worse than no advice — someone would have acted on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * fix(issues): repair the duplicated table from the sixth main merge Sixth conflict today, and the first that auto-merged *wrongly*: git's text merge concatenated both tables, duplicating all 63 open rows. PR #1421 had landed on main using #128/#129/#130 — the exact id collision #112 describes — so both sides had those ids with different content and the merge kept both. `npm run check:outstanding-issues` caught it and stated the correct resolution verbatim: renumber the incoming rows above the marker and bump it, rather than taking one side wholesale and dropping the other's rows. Done exactly that — main's table is authoritative, this branch's four rows renumber to #131/#132/#133/#134, marker to 135. Verified both sides' rows survive: main's #128-#130 and mine are all present and distinct. Worth noting main's new #129 (`update-branch` API does not honour the `merge=ledger` driver) is the server-side twin of my #134 (the driver is absent wherever `npm install` was skipped). Same root cause from two directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * docs: record PR 1424 review --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
docs/outstanding-issues.md, per the repo's outstanding-work ledger convention.#126(renumbered from#117after a concurrent collision with PR docs(issues): archive #113 and #114, open #117 for the stale spec reference #1420's own#117/#125— see Notes): this repo'sci.ymlon: pull_requestblock has no explicittypes:, which defaults to GitHub's[opened, synchronize, reopened].ready_for_reviewis not in that list, so marking a draft PR ready does not by itself retrigger the full required-check suite — PR CI testing approach review: flake, waste, and when-to-run matrix #1406 sat with only 3-4 minimal checks for 30+ minutes until an actual new commit (asynchronizeevent) landed. Distinct from the already-tracked#116(a real merge conflict silencespull_request-triggered CI entirely) — this is a missing trigger type on a conflict-free, ready-to-check PR.#127(renumbered from#118): GitHub'supdate-branchAPI doesn't honor this repo'smerge=union.gitattributesdriver ondocs/branch-review-ledger.md. On PR CI testing approach review: flake, waste, and when-to-run matrix #1406 the API 422'd with "merge conflict between base and head" whilegit merge-tree --write-tree origin/mainconfirmed a clean merge — GitHub's server-side merge implementation doesn't read custom merge drivers, so it saw a real textual conflict at the exact hunk local git'suniondriver auto-resolves. Worked around by mergingorigin/mainlocally and pushing directly.#128(new, added while re-checking PR fix(chrome): overlay the phone header so hiding it never moves content #1396 during babysitting): PR fix(chrome): overlay the phone header so hiding it never moves content #1396 repeatedly declared a physical-device Safari/PWA acceptance checklist (docs/phone-chrome-physical-acceptance.md) as required before merge — restated three times across its own review thread, including after its final fix — because headless Chromium cannot certify Safari chrome minimisation or cold-launch PWA paint. It merged anyway with the checklist still a blank template and no device evidence anywhere. Also notes a related pre-paint/cold-load hydration test the same PR's review flagged as missing but never filed. This is a real, unaddressed governance gap on a merged shared-chrome change, not a formality.Verification
npm run check:outstanding-issues— self-test passed; guard passed:123 rows (61 open, 62 archived), unique ids, next-id=129 above the highestnpx prettier --check docs/outstanding-issues.md— cleannpm run verify:ui/eval:*/check:production-readiness— no UI, retrieval, ranking, ingestion, privacy, or Supabase behavior changed; this is a ledger-only docs append.RAG impact: no retrieval behaviour change — no file under
src/lib/rag/**, clinical-search, retrieval-selection, ranking-config, the eval harness, the golden fixture, or the retrieval RPCs is touched.Risk and rollout
docs/branch-review-ledger.md, but no existing row was edited — only new rows appended and thenext-idmarker bumped.Clinical Governance Preflight
Not applicable — the diff touches only
docs/outstanding-issues.md. No ingestion, answer generation, search/ranking, source rendering, document access, privacy, production environment, or clinical output behavior is touched.Notes
next-idallocation race before pushing (per the repo's own#112finding about exactly this race): confirmedorigin/main's marker was still117immediately before the first push, so#117/#118were free at that time. A concurrently-open draft PR (#1420) also claimed#117for an unrelated finding and landed first onmain— exactly the race this row's own#112predicted. Codex flagged this PR's#117/#118wording separately (both fixed and resolved — see thread history); a follow-up commit from another session then renumbered this PR's rows to#126/#127and bumped the marker to128once the collision materialized, verified clean (check:outstanding-issuespassed, no duplicate ids).#128was added afterward while re-checking PR fix(chrome): overlay the phone header so hiding it never moves content #1396 during the same babysitting pass.Generated by Claude Code