week_6: Module C (The Librarian) — C.4 decision engine + golden-set decision gate - #990
Open
PRAteek-singHWY wants to merge 15 commits into
Open
week_6: Module C (The Librarian) — C.4 decision engine + golden-set decision gate#990PRAteek-singHWY wants to merge 15 commits into
PRAteek-singHWY wants to merge 15 commits into
Conversation
… once report_retrieval_recall and report_calibration each built the live pipeline (DB + embedding model + cross-encoder) independently, loading it twice and reranking every positive row twice per --use_live_embeddings run. Build it once in main and pass (retriever, reranker) into both reports, matching _build_live_pipeline's stated intent. Behavior-preserving: recall@20 285/292, rerank top-1 220/292, ECE 0.046 PASS unchanged.
…ecision gate C.3 (Week 5) produces one honest, calibrated confidence per chunk; C.4 turns it into the action — auto-link into the OpenCRE graph, or route to a human — which is the accuracy gate of the whole pipeline. - decision_engine.py: pure `decide(confidence, candidates, *, threshold, adversarial, update_ambiguous) -> DecisionResult`. Links the top-1 iff confidence >= threshold AND candidates exist AND no blocking flag; otherwise reviews with a reason_code. Reason precedence NO_CANDIDATES > ADVERSARIAL_FLAG > UPDATE_AMBIGUOUS > BELOW_THRESHOLD. Frozen result, versioned ENGINE_NAME, custom DecisionError — mirrors the C.1/C.2/C.3 model-free seams. Does not import the C.3 scaler (confidence-in -> decision-out), so it is hermetically testable. - decision_engine_test.py: 14 hermetic tests — table-driven over every confidence/flag combination, the inclusive >= boundary, all four reason codes, precedence order, and the input guards. - evaluate_librarian.py: additive report_decision_accuracy — fits T on positive+hard_negative, runs the live C.1->C.4 decision over the golden set, and reports overall agreement plus auto-link recall vs review recall (a single accuracy hides that at tau=0.80 the softmax top-1 mass of a correct-but-close winner is often ~0.5, so correct positives route to review — the safe direction; W7 tunes tau). Informational, not a gate: the SafetyGuard flags are not wired yet, so flag-based reason codes lag until that lands. Emitters (LinkProposal/ReviewItem writers) and the C.0->C.4 pipeline glue follow in a stacked week_6b PR.
PRAteek-singHWY
requested review from
Pa04rth,
northdpole,
paoga87 and
robvanderveer
as code owners
July 23, 2026 12:46
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:
Summary by CodeRabbit
WalkthroughAdds temperature-scaling calibration, confidence-based link/review decisions, hermetic tests, and live evaluation reporting with shared retrieval and reranking audits. ChangesLibrarian calibration and routing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
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: 1
🧹 Nitpick comments (2)
application/utils/librarian/calibration/temperature.py (1)
63-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between
_softmax_topandprobabilities.Both implement the same "asarray + empty check + softmax" logic independently.
confidence()could derive fromprobabilities()instead of a separate module-level helper, keeping the empty-shortlist guard in one place.♻️ Suggested consolidation
def confidence(self, logits: Sequence[float]) -> float: """P(the top candidate is correct) — the top-1 mass of the softmax. This is the number the W6 decision engine thresholds on. """ - return _softmax_top(logits, self.temperature) + return float(self.probabilities(logits).max())Also applies to: 105-117
🤖 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/calibration/temperature.py` around lines 63 - 68, Consolidate the duplicated shortlist conversion, empty-check, and softmax logic by removing or bypassing `_softmax_top` and deriving `confidence()` from the existing `probabilities()` implementation. Ensure `probabilities()` remains the single guard for empty candidate shortlists while preserving the top-1 probability result and temperature behavior.scripts/evaluate_librarian.py (1)
234-244: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftLive retrieve+rerank (and the temperature fit) is redundantly recomputed 2-3x per row.
report_decision_accuracyrebuilds the exact samecal_rowsset and rerunsretriever.retrieve/reranker.rerankper row to refit a secondTemperatureScaler, duplicating workreport_calibration(called right before it inmain, L458-459) already did. Then its owngradedloop rerunsretrieve/rerankagain for rows that overlap withcal_rows(e.g. positive-slice rows with an expected decision). Since retrieval/reranking against a live embedding model + cross-encoder is the expensive part this harness gates behind--use_live_embeddings, this triples model calls for no functional benefit — the fit and audits are deterministic given the same inputs.Consider having
report_calibrationreturn the fittedTemperatureScaler(and/or the per-row audits) soreport_decision_accuracyreuses them instead of recomputing, and caching each row'sretrieve+rerankresult by row id so thecal_rows/gradedloops don't redo live calls for the same row.Also applies to: 292-333
🤖 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 234 - 244, Refactor report_calibration and report_decision_accuracy to reuse the fitted TemperatureScaler and per-row rerank audits instead of rerunning retrieve and rerank. Have report_calibration return the scaler and/or cached audits, pass them from main into report_decision_accuracy, and ensure overlapping cal_rows and graded rows retrieve each row only once, keyed by a stable row identifier.
🤖 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/utils/librarian/calibration/__init__.py`:
- Around line 1-13: Update the package docstring in the module-level
documentation to describe the implemented shortlist-wide softmax calibration
used by temperature.py, replacing the single-logit sigmoid formula and related
claims. Explain that logits are scaled by a fitted scalar temperature and
normalized across each candidate shortlist, while preserving the existing
purpose, NLL fitting, and ECE context.
---
Nitpick comments:
In `@application/utils/librarian/calibration/temperature.py`:
- Around line 63-68: Consolidate the duplicated shortlist conversion,
empty-check, and softmax logic by removing or bypassing `_softmax_top` and
deriving `confidence()` from the existing `probabilities()` implementation.
Ensure `probabilities()` remains the single guard for empty candidate shortlists
while preserving the top-1 probability result and temperature behavior.
In `@scripts/evaluate_librarian.py`:
- Around line 234-244: Refactor report_calibration and report_decision_accuracy
to reuse the fitted TemperatureScaler and per-row rerank audits instead of
rerunning retrieve and rerank. Have report_calibration return the scaler and/or
cached audits, pass them from main into report_decision_accuracy, and ensure
overlapping cal_rows and graded rows retrieve each row only once, keyed by a
stable row identifier.
🪄 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 Plus
Run ID: 3c77096d-5a49-4d8f-9eee-e6a67592cb12
📒 Files selected for processing (7)
application/tests/librarian/decision_engine_test.pyapplication/tests/librarian/temperature_test.pyapplication/utils/librarian/__init__.pyapplication/utils/librarian/calibration/__init__.pyapplication/utils/librarian/calibration/temperature.pyapplication/utils/librarian/decision_engine.pyscripts/evaluate_librarian.py
…string The calibration/__init__.py docstring still described the rejected single-logit `p = sigmoid(z/T)` approach. temperature.py actually calibrates the softmax over the whole shortlist (`p = softmax(logits / T)`, confidence = top-1 mass) — the sigmoid-on-one-logit approach cannot be calibrated by temperature alone. Docstring now matches the implementation.
This was referenced Jul 25, 2026
northdpole
reviewed
Jul 31, 2026
northdpole
left a comment
northdpole
left a comment
Collaborator
There was a problem hiding this comment.
Maintainer review — Module C Week 6 (#990)
Decision engine looks solid: precedence, inclusive τ, guards, and table tests are clean. No blocking logic bugs in the unique Week-6 surface.
Depends on #974 for the stack base (please rebase after that gate fix lands). One docstring nit inline.
Non-blocking note: report_decision_accuracy intentionally always returns 0 (informational until SafetyGuard + τ tuning) — fine; just don't confuse that with the ECE gate in #974.
| Calibration + decision routing (C.3-C.4, W5-W6) onward is not built yet. | ||
| W5 (C.3): confidence calibration — temperature scaling maps a rerank logit to | ||
| an honest probability (fit by NLL on the golden set, gated ECE < 0.10). | ||
| Decision routing (C.4, W6) onward is not built yet. |
Collaborator
There was a problem hiding this comment.
Nit — package docstring is stale
This still says "Decision routing (C.4, W6) onward is not built yet." Week 6 adds decide() here. Please update the scope blurb to mention C.4 / W6 (and leave W6b emitter/pipeline / W8 writers as not-yet if you prefer).
Contributor
Author
There was a problem hiding this comment.
Fixed in 86d3d5b5. Added the W6 (C.4) scope line for decide() and moved the not-yet marker down to the W6b emitter/pipeline glue and the W8 queue/graph writers. Will rebase onto gsocmodule_C_week_5 once #974 lands.
…tring scope The application/utils/librarian package docstring still said "Decision routing (C.4, W6) onward is not built yet", but W6 adds the C.4 decision engine (decide()) in this package. Add the W6 (C.4) scope line and move the not-yet marker to the W6b emitter/pipeline glue and the W8 queue/graph writers.
…hortlists, dedupe softmax Three nitpicks from the bot review, none of which changed a metric: - evaluate_librarian: the live retrieve+rerank was recomputed per report. The models were already built once in main, but report_retrieval_recall and report_calibration each re-ran the pipeline over the positive slice, so every positive row paid for two cross-encoder passes. live_audits() now retrieves and reranks each row once, keyed by golden row id, and both reports read the same shortlists. Rows without an audit no longer count toward a report's denominator, so the printed fractions cannot divide by unscored rows. - temperature: _softmax_top duplicated the empty-shortlist guard and the softmax already in TemperatureScaler.probabilities. Both now route through one _softmax_at helper, and confidence() derives from probabilities() so the two can never disagree. - temperature is now clean under the --strict mypy the coding guidelines ask for: annotated _paired's return and the bounds tuple, re-asserted the array type over untyped scipy, and hoisted the label conversion out of the fit objective (it was re-validated on every optimiser iteration anyway). Behaviour is unchanged: 113 librarian tests pass and the hermetic harness run still exits 0. (cherry picked from commit 2bbc76e)
…on gate must fail
report_calibration returned 0 ("skipped") when the live calibration set was
degenerate (single-class labels, empty after dropping empty shortlists, or a
--slice with one class). Under --use_live_embeddings that let a run exit 0 without
the ECE < 0.10 gate ever running, so CI could greenwash a live run in which
calibration was never checked. A skipped gate now returns 1 (fail), with a message
stating the row/class counts, so exit 0 means the gate actually ran and passed.
(cherry picked from commit 62abf2d)
…he live reports report_decision_accuracy rebuilt the positive + hard_negative calibration set from its own retrieve+rerank pass and fit its own temperature, duplicating what report_calibration had already done a few lines earlier. On a live run that meant a third pipeline pass over the calibration slices and two independent fits of the same T, with nothing guaranteeing the two agreed. Now there is one of each: - calibration_set() extracts the (shortlist, label) derivation, so the C.3 gate and the C.4 report read the same pairs off the same shared audits. - report_calibration returns (status, scaler); report_decision_accuracy takes the fitted scaler instead of fitting its own, so C.4 thresholds on exactly the T the ECE gate measured. - A degenerate set yields (1, None) and C.4 is skipped with a message: no fitted T means no honest confidence to threshold, and the run has already failed. - The live audit set now covers expected-decision rows too. C.4 grades those and they are not confined to the positive/hard_negative slices, so keying them off the calibration slices alone would have dropped them. Adds evaluate_harness_test.py. The live reports only run under --use_live_embeddings, so nothing exercised their wiring — which is exactly the code that has to share one pipeline pass and one T across three reports. A counting stub asserts the pipeline is called once per row and not once per report, that only the two calibration slices enter the fit, and that a degenerate set returns status 1 with no scaler rather than reporting success. Verified the last one fails if the gate is flipped back to 0. 134 librarian tests pass; the hermetic harness run still exits 0.
PRAteek-singHWY
added a commit
to PRAteek-singHWY/OpenCRE
that referenced
this pull request
Aug 4, 2026
…hortlists, dedupe softmax Three nitpicks from the bot review, none of which changed a metric: - evaluate_librarian: the live retrieve+rerank was recomputed per report. The models were already built once in main, but report_retrieval_recall and report_calibration each re-ran the pipeline over the positive slice, so every positive row paid for two cross-encoder passes. live_audits() now retrieves and reranks each row once, keyed by golden row id, and both reports read the same shortlists. Rows without an audit no longer count toward a report's denominator, so the printed fractions cannot divide by unscored rows. - temperature: _softmax_top duplicated the empty-shortlist guard and the softmax already in TemperatureScaler.probabilities. Both now route through one _softmax_at helper, and confidence() derives from probabilities() so the two can never disagree. - temperature is now clean under the --strict mypy the coding guidelines ask for: annotated _paired's return and the bounds tuple, re-asserted the array type over untyped scipy, and hoisted the label conversion out of the fit objective (it was re-validated on every optimiser iteration anyway). Behaviour is unchanged: 113 librarian tests pass and the hermetic harness run still exits 0.
PRAteek-singHWY
added a commit
to PRAteek-singHWY/OpenCRE
that referenced
this pull request
Aug 4, 2026
…hortlists, dedupe softmax Three nitpicks from the bot review, none of which changed a metric: - evaluate_librarian: the live retrieve+rerank was recomputed per report. The models were already built once in main, but report_retrieval_recall and report_calibration each re-ran the pipeline over the positive slice, so every positive row paid for two cross-encoder passes. live_audits() now retrieves and reranks each row once, keyed by golden row id, and both reports read the same shortlists. Rows without an audit no longer count toward a report's denominator, so the printed fractions cannot divide by unscored rows. - temperature: _softmax_top duplicated the empty-shortlist guard and the softmax already in TemperatureScaler.probabilities. Both now route through one _softmax_at helper, and confidence() derives from probabilities() so the two can never disagree. - temperature is now clean under the --strict mypy the coding guidelines ask for: annotated _paired's return and the bounds tuple, re-asserted the array type over untyped scipy, and hoisted the label conversion out of the fit objective (it was re-validated on every optimiser iteration anyway). Behaviour is unchanged: 113 librarian tests pass and the hermetic harness run still exits 0. (cherry picked from commit 2bbc76e) (cherry picked from commit ae6bb69)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
application/tests/librarian/evaluate_harness_test.py (1)
211-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the decision-report behavior that these tests name.
test_grades_expected_decision_rows_off_shared_auditspasses ifreport_decision_accuracy()returns0without reporting any metrics. Assert the reported counts or return structured metrics.
test_no_graded_rows_is_not_an_errorcreates an audit for a row that_golden_row()marks aslinked. Thegradedlist is therefore nonempty. Pass an empty audit map to execute the no-graded-rows branch.As per coding guidelines, “New behavior and importers should follow a test-first workflow.”
🤖 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/evaluate_harness_test.py` around lines 211 - 247, Strengthen the tests around harness.report_decision_accuracy by asserting its reported decision metrics or structured result, rather than only its status and pipeline-call counts. In test_no_graded_rows_is_not_an_error, pass an empty audit map so the no-graded-rows branch is exercised instead of creating a linked audit from _golden_row; preserve the expected zero status.Source: Coding guidelines
🤖 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/evaluate_harness_test.py`:
- Line 123: Update the assignment from harness.report_calibration in the test to
bind the unused status result to _, while preserving the scaler binding and
existing test behavior.
In `@scripts/evaluate_librarian.py`:
- Around line 110-113: Update the live-evaluation descriptions in
_build_live_pipeline(), the --use_live_embeddings help text, and the offline
message to state that the live pipeline produces the C.4 decision report and may
return a nonzero calibration status.
---
Nitpick comments:
In `@application/tests/librarian/evaluate_harness_test.py`:
- Around line 211-247: Strengthen the tests around
harness.report_decision_accuracy by asserting its reported decision metrics or
structured result, rather than only its status and pipeline-call counts. In
test_no_graded_rows_is_not_an_error, pass an empty audit map so the
no-graded-rows branch is exercised instead of creating a linked audit from
_golden_row; preserve the expected zero status.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0d5434e-4076-49a9-b00c-d711340a56dc
📒 Files selected for processing (3)
application/tests/librarian/evaluate_harness_test.pyapplication/utils/librarian/calibration/temperature.pyscripts/evaluate_librarian.py
🚧 Files skipped from review as they are similar to previous changes (1)
- application/utils/librarian/calibration/temperature.py
Contributor
Author
|
Pushed on top of the docstring fix. Carried the #974 fixes onto this branch. This branch is cumulative off
Adds 134 librarian tests pass; the hermetic harness run still exits 0. |
Follow-up to the shared-audit refactor. live_audits keys the reranked shortlists by row.id and the reports read them back with audits[row.id], so two rows sharing an id would collapse in that dict: the surviving audit gets reused for the earlier row and its scores are reported against the wrong text. Before the refactor each report recomputed per row, so a duplicate id was harmless; keying by it turned a harmless quirk into a silently wrong number. GoldenDatasetRow only requires an id to be non-empty, so uniqueness is enforced at load time and the harness refuses the file instead of printing a wrong metric. The committed dataset has 319 distinct ids, so nothing changes today; this closes the trap the refactor opened. Adds a test that a forced collision raises and names the offending id. (cherry picked from commit f352dfc)
Two follow-ups from the bot review of the last push, neither behavioural: - The live-path descriptions still predated C.4. The module docstring claimed the semantic path was stubbed, _build_live_pipeline named only recall and calibration as its consumers, --use_live_embeddings help listed only recall and top-1, and the offline message omitted the decision report. All four now say what the run actually does, including that C.3 is the one live report that sets the exit status (a failed or skipped gate returns nonzero) while C.4 is informational until SafetyGuard and tau tuning land. - evaluate_harness_test bound the calibration status it never asserted (Ruff RUF059). Bound to _status: the test is about the pipeline not being re-run and the scaler coming back, and the gate outcome on stub logits is not a meaningful assertion. ruff check is clean on both files. 136 librarian tests pass; the hermetic harness run still exits 0.
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 `@application/tests/librarian/dataset_test.py`:
- Around line 113-120: Update _load_harness to validate that spec from
spec_from_file_location is not None before passing it to module_from_spec or
accessing its loader. If the specification cannot be created, raise a clear
error indicating that scripts/evaluate_librarian.py is missing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: e37bcc6d-95de-4455-a3ae-cf975c7b6f14
📒 Files selected for processing (3)
application/tests/librarian/dataset_test.pyapplication/tests/librarian/evaluate_harness_test.pyscripts/evaluate_librarian.py
🚧 Files skipped from review as they are similar to previous changes (1)
- application/tests/librarian/evaluate_harness_test.py
Comment on lines
+113
to
+120
| def _load_harness(self): | ||
| import importlib.util | ||
|
|
||
| path = os.path.join(_REPO_ROOT, "scripts", "evaluate_librarian.py") | ||
| spec = importlib.util.spec_from_file_location("evaluate_librarian", path) | ||
| module = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(module) | ||
| return module |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '110,122p' application/tests/librarian/dataset_test.py
fd -H -t f '^(Makefile|mypy\.ini|pyproject\.toml|setup\.cfg)$' . -0 |
xargs -0 -r rg -n -C 3 'mypy|exclude|files|application/tests'Repository: OWASP/OpenCRE
Length of output: 1198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- library imports/usages ---'
rg -n 'class .*Dataset|def _load_harness|_load_harness\(|spec_from_file_location|module_from_spec|No matching .*_test' application/tests/librarian/dataset_test.py -C 3
printf '%s\n' '--- mypy config files ---'
fd -H -t f '^(Makefile|mypy\.ini|pyproject\.toml|setup\.cfg)$' . -0
printf '%s\n' '--- deterministic static signature evidence ---'
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("application/tests/librarian/dataset_test.py")
tree = ast.parse(path.read_text())
for node in tree.body:
if isinstance(node, ast.ClassDef) and node.name == "LibrarianDatasetTests":
for sub in node.body:
if isinstance(sub, ast.FunctionDef) and sub.name == "_load_harness":
body = sub.body
first_return = next((n for n in body if isinstance(n, ast.Return)), None)
if first_return:
line = first_return.lineno
line_text = body[line - 1].lineno
print("return_first=found", first_return.col_offset)
print("line", first_return.lineno)
print("statements=", len(body), [type(n).__name__ for n in body])
print("last_import_line", next((n.lineno for n in reversed(body) if isinstance(n, ast.Import)), None))
PYRepository: OWASP/OpenCRE
Length of output: 1895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- library imports/usages ---'
rg -n 'class .*Dataset|def _load_harness|_load_harness\(|spec_from_file_location|module_from_spec|No matching .*_test' application/tests/librarian/dataset_test.py -C 3
printf '%s\n' '--- mypy config files ---'
fd -H -t f '^(Makefile|mypy\.ini|pyproject\.toml|setup\.cfg)$' . -0
printf '%s\n' '--- deterministic static signature evidence ---'
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("application/tests/librarian/dataset_test.py")
tree = ast.parse(path.read_text())
for node in tree.body:
if isinstance(node, ast.ClassDef) and node.name == "LibrarianDatasetTests":
for sub in node.body:
if isinstance(sub, ast.FunctionDef) and sub.name == "_load_harness":
body = sub.body
first_return = next((n for n in body if isinstance(n, ast.Return)), None)
if first_return:
print("return_first_at_line=", first_return.lineno)
print("method_statements=", len(body), [type(n).__name__ for n in body])
print("last_import_line=", next((n.lineno for n in reversed(body) if isinstance(n, ast.Import)), None))
PYRepository: OWASP/OpenCRE
Length of output: 1895
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
path = Path("application/tests/librarian/dataset_test.py")
tree = ast.parse(path.read_text())
for node in tree.body:
if isinstance(node, ast.ClassDef) and node.name == "TestLoadDatasetRejectsDuplicateIds":
for sub in node.body:
if isinstance(sub, ast.FunctionDef) and sub.name == "_load_harness":
lines = source_lines(path, sub)
return_stmt = next((i for i, stmt in enumerate(lines[1:]) if isinstance(stmt, ast.Return)), None)
if return_stmt is not None:
print("method_return_at_line=", sub.lineno + return_stmt + 1)
print("returns_none_before_loader_access=", isinstance(stmt, ast.Return) and isinstance(stmt.value, ast.Constant) and stmt.value.value is None)
break
break
def source_lines(path, fn_node):
text = path.read_text().splitlines()
return text[fn_node.lineno - 1 : fn_node.end_lineno]
PYRepository: OWASP/OpenCRE
Length of output: 271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
from pathlib import Path
def source_lines(path, fn_node):
return path.read_text().splitlines()[fn_node.lineno - 1 : fn_node.end_lineno]
path = Path("application/tests/librarian/dataset_test.py")
tree = ast.parse(path.read_text())
for node in tree.body:
if isinstance(node, ast.ClassDef) and node.name == "TestLoadDatasetRejectsDuplicateIds":
for sub in node.body:
if isinstance(sub, ast.FunctionDef) and sub.name == "_load_harness":
print("method_start=", sub.lineno, "end=", sub.end_lineno)
print("source:")
for i, line in enumerate(source_lines(path, sub), 1):
print(f"{sub.lineno + i - 1}: {line}")
for stmt in sub.body:
print("stmt", type(stmt).__name__, "line", stmt.lineno, getattr(stmt, "value", None))
if isinstance(sub.body[-1], ast.Return):
is_none = isinstance(sub.body[-1].value, ast.Constant) and sub.body[-1].value.value is None
print("last_stmt_is_return", True)
print("returns_none_return", is_none)
break
break
PYRepository: OWASP/OpenCRE
Length of output: 921
Guard the optional import specification before creating and executing the module.
importlib.util.spec_from_file_location() can return None; module_from_spec(spec) and spec.loader.exec_module(module) could then receive or access None. Check spec before using it, and handle failure with a clear message if scripts/evaluate_librarian.py is missing.
🤖 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/dataset_test.py` around lines 113 - 120, Update
_load_harness to validate that spec from spec_from_file_location is not None
before passing it to module_from_spec or accessing its loader. If the
specification cannot be created, raise a clear error indicating that
scripts/evaluate_librarian.py is missing.
Source: Coding guidelines
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 6 of Module C. Week 5 produced one honest confidence per chunk; this PR turns that number into the actual decision - auto-link into the graph, or route to a human - which is the accuracy gate of the whole pipeline.
Overview
Week 3 built the search step (C.1), Week 4 the rerank step (C.2), and Week 5 the calibration step (C.3) - a single scalar
Tthat maps the reranked shortlist to a trustworthyconfidence = softmax(logits / T).The problem: a calibrated confidence is only useful if something acts on it. Auto-linking a wrong CRE pollutes the graph; sending everything to a human defeats the point. We need a rule that auto-links when it's safe and escalates when it isn't.
This PR's role: build the decision step (C.4) -
decision_engine.decide(). It links the top-1 candidate iffconfidence >= thresholdand there is a candidate and no blocking safety flag; otherwise it routes to review with areason_code. It's a pure function of(confidence, candidates, flags, threshold) -> DecisionResult- it does not import the C.3 scaler (confidence-in → decision-out), so it stays model-free and hermetically testable, mirroring the C.1/C.2/C.3 seams. Reason-code precedence is total:NO_CANDIDATES > ADVERSARIAL_FLAG > UPDATE_AMBIGUOUS > BELOW_THRESHOLD.The harness gains a decision-accuracy gate: run the live C.1→C.4 decision over the golden set and measure how often
decide()lands on the expected auto-link-vs-review call.Scope: 1 new module + 1 new test + additive harness wiring. The SafetyGuard flags (
adversarial/update_ambiguous) are accepted bydecide()but not yet produced - nothing sets them, so they default to False (declared-degraded until that lands). No frontend, no migration, no behaviour change to OpenCRE proper.What changed
decision_engine.py(new)decide(confidence, candidate_cre_ids, *, threshold, adversarial, update_ambiguous) -> DecisionResult. Links the top-1 iffconfidence >= thresholdAND candidates exist AND no blocking flag; else reviews with areason_code. FrozenDecisionResult, versionedENGINE_NAME, customDecisionError, input guards on confidence/threshold. Model-free and confidence-agnostic so it is hermetically testable - mirrors the C.1embed_fn/ C.2score_fn/ C.3 scaler seams.evaluate_librarian.pyreport_decision_accuracy: fitsTon positive+hard_negative, runs the live C.1→C.4 decision over the golden set, and reports overall agreement plus auto-link recall vs review recall (a single accuracy hides that attau=0.80the softmax top-1 mass of a correct-but-close winner is often ~0.5, so correct positives route to review - the safe direction). Informational, not a hard gate: tuningtauis the Week-7 experiment, and flag-based reason codes lag until the SafetyGuard lands.report_calibration(W5) untouched.decision_engine_test.py(new)>=boundary, all four reason codes, the precedence order, and the input guards.How the pieces connect
flowchart TB conf["C.3 calibrated confidence<br/>+ reranked candidates + flags"] subgraph C4["C.4 - decision engine (this PR)"] rule["decide(): confidence at or above tau ?<br/>AND candidates exist AND no blocking flag"] res["DecisionResult<br/>(decision, confidence, cre_ids, reason_code)"] rule --> res end conf --> rule res --> link["linked -> LinkProposal (W6b emits)"] res --> review["review -> ReviewItem + reason_code<br/>NO_CANDIDATES / ADVERSARIAL_FLAG /<br/>UPDATE_AMBIGUOUS / BELOW_THRESHOLD"]Results
Reading the C.4 line by direction, because a single accuracy hides the story:
tau=0.80many correct-but-close positives fall below the bar and route to review (the safe direction). This is a conservative starting point; Week 7's threshold sweep is exactly the lever that lifts it.What is intentionally not here
DecisionResultinto the RFCLinkProposal/ReviewItemand wiring C.0→C.4. Ships stacked asweek_6b.ood/conformal/ update-detection) that would populateadversarial/update_ambiguous.Tfor the live decision path, and live B→C integration + graph writes (W8) - the pipeline stays dry-run.How to verify locally