feat(evaluation): official 50-case offline scorer and transcript contract - #298
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe evaluation package adds a validated 50-case official dataset and an offline transcript scorer. It also replaces chunk-based retrieval recall with a document-level v2 gate derived from 43 Allow cases. ChangesOfficial evaluation
Retrieval recall v2
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
evaluation/src/orgmemory_eval/retrieval_recall.py (1)
191-194: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReuse the atomic writer from
official_scorer.This CLI writes the report with
write_textaftermkdir.official_scorer.write_json_atomicallyperforms the same task in the same package, and it writes through a temporary file plusreplaceso an interrupted run cannot leave a truncated report.Two report writers with different durability guarantees now exist in one package. Extract the atomic helper into a shared module and call it from both CLIs.
♻️ Proposed reuse
Move
write_json_atomicallyinto a shared module, for exampleorgmemory_eval/report_io.py, then:report = score(golden, observations) - args.output.parent.mkdir(parents=True, exist_ok=True) - args.output.write_text( - json.dumps(report, ensure_ascii=False, indent=2) + "\n", encoding="utf-8" - ) + write_json_atomically(args.output, report)Import the same helper in
official_scorer.pyinstead of defining it there.🤖 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 `@evaluation/src/orgmemory_eval/retrieval_recall.py` around lines 191 - 194, Extract official_scorer’s write_json_atomically into a shared orgmemory_eval report I/O module, preserving its temporary-file-and-replace behavior. Update both official_scorer and the report-writing flow in retrieval_recall to import and call this shared helper, removing the direct mkdir/write_text implementation.
🤖 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 `@evaluation/README.md`:
- Around line 75-80: Update the official-eval command block in
evaluation/README.md to run uv sync --frozen --dev immediately after
Set-Location evaluation and before uv run orgmemory-official-eval, matching the
setup used by the other evaluation command blocks.
In `@evaluation/src/orgmemory_eval/official_cases.py`:
- Around line 86-102: Extend OfficialDataset validation alongside
require_complete_official_case_ids to count the cases by their Allow/Deny
outcome and reject any fixture that is not exactly 43 Allow and 7 Deny cases.
Ensure load_official_cases and both CLI flows receive this model-level
validation while preserving the existing count and ID checks.
In `@evaluation/src/orgmemory_eval/official_scorer.py`:
- Around line 35-37: Move the shared QuestionId, UserId, and DocumentId
constraint aliases into official_cases, exporting them there alongside the
existing official ID definitions. Update official_scorer to import and reuse
those aliases instead of redeclaring the patterns, leaving the scorer’s
validation behavior unchanged.
- Around line 496-514: Update evaluation/src/orgmemory_eval/official_scorer.py
lines 496-514 so main returns int, capture the generated report, return its
offline_gate_passed verdict as the process status, and invoke it via raise
SystemExit(main()) in the __main__ block. Apply the same change to
evaluation/src/orgmemory_eval/retrieval_recall.py lines 185-194, returning
gatePassed and raising SystemExit(main()), so failed gates produce non-zero exit
statuses while reports are still written.
- Around line 210-214: Update the Allow-result construction in
score_official_transcript so the permission object always includes
denied_cited_document_ids and verbatim_match_document_ids as empty lists when
passed is true. Preserve the existing populated lists for Deny cases and update
the corresponding Allow assertions in test_official_scorer.py to validate the
uniform shape.
- Around line 359-368: Update the judge evaluation flow in
score_official_transcript around judge.evaluate so exceptions and None results
are captured as per-case judge failures instead of aborting scoring; continue
producing the deterministic permission and citation verdicts, record the failure
in the case result as judge_error, and expose the total judge failure count in
judge_summary.
In `@evaluation/src/orgmemory_eval/retrieval_recall.py`:
- Around line 176-182: Update parse_args to accept an optional Sequence[str]
argv parameter, importing Sequence from collections.abc if needed, and pass argv
to argparse.ArgumentParser.parse_args. Update main to accept and forward argv to
parse_args so callers can invoke the CLI with explicit arguments without
modifying sys.argv.
- Around line 113-117: Remove the decorative top_k parameter from the report
function and replace its remaining uses, including the "topK" field, with the
module-level RECALL_TOP_K constant. Update
test_score_rejects_non_default_top_k_for_v2_report to reflect that the function
no longer accepts a configurable top_k, while preserving the hardcoded At40 and
At60 result keys.
- Line 46: Update the official_source_sha256 field in the relevant model to
validate a complete SHA-256 hexadecimal digest rather than the broad Identifier
constraint. Preserve derive_golden_dataset’s existing digest output while
rejecting truncated, malformed, or non-hex values.
In `@evaluation/tests/test_official_cases.py`:
- Around line 60-75: Add tests in the official-case test module that preserve 50
entries while introducing duplicate, missing, or unexpected case IDs so
require_complete_official_case_ids is exercised and its missing=/unexpected=
validation is asserted. Also add tests covering normalize_document_ids rejection
paths, using valid-length fixtures and asserting the expected ValidationError
messages.
In `@evaluation/tests/test_official_scorer.py`:
- Around line 22-24: The official fixture is loaded and validated repeatedly
because both dataset helpers call load_official_cases directly. Update
evaluation/tests/test_official_scorer.py lines 22-24 by caching official_dataset
or making it module-scoped, and apply the same caching to golden_dataset in
evaluation/tests/test_retrieval_recall.py lines 15-17; preserve existing helper
call behavior while ensuring each module loads the fixture only once.
- Around line 97-132: Extend
test_reports_allow_wrong_doc_deny_refusal_deny_leak_and_multi_doc_partial to add
cases covering the four untested official_scorer.py verdict branches:
ALLOW_ANSWER_SIGNAL_MISSING, DENY_SIGNAL_MISSING, MISSING, and
UNEXPECTED_DOCUMENTS. Use representative Allow and Deny rows with the specified
missing/invalid signals or citation sets, then assert each resulting permission
or citation verdict and any relevant report counts or gate outcome.
- Around line 291-302: Expand
test_judge_plugin_loader_requires_an_explicit_enabled_protocol to cover
load_judge_plugin rejection paths: assert invalid module:factory syntax raises,
a factory result failing the OfficialJudge check is rejected, and a judge with
enabled set to False is rejected. Retain the existing successful enabled=True
assertion and use the loader’s actual exception contract for each case.
In `@evaluation/tests/test_retrieval_recall.py`:
- Around line 49-53: Update
test_recall_at_k_deduplicates_retrieved_ids_and_honors_cutoff to include
duplicate retrieved IDs within the first k entries, and add an assertion that
pins recall_at_k’s intended deduplication behavior while retaining the cutoff
coverage. Ensure the test data verifies duplicates do not incorrectly consume
ranking slots.
- Around line 66-87: Add tests around score to cover both missing branches: add
a P031 observation variant with one document removed and assert its
approximately -100/86 delta still passes the two-point tolerance, and add an
incomplete ObservationSet whose dataset_id differs from the golden dataset,
asserting score raises ValueError with the existing dataset-mismatch message.
Reuse the existing observations, golden_dataset, and ObservationSet helpers.
---
Outside diff comments:
In `@evaluation/src/orgmemory_eval/retrieval_recall.py`:
- Around line 191-194: Extract official_scorer’s write_json_atomically into a
shared orgmemory_eval report I/O module, preserving its
temporary-file-and-replace behavior. Update both official_scorer and the
report-writing flow in retrieval_recall to import and call this shared helper,
removing the direct mkdir/write_text implementation.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ebdf7fa2-ae2e-463b-af4b-b4a49602bd98
⛔ Files ignored due to path filters (2)
docs/increments/active/2026-08-05-multi-snapshot-query-prototype/plan.mdis excluded by!docs/**docs/increments/active/2026-08-05-multi-snapshot-query-prototype/results.mdis excluded by!docs/**
📒 Files selected for processing (9)
evaluation/README.mdevaluation/fixtures/retrieval-recall-golden-v1.jsonevaluation/pyproject.tomlevaluation/src/orgmemory_eval/official_cases.pyevaluation/src/orgmemory_eval/official_scorer.pyevaluation/src/orgmemory_eval/retrieval_recall.pyevaluation/tests/test_official_cases.pyevaluation/tests/test_official_scorer.pyevaluation/tests/test_retrieval_recall.py
💤 Files with no reviewable changes (1)
- evaluation/fixtures/retrieval-recall-golden-v1.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Treat the repository and runtime evidence as the engineering system of record; do not treat chat or Northstar as authoritative.
Before changing a domain, read its specification, test-coverage document, and applicable decision filenames.
Material decisions about domain boundaries, authorization, persistence, publication, concurrency, cache isolation, parity scope, or deployment require an independent architecture challenge and documented alternatives before implementation.
Do not use completed increment documents as the source for current behavior; use them only for history or archaeology.
Before using unfamiliar Spring Boot, Spring Modulith, Spring AI, Gradle, React, Vite, Tailwind, TypeScript, Next.js, or Fumadocs APIs, consult current official documentation, Context7, and the relevant verification skill.
Readdocs/guidelines/agent-safety.mdbefore retrieval, AI, MCP, permission, upload, graph, or export work; never commit secrets or customer data.
Keepddl-auto=validateand pair every persisted-model change with a Flyway migration.
Use the testing harness; a terminating clean test is the JVM context gate, andbootRunis not verification.
Files:
evaluation/pyproject.tomlevaluation/README.mdevaluation/tests/test_official_cases.pyevaluation/src/orgmemory_eval/official_cases.pyevaluation/tests/test_official_scorer.pyevaluation/tests/test_retrieval_recall.pyevaluation/src/orgmemory_eval/retrieval_recall.pyevaluation/src/orgmemory_eval/official_scorer.py
🪛 ast-grep (0.45.0)
evaluation/tests/test_official_cases.py
[info] 14-14: use jsonify instead of json.dumps for JSON output
Context: json.dumps(cases, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
evaluation/src/orgmemory_eval/official_scorer.py
[info] 464-464: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload, ensure_ascii=False, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (26)
evaluation/src/orgmemory_eval/official_cases.py (5)
11-20: LGTM!
56-62: LGTM!
64-76: LGTM!
83-118: LGTM!
121-127: LGTM!evaluation/tests/test_official_cases.py (1)
22-40: LGTM!evaluation/src/orgmemory_eval/official_scorer.py (9)
49-71: LGTM!
140-157: LGTM!
160-179: LGTM!
182-195: LGTM!
216-275: LGTM!
278-337: LGTM!
387-447: LGTM!
450-475: LGTM!
135-138: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-209): Generation of Error Message Containing Sensitive Information
Reachability: Internal
Do not echo transcript field values into the error message.
Line 138 interpolates the full pydantic
ValidationErrorinto the raisedValueError. Pydantic v2 error messages include aninput_valueexcerpt for the failing field. Ifanswer_textfails validation, or if a forbidden extra field carries answer content, the raw answer text reaches stderr and any CI log that captures the run.This contradicts the documented guarantee.
README.mdline 73 states that the report omits raw answers, andtest_official_scorer.pyline 94 asserts that noanswer_textappears in report cases. That control covers the report artifact only. It does not cover this error path. Deny-case answers are exactly the content the evidence-leak scoring is meant to keep out of artifacts.Report the line number and the failing field locations without the input values.
🛡️ Proposed redaction
try: rows.append(TranscriptRow.model_validate_json(line)) except ValidationError as failure: - raise ValueError(f"invalid transcript line {line_number}: {failure}") from failure + locations = sorted( + {".".join(str(part) for part in error["loc"]) for error in failure.errors()} + ) + raise ValueError( + f"invalid transcript line {line_number}: invalid fields {locations}" + ) from failureNote that
from failurekeeps the original exception chained. If the chained traceback is also printed, suppress it withfrom Noneand rely on the redacted message.Run the following script to confirm how pydantic 2.12.5 renders input values in these messages:
#!/bin/bash # Description: Check whether pydantic ValidationError text echoes the offending input value. set -euo pipefail fd -t f 'pyproject.toml' evaluation --exec cat -n {} python - <<'PY' try: import pydantic except ImportError: print("pydantic not installed in sandbox; verify against the project lockfile instead") raise SystemExit(0) print("pydantic", pydantic.VERSION) from pydantic import BaseModel, ConfigDict, ValidationError class Row(BaseModel): model_config = ConfigDict(extra="forbid") answer_text: str for payload in ('{"answer_text": 12345}', '{"answer_text": "ok", "leaked": "SENSITIVE-ANSWER-BODY"}'): try: Row.model_validate_json(payload) except ValidationError as failure: text = str(failure) print("---") print(text) print("echoes value:", "12345" in text or "SENSITIVE-ANSWER-BODY" in text) print("errors() locs:", [e["loc"] for e in failure.errors()]) PYevaluation/pyproject.toml (1)
19-19: LGTM!evaluation/README.md (2)
29-73: LGTM!
82-110: LGTM!evaluation/tests/test_official_scorer.py (3)
135-224: LGTM!
227-253: LGTM!
305-316: LGTM!evaluation/src/orgmemory_eval/retrieval_recall.py (3)
10-35: LGTM!
83-98: LGTM!
129-173: LGTM!evaluation/tests/test_retrieval_recall.py (2)
38-46: LGTM!
73-82: LGTM!
…rness # Conflicts: # docs/increments/active/2026-08-05-multi-snapshot-query-prototype/results.md # evaluation/tests/test_retrieval_recall.py
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TegamiThis repository uses Tegami to manage releases. When your changes affect published packages, add a changelog file under Create a changelog → · Changelog format Release preview
This PR does not add changelog files. Pending changelogs from other branches are included in the preview above. Run Managed by Tegami. |
1 participant
Summary
Loop C of the speed+correctness evaluation series:
demo/fixtures/public-evaluation.json(50 official hackathon cases; SHA-2566deb67ef…, file untouched — immutability test included) becomes the single evaluation source.orgmemory-official-evalCLI over a documented transcript JSONL contract (orgmemory.official-transcript.v1): per-case permission and citation verdicts, exact citation-set equality, P031 partial ≠ pass, median/observed-max latency+TTFT by difficulty and answer type (no small-N p95 claims), raw answers omitted from reports.DENY_EVIDENCE_LEAKtakes precedence.batch_evalcriteria (comprehensiveness/diversity/empowerment), pluggable, default off, zero network in tests.Implementation by Codex
gpt-5.6-sol(high) under handoff; coordinator-reviewed (Deny-semantics correction round-tripped before this PR). The live production sweep and scored report follow as the next loop iteration.Verification
uv run --frozen pytest56 PASS (coordinator re-ran independently);ruff checkPASS; terminatingclean testPASS; official fixture hash check PASS; no Java changes.skip-release: offline evaluation tooling only; no product behavior.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation