week_1: Module C (The Librarian) — RFC contracts + eval harness + golden dataset#922
Merged
Merged
Conversation
dataset Contracts + regression ruler before any pipeline code, per the OIE RFC's 'test before the code' directive. RFC OWASP#734 envelopes (KnowledgeItem in, LinkProposal/ReviewItem out) as Pydantic v2, drift-guarded against the vendored owasp-graph schemas; TRACT hub-firewall + multi-link scoring; 319-row golden dataset derived from standards_cache.sqlite with --check drift detection. One prod edit: pydantic>=2,<3 pin.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThis PR adds Module C (The Librarian): RFC schemas and Pydantic models, config and helper utilities, golden dataset generation and validation, an evaluation harness, and related tests and dependency updates. ChangesModule C — The Librarian
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
application/utils/librarian/scoring.py (1)
21-22: 💤 Low valueOptional: Remove unreachable defensive check.
Since line 19 returns early when both sets are empty, the union on line 20 cannot be empty, making lines 21-22 unreachable. You may remove this check for clarity.
♻️ Simplify by removing unreachable code
def jaccard(a: Sequence[str], b: Sequence[str]) -> float: sa, sb = set(a), set(b) if not sa and not sb: return 1.0 union = sa | sb - if not union: - return 0.0 return len(sa & sb) / len(union)🤖 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 `@application/utils/librarian/scoring.py` around lines 21 - 22, Remove the unreachable defensive check that returns 0.0 when "union" is falsy: because there is already an early return when both input sets are empty (the return at line 19), "union" cannot be empty here—delete the if not union: return 0.0 block in the scoring calculation (the lines referencing the variable "union") so the function's flow is clearer and avoids dead code.application/tests/librarian/config_loader_test.py (1)
48-53: ⚡ Quick winAdd boundary tests for numeric domain constraints.
Once loader invariants are enforced, this suite should assert failures for out-of-range numeric env values (e.g.,
LINK_THRESHOLD=1.2, negativeTOP_K,TOP_K_RERANK > TOP_K_RETRIEVAL) to lock behavior and prevent regressions.🤖 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 `@application/tests/librarian/config_loader_test.py` around lines 48 - 53, Extend the test suite around load_config by adding boundary tests that assert it raises ValueError for out-of-range numeric env vars: use mock.patch.dict to set LINK_THRESHOLD="1.2" (above 1.0) and assertRaises(ValueError); set CRE_LIBRARIAN_TOP_K_RETRIEVAL to a negative value (e.g., "-1") and assertRaises(ValueError); and set CRE_LIBRARIAN_TOP_K_RERANK greater than CRE_LIBRARIAN_TOP_K_RETRIEVAL (e.g., "5" vs "3") and assertRaises(ValueError). Keep the new tests alongside test_bad_int_env_raises and call load_config() inside each assertRaises block so loader invariants for LINK_THRESHOLD, CRE_LIBRARIAN_TOP_K_RETRIEVAL, and CRE_LIBRARIAN_TOP_K_RERANK are enforced.
🤖 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 `@application/tests/librarian/fixtures/golden_dataset.schema.json`:
- Around line 23-24: The JSON Schema currently allows empty strings/arrays for
fields that GoldenDatasetRow validation requires to be non-empty; update the
schema entries for "explicit_cre_ref" and "prior_text" to enforce non-empty
strings (add "minLength": 1) and add "minItems": 1 to any array fields that must
be non-empty (notably the "expected.cre_ids" schema for decision="linked"); scan
the other occurrences mentioned (around the other schema blocks at the indicated
ranges) and apply the same constraints so the JSON Schema matches
GoldenDatasetRow's truthy checks.
In `@application/utils/librarian/_rfc_schemas/source-ref.json`:
- Around line 20-25: The schema currently only requires "repo" and "commit_sha"
when type is "github" but does not require "url" for other types; update the
JSON Schema in the "allOf" array so that when "type" is "url" or "rss" (or when
type is not "github") the schema's "then" adds "required": ["url"]; locate the
existing conditional block using "allOf" / "if" / "then" in source-ref.json (the
block that checks { "properties": { "type": { "const": "github" } } }) and add
an additional conditional (or replace with an "if" using
"properties":{"type":{"enum":["url","rss"]}}) whose "then" enforces the "url"
required property for those source types.
In `@application/utils/librarian/config_loader.py`:
- Around line 22-33: The load_config function returns a LibrarianConfig without
validating numeric invariants; update load_config to validate parsed values
(from os.getenv) and raise clear ValueError messages when invalid: ensure
top_k_retrieval and top_k_rerank are positive ints, top_k_rerank <=
top_k_retrieval, batch_size > 0, link_threshold is between 0.0 and 1.0
inclusive, and ece_target and conformal_alpha are between 0.0 and 1.0 inclusive;
perform these checks immediately after converting the env values and before
constructing LibrarianConfig so callers get immediate, descriptive failures
(include the offending variable name and its value in each error).
In `@scripts/build_golden_dataset.py`:
- Around line 325-327: The code currently silently skips curated ASVS rows when
_fetch_asvs_cre(conn, entry["asvs_section_id"]) returns None; change this to
fail fast by raising a clear exception (or calling sys.exit with an error
message) that includes the missing asvs_section_id and any relevant entry
identifiers so the pipeline stops and the issue is obvious; update both
occurrences (the block around cre = _fetch_asvs_cre(...) at the shown spot and
the similar block at the other occurrence around lines 356-358) to perform the
fail-fast behavior instead of continue.
- Around line 445-447: When running in --check mode (the block that inspects
Path(args.out).read_text()), handle a missing output file explicitly instead of
letting read_text() raise: first test Path(args.out).exists() and if it does not
exist print or log a clear error and exit non-zero (e.g. sys.exit(1)) with a
descriptive message, otherwise continue to read_text() and compare contents;
update the code in the args.check branch (referencing args.check,
Path(args.out), and read_text()) to perform the existence check and clean exit.
In `@scripts/evaluate_librarian.py`:
- Around line 67-68: The slice currently uses a truthy check on args.limit so
passing --limit 0 is treated as no limit; change the condition around the rows
slicing to explicitly check for None (e.g., if args.limit is not None) so that
args.limit == 0 correctly results in rows = rows[:0] and yields zero evaluated
rows; update the block that references args.limit and rows to use this explicit
None check.
---
Nitpick comments:
In `@application/tests/librarian/config_loader_test.py`:
- Around line 48-53: Extend the test suite around load_config by adding boundary
tests that assert it raises ValueError for out-of-range numeric env vars: use
mock.patch.dict to set LINK_THRESHOLD="1.2" (above 1.0) and
assertRaises(ValueError); set CRE_LIBRARIAN_TOP_K_RETRIEVAL to a negative value
(e.g., "-1") and assertRaises(ValueError); and set CRE_LIBRARIAN_TOP_K_RERANK
greater than CRE_LIBRARIAN_TOP_K_RETRIEVAL (e.g., "5" vs "3") and
assertRaises(ValueError). Keep the new tests alongside test_bad_int_env_raises
and call load_config() inside each assertRaises block so loader invariants for
LINK_THRESHOLD, CRE_LIBRARIAN_TOP_K_RETRIEVAL, and CRE_LIBRARIAN_TOP_K_RERANK
are enforced.
In `@application/utils/librarian/scoring.py`:
- Around line 21-22: Remove the unreachable defensive check that returns 0.0
when "union" is falsy: because there is already an early return when both input
sets are empty (the return at line 19), "union" cannot be empty here—delete the
if not union: return 0.0 block in the scoring calculation (the lines referencing
the variable "union") so the function's flow is clearer and avoids dead code.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: 90fc550c-1b08-4bf8-aa33-273c1b81a94c
📒 Files selected for processing (24)
application/tests/librarian/__init__.pyapplication/tests/librarian/config_loader_test.pyapplication/tests/librarian/dataset_test.pyapplication/tests/librarian/fixtures/golden_dataset.jsonapplication/tests/librarian/fixtures/golden_dataset.schema.jsonapplication/tests/librarian/fixtures/sample_knowledge_queue.jsonlapplication/tests/librarian/hub_firewall_test.pyapplication/tests/librarian/schemas_test.pyapplication/tests/librarian/scoring_test.pyapplication/utils/librarian/__init__.pyapplication/utils/librarian/_rfc_schemas/knowledge-item.jsonapplication/utils/librarian/_rfc_schemas/link-proposal.jsonapplication/utils/librarian/_rfc_schemas/locator.jsonapplication/utils/librarian/_rfc_schemas/proposed-link.jsonapplication/utils/librarian/_rfc_schemas/review-item.jsonapplication/utils/librarian/_rfc_schemas/source-ref.jsonapplication/utils/librarian/config_loader.pyapplication/utils/librarian/hub_firewall.pyapplication/utils/librarian/knowledge_source.pyapplication/utils/librarian/schemas.pyapplication/utils/librarian/scoring.pyrequirements.txtscripts/build_golden_dataset.pyscripts/evaluate_librarian.py
…nts, fail-fast build, edge cases
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
application/tests/librarian/config_loader_test.py (1)
20-23: ⚡ Quick winPrefer
AttributeErrorover genericException.Frozen dataclasses raise
AttributeError(orFrozenInstanceError, which subclasses it in Python 3.10+) when attributes are mutated. Using the genericExceptionis too permissive and could mask other unexpected errors.♻️ Proposed refinement
def test_config_is_frozen(self): cfg = load_config() - with self.assertRaises(Exception): + with self.assertRaises(AttributeError): cfg.link_threshold = 0.5 # type: ignore[misc]🤖 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 `@application/tests/librarian/config_loader_test.py` around lines 20 - 23, The test test_config_is_frozen currently uses self.assertRaises(Exception) which is too broad; change it to expect AttributeError (or FrozenInstanceError if you prefer Python-3.10-specific) when attempting to mutate the frozen dataclass returned by load_config(); update the assertion in test_config_is_frozen so the with self.assertRaises(...) targets AttributeError and keep the mutation line (cfg.link_threshold = 0.5) unchanged.
🤖 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.
Nitpick comments:
In `@application/tests/librarian/config_loader_test.py`:
- Around line 20-23: The test test_config_is_frozen currently uses
self.assertRaises(Exception) which is too broad; change it to expect
AttributeError (or FrozenInstanceError if you prefer Python-3.10-specific) when
attempting to mutate the frozen dataclass returned by load_config(); update the
assertion in test_config_is_frozen so the with self.assertRaises(...) targets
AttributeError and keep the mutation line (cfg.link_threshold = 0.5) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 17638fe7-c137-4db7-a644-ccc816fcfa61
📒 Files selected for processing (6)
application/tests/librarian/config_loader_test.pyapplication/tests/librarian/fixtures/golden_dataset.schema.jsonapplication/utils/librarian/config_loader.pyapplication/utils/librarian/scoring.pyscripts/build_golden_dataset.pyscripts/evaluate_librarian.py
💤 Files with no reviewable changes (1)
- application/utils/librarian/scoring.py
🚧 Files skipped from review as they are similar to previous changes (4)
- application/utils/librarian/config_loader.py
- application/tests/librarian/fixtures/golden_dataset.schema.json
- scripts/evaluate_librarian.py
- scripts/build_golden_dataset.py
northdpole
approved these changes
Jul 1, 2026
northdpole
left a comment
northdpole
left a comment
Collaborator
There was a problem hiding this comment.
Strong W1 — RFC drift guard, golden dataset, hub-firewall, and eval harness are exactly what we want before linking logic lands. This sets the bar for the stacked W2/W3 PRs. A few quick fixes before merge:
Please fix before merge
1. Pin test dependencies in requirements.txt
schemas_test.py / dataset_test.py import jsonschema and referencing, but neither is declared directly (only transitive today). Add explicit pins so CI stays reproducible.
2. Document librarian env vars in .env.example
Mirror Module B: document the seven CRE_LIBRARIAN_* settings (CRE_LIBRARIAN_CROSSENCODER_MODEL, TOP_K_RETRIEVAL, TOP_K_RERANK, LINK_THRESHOLD, BATCH_SIZE, ECE_TARGET, CONFORMAL_ALPHA) with defaults matching config_loader.py.
3. Q-D scoring rule — confirmed; clear the TODO
The default multi-link rule is approved: correct iff Jaccard(expected, predicted) ≥ 0.5 and top-1 ∈ expected set. Please remove the provisional TODO(Q-D) in scoring.py (and any “pending mentor” wording) so the harness is the canonical baseline going forward.
4. CodeRabbit follow-ups (where still applicable)
Please address any open bot comments that still apply on the current diff — at minimum evaluate_librarian.py --limit 0 (is not None), and anything else you agree is valid.
Cross-module note (not blocking W1)
Module B’s interim QueuePayload on main is minimal (source string + text + confidence); your KnowledgeQueueItem mirrors the future knowledge_queue SQL row — B-W4 should align the table/write path with KnowledgeQueueItem, not the slim payload.
Production
No concerns — additive only, nothing wired into web/worker/import yet.
Ping when 1–3 (and doc/example) are in; happy to merge right after.
…confirm Q-D rule Maintainer review on OWASP#922: - Pin jsonschema>=4.26.0 and referencing>=0.37.0 (imported directly by librarian schemas_test.py / dataset_test.py, previously only transitive) so CI stays reproducible. - Document the seven CRE_LIBRARIAN_* settings in .env.example with defaults matching config_loader.py. - Q-D multi-link rule confirmed (Jaccard >= 0.5 AND top-1 in expected set): drop the provisional TODO / "pending mentor" wording; harness scoring is now the canonical baseline.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.env.example:
- Around line 56-67: The Librarian example block is stale and the keys are out
of order, which also triggers dotenv-linter warnings. Update the comment to
reflect that application/utils/librarian/config_loader.py already consumes these
CRE_LIBRARIAN_* settings, then reorder the environment variables in the same
contract/order used by the real loader or config definition so the example
matches runtime behavior.
🪄 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.yml
Review profile: CHILL
Plan: Pro
Run ID: cf7ee009-e73d-4248-add8-28204507f222
📒 Files selected for processing (3)
.env.exampleapplication/utils/librarian/scoring.pyrequirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- application/utils/librarian/scoring.py
Contributor
Author
|
Thanks for the review, @northdpole, All the before-merge items are in (
Also noted the cross-module point - I'll make sure B-W4 aligns the write path with |
5 tasks
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
2 participants
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.
week_1: Module C (The Librarian) — contracts + eval harness + golden dataset
Overview
This is the Week 1 deliverable for Module C (The Librarian). Following the OIE RFC's "build the test before the code" principle, this PR establishes the data contracts and the evaluation harness for Module C — before any linking logic. There is no retriever, no embeddings, and no decision logic here; those land in later weeks and will be measured against what this PR provides.
Module C consumes a
KnowledgeItemfrom Module B and produces one of two outputs: aLinkProposal(auto-link to the OpenCRE graph) or aReviewItem(route to human review in Module D). This PR makes those contracts concrete, guards them against drift from the RFC, and builds an evaluation harness plus a real 319-row dataset so that every future change to Module C is verifiable in CI.Scope: 24 files. The only change to existing code is a one-line
pydanticversion pin. No frontend, no migrations, no pipeline wiring.What changed
schemas.pyKnowledgeItem(input),LinkProposal/ReviewItem(output) — plus shared sub-models. The RFC's conditional rules are enforced as validators._rfc_schemas/*.json(6) +schemas_test.pyconfig_loader.pyCRE_LIBRARIAN_*settings (thresholds, model name, top-k) loaded into one frozen config.knowledge_source.pyKnowledgeSourceinterface with a fixture-backed stub. The real database-backed reader arrives later and yields the same shape.evaluate_librarian.pyhub_firewall.pyscoring.pybuild_golden_dataset.py+fixtures/golden_dataset.json(and its schema)standards_cache.sqlite. A--checkmode fails CI if the committed file drifts from the database.*_test.pyfiles + fixturesrequirements.txtpydantic→pydantic>=2,<3(the contracts use Pydantic v2).How the pieces connect
flowchart TB subgraph RFC["RFC #734 schemas (vendored, pinned)"] rfc["_rfc_schemas/*.json"] end subgraph CONTRACTS["Contracts and config"] schemas["schemas.py<br/>KnowledgeItem · LinkProposal · ReviewItem"] config["config_loader.py"] ksource["knowledge_source.py"] end subgraph DATA["Golden dataset"] db[("standards_cache.sqlite")] build["build_golden_dataset.py<br/>derive and --check"] gold["golden_dataset.json<br/>319 rows"] end subgraph HARNESS["Eval harness"] eval["evaluate_librarian.py"] firewall["hub_firewall.py"] scoring["scoring.py"] end subgraph TESTS["Tests (48 passing)"] t1["schemas_test.py (drift guard)"] t2["dataset_test.py"] t3["scoring_test.py"] t4["hub_firewall_test.py"] t5["config_loader_test.py"] end rfc -->|"validated against"| schemas db --> build --> gold schemas --> eval config --> eval firewall --> eval scoring --> eval gold --> eval schemas --> ksource rfc --> t1 schemas --> t1 gold --> t2 build -. "--check" .-> t2 scoring --> t3 firewall --> t4 config --> t5There are three flows. First, the vendored RFC schemas are what
schemas.pyis validated against — this is the drift guard. Second, the dataset is derived from the database intogolden_dataset.json, with--checkguarding against drift. Third, the harness loads the dataset and composes the contracts, config, firewall, and scorer.The dataset, by slice
What is intentionally not here
Retriever, embeddings, cross-encoder, decision engine, CLI wiring, and database models/migrations are all later weeks. Week 1 is the contracts and the evaluation harness only.
How to verify locally
Note on the dataset design
The dataset is derived from the database but committed as a static file, so CI can load the JSON directly without needing the database;
--checkis what proves the committed file still matches the database. From here on, every Module C PR is scored against these 319 real rows.