docs+tooling(eval): ADDENDUM 4 close-out — Phase C record, boundary-case policy, artifact trend tool (Phase D)#1006
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EXsJcLrbZUXwnBeG91cVo9
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughAdds an ChangesEvaluation observability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
BigSimmo
changed the title
…DUM 4 Phase D) - docs/observability-slos.md §3.1: standing boundary-case and metric-interpretation policy — top-5 boundary cases are a human decision; fixture/snapshot lockstep + regeneration procedure; the 2026-07-20 mrr baseline step; irrelevant@10 is a labeling question before a ranking question; ordering-headroom rules with the two refuted approaches recorded (weight tuning no-op; comparator-key spread regression) and the constraints any third attempt must satisfy - scripts/eval-trend.mjs + eval:trend script: run-over-run metric table and per-case rr trend from downloaded eval-canary-output artifacts (offline, read-only) — the durable trend record on top of the artifact retention - smoke-verified against the real canary #53 artifact Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EXsJcLrbZUXwnBeG91cVo9
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/eval-trend.test.ts (1)
37-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the CLI invocation without
--case.These tests exercise only
buildTrendRowsandbuildCaseTrend, so they will not catchmain()dropping the first artifact in the documented default invocation. Cover one- and two-file CLI calls, including preserved input order.🤖 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 `@tests/eval-trend.test.ts` around lines 37 - 60, Add regression coverage for the CLI entry point main() when invoked without --case, verifying one-file and two-file artifact calls preserve all inputs and their original order. Keep the existing buildTrendRows and buildCaseTrend tests, and exercise the documented default CLI invocation rather than only the underlying helpers.
🤖 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 `@scripts/eval-trend.mjs`:
- Around line 64-66: Update the files filter in the argument-parsing flow around
caseFlag and caseId so it removes the argument following --case only when --case
is present; when absent, preserve every artifact argument, including the first
file, while continuing to exclude the --case flag and its associated case
identifier.
---
Nitpick comments:
In `@tests/eval-trend.test.ts`:
- Around line 37-60: Add regression coverage for the CLI entry point main() when
invoked without --case, verifying one-file and two-file artifact calls preserve
all inputs and their original order. Keep the existing buildTrendRows and
buildCaseTrend tests, and exercise the documented default CLI invocation rather
than only the underlying helpers.
🪄 Autofix (Beta)
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 Plus
Run ID: d70471d3-6d9e-49b3-a631-10742ea73538
📒 Files selected for processing (5)
docs/branch-review-ledger.mddocs/observability-slos.mdpackage.jsonscripts/eval-trend.mjstests/eval-trend.test.ts
| const caseFlag = argv.indexOf("--case"); | ||
| const caseId = caseFlag >= 0 ? argv[caseFlag + 1] : undefined; | ||
| const files = argv.filter((arg, index) => arg !== "--case" && index !== caseFlag + 1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not drop the first artifact when --case is absent.
With no --case, caseFlag is -1, so index !== caseFlag + 1 becomes index !== 0. The documented two-file invocation therefore reports only the second run, while a single-file invocation exits with “no files”.
Proposed fix
const caseFlag = argv.indexOf("--case");
const caseId = caseFlag >= 0 ? argv[caseFlag + 1] : undefined;
- const files = argv.filter((arg, index) => arg !== "--case" && index !== caseFlag + 1);
+ const files =
+ caseFlag >= 0
+ ? argv.filter((_, index) => index !== caseFlag && index !== caseFlag + 1)
+ : argv;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const caseFlag = argv.indexOf("--case"); | |
| const caseId = caseFlag >= 0 ? argv[caseFlag + 1] : undefined; | |
| const files = argv.filter((arg, index) => arg !== "--case" && index !== caseFlag + 1); | |
| const caseFlag = argv.indexOf("--case"); | |
| const caseId = caseFlag >= 0 ? argv[caseFlag + 1] : undefined; | |
| const files = | |
| caseFlag >= 0 | |
| ? argv.filter((_, index) => index !== caseFlag && index !== caseFlag + 1) | |
| : argv; |
🤖 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 `@scripts/eval-trend.mjs` around lines 64 - 66, Update the files filter in the
argument-parsing flow around caseFlag and caseId so it removes the argument
following --case only when --case is present; when absent, preserve every
artifact argument, including the first file, while continuing to exclude the
--case flag and its associated case identifier.
2 participants
Summary
ADDENDUM 4 close-out (Phase D remainder + records). Three pieces, all offline:
f96217cfailed 3/36 (doc_recall 1.0→0.9167, mrr 0.8921→0.8138), the Revert "feat(rag): per-candidate saturation-tail primaries" pending retrieval review (#1004) #1005 revert stands, root cause identified (spreading comparator keys above the relevance score lets raw ts_rank override boost/title/subject-aware ordering), disposition recorded (no re-land as-is; any retry = below-relevance key + differently-relevant discriminating fixtures + fresh pair).docs/observability-slos.md§3.1): top-5 boundary cases are a human decision; fixture↔snapshot lockstep + regeneration procedure; the 2026-07-20 mrr baseline step;irrelevant@10is a labeling question before a ranking question; the ordering-headroom rules with both refuted approaches on record.scripts/eval-trend.mjs,npm run eval:trend): run-over-run metric table + per-case rr trend from downloadedeval-canary-outputartifacts — the durable trend record on top of the existing 30-day artifact retention, no new infrastructure. Smoke-verified against the real canary Harden authentication and protect sensitive tokens #53 artifact.Verification
npm run test -- tests/eval-trend.test.ts— 2/2npm run lint+npm run typecheck— cleannpm run check:knip— clean (new script + test wired)Risk and rollout
Clinical Governance Preflight
N/A — documentation + offline tooling only; no ingestion, answer generation, search/ranking, source rendering, document access, privacy, production-env, or clinical-output behaviour changed.
Notes
🤖 Generated with Claude Code
https://claude.ai/code/session_01EXsJcLrbZUXwnBeG91cVo9
Summary by CodeRabbit
New Features
Documentation
Tests