feat(kb): document-level learning-goals fact extraction (RCA-11)#7
Merged
ramirocantu merged 2 commits intoJun 1, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request implements RCA-11, a document-level PDF fact extraction redesign with quality metrics infrastructure. It replaces per-page atomic fact extraction with document-level chunked extraction, adds concept/context fact discrimination, introduces extraction and tagging quality measurement modules, and provides an evaluation CLI harness. ChangesRCA-11 Extraction Refactor with Quality Metrics
🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🤖 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 `@app/services/eval/metrics.py`:
- Around line 121-155: In regression_blocks_pivot, guard against incomparable
runs by checking baseline.n_cases vs candidate.n_cases before computing deltas;
if they differ, return an EvalReport with blocks_pivot=True and a clear reason
like "n_cases mismatch: baseline=X candidate=Y" (populate baseline and
candidate, set mean_jaccard_delta and set_equality_delta to 0.0 or a neutral
value, and avoid applying the tolerance logic), otherwise proceed with the
existing delta/tolerance checks.
In `@scripts/run_rca11_extract_eval.py`:
- Around line 58-60: The _load_transcriptions function returns transcriptions in
fixture order which may be unstable; modify it to parse the raw list into
(int(page), str(text)) tuples and then sort that list by the integer page before
returning. Locate _load_transcriptions and ensure the returned list is sorted
(e.g., using sorted(..., key=lambda t: t[0]) or list.sort()) so downstream
chunking operates on page-ordered transcriptions.
In `@SPEC.md`:
- Around line 28-45: In SPEC.md update the fenced code block that lists files
and symbols so it includes a language identifier (e.g., change ``` to ```text)
to satisfy MD040; locate the block containing entries like
app/services/kb/pdf_ingest.py, EXTRACTOR_VERSION, ingest_pdf,
FactExtraction.facts and add the language tag at the opening fence.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a8ddb19-7c76-44fa-a1d2-22a2ecc280f3
📒 Files selected for processing (12)
SPEC.mdapp/services/eval/__init__.pyapp/services/eval/extract_metrics.pyapp/services/eval/metrics.pyapp/services/kb/pdf_ingest.pyscripts/run_rca11_extract_eval.pytests/fixtures/rca11_reference_transcriptions.jsontests/test_eval_extract_metrics.pytests/test_fence_guards.pytests/test_kb_inbox.pytests/test_kb_pdf_ingest.pytests/test_pdf_ingest_api.py
Rework the PDF atomic-fact extractor (pdf-vision-v1 -> v2) to stop over-extracting lecture-context noise. - Vision transcription enriched to describe diagrams/figures/graphs so visual facts survive the transcription bottleneck. - Extraction is now document-level + learning-goals-driven synthesis with a concept/context discernment gate; context facts are dropped at persist and counted (IngestReport.dropped_context_facts). - Add app/services/eval extraction-quality harness + extract_metrics + scripts/run_rca11_extract_eval.py; reference transcription fixture. - Revive app/services/eval (removed from the deleted-surfaces fence). Measured v1->v2 on Phys McatKing.pdf: 124 -> 51 facts, keyword-noise fraction 0.27 -> 0.065. mise run check green (666 passed). SPEC.md seeded (issue-scoped, B1 logged). Spun off RCA-12/13/14. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ramirocantu
force-pushed
the
ramirocanturamirez/rca-11-atomic-fact-extractor-over-extracts-examplelecture-context
branch
from
June 1, 2026 03:23
7d0bc15 to
b3ce0d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/services/eval/metrics.py (1)
121-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against non-comparable runs before applying tolerance gate.
Lines 133-134 compute deltas even when
baseline.n_casesandcandidate.n_casesdiffer, which can produce incorrectblocks_pivotdecisions from incomparable samples. Add a fail-closed mismatch check first.🛡️ Proposed fix to add n_cases guard
def regression_blocks_pivot( baseline: EvalRunResult, candidate: EvalRunResult, *, mean_jaccard_tolerance: float = MEAN_JACCARD_TOLERANCE, set_equality_tolerance: float = SET_EQUALITY_TOLERANCE, ) -> EvalReport: """V-L2: candidate must not drop more than the tolerance on either metric. Returns an `EvalReport` with `blocks_pivot=True` and a human-readable `reason` when either delta exceeds the tolerance. """ mean_delta = candidate.mean_jaccard - baseline.mean_jaccard set_delta = candidate.set_equality_rate - baseline.set_equality_rate + + if baseline.n_cases != candidate.n_cases: + return EvalReport( + baseline=baseline, + candidate=candidate, + mean_jaccard_delta=mean_delta, + set_equality_delta=set_delta, + blocks_pivot=True, + reason=( + f"n_cases mismatch: baseline={baseline.n_cases}, " + f"candidate={candidate.n_cases}" + ), + ) reasons: list[str] = []🤖 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 `@app/services/eval/metrics.py` around lines 121 - 156, In regression_blocks_pivot, guard against comparing runs with different sample sizes by checking baseline.n_cases vs candidate.n_cases before computing mean_jaccard_delta and set_equality_delta; if they differ, return an EvalReport (using the existing EvalReport constructor) with baseline and candidate, blocks_pivot=True and a clear reason like "n_cases mismatch: baseline X vs candidate Y" (you can set the delta fields to 0.0 or None consistently with other callers), otherwise proceed with the existing delta/tolerance logic.
🤖 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 `@app/services/eval/metrics.py`:
- Around line 87-96: The function jaccard contains an unreachable check after
computing union = a | b; remove the dead check "if not union: return 1.0" (and
its comment) because the prior guard "if not a and not b: return 1.0" guarantees
union cannot be empty; update the function to compute and return len(a & b) /
len(union) after the initial empty-both guard (keep the docstring and signature
for jaccard(a: frozenset[str], b: frozenset[str]) -> float).
In `@tests/test_kb_inbox.py`:
- Line 72: The test currently asserts a hard-coded extractor version string via
assert facts[0].extractor_version == "pdf-vision-v2"; replace the literal with
the shared EXTRACTOR_VERSION constant so tests follow the single source of
truth—locate the assertion in tests/test_kb_inbox.py and change it to compare
against EXTRACTOR_VERSION (ensure the test imports or references the
EXTRACTOR_VERSION constant from the module where it’s defined).
In `@tests/test_pdf_ingest_api.py`:
- Line 67: Replace the hard-coded "pdf-vision-v2" in the assertion that checks
body["extractor_version"] with the shared extractor version constant (e.g.,
EXTRACTOR_VERSION) so tests follow version bumps automatically; import that
constant into tests/test_pdf_ingest_api.py from the module that defines it (the
shared extractor constants module used by the production code) and use
EXTRACTOR_VERSION in the assert instead of the literal string.
---
Duplicate comments:
In `@app/services/eval/metrics.py`:
- Around line 121-156: In regression_blocks_pivot, guard against comparing runs
with different sample sizes by checking baseline.n_cases vs candidate.n_cases
before computing mean_jaccard_delta and set_equality_delta; if they differ,
return an EvalReport (using the existing EvalReport constructor) with baseline
and candidate, blocks_pivot=True and a clear reason like "n_cases mismatch:
baseline X vs candidate Y" (you can set the delta fields to 0.0 or None
consistently with other callers), otherwise proceed with the existing
delta/tolerance logic.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c9e160eb-b7d7-4090-8f3e-18d1b24e0faa
📒 Files selected for processing (11)
app/services/eval/__init__.pyapp/services/eval/extract_metrics.pyapp/services/eval/metrics.pyapp/services/kb/pdf_ingest.pyscripts/run_rca11_extract_eval.pytests/fixtures/rca11_reference_transcriptions.jsontests/test_eval_extract_metrics.pytests/test_fence_guards.pytests/test_kb_inbox.pytests/test_kb_pdf_ingest.pytests/test_pdf_ingest_api.py
- metrics.py: fail-closed n_cases mismatch guard in regression_blocks_pivot - metrics.py: drop unreachable empty-union check in jaccard - run_rca11_extract_eval.py: sort transcriptions by page in loader - tests: use shared EXTRACTOR_VERSION constant instead of literal - SPEC.md: add language tag to §I fenced block (MD040) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ramirocantu
deleted the
ramirocanturamirez/rca-11-atomic-fact-extractor-over-extracts-examplelecture-context
branch
June 1, 2026 16:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Reworks the PDF atomic-fact extractor (
pdf-vision-v1→pdf-vision-v2) to stop over-extracting lecture-context noise. Closes RCA-11.The pipeline was mechanically sound (RCA-10) but per-page extraction faithfully restated every sentence as a "fact" — no learning-goals framing, no example/meta exclusion. A 6-page physics PDF produced 124 facts, ~33 of them lecture-context noise (avg tag confidence 0.28, 81/108
manual_review).Changes
_EXTRACT_MAX_DOC_CHARS) and extracted in one pass that reasons about the document's learning goals and may synthesize standalone claims. Nano single-tasks.kind ∈ {concept, context};context(worked-example / figure / problem-restatement prose) is dropped at persist and counted inIngestReport.dropped_context_facts. No DB migration.app/services/eval/withextract_metrics.py(keyword-noise fraction, yield, before/after delta) +scripts/run_rca11_extract_eval.py(capture / replay modes) + a reference-transcription fixture for cheap deterministic re-runs.EXTRACTOR_VERSION→pdf-vision-v2.Measurement (V-L2,
Phys McatKing.pdf)Kept facts read as durable physics knowledge. Manual review surfaced a few confidently wrong facts (e.g. electron-shell KE) and garbled formulas — a distinct failure mode (false signal, not noise) tracked in RCA-13.
Tests
mise run checkgreen — 666 passed / 1 skipped, ruff + pyright clean. New: kind-carries, malformed-kind-defaults, doc-level-single-call, chunking, context-drop, eval metrics; reworked token-accounting + version assertions.Notes / follow-ups
content_hashexcludesextractor_versionandPdfSource.sha256short-circuits re-ingest before extraction — bumping to v2 does not re-extract existing PDFs (delete + re-ingest to refresh).app.services.evalremoved from the deleted-surfaces fence guard (revived intentionally).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores