week_4: Module C (The Librarian) — C.2 cross-encoder reranker#957
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
Summary by CodeRabbit
WalkthroughThis PR adds a cross-encoder reranker (C.2) to the Librarian pipeline, integrating it into candidate retrieval, the CLI's dry-run pipeline, and the evaluation harness with rerank top-1 metrics. It also refactors schema-version validation and Pydantic coercion helpers, adds a database embedding-lookup method, and updates dependency pins. ChangesLibrarian cross-encoder reranking pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
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: 5
🧹 Nitpick comments (7)
application/utils/librarian/cross_encoder.py (1)
101-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
strict=Truetozip()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 winNon-stable tie-breaking in top-K selection.
np.argsortdefaults 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 withcross_encoder.py's explicit stable-sort guarantee forreranked[].🔧 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 valueMinor duplication in the validate-and-wrap pattern.
Both entry points repeat the same
isinstancecheck +model_validate+except ValidationError → MalformedKnowledgeItemErrorblock. 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_runflag is parsed but never used.
args.dry_run(Line 187-189) is defined but not referenced anywhere else inmain(). 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 valueAnnotate
OVERRIDESasClassVarto satisfy Ruff RUF012.Static analysis flags the mutable dict class attribute. Since it's never mutated, adding a
ClassVarannotation 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 winPin
sentence-transformersto 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 breakingtransformers/torchupgrades.🤖 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 | 🔵 TrivialDuplicated schema_version pattern validation across three envelopes.
KnowledgeItem._rfc_rules,LinkProposal._schema_version_pattern, andReviewItem._schema_version_patternall 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: strAlso 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
📒 Files selected for processing (37)
.env.exampleapplication/cmd/cre_main.pyapplication/database/db.pyapplication/tests/librarian/__init__.pyapplication/tests/librarian/candidate_retriever_test.pyapplication/tests/librarian/config_loader_test.pyapplication/tests/librarian/cross_encoder_test.pyapplication/tests/librarian/dataset_test.pyapplication/tests/librarian/explicit_link_resolver_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/tests/librarian/section_validator_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/candidate_retriever.pyapplication/utils/librarian/config_loader.pyapplication/utils/librarian/cross_encoder.pyapplication/utils/librarian/explicit_link_resolver.pyapplication/utils/librarian/hub_firewall.pyapplication/utils/librarian/knowledge_source.pyapplication/utils/librarian/schemas.pyapplication/utils/librarian/scoring.pyapplication/utils/librarian/section_validator.pycre.pyrequirements.txtscripts/benchmark_retriever.pyscripts/build_golden_dataset.pyscripts/evaluate_librarian.py
PRAteek-singHWY
added a commit
to PRAteek-singHWY/OpenCRE
that referenced
this pull request
Jul 3, 2026
- 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)
5 tasks
northdpole
approved these changes
Jul 9, 2026
Contributor
Author
Thank you @northdpole , will resolve these conflicts altogether once #937 gets merged. |
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.
- 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.
northdpole
force-pushed
the
gsocmodule_C_week_4
branch
from
31b6145 to
ea53658
Compare
northdpole
approved these changes
Jul 9, 2026
northdpole
left a comment
northdpole
left a comment
Collaborator
There was a problem hiding this comment.
Re-approved after W4-only rebase onto main (#937 merged). C.2 cross-encoder + live eval id-space fix look good.
northdpole
pushed a commit
that referenced
this pull request
Jul 9, 2026
- 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)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/build_golden_dataset.py (1)
271-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead branch:
stdis already"OTHER"before theif.The
if "Top 10" in name:branch reassignsstdto the same value it already holds, making the conditional a no-op.♻️ Simplify the assignment
std = "OTHER" -if "Top 10" in name: - std = "OTHER" # closest enum; not strictly ASVS/WSTG/NIST prefix = "top10" if "Top 10" in name else "cwe"🤖 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/build_golden_dataset.py` around lines 271 - 273, The `std` assignment in `build_golden_dataset.py` has a dead conditional because `std` is already set to "OTHER" before the `if "Top 10" in name:` check. Simplify the logic around this `std` initialization by removing the no-op branch or making the conditional assign a different value only when needed, and keep the behavior in the surrounding dataset-building code 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.
Inline comments:
In `@application/cmd/cre_main.py`:
- Around line 1130-1131: The fast-path lookup in resolve() is using CRE row UUID
keys from get_embeddings_by_doc_type(), but it needs external CRE IDs to match
inputs like 616-305. Update the known_ids setup in cre_main.py by translating
the embedding map keys from internal UUIDs to cre.external_id first, following
the same approach used in scripts/evaluate_librarian.py, so the resolver can hit
the explicit match path before falling back to semantic retrieval.
---
Nitpick comments:
In `@scripts/build_golden_dataset.py`:
- Around line 271-273: The `std` assignment in `build_golden_dataset.py` has a
dead conditional because `std` is already set to "OTHER" before the `if "Top 10"
in name:` check. Simplify the logic around this `std` initialization by removing
the no-op branch or making the conditional assign a different value only when
needed, and keep the behavior in the surrounding dataset-building code
unchanged.
🪄 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: b20aedf2-58d2-4af1-a1fc-aa906d55577e
📒 Files selected for processing (13)
application/cmd/cre_main.pyapplication/database/db.pyapplication/tests/librarian/config_loader_test.pyapplication/tests/librarian/cross_encoder_test.pyapplication/utils/librarian/__init__.pyapplication/utils/librarian/candidate_retriever.pyapplication/utils/librarian/cross_encoder.pyapplication/utils/librarian/knowledge_source.pyapplication/utils/librarian/schemas.pyapplication/utils/librarian/section_validator.pyrequirements.txtscripts/build_golden_dataset.pyscripts/evaluate_librarian.py
✅ Files skipped from review due to trivial changes (2)
- application/utils/librarian/init.py
- application/tests/librarian/config_loader_test.py
🚧 Files skipped from review as they are similar to previous changes (5)
- requirements.txt
- application/database/db.py
- application/utils/librarian/candidate_retriever.py
- application/tests/librarian/cross_encoder_test.py
- application/utils/librarian/cross_encoder.py
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/build_golden_dataset.py (1)
271-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead branch:
stdis already"OTHER"before theif.The
if "Top 10" in name:branch reassignsstdto the same value it already holds, making the conditional a no-op.♻️ Simplify the assignment
std = "OTHER" -if "Top 10" in name: - std = "OTHER" # closest enum; not strictly ASVS/WSTG/NIST prefix = "top10" if "Top 10" in name else "cwe"🤖 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/build_golden_dataset.py` around lines 271 - 273, The `std` assignment in `build_golden_dataset.py` has a dead conditional because `std` is already set to "OTHER" before the `if "Top 10" in name:` check. Simplify the logic around this `std` initialization by removing the no-op branch or making the conditional assign a different value only when needed, and keep the behavior in the surrounding dataset-building code 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.
Inline comments:
In `@application/cmd/cre_main.py`:
- Around line 1130-1131: The fast-path lookup in resolve() is using CRE row UUID
keys from get_embeddings_by_doc_type(), but it needs external CRE IDs to match
inputs like 616-305. Update the known_ids setup in cre_main.py by translating
the embedding map keys from internal UUIDs to cre.external_id first, following
the same approach used in scripts/evaluate_librarian.py, so the resolver can hit
the explicit match path before falling back to semantic retrieval.
---
Nitpick comments:
In `@scripts/build_golden_dataset.py`:
- Around line 271-273: The `std` assignment in `build_golden_dataset.py` has a
dead conditional because `std` is already set to "OTHER" before the `if "Top 10"
in name:` check. Simplify the logic around this `std` initialization by removing
the no-op branch or making the conditional assign a different value only when
needed, and keep the behavior in the surrounding dataset-building code
unchanged.
🪄 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: b20aedf2-58d2-4af1-a1fc-aa906d55577e
📒 Files selected for processing (13)
application/cmd/cre_main.pyapplication/database/db.pyapplication/tests/librarian/config_loader_test.pyapplication/tests/librarian/cross_encoder_test.pyapplication/utils/librarian/__init__.pyapplication/utils/librarian/candidate_retriever.pyapplication/utils/librarian/cross_encoder.pyapplication/utils/librarian/knowledge_source.pyapplication/utils/librarian/schemas.pyapplication/utils/librarian/section_validator.pyrequirements.txtscripts/build_golden_dataset.pyscripts/evaluate_librarian.py
✅ Files skipped from review due to trivial changes (2)
- application/utils/librarian/init.py
- application/tests/librarian/config_loader_test.py
🚧 Files skipped from review as they are similar to previous changes (5)
- requirements.txt
- application/database/db.py
- application/utils/librarian/candidate_retriever.py
- application/tests/librarian/cross_encoder_test.py
- application/utils/librarian/cross_encoder.py
🛑 Comments failed to post (1)
application/cmd/cre_main.py (1)
1130-1131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1. Check what extract_cre_refs extracts (external IDs vs UUIDs) rg -n "def extract_cre_refs" application/utils/librarian/ -A 15 # 2. Check the Embeddings model — what does cre_id store? rg -n "class Embeddings" application/database/db.py -A 20 # 3. Check how embeddings are stored (what cre_id is set to) rg -n "def add_embedding" application/database/db.py -A 15 # 4. Confirm the evaluation harness translates IDs but run_librarian does not rg -n "id_to_ext\|_to_ext\|known_ids" scripts/evaluate_librarian.py application/cmd/cre_main.py -C 3Repository: OWASP/OpenCRE
Length of output: 3270
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the explicit resolver and its ID format sed -n '1,140p' application/utils/librarian/explicit_link_resolver.py # Inspect the embeddings model and how embeddings are written/retrieved sed -n '173,230p' application/database/db.py sed -n '2461,2525p' application/database/db.py sed -n '2550,2615p' application/database/db.py # Inspect the caller that builds known_ids sed -n '1110,1160p' application/cmd/cre_main.py # Inspect the evaluation harness ID translation helpers and usage sed -n '1,220p' scripts/evaluate_librarian.pyRepository: OWASP/OpenCRE
Length of output: 21424
Translate embedding keys to external CRE IDs before building
known_ids
get_embeddings_by_doc_type()is keyed by the CRE row UUID (cre_id=db_object.id), butresolve()only matches external IDs like616-305, so the explicit fast path never hits and everything falls back to semantic retrieval.scripts/evaluate_librarian.pyalready maps UUIDs tocre.external_idbefore using the resolver;application/cmd/cre_main.pyneeds the same translation here.🤖 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/cmd/cre_main.py` around lines 1130 - 1131, The fast-path lookup in resolve() is using CRE row UUID keys from get_embeddings_by_doc_type(), but it needs external CRE IDs to match inputs like 616-305. Update the known_ids setup in cre_main.py by translating the embedding map keys from internal UUIDs to cre.external_id first, following the same approach used in scripts/evaluate_librarian.py, so the resolver can hit the explicit match path before falling back to semantic retrieval.
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.
Hi @northdpole — Week 4 of Module C. This one adds the cross-encoder that re-reads the shortlist C.1 produced.
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 leavescandidates[]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) —CrossEncoderRerankerover an injectedscore_fn, the same DI pattern as C.1'sembed_fn, so the module never imports torch and stays hermetically testable.build_cross_encoder_score_fnlazily loads the pinnedms-marco-MiniLM-L-6-v2. Stable sort so ties keep C.1's cosine order; typed errors; aRERANKER_NAMEaudit 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.py—get_embedding_contents_by_doc_type: the{id → embeddings_content}pair text the reranker scores against (mirrorsget_embeddings_by_doc_type).cre_main.py—run_librariannow 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— addsentence-transformers;__init__.pyscope note now covers C.2.Results (positive slice, live)
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)