docs(issues): record why three closures were rejected, and that #231's instrument now exists - #1957
Conversation
…s instrument now exists Four corrections, queued as immutable inbox requests. #235, #237 and #238 each had a `done` queued in PR #1940 and cancelled on review. The cancellations were right: all three ask for visual or browser proof and were closed on executable evidence. Each row now records the attempt, the reason it was refused, and a Stop rule naming the evidence class that must not be used again — so the next reader does not repeat it: - #235: section 7.1 opens with "records executable evidence RATHER THAN committing image baselines", so the section that looks like the evidence says in its first line that it is not - #237: jsdom does not lay out text, so a 320px assertion proves the string is present, not that it fits - #238: the risk is ancestor-scoped CSS/contain/transform on five specific host surfaces, which no Sheet component test can see All three also drop their IN FLIGHT do-not-start prefix; PRs #1841 and #1842 have merged, so the warning was blocking rather than protecting. #231's "Next: instrument and reproduce" is stale — commit a3bc4da added scripts/probe-generation-quality.ts and adjudicated PR #1861 as superseded for phase 1. The row now says so, so nobody reviews #1861 as the live fix or rebuilds the probe. Next is running it where credentials exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017paT42ZVMf8jaLtkjFxdy5
📝 WalkthroughWalkthroughThe PR adds one branch review record and four outstanding-issue inbox records. The records document issue status, rejected closure conditions, validation evidence, follow-up actions, and restrictions on unsupported closure evidence. ChangesIssue record updates
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk:🔵 Low · up to The PR only adds issue-ledger request records, but its review record omits the decisive verification output and required ledger lookup. Merge is reasonable with explicit owner follow-up to add or accept this bounded evidence gap. Possibly related PRs
🚥 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. |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017paT42ZVMf8jaLtkjFxdy5
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:e4372b5b4e
ℹ️ 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".
| "action": "update", | ||
| "payload": { | ||
| "id": "#231", | ||
| "detail": "PARTIAL 2026-08-12: This PR fixes the clinically consequential stale-fallback path: every answer whose routing or degraded reason contains generation_fallback is excluded from rag_response_cache. Offline evidence: 96 focused answer-route tests and 574 RAG fixture/contract tests passed. Approved live baseline/final canaries preserved 36/36 document and content recall at 1.0 with zero per-case reciprocal-rank regressions; the final 44-case answer gate had zero citation or numeric-grounding failures. A budget extension was tested and rejected: four cache-bypassed 'Lithium dosing?' probes remained grounded, cited safe extractive fallbacks at 35-40 second candidate budgets; the decisive 40-second probe completed generation in 25.272 seconds and 27.237 seconds total with route_deadline_exceeded=false, but failed generation quality. Therefore OPENAI_ANSWER_TIMEOUT_MS and the route budget are not the current residual binding cause. INSTRUMENT NOW EXISTS 2026-08-14: the \"Next: instrument\" half of this row is done. Commit a3bc4da adds scripts/probe-generation-quality.ts — one cache-bypassed live answer reporting the structured generation_quality_gate_reasons, provider-backed, refusing demo mode, never caching or logging the probe. The same commit adjudicates PR #1861: superseded for phase 1, close recommended, with the numeric-retry half deferred to phase 2 pending probe evidence. So do not review #1861 as though it were the live fix, and do not re-implement the probe. Next: run scripts/probe-generation-quality.ts in an environment that has OPENAI and Supabase credentials — it is blocked in offline containers, which is why it has not been run yet — then make a separate bounded output-quality fix with an offline fixture and live canary. Stop: do not increase route/provider timeouts or cache any generation fallback." |
There was a problem hiding this comment.
When an operator supplies any query—especially clinical text—the probe prints the complete query at scripts/probe-generation-quality.ts:38 and includes it again in the JSON report via the query field at lines 55–56, despite this durable ledger entry saying it never logs the probe. Terminal or CI output may therefore retain the text; distinguish logQuery: false preventing database persistence from the script's stdout logging, or redact the query before describing the probe as non-logging.
AGENTS.md reference: AGENTS.md:L895-L898
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🔇 Additional comments (5)
docs/branch-review-records/4859907cc6069250fcef7f0156490f1e04d0fd60e25fab0edbf33408b720efa9.record.md (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Complete the required verification evidence before accepting this record.
Line 1 reports
verify:pr-localas11 completed, 0 failed, but it does not include the decisive gate output line. The supplied context also does not show the required ledger lookup for PR#1957.Run:
Include the decisive
verify:pr-localandledger:lookupoutput lines in the review evidence. Do not rely on an exit code alone.As per coding guidelines: “When reporting verification, include the decisive gate output line; an exit code of 0 alone is insufficient evidence,” and run
npm run ledger:lookup -- <ref> --scope "<scope>"before reviewing a branch or pull request.docs/outstanding-issues-inbox/7f5e6922-8d0b-424a-b359-12bed3a4e315.json (1)
1-10: LGTM!docs/outstanding-issues-inbox/bd11cfe9-1627-425f-a8f7-e0e202b980a4.json (1)
1-10: LGTM!docs/outstanding-issues-inbox/ec6d0c23-2f19-4159-9c73-49bdc103b61e.json (1)
1-10: LGTM!docs/outstanding-issues-inbox/38c6095f-41f5-4925-b1ea-f5af5187885d.json (1)
1-10: LGTM!
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 295ba699-1eea-42be-af21-ae0689733dee
📒 Files selected for processing (5)
docs/branch-review-records/4859907cc6069250fcef7f0156490f1e04d0fd60e25fab0edbf33408b720efa9.record.mddocs/outstanding-issues-inbox/38c6095f-41f5-4925-b1ea-f5af5187885d.jsondocs/outstanding-issues-inbox/7f5e6922-8d0b-424a-b359-12bed3a4e315.jsondocs/outstanding-issues-inbox/bd11cfe9-1627-425f-a8f7-e0e202b980a4.jsondocs/outstanding-issues-inbox/ec6d0c23-2f19-4159-9c73-49bdc103b61e.json
Summary
Four ledger corrections, queued as immutable inbox requests. No canonical-ledger edit; apply with
npm run issues:reconcile.#235,#237,#238— a rejected closure is only useful if the row remembers it. Each had adonequeued in PR docs(issues): retire the stale IN FLIGHT annotations, archive 9 verified rows #1940 and cancelled on review. The cancellations were right: all three rows ask for visual or browser proof and were closed on executable evidence. Without recording that, the same misreading is available to the next reader at the same cost. Each row now carries the attempt, the reason it was refused, and aStoprule naming the evidence class that must not be used again:#235—ADOPTION.md§7.1 opens with "This PR records executable evidence rather than committing image baselines". The section that looks like the evidence says in its own first line that it is not. Stop rule: do not close on unit, DOM or contract evidence of any kind.#237— jsdom does not lay out text, so a 320px assertion proves the string is present, not that it fits — which is the entire question for a low-confidence extraction in a sparse grid. Stop rule: only a real browser at 320px settles it.#238— the residual risk is ancestor-scoped CSS /contain/transformon five specific host surfaces, a property of where each overlay sits in the real page. No Sheet component test can see it. Stop rule: the question is about the host surfaces, not about Sheet.IN FLIGHT — do not startprefix. PRs feat(ui): harden design-system a11y contracts (cn, warning contrast, MissingValue, forced-colors, LinkAction) #1841 and docs(ui): record DS adoption evidence and overlay/links checks (221,233,234,235,238,245) #1842 have merged, so that warning was blocking rather than protecting — the same stale-safeguard class PR docs(issues): retire the stale IN FLIGHT annotations, archive 9 verified rows #1940 addressed for the rest of the cluster.#231— its "Next: instrument and reproduce" is stale. Commita3bc4daaddedscripts/probe-generation-quality.ts(one cache-bypassed live answer reporting the structuredgeneration_quality_gate_reasons; provider-backed, refuses demo mode, never caches or logs the probe) and adjudicated PR fix(rag): retry unverified numeric answers safely #1861 as superseded for phase 1, close recommended, with the numeric-retry half deferred to phase 2 pending probe evidence. The row now records both, so nobody reviews fix(rag): retry unverified numeric answers safely #1861 as though it were the live fix or rebuilds the probe. What remains is running it where credentials exist — it is blocked in offline containers, which is why it has not run.Each
updaterewrites the whole detail cell, so the existing substance of every row was read first and carried forward verbatim; only the stale prefix was dropped and the correction appended.Verification
npm run verify:pr-localThe first run of this gate failed on
check:ledger-write-discipline— it compares two committed refs, and the requests were still uncommitted, so it refused rather than passing vacuously. Re-run after committing; the summary above is from that second run.UI verification not run: no UI, routing, styling, or browser-behaviour change in this diff. That is also precisely the point of three of these four corrections — the browser proof they ask for is the work still outstanding, not something this PR supplies.
Risk and rollout
docs/outstanding-issues-inbox/. No application source, no dependency, no schema, no workflow, and the canonical ledger is untouched until a separate reconcile applies them.git revertthe single commit, or delete the request files before reconciliation.Notes
## Clinical Governance Preflightsection:classifyPullRequestFilesreturnsclinicalRisk: false,operationalRisk: false,ragRanking: false,ui: falsefordocs/outstanding-issues-inbox/**.#231is a clinical-answer row, but this diff only edits its ledger text — it changes no ingestion, answer-generation, ranking, source-rendering, document-access or privacy code.AGENTS.md.Generated by Claude Code
Summary by CodeRabbit