Skip to content

week_4: Module C (The Librarian) — C.2 cross-encoder reranker#957

Open
PRAteek-singHWY wants to merge 17 commits into
OWASP:mainfrom
PRAteek-singHWY:gsocmodule_C_week_4
Open

week_4: Module C (The Librarian) — C.2 cross-encoder reranker#957
PRAteek-singHWY wants to merge 17 commits into
OWASP:mainfrom
PRAteek-singHWY:gsocmodule_C_week_4

Conversation

@PRAteek-singHWY

@PRAteek-singHWY PRAteek-singHWY commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Hi @northdpole — Week 4 of Module C. This one adds the cross-encoder that re-reads the shortlist C.1 produced.

Stacked on #937 (Week 3). Based on gsocmodule_C_week_3, so until #922#925#937 land the diff shows the earlier commits too — I'll rebase onto main as they merge, shrinking it to just the W4 files.

The problem it fixes

Week 3's search is fast but rough. The bi-encoder fingerprints the section and each CRE separately, then compares them — great for narrowing hundreds of CREs down to 20, but it never actually reads a section and a CRE together, so the exact ordering inside those 20 is unreliable. The real answer might be sitting at #7, not #1.

What this does

Takes those 20 candidates and reads each one side-by-side with the section as a single combined input, and scores "do these two actually match?" — then re-sorts the 20 and keeps the best 5. It fills the reranked[] slot we deliberately left empty in Week 3, and leaves candidates[] untouched so the pre-rerank shortlist stays auditable.

Two model kinds, one line: the bi-encoder (W3) fingerprints each thing alone — fast, whole-hub, rough. The cross-encoder (W4) reads the pair together — slow, so we only run it on the 20, but much more accurate. W3 skims 20 résumés to make a shortlist; W4 interviews those 20 one-on-one to pick the top 5.

What changed

  • cross_encoder.py (new)CrossEncoderReranker over an injected score_fn, the same DI pattern as C.1's embed_fn, so the module never imports torch and stays hermetically testable. build_cross_encoder_score_fn lazily loads the pinned ms-marco-MiniLM-L-6-v2. Stable sort so ties keep C.1's cosine order; typed errors; a RERANKER_NAME audit tag.
  • cross_encoder_test.py (new) — 9 hermetic tests (re-ordering by cross-encoder score, top-N truncation, tie stability, audit shape, and the missing-text / score-count failure modes).
  • db.pyget_embedding_contents_by_doc_type: the {id → embeddings_content} pair text the reranker scores against (mirrors get_embeddings_by_doc_type).
  • cre_main.pyrun_librarian now reranks C.1's audit and logs the reranked top-N per section.
  • evaluate_librarian.py — v1 full pipeline; reports the live rerank top-1 alongside recall@k. Offline path unchanged.
  • requirements.txt — add sentence-transformers; __init__.py scope note now covers C.2.

Results (positive slice, live)

retrieval recall@20 (C.1): any-hit 285/292 (98%), all-hit 274/292 (94%)
rerank top-1     (C.2): 220/292 (75%)   (target >= 0.80)

Measured live against a populated standards_cache.sqlite (428 CRE embeddings, gemini/gemini-embedding-001, dim 3072), hub-firewall ON, top_n_rerank=5.

Reading it: the retriever puts the right CRE in the top-20 98% of the time, so the shortlist the reranker sees is almost always complete. The cross-encoder then lands the correct CRE at #1 for 75% of rows — solid, but 5 points under the 0.80 target. The gap is the honest state at W4: the reranker sometimes demotes a correct near-tie. Calibration (W5) + the threshold experiment (W7) are the levers to close it; the target is a W6/W8 gate, not a W4 merge blocker. The reranker logic itself is fully covered offline by the hermetic tests above.

Not here (later weeks)

  • Calibration + the confident yes/no decision (C.3–C.4, W5–W6).
  • Graph writes / wiring into the worker (W8) — still dry-run only; the CLI stays opt-in and only costs the embedding API on manual runs.

PRAteek-singHWY and others added 11 commits June 9, 2026 17:17
  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.
…inkResolver

The C.0 deterministic input boundary, per the proposal's W2/W3 'data
preparation layer' (named validator, not normalizer: the RFC assigns text
normalization to Module A; C validates and adapts, never transforms text).

- section_validator.py: typed-error validation of both upstream shapes
  (B's knowledge_queue row + RFC KnowledgeItem envelope) into an internal
  Section; synthesizes RFC identity fields (chunk_id/artifact_id/source/
  locator) from B's reduced row; strips volatile audit metadata.
- explicit_link_resolver.py: deterministic ddd-ddd fast path, no ML.
  Fail-safe: only a single known reference auto-links; unknown or
  conflicting references route to review.
- evaluate_librarian.py: harness now runs every golden row through C.0,
  prints per-slice validation pass rate, and gates the explicit slice at
  100% resolver correctness (exit 1 on regression). Gate: PASS 5/5.
- Table-driven tests for every rejection class and resolver outcome.
  mypy --strict clean, black clean, 66 tests green.
…ipeline switch

The semantic search step. For sections with no explicit CRE id, embed the
text and cosine-rank the CRE vector hub, returning the top-K (default 20)
candidates as an RFC RetrievalAudit (reranked[] empty until W4).

Two interchangeable backends behind one retrieve() seam, selected by
CRE_LIBRARIAN_RETRIEVER_BACKEND:
- CandidateRetriever: in-memory sklearn cosine (SQLite/CI/harness)
- PgVectorRetriever: Postgres-side <=> cosine over embedding_vec
build_retriever() is the factory. Reuses OpenCRE's embedding stack
(PromptHandler.get_text_embeddings, db.get_embeddings_by_doc_type).

Dim gate fails loudly on empty/ragged hubs and query/hub width mismatch.
CLI switch (--run_librarian / --librarian_dry_run) runs the pipeline
dry-run; harness --use_live_embeddings measures retrieval recall@k.

The pgvector schema migration is a W8 deliverable (validated against real
Postgres there); W3 ships only the code path, unit-tested via a fake
connection. 82 librarian tests pass.
Resolve the CodeRabbit review comments on the Module C PR:
- schemas.py: enforce RFC format parity — AnyUrl for url fields,
  datetime for committed_at/filtered_at/classified_at/created_at
- section_validator.py: replace assert with a typed
  MalformedKnowledgeItemError guard (survives python -O)
- knowledge_source.py: skip+log malformed JSONL rows instead of
  aborting the whole iteration on ValidationError
- config_loader_test.py: isolate os.environ and assert the specific
  FrozenInstanceError
- dataset_test.py: add a timeout to the determinism subprocess call
- build_golden_dataset.py: rename unused loop var node_id -> _node_id
- section_validator_test.py: assert committed_at as the typed datetime
…ASP#925)

GHSA pydantic ReDoS affects >=2.0.0,<2.4.0; first patched in 2.4.0.
…n-Postgres

Maintainer nit on OWASP#937: CRE_LIBRARIAN_RETRIEVER_BACKEND=pgvector is selectable
via env, but the embedding_vec column doesn't land until W8. Emit a loud
startup warning at pipeline-switch time when the backend is pgvector on a
non-Postgres (e.g. SQLite) database, since retrieve() would otherwise fail
with an opaque SQL error.
- config_loader_test: assert retriever_backend in both the defaults and the
  override tests (was the only LibrarianConfig field left unchecked).
- __init__.py: refresh the stale 'No linking logic yet' scope note to reflect
  W1 contracts + W2 C.0 boundary + W3 C.1 retriever.

Other OWASP#937 CodeRabbit findings were already addressed on this branch
(knowledge_source model_validate_json error handling, schemas AnyUrl/datetime
fields, section_validator assert->if, test_config_is_frozen isolation +
FrozenInstanceError, dataset_test subprocess timeout, build_golden_dataset
unused loop var).
Address northdpole's production note on OWASP#937: record at the run_librarian
entrypoint that it is opt-in CLI only (not on Procfile, not wired into web or
worker until W8) and that the paid embedding API cost is incurred only on manual
runs, never by the running deployment.
The C.1 bi-encoder (W3) fingerprints the section and each CRE separately, so
the ordering inside the top-K shortlist is rough — the right CRE can sit at OWASP#7.
C.2 reads each (section, candidate-CRE) pair together, re-sorts the shortlist by
that score, and keeps the best N; it fills RetrievalAudit.reranked[] (the slot
C.1 left empty) and leaves candidates[] untouched for audit.

- cross_encoder.py: CrossEncoderReranker over an injected score_fn (mirrors
  C.1's embed_fn DI seam — the module never imports torch), plus a lazy
  build_cross_encoder_score_fn wiring ms-marco-MiniLM-L-6-v2. Typed errors,
  RERANKER_NAME audit tag, stable sort so ties keep cosine order.
- cross_encoder_test.py: 9 hermetic tests (reorder, top-N, tie stability,
  audit shape, missing-text and score-count failure modes).
- db.py: get_embedding_contents_by_doc_type — {id -> embeddings_content},
  the pair text the reranker scores against (mirrors get_embeddings_by_doc_type).
- cre_main.run_librarian: rerank C.1's audit and log the reranked top-N.
- evaluate_librarian.py v1: full C.1 -> C.2 pipeline; report live rerank top-1
  alongside recall@k (W4 target >= 0.80). Offline path unchanged.
- requirements.txt: add sentence-transformers.
- __init__.py: scope note now covers C.2.
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 060b213d-c6ca-4065-8342-3bb3896b7d19

📥 Commits

Reviewing files that changed from the base of the PR and between d278ad2 and 31b6145.

📒 Files selected for processing (1)
  • .env.example
✅ Files skipped from review due to trivial changes (1)
  • .env.example

Summary by CodeRabbit

  • New Features

    • Added an optional Librarian workflow to the CLI for candidate retrieval and ranking.
    • Introduced configurable environment settings for retrieval, reranking, and tuning.
    • Added a benchmark and evaluation flow for comparing retrieval results and scoring datasets.
  • Bug Fixes

    • Improved validation for incoming knowledge items and section data.
    • Added safeguards to prevent leaking matching text in evaluation outputs.
    • Strengthened schema checks and dataset consistency rules for more reliable runs.

Walkthrough

This PR adds “Module C — The Librarian,” including configuration, RFC-aligned schemas, semantic retrieval and reranking, deterministic explicit resolution, section validation, CLI wiring, and golden-dataset generation/evaluation.

Changes

Module C — Librarian pipeline

Layer / File(s) Summary
Configuration and RFC schema contracts
.env.example, application/utils/librarian/__init__.py, application/utils/librarian/config_loader.py, application/utils/librarian/schemas.py, application/utils/librarian/_rfc_schemas/*, requirements.txt, application/tests/librarian/config_loader_test.py, application/tests/librarian/schemas_test.py
Adds CRE_LIBRARIAN_* env vars, LibrarianConfig/load_config(), Pydantic RFC/internal schema models, vendored RFC JSON Schemas, module documentation, dependency pins, and matching config/schema tests.
Semantic candidate retriever
application/database/db.py, application/utils/librarian/candidate_retriever.py, scripts/benchmark_retriever.py, application/tests/librarian/candidate_retriever_test.py
Implements CandidatePool, CandidateRetriever, PgVectorRetriever, and build_retriever; adds get_embedding_contents_by_doc_type and a benchmark script comparing backends; hermetic tests cover ranking, validation, and pgvector wiring.
Cross-encoder reranker
application/utils/librarian/cross_encoder.py, application/tests/librarian/cross_encoder_test.py
Adds CrossEncoderReranker to rescore shortlist candidates and build_cross_encoder_score_fn for sentence-transformers models, with tests for ordering and failure modes.
Explicit CRE resolution and hub firewall
application/utils/librarian/explicit_link_resolver.py, application/utils/librarian/hub_firewall.py, application/tests/librarian/explicit_link_resolver_test.py, application/tests/librarian/hub_firewall_test.py
Adds deterministic CRE-reference extraction/resolution and hub leak filtering utilities, with tests covering resolution outcomes and leak detection.
Section validation and knowledge source
application/utils/librarian/section_validator.py, application/utils/librarian/knowledge_source.py, application/tests/librarian/section_validator_test.py, application/tests/librarian/fixtures/sample_knowledge_queue.jsonl, application/utils/librarian/scoring.py, application/tests/librarian/scoring_test.py
Adds validators converting queue rows and KnowledgeItem envelopes into Section, a JSONL fixture-backed source, extra fixture rows, and scoring helpers with boundary tests.
CLI wiring and end-to-end librarian run
application/cmd/cre_main.py, cre.py
Adds Librarian CLI flags and run_librarian() orchestration for explicit resolution, semantic retrieval/reranking, and dry-run logging.
Golden dataset generation and evaluation harness
scripts/build_golden_dataset.py, application/tests/librarian/fixtures/golden_dataset.schema.json, application/tests/librarian/dataset_test.py, scripts/evaluate_librarian.py
Adds a deterministic dataset builder, a JSON Schema for dataset rows, CI shape/determinism checks, and an evaluation script scoring explicit/retrieval predictions against the golden dataset.

Estimated code review effort: 4 (Complex) | ~75 minutes

Suggested reviewers: Pa04rth

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly names the main change: Module C.2 cross-encoder reranking.
Description check ✅ Passed The description is directly about the Module C.2 cross-encoder reranker and related integration work.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (7)
application/utils/librarian/cross_encoder.py (1)

101-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add strict=True to zip() per static analysis hint.

Ruff flags this zip() call (B905). Lengths are already validated above, so this is purely defensive/lint hygiene.

🔧 Proposed fix
         reranked = [
             c.model_copy(update={"score_rerank": float(s)})
-            for c, s in zip(candidates, scores)
+            for c, s in zip(candidates, scores, strict=True)
         ]
🤖 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/cross_encoder.py` around lines 101 - 104, The
reranking comprehension in cross_encoder.py uses zip() without an explicit
strict setting, which Ruff flags with B905. Update the zip(candidates, scores)
call inside the reranked list construction to use strict=True, keeping the
existing length validation intact, so the CrossEncoder rerank path remains
lint-clean and defensive.

Source: Linters/SAST tools

application/utils/librarian/candidate_retriever.py (1)

156-160: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Non-stable tie-breaking in top-K selection.

np.argsort defaults to quicksort (not stable), so cosine ties can be ordered arbitrarily/nondeterministically between runs. This directly contradicts the reproducibility goal the audit trail exists for, and is inconsistent with cross_encoder.py's explicit stable-sort guarantee for reranked[].

🔧 Proposed fix
-        top_idx = np.argsort(scores)[-k:][::-1]
+        top_idx = np.argsort(scores, kind="stable")[-k:][::-1]
🤖 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/candidate_retriever.py` around lines 156 - 160,
The top-K selection in candidate_retriever.py is using np.argsort without a
stable sort, so tied cosine scores can return in a nondeterministic order.
Update the ranking logic around the query cosine_similarity and top_idx
computation to use a stable descending sort, and keep the existing k cap
behavior. Make the tie-breaking deterministic in the same spirit as
cross_encoder.py’s stable reranked[] handling so repeated runs produce the same
CRE ordering.
application/utils/librarian/section_validator.py (1)

110-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor duplication in the validate-and-wrap pattern.

Both entry points repeat the same isinstance check + model_validate + except ValidationError → MalformedKnowledgeItemError block. Could be factored into a small private helper (e.g. _validate_or_raise(model_cls, raw)), but with only two call sites the current duplication is minor.

Also applies to: 153-157

🤖 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/section_validator.py` around lines 110 - 114, The
same validate-and-wrap pattern is duplicated in the section validator flow,
where raw items are checked with isinstance and then passed through
model_validate with ValidationError converted to MalformedKnowledgeItemError.
Refactor this shared logic in the relevant validator methods, likely around the
KnowledgeQueueItem handling and the other call site mentioned in the comment,
into a small private helper such as _validate_or_raise(model_cls, raw), and
update both entry points to call it.
scripts/evaluate_librarian.py (1)

178-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

--dry_run flag is parsed but never used.

args.dry_run (Line 187-189) is defined but not referenced anywhere else in main(). The harness never writes regardless of this flag (per the docstring: "no writes (always true pre-W8)"), so the flag is currently a no-op that may mislead users into thinking it toggles behavior.

🤖 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/evaluate_librarian.py` around lines 178 - 207, The --dry_run option
in main() is currently a no-op, so either wire args.dry_run into the harness
flow where writes would occur or remove the flag and its help text if dry-run is
permanently always enabled. Update the argument parsing in
evaluate_librarian.main and any downstream write path to consistently respect
this flag, using args.dry_run as the deciding signal.
application/tests/librarian/config_loader_test.py (1)

30-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Annotate OVERRIDES as ClassVar to satisfy Ruff RUF012.

Static analysis flags the mutable dict class attribute. Since it's never mutated, adding a ClassVar annotation resolves the lint warning.

🧹 Proposed fix
+from typing import ClassVar
+
 class TestConfigLoaderOverrides(unittest.TestCase):
-    OVERRIDES = {
+    OVERRIDES: ClassVar[dict] = {
         "CRE_LIBRARIAN_RETRIEVER_BACKEND": "pgvector",
🤖 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 30 - 39, The
class attribute OVERRIDES in config_loader_test is a mutable dict that Ruff
flags as a non-instance field; annotate it as ClassVar to make its intent
explicit. Update the test class definition where OVERRIDES is declared so the
attribute is marked as a ClassVar while keeping the existing constant values
unchanged, which will satisfy RUF012 and clarify it is not meant to be mutated
per instance.

Source: Linters/SAST tools

requirements.txt (1)

95-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin sentence-transformers to a tested release range. The Module C.2 cross-encoder depends on it directly, and leaving it unpinned makes installs non-reproducible and leaves you open to breaking transformers/torch upgrades.

🤖 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 `@requirements.txt` at line 95, The dependency entry for sentence-transformers
is unpinned, which makes installs non-reproducible and can break the Module C.2
cross-encoder path. Update the requirements list to constrain
sentence-transformers to a tested version range that is compatible with the
existing transformers and torch stack, and keep the dependency definition in the
same requirements entry so future installs resolve consistently.
application/utils/librarian/schemas.py (1)

213-221: 📐 Maintainability & Code Quality | 🔵 Trivial

Duplicated schema_version pattern validation across three envelopes.

KnowledgeItem._rfc_rules, LinkProposal._schema_version_pattern, and ReviewItem._schema_version_pattern all independently re-check _SCHEMA_VERSION_RE.match(self.schema_version). Consider extracting a shared mixin/base class with this validator to avoid drift if the pattern or error message changes later.

♻️ Proposed refactor
+class _SchemaVersioned(BaseModel):
+    schema_version: str
+
+    `@model_validator`(mode="after")
+    def _schema_version_pattern(self):
+        if not _SCHEMA_VERSION_RE.match(self.schema_version):
+            raise ValueError(r"schema_version must match ^0\.\d+\.\d+$")
+        return self
+
+
-class KnowledgeItem(BaseModel):
+class KnowledgeItem(_SchemaVersioned):
     ...
-    schema_version: str
     ...
-class LinkProposal(BaseModel):
+class LinkProposal(_SchemaVersioned):
     ...
-    schema_version: str
     ...
-class ReviewItem(BaseModel):
+class ReviewItem(_SchemaVersioned):
     ...
-    schema_version: str

Also applies to: 239-243, 263-267

🤖 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/schemas.py` around lines 213 - 221, The
schema_version regex check is duplicated in KnowledgeItem._rfc_rules,
LinkProposal._schema_version_pattern, and ReviewItem._schema_version_pattern, so
extract that validation into a shared base class or mixin and have all three
envelopes reuse it. Keep the existing status-specific checks in KnowledgeItem,
but centralize the schema_version match and error message to avoid drift and
make future changes consistent.
🤖 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/cmd/cre_main.py`:
- Around line 1078-1088: In cre_main.py, the pgvector backend check in the
retriever setup only emits a warning when the database dialect is not
postgresql, but this mismatch should fail fast; update the backend selection
logic around RetrieverBackend.pgvector to raise a clear configuration error
immediately after detecting the non-Postgres dialect, rather than continuing to
build the retriever and proceed into the batch. Use the existing mismatch
handling style from build_retriever and make the error message explicit about
pgvector requiring Postgres with the embedding_vec column.
- Around line 1022-1047: Update the run_librarian docstring so it matches the
implementation: it currently says C.2 cross-encoder rerank is not built yet, but
the function creates CrossEncoderReranker and calls rerank. Revise the pipeline
description to mention reranking is implemented, and keep the Ops note accurate
about manual CLI use and embedding API cost.
- Around line 1137-1149: The semantic retrieval/rerank block in the section loop
is not protected by the existing section-level guard, so exceptions from
retriever.retrieve() or reranker.rerank() can abort the whole dry-run. Wrap this
path in the same per-section try/catch used elsewhere in cre_main.py, using the
local section processing around semantic, audit, and logger.info. On failure,
log a warning with the section identifier and error details, then mark the
section as rejected/skipped so later sections and the summary still run.

In `@application/utils/librarian/knowledge_source.py`:
- Around line 39-47: The warning in the knowledge source parsing path is logging
the full ValidationError, which can expose raw row contents. Update the
exception handling in the KnowledgeQueueItem.model_validate_json block to avoid
passing exc directly to logger.warning; instead log
exc.errors(include_input=False) or only the error locations/types so malformed
data fields are not emitted.

In `@scripts/build_golden_dataset.py`:
- Around line 197-210: The `_fetch_asvs_cre` helper is currently masking
ambiguous mappings by using `ORDER BY c.external_id` with `LIMIT 1`, so it can
silently return the wrong CRE when a section maps to multiple entries. Update
`_fetch_asvs_cre` in `build_golden_dataset.py` to detect when `section_id`
resolves to more than one CRE and raise a `ValueError` instead of picking the
first result, matching the fail-loud behavior already used by `build_explicit`
and `build_update` when `cre` is missing.

---

Nitpick comments:
In `@application/tests/librarian/config_loader_test.py`:
- Around line 30-39: The class attribute OVERRIDES in config_loader_test is a
mutable dict that Ruff flags as a non-instance field; annotate it as ClassVar to
make its intent explicit. Update the test class definition where OVERRIDES is
declared so the attribute is marked as a ClassVar while keeping the existing
constant values unchanged, which will satisfy RUF012 and clarify it is not meant
to be mutated per instance.

In `@application/utils/librarian/candidate_retriever.py`:
- Around line 156-160: The top-K selection in candidate_retriever.py is using
np.argsort without a stable sort, so tied cosine scores can return in a
nondeterministic order. Update the ranking logic around the query
cosine_similarity and top_idx computation to use a stable descending sort, and
keep the existing k cap behavior. Make the tie-breaking deterministic in the
same spirit as cross_encoder.py’s stable reranked[] handling so repeated runs
produce the same CRE ordering.

In `@application/utils/librarian/cross_encoder.py`:
- Around line 101-104: The reranking comprehension in cross_encoder.py uses
zip() without an explicit strict setting, which Ruff flags with B905. Update the
zip(candidates, scores) call inside the reranked list construction to use
strict=True, keeping the existing length validation intact, so the CrossEncoder
rerank path remains lint-clean and defensive.

In `@application/utils/librarian/schemas.py`:
- Around line 213-221: The schema_version regex check is duplicated in
KnowledgeItem._rfc_rules, LinkProposal._schema_version_pattern, and
ReviewItem._schema_version_pattern, so extract that validation into a shared
base class or mixin and have all three envelopes reuse it. Keep the existing
status-specific checks in KnowledgeItem, but centralize the schema_version match
and error message to avoid drift and make future changes consistent.

In `@application/utils/librarian/section_validator.py`:
- Around line 110-114: The same validate-and-wrap pattern is duplicated in the
section validator flow, where raw items are checked with isinstance and then
passed through model_validate with ValidationError converted to
MalformedKnowledgeItemError. Refactor this shared logic in the relevant
validator methods, likely around the KnowledgeQueueItem handling and the other
call site mentioned in the comment, into a small private helper such as
_validate_or_raise(model_cls, raw), and update both entry points to call it.

In `@requirements.txt`:
- Line 95: The dependency entry for sentence-transformers is unpinned, which
makes installs non-reproducible and can break the Module C.2 cross-encoder path.
Update the requirements list to constrain sentence-transformers to a tested
version range that is compatible with the existing transformers and torch stack,
and keep the dependency definition in the same requirements entry so future
installs resolve consistently.

In `@scripts/evaluate_librarian.py`:
- Around line 178-207: The --dry_run option in main() is currently a no-op, so
either wire args.dry_run into the harness flow where writes would occur or
remove the flag and its help text if dry-run is permanently always enabled.
Update the argument parsing in evaluate_librarian.main and any downstream write
path to consistently respect this flag, using args.dry_run as the deciding
signal.
🪄 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: 0a523fa1-7511-47d6-8c56-2cf37ae03123

📥 Commits

Reviewing files that changed from the base of the PR and between 0e16c2e and aabd1d0.

📒 Files selected for processing (37)
  • .env.example
  • application/cmd/cre_main.py
  • application/database/db.py
  • application/tests/librarian/__init__.py
  • application/tests/librarian/candidate_retriever_test.py
  • application/tests/librarian/config_loader_test.py
  • application/tests/librarian/cross_encoder_test.py
  • application/tests/librarian/dataset_test.py
  • application/tests/librarian/explicit_link_resolver_test.py
  • application/tests/librarian/fixtures/golden_dataset.json
  • application/tests/librarian/fixtures/golden_dataset.schema.json
  • application/tests/librarian/fixtures/sample_knowledge_queue.jsonl
  • application/tests/librarian/hub_firewall_test.py
  • application/tests/librarian/schemas_test.py
  • application/tests/librarian/scoring_test.py
  • application/tests/librarian/section_validator_test.py
  • application/utils/librarian/__init__.py
  • application/utils/librarian/_rfc_schemas/knowledge-item.json
  • application/utils/librarian/_rfc_schemas/link-proposal.json
  • application/utils/librarian/_rfc_schemas/locator.json
  • application/utils/librarian/_rfc_schemas/proposed-link.json
  • application/utils/librarian/_rfc_schemas/review-item.json
  • application/utils/librarian/_rfc_schemas/source-ref.json
  • application/utils/librarian/candidate_retriever.py
  • application/utils/librarian/config_loader.py
  • application/utils/librarian/cross_encoder.py
  • application/utils/librarian/explicit_link_resolver.py
  • application/utils/librarian/hub_firewall.py
  • application/utils/librarian/knowledge_source.py
  • application/utils/librarian/schemas.py
  • application/utils/librarian/scoring.py
  • application/utils/librarian/section_validator.py
  • cre.py
  • requirements.txt
  • scripts/benchmark_retriever.py
  • scripts/build_golden_dataset.py
  • scripts/evaluate_librarian.py

Comment thread application/cmd/cre_main.py
Comment thread application/cmd/cre_main.py
Comment thread application/cmd/cre_main.py
Comment thread application/utils/librarian/knowledge_source.py
Comment thread scripts/build_golden_dataset.py Outdated
- run_librarian: fix stale docstring (C.2 rerank is built), guard the
  semantic retrieve/rerank per-section so one bad section can't abort the
  dry-run batch
- knowledge_source: log ValidationError.errors(include_input=False), never
  the raw queue row (no content leak)
- build_golden_dataset: fail loud on ambiguous ASVS->CRE mappings instead of
  silently picking one (matches build_explicit/build_update)
- candidate_retriever: stable descending sort so tied cosine scores are
  deterministic
- cross_encoder: zip(..., strict=True) (B905)
- config_loader_test: annotate OVERRIDES as ClassVar (RUF012)
- schemas: centralize the schema_version check across the three envelopes
- section_validator: extract _validate_or_raise helper for both call sites
- requirements: pin sentence-transformers>=5.0,<6.0 (tested with 5.6)
- evaluate_librarian: drop the no-op --dry_run flag (harness never writes)
…k top-1

The embeddings pool and cross-encoder cre_texts are keyed by the CRE
internal UUID, but the golden dataset expects external_ids (e.g. 616-305).
Without translating, every comparison missed (recall@20 and top-1 read 0%).
Map both via cre.id->external_id (pass-through for DBs already keyed by
external_id) so the live numbers are measurable and reproducible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant