docs+test: correct the ordering-safe index claim, and make the near-bottom refusal guard non-vacuous - #1423
Conversation
My error, live in a merged runbook. The 2026-07-29 correction identified the
unordered LIMIT 12 in rag-candidate-sources.ts:482 as the RAG hazard, then
attributed it only to the trigram index and listed documents_status_id_idx as
ordering-safe. Re-reading the statement it describes:
query = query.or(filters).eq("status", "indexed").limit(12);
One statement carries both the status equality and the unordered LIMIT 12, and
(status, id) is exactly the index serving that equality. So the mechanism the
doc already documents applies to it verbatim: a new plan can return a different
twelve title-alias rows into candidate assembly. It is canary-gated.
The generalisation that produced the error is worth naming, because it is the
trap: (status, id) has two consumers. search-scope.ts:271-277 pages with an
explicit .order("id") and is genuinely safe; rag-candidate-sources.ts:482 is
unordered and is not. Safety of one consumer is not transitive to the other.
Also marks the SQL block itself, which is what an operator actually copies. The
prose gated the indexes while the block still listed all three together with no
distinction, so a copy-paste run would apply the canary-gated ones regardless.
Gates: prettier clean; docs:check-links 1381 references resolve;
docs:check-scripts 395 npm-run references resolve. No code or SQL applied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiFThis 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:46 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 (3)
Comment |
…gs-impl-s8g01v Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Follow-up to the #1396 review. The overlay branch of this test was moved until it passed, and nothing checked that its scroll position still exercised a refusal, so it could have stopped guarding anything silently. Derives both target offsets from computeScrollHideUpdate's own clauses instead of chosen numbers, because the two motions refuse for different reasons: in-flow runwayAfterCollapse <= revealIntentDistance + collapseRunwaySlack reserve-only offset > postCollapseMaxOffset + bottomClampTolerance Adds non-vacuity assertions ahead of the behavioural check. "Chrome stayed visible" only evidences a refusal if the offset sat inside the refusal band and the other clauses were satisfied. The load-bearing one is postCollapseMaxOffset >= topRevealOffset + hideIntentDistance: without it, a short post-collapse range refuses the hide on its own and the near-bottom clause is never reached, so the test passes while proving nothing. Renames the test. "96px of near-bottom runway" named collapseRunwaySlack, which exists only on the in-flow branch; reserve-only has no slack term. One test covers both owners, so the title now names the behaviour. Nothing outside the definition referenced the old name, so no flake-ledger identity is broken. No production hide policy changed — only what the test asserts about it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF
…01v' into claude/latency-findings-impl-s8g01v
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:40d35e697c
ℹ️ 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.
Codex review on this PR caught that docs/outstanding-issues.md #102 still called documents_status_id_idx ordering-safe after this PR's own runbook correction reclassified it as canary-gated (its rag-candidate-sources.ts consumer shares the RAG-hazard mechanism with the trigram index; only the search-scope.ts ordered consumer stays safe). Mirrors the runbook's exact corrected wording so an operator following the ledger entry alone doesn't retain an unguarded index.
# Conflicts: # docs/outstanding-issues.md
Uh oh!
There was an error while loading. Please reload this page.
Codex review is right: the new check accepted `unset` alongside `unspecified`, and those are not the same state. Per gitattributes, an Unspecified `merge` attribute is the documented default 3-way text merge — the contract this PR establishes — while Unset (`-merge`) takes the current branch's version and declares the merge conflicted, so every two-sided edit becomes a manual resolution. A global or future attributes file could therefore have violated the contract with the gate still printing "no merge driver". Reproduced before fixing: appending `docs/outstanding-issues.md -merge` made `git check-attr` report `merge: unset` and the guard passed. It now fails with a message naming the Unset/Unspecified distinction and telling the reader to drop the negated attribute rather than add one. The acceptance decision moves into an exported `mergeAttributeProblem` so the distinction is unit-tested rather than only reasoned about, with four cases in tests/repo-hygiene.test.ts: `unspecified` accepted; `unset`, `union`/`ledger`, and an empty reading all rejected. The empty case matters because an unparsed check-attr output would otherwise make the whole check vacuous. Verified: with `-merge` present the gate fails on the new message; with it removed it passes "no merge driver". repo-hygiene 47 passed (47). verify:cheap exit 0 — Test Files 435 passed (435), Tests 4508 passed | 4 skipped (4512). Whole-tree prettier clean. (Test total differs from this branch's earlier run because it now carries main's #1423/#1427/#1438; this commit adds four.) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ
Addresses the Codex P2 on this PR and fixes my own mistake in the first attempt.
The correction itself is unchanged in substance: `documents_status_id_idx` is
canary-gated, not ordering-safe, because `rag-candidate-sources.ts:482` pairs
`.eq("status","indexed")` with an unordered `.limit(12)` and `(status, id)`
serves that equality. The reasoning stays in the runbook, which is the
authority; the ledger row now just points at it.
What changed is the shape. My first version added prose to #102's Detail cell,
which is the widest cell in that column — so Prettier repadded every row and the
diff became 63 insertions / 63 deletions. Row #133 records that this exact
whole-table behaviour has already caused mechanical conflicts that silently
stopped all CI, and it is very likely how the original correction was lost
between #1423 and main in the first place.
Three attempts established the rule empirically, since my first two hypotheses
were both wrong: growing the cell reflows (68 lines), *shrinking* it also
reflows (68 lines, because the column width tracks this cell), and trailing
whitespace padding does not help because Prettier trims it. The replacement is
therefore exactly the same length as the text it replaces — 97 characters — and
the diff is now 1 insertion / 1 deletion with Prettier reporting the file
unchanged.
Verified on an untouched row: before this, #100's Detail cell lost exactly one
padding space (3533 -> 3532 chars), which is what proved the column width, not
my cell's content, was driving the churn.
Gates: outstanding-issues guard 137 rows / unique ids; branch-review-ledger 117
live + 1206 archived, no duplicates; prettier clean. No SQL applied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF* fix(ledger): remove merge=union from the issues ledger, per its own #133 Ledger #133 already recorded union as the wrong driver for this file — "two sides each bumping the marker produce two `next-id` lines, corrupting the file silently where a conflict would fail loudly" — but `.gitattributes` still set it and `check-outstanding-issues.mjs` *required* it, so the repo's own tested conclusion was contradicted by its own config. PR #1430 confirmed the cost at scale: four merges in one session, each reporting success while duplicating the entire open-items table (`#59 appears 2 times (lines 101, 166)` and so on for every row), each needing a manual rebuild from origin/main. Union also makes `git merge-tree` report a clean tree, so the pre-merge conflict check cannot warn. Unlike docs/branch-review-ledger.md — which keeps its custom `merge=ledger` driver, union plus exact-row dedupe — this file allocates IDs by read-modify-write. Concurrent appends therefore need manual renumbering whatever the driver does (hit twice on 2026-07-30: #125 and #135 collisions), so union bought nothing and only hid the overlap. Default 3-way merge conflicts honestly instead. The gate's attribute check is inverted rather than deleted, so a driver reappearing here is a red gate. AGENTS.md, docs/process-hardening.md, .claude/skills/issues/SKILL.md and docs/scripts-index.md are updated to match, and #133's driver half is marked resolved with its still-open half (fixed-width padding making every row edit one hunk) left intact. Verified: reintroducing `docs/outstanding-issues.md merge=union` fails the gate with "must have NO merge driver (found merge=union)", and removing it passes with "no merge driver" — the gate bites, not just passes. verify:cheap exit 0: Test Files 434 passed (434), Tests 4563 passed | 4 skipped (4567). Whole-tree prettier clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ * fix(gate): reject `-merge` too, not just a named driver Codex review is right: the new check accepted `unset` alongside `unspecified`, and those are not the same state. Per gitattributes, an Unspecified `merge` attribute is the documented default 3-way text merge — the contract this PR establishes — while Unset (`-merge`) takes the current branch's version and declares the merge conflicted, so every two-sided edit becomes a manual resolution. A global or future attributes file could therefore have violated the contract with the gate still printing "no merge driver". Reproduced before fixing: appending `docs/outstanding-issues.md -merge` made `git check-attr` report `merge: unset` and the guard passed. It now fails with a message naming the Unset/Unspecified distinction and telling the reader to drop the negated attribute rather than add one. The acceptance decision moves into an exported `mergeAttributeProblem` so the distinction is unit-tested rather than only reasoned about, with four cases in tests/repo-hygiene.test.ts: `unspecified` accepted; `unset`, `union`/`ledger`, and an empty reading all rejected. The empty case matters because an unparsed check-attr output would otherwise make the whole check vacuous. Verified: with `-merge` present the gate fails on the new message; with it removed it passes "no merge driver". repo-hygiene 47 passed (47). verify:cheap exit 0 — Test Files 435 passed (435), Tests 4508 passed | 4 skipped (4512). Whole-tree prettier clean. (Test total differs from this branch's earlier run because it now carries main's #1423/#1427/#1438; this commit adds four.) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XrPbbfU9yWuEjEVypCr4ZQ --------- Co-authored-by: Claude <noreply@anthropic.com>
…#1440) * docs(issues): re-apply #102's canary-gated correction lost to a merge Codex raised this as a P2 on PR #1423: with the runbook corrected, ledger #102 still told an operator the `(status,id)` use was ordering-safe, so the two operator entry points contradicted each other. A reply on that thread claimed the fix landed in 2bdc245. It did not reach `main`. Verified by content rather than by SHA, which matters here: #1423 was squash-merged, so `git merge-base --is-ancestor` returns NO for every original commit and proves nothing either way. Reading `docs/outstanding-issues.md` at `origin/main` (0310c6a) shows the retracted sentence still present, while the runbook correction and the test change from the same PR both landed. Most likely cause, and it is a documented hazard: #1428 ("archive five completed ledger rows") touched this same file on `main`, and #112 records that `docs/outstanding-issues.md` conflicts present as ordinary content conflicts a hurried resolution can settle by dropping one side. Union merge has since been added to this file, but it would not have saved this case — both sides edited the same sentence. The row now mirrors the runbook's classification and names the runbook as the authority if the two ever diverge again, so a future reader has a tie-breaker rather than two equal-looking claims. Gates: verify:cheap exit 0, Test Files 434 passed (434), Tests 4563 passed | 4 skipped. Outstanding-issues guard, branch-review-ledger guard, docs:check-links, docs:check-scripts, prettier all pass. No SQL applied; no index created. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF * docs(issues): correct #102 within the existing table width Addresses the Codex P2 on this PR and fixes my own mistake in the first attempt. The correction itself is unchanged in substance: `documents_status_id_idx` is canary-gated, not ordering-safe, because `rag-candidate-sources.ts:482` pairs `.eq("status","indexed")` with an unordered `.limit(12)` and `(status, id)` serves that equality. The reasoning stays in the runbook, which is the authority; the ledger row now just points at it. What changed is the shape. My first version added prose to #102's Detail cell, which is the widest cell in that column — so Prettier repadded every row and the diff became 63 insertions / 63 deletions. Row #133 records that this exact whole-table behaviour has already caused mechanical conflicts that silently stopped all CI, and it is very likely how the original correction was lost between #1423 and main in the first place. Three attempts established the rule empirically, since my first two hypotheses were both wrong: growing the cell reflows (68 lines), *shrinking* it also reflows (68 lines, because the column width tracks this cell), and trailing whitespace padding does not help because Prettier trims it. The replacement is therefore exactly the same length as the text it replaces — 97 characters — and the diff is now 1 insertion / 1 deletion with Prettier reporting the file unchanged. Verified on an untouched row: before this, #100's Detail cell lost exactly one padding space (3533 -> 3532 chars), which is what proved the column width, not my cell's content, was driving the churn. Gates: outstanding-issues guard 137 rows / unique ids; branch-review-ledger 117 live + 1206 archived, no duplicates; prettier clean. No SQL applied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF * docs: record issue correction review --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
Two independently low-risk follow-ups from the #1396 review, each its own commit and each separately revertible while this PR is open.
1.
docs/operator-apply-performance-latency-remediation.md— correct my mislabelled "ordering-safe" index. Docs only; no SQL applied.The runbook's 2026-07-29 correction identified the unordered
LIMIT 12inrag-candidate-sources.ts:482as the RAG hazard for the additivedocumentsindexes (ledger#102), then attributed it only to the bare-column trigram index and listeddocuments_status_id_idxas "ordering-safe". Re-reading the statement it describes:One statement carries both the
statusequality and the unorderedLIMIT 12— and(status, id)is exactly the index serving that equality. The mechanism the runbook already documents applies to it verbatim: a new plan can return a different twelve title-alias rows into candidate assembly. Reclassified as canary-gated.The generalisation that produced the error is named in the doc, because it is the reusable lesson: the index has two consumers.
search-scope.ts:271-277pages with an explicit.order("id")and is genuinely safe;rag-candidate-sources.ts:482is unordered and is not. Safety of one consumer is not transitive to the other. The SQL block is now marked inline too — the prose gated the indexes while the block still listed all three with no distinction, and the block is what an operator actually copies.2.
tests/ui-phone-scroll.spec.ts— make the calculator near-bottom refusal guard non-vacuous, and rename it. Test only; no production behaviour changed.The overlay branch of this test was moved until it passed during #1396, and nothing checked that its scroll position still exercised a refusal — so it could have stopped guarding anything silently. Both target offsets are now derived from
computeScrollHideUpdate's own clauses rather than from chosen numbers, because the two motions refuse for different reasons:Non-vacuity assertions now run ahead of the behavioural check. "Chrome stayed visible" only evidences a refusal if the offset sat inside the refusal band and the other clauses were satisfied. The load-bearing one is
postCollapseMaxOffset >= topRevealOffset + hideIntentDistance: without it, a short post-collapse range refuses the hide on its own, the near-bottom clause is never reached, and the test passes while proving nothing.Renamed, because
96iscollapseRunwaySlackand that term exists only on the in-flow branch — reserve-only has no slack test at all. One test covers both owners, so the title now names the behaviour. Nothing outside the definition referenced the old name, so no flake-ledger identity is broken.RAG impact: no retrieval behaviour change — item 1 changes only prose in an operator runbook and item 2 changes only a Playwright assertion. Neither applies an index nor touches retrieval, ranking, selection, alias or citation code. Item 1's effect on retrieval is the opposite of a behaviour change: it withdraws a claim that an index was safe to apply without a live eval-canary pair, making the documented gate stricter rather than looser.
Verification
npm run verify:cheap— exit 0,Test Files 431 passed (431),Tests 4493 passed | 4 skipped (4497).npm run typecheck,npx prettier --check— clean.npm run docs:check-links—docs link check passed: 1381 repo path references resolve.npm run docs:check-scripts—docs script-ref check passed: 395 npm-run reference(s) resolve to real scripts.npm run check:branch-review-ledgerafter eachmainmerge —1296 table records … no conflict markers, mojibake, heading records, or duplicates(themerge=unionduplication watch condition, ledger#088).1 passed.offset <= postCollapseMaxOffset + bottomClampToleranceclause fromuse-hide-on-scroll.tsmakes the test fail onexpect(frames.every(frame => !frame.headerHidden && !frame.dockHidden)).toBe(true). The policy file was restored andgit diffconfirmed clean.src/lib/rag/rag-candidate-sources.ts:470-495directly, not inferred from the runbook's own description.That broken-shape check found a real defect in the guard, which is worth recording because it is the same class of error the guard exists to catch. The first version read
scrollTopafter the frame loop. When the policy wrongly allows the hide, the reserve collapses,maxOffsetshrinks and the browser clampsscrollTop— so the value intended as "the offset under test" became "the offset after the bug". A policy regression therefore surfaced as "the test setup is wrong" rather than as a behavioural failure. Fixed by capturingtargetScrollTopbefore dispatching and never re-reading it; the second broken-shape run then failed on the behavioural assertion as intended.Not run, and why: no browser gate applies to item 1.
check:driftwas not run becausesupabase/**is untouched. Every provider-backed gate (eval:*,verify:release,check:supabase-project,test:live) was not run and is not implicated — no OpenAI, Supabase or hosted-CI call was made in producing or verifying either item.Risk and rollout
Clinical KB Databasebelieving it could not affect retrieval selection, and that belief was not established.git reverteither commit independently while this PR is open. Note that a squash merge would fold them into one, so a post-merge revert of a single item means reverting hunks by hand. No migration, schema, data, or configuration change in either.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes on the above: this PR changes runbook prose and one Playwright assertion, so every item is satisfied by construction rather than by inspection of new behaviour. No answer generation, citation, verification, source-governance or document-access code is touched; no credential or Supabase value appears in the diff; the pinned Supabase target is referenced only as unchanged context. Clinical decision-support behaviour is unaffected, so the SaMD classification is unchanged. The one clinically-relevant effect is indirect and protective: the runbook no longer tells an operator that a retrieval-path index is safe to apply without the canary this repository's own rules require.
Notes
746a88edis an ancestor ofmain, so nothing was orphaned, and the branch was re-created carrying only new work plusmainsyncs — confirmed withgit rev-list --left-right --count. This is a new PR, not a reuse of the merged one.#102still tracks the operator apply itself. Item 1 corrects the instructions only; it does not change that item's status or apply anything.docs/phone-chrome-physical-acceptance.md, and the fix(chrome): overlay the phone header so hiding it never moves content #1396 review ledger row.🤖 Generated with Claude Code
https://claude.ai/code/session_01361jh3eYVjJCzXWjAhdZiF