week_6b: Module C (The Librarian) — C.4 envelope emitter + C.0→C.4 pipeline glue - #991
Merged
Merged
Conversation
PRAteek-singHWY
requested review from
Pa04rth,
northdpole,
paoga87 and
robvanderveer
as code owners
July 23, 2026 12:46
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe PR adds a deterministic decision engine, envelope emitter, and dry-run librarian pipeline. It integrates shared calibration into live evaluation reports and adds hermetic tests for these flows. ChangesLibrarian decision and envelope flow
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 (3)
application/utils/librarian/pipeline.py (2)
58-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstructor params
source/retriever/reranker/scalerare untyped.Unlike
threshold: float/pipeline_run_id: strin the same signature, and unlike the fully-typeddecision_engine.py/emitter.py, these duck-typed seams carry no type hints at all. ConsiderProtocolclasses (e.g.RetrieverLike,RerankerLike,ScalerLike) or at minimumAnyannotations formake mypyconsistency.As per coding guidelines, "Run
make mypyfor Python type checking."🤖 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/pipeline.py` around lines 58 - 73, Update the constructor in the pipeline class around __init__ to annotate source, retriever, reranker, and scaler, preferably using appropriate Protocol types such as RetrieverLike, RerankerLike, and ScalerLike; use Any only where no suitable interface exists. Preserve the existing threshold and pipeline_run_id annotations and run make mypy to verify the changes.Source: Coding guidelines
75-104: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPer-row failures in
retrieve/rerank/confidence/decide/emitabort the whole run.Only
section_from_queue_rowis guarded with try/except; a single failure from any other stage propagates and aborts the entire batch, discarding all envelopes/stats accumulated so far. This is currently safe with hermetic stubs, but the docstring states these seams are meant to become live DB/embedding/cross-encoder calls — worth hardening before that wiring lands.♻️ Suggested per-row error containment
- audit = self._retriever.retrieve(section.text) - audit = self._reranker.rerank(section.text, audit) - reranked = [c for c in audit.reranked if c.score_rerank is not None] - logits = [float(c.score_rerank) for c in reranked] - cre_ids = [c.cre_id for c in reranked] - confidence = self._scaler.confidence(logits) if logits else 0.0 - - result = decide(confidence, cre_ids, threshold=self._threshold) - envelope = emit(section, audit, result, pipeline_run_id=self._run_id, at=at) - envelopes.append(envelope) - if isinstance(envelope, LinkProposal): - linked += 1 - else: - review += 1 + try: + audit = self._retriever.retrieve(section.text) + audit = self._reranker.rerank(section.text, audit) + reranked = [c for c in audit.reranked if c.score_rerank is not None] + logits = [float(c.score_rerank) for c in reranked] + cre_ids = [c.cre_id for c in reranked] + confidence = self._scaler.confidence(logits) if logits else 0.0 + result = decide(confidence, cre_ids, threshold=self._threshold) + envelope = emit(section, audit, result, pipeline_run_id=self._run_id, at=at) + except Exception: + errored += 1 # new RunStats field + continue + envelopes.append(envelope) + if isinstance(envelope, LinkProposal): + linked += 1 + else: + review += 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/pipeline.py` around lines 75 - 104, Update the per-item processing in run so failures from retrieve, rerank, confidence, decide, or emit are contained to that row instead of aborting the batch. Wrap the full processing pipeline after section_from_queue_row in a per-row try/except, increment the appropriate skipped/error statistic for failed rows, and continue processing later items while preserving already-created envelopes and existing successful-row counts.scripts/evaluate_librarian.py (1)
288-307: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
report_decision_accuracyre-derives the calibration set and re-fitsT, duplicatingreport_calibration's work.Both functions build the identical
positive+hard_negative(shortlist, label) set and callfit_temperatureon it (lines 234-253 inreport_calibration, lines 293-307 here). Sincemain()calls both sequentially over the sameretriever/reranker, this doubles the live per-rowreranker.rerank()(cross-encoder inference) cost with no behavioral difference — the fittedTwill be identical.Consider having
report_calibrationreturn(status, scaler)and passing the scaler intoreport_decision_accuracy, or extracting the calibration-set-building + fit into one shared helper called once frommain().🤖 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 288 - 307, The calibration data and temperature scaler are redundantly recomputed in report_decision_accuracy after report_calibration. Refactor report_calibration and main so calibration fitting occurs once, returns its status and fitted scaler, and passes that scaler into report_decision_accuracy; remove the duplicate cal_rows construction, reranker.rerank calls, label validation, and fit_temperature invocation while preserving existing skip/status behavior.
🤖 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/emitter.py`:
- Around line 52-62: Update _proposed_links so entries used as
ReviewItem.suggested_links are not labeled with AUTO_LINK_TYPE; use the
established distinct suggested-review link type if available, or omit link_type
when constructing ProposedLink. Preserve the existing confidence, rationale, and
CRE ID values.
---
Nitpick comments:
In `@application/utils/librarian/pipeline.py`:
- Around line 58-73: Update the constructor in the pipeline class around
__init__ to annotate source, retriever, reranker, and scaler, preferably using
appropriate Protocol types such as RetrieverLike, RerankerLike, and ScalerLike;
use Any only where no suitable interface exists. Preserve the existing threshold
and pipeline_run_id annotations and run make mypy to verify the changes.
- Around line 75-104: Update the per-item processing in run so failures from
retrieve, rerank, confidence, decide, or emit are contained to that row instead
of aborting the batch. Wrap the full processing pipeline after
section_from_queue_row in a per-row try/except, increment the appropriate
skipped/error statistic for failed rows, and continue processing later items
while preserving already-created envelopes and existing successful-row counts.
In `@scripts/evaluate_librarian.py`:
- Around line 288-307: The calibration data and temperature scaler are
redundantly recomputed in report_decision_accuracy after report_calibration.
Refactor report_calibration and main so calibration fitting occurs once, returns
its status and fitted scaler, and passes that scaler into
report_decision_accuracy; remove the duplicate cal_rows construction,
reranker.rerank calls, label validation, and fit_temperature invocation while
preserving existing skip/status behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7118b7e4-fc18-4e63-bd3f-3ab1008a3300
📒 Files selected for processing (11)
application/tests/librarian/decision_engine_test.pyapplication/tests/librarian/emitter_test.pyapplication/tests/librarian/pipeline_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.pyapplication/utils/librarian/emitter.pyapplication/utils/librarian/pipeline.pyscripts/evaluate_librarian.py
PRAteek-singHWY
added a commit
to PRAteek-singHWY/OpenCRE
that referenced
this pull request
Jul 24, 2026
…ions as auto-links
_proposed_links stamped link_type "Automatically linked to" on every ProposedLink,
including a ReviewItem's suggested_links — but those are candidates for a human to
consider, not auto-links. Parametrize the link type: LinkProposal.links use
AUTO_LINK_TYPE, ReviewItem.suggested_links use SUGGESTED_LINK_TYPE ("Related",
mirrors cre_defs.LinkTypes.Related). Test asserts a review suggestion is never
labelled as an auto-link.
PRAteek-singHWY
force-pushed
the
gsocmodule_C_week_6b
branch
from
July 24, 2026 15:10
6f6c262 to
8d9fda5
Compare
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 6b (#991)
Emitter + dry-run pipeline glue look good: type guards, Related vs Automatically linked to, injected at / pipeline_run_id, and hermetic tests are in good shape. No blocking bugs in the unique Week-6b surface.
Stacked on #990 / #974 — rebase as those land. Inline notes are non-blocking for this dry-run PR but matter before W8 persistence.
| cre_ids = [c.cre_id for c in reranked] | ||
| confidence = self._scaler.confidence(logits) if logits else 0.0 | ||
|
|
||
| result = decide(confidence, cre_ids, threshold=self._threshold) |
Collaborator
There was a problem hiding this comment.
Note (pre-W8) — SafetyGuard flags not wired
decide(...) is called without adversarial= / update_ambiguous=, so those reason codes can never fire from this pipeline yet. Fine for dry-run Week 6b, but must be wired before any graph / queue write-back (W8), or auto-links will ignore the safety path the decision engine already supports.
Contributor
Author
There was a problem hiding this comment.
Agreed, and noted as a W8 blocker rather than a change here. decide(...) gets adversarial= / update_ambiguous= wired in before any graph or queue write-back, so the safety reason codes can actually fire. Leaving it unwired in this dry-run PR since there is no SafetyGuard to feed them yet.
| for item in self._source.items(): | ||
| total += 1 | ||
| try: | ||
| section = section_from_queue_row(item) |
Collaborator
There was a problem hiding this comment.
Note (B↔C integration) — queue row shape still the C mirror
section_from_queue_row / C's KnowledgeQueueItem still expect the flat source_repo / source_path / source_commit_sha mirror. Module B's live knowledge_queue (#989) is a richer row (locator_*, content_hash, provenance columns, …). Already called out in section_validator for W8 — please keep this on the W8 checklist so the dry-run pipeline here does not silently assume the wrong row shape when wired to Postgres.
Contributor
Author
There was a problem hiding this comment.
Kept on the W8 checklist. Reconciling section_from_queue_row / KnowledgeQueueItem against Module B's live knowledge_queue (#989) is a real schema reconciliation, not a rename: B's row carries locator_*, content_hash, and the provenance columns, where C currently assumes the flat source_repo / source_path / source_commit_sha mirror. The dry-run pipeline here stays on the mirror shape deliberately, and the adapter seam is where the mapping lands once B freezes the table.
PRAteek-singHWY
added a commit
to PRAteek-singHWY/OpenCRE
that referenced
this pull request
Aug 4, 2026
…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
…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. (cherry picked from commit 6ef7865)
PRAteek-singHWY
added a commit
to PRAteek-singHWY/OpenCRE
that referenced
this pull request
Aug 4, 2026
…in per-row failures Two nitpicks on pipeline.py, both aimed at the wiring rather than the dry-run behaviour: - The injected seams were the only untyped parameters in the signature, next to an annotated threshold/pipeline_run_id and fully-typed decision_engine/emitter. Each is structurally one method, so they are now Protocols — KnowledgeSource, Retriever, Reranker, Scaler — which keeps stubs as trivial as before while the call sites are checked. pipeline.py is clean under the --strict mypy the coding guidelines ask for; KnowledgeSource.items() is typed to what section_from_queue_row actually accepts, not a looser Mapping. - Only the C.0 boundary was guarded, so a failure from retrieve / rerank / confidence / decide / emit propagated and abandoned the whole batch along with every envelope already built. Those stages are stubs today but become live DB, embedding, and cross-encoder calls in W8, where one timeout must not cost the run. Failures are now contained per row: the row is logged with its chunk and artifact id, counted in a new RunStats.errored, and the run continues. errored is separate from skipped on purpose — a boundary rejection is expected input hygiene, an error is a fault worth chasing. It defaults to 0, so existing RunStats callers are unaffected. Adds five containment tests: each seam failing in isolation, one bad row in a batch of three leaving the other two linked, and errored not being conflated with skipped. Verified all five fail if the containment is removed. 153 librarian tests pass; the hermetic harness run still exits 0.
Contributor
Author
|
Pushed. Both Seams are typed. They were the only untyped parameters in the signature, next to an annotated Per-row failures are contained. Only the C.0 boundary was guarded, so a failure from Five containment tests added: each seam failing in isolation, one bad row in a batch of three leaving the other two linked, and The two W8 notes on this PR stay open by design, as agreed in the threads above: the SafetyGuard flag wiring, and reconciling the queue row shape against Module B's live 153 librarian tests pass; the hermetic harness run still exits 0. |
northdpole
requested changes
Aug 6, 2026
northdpole
left a comment
northdpole
left a comment
Collaborator
There was a problem hiding this comment.
Maintainer review — Module C Week 6b (#991) — refresh
Verdict: Emitter + dry-run pipeline glue look good; same blockers as #990 (black + stack).
What looks solid
emit/build_link_proposal/build_review_itemtype-guard correctly; linked vs review envelopes are distinct- Auto-link uses
Automatically linked to; review suggestions useRelated— important so suggestions never claim auto-link - Injected
at/pipeline_run_idkeep runs reproducible LibrarianPipelineper-row error containment (skippedvserrored) is the right shape for W8 live seams- Declared-degraded
UpdateDetectionuntil SafetyGuard — fine for dry-run W6b - Hermetic emitter + pipeline tests are in good shape
Blockers
- Lint / black — same failure class as #990 (
evaluate_harness_test.py). Please black-format and push. - Stack — sits on #990 / #974. Land those first (or rebase as they merge); do not merge W6b ahead of W6.
Non-blocking (before W8 persistence)
- Protocol typing on constructor seams is nice (CodeRabbit); not required for dry-run merge
- When persistence lands, keep auto-link vs suggested link types strictly separate in write paths
After black + stack resolve, approve-ready.
Collaborator
Collaborator
PRAteek-singHWY
added a commit
to PRAteek-singHWY/OpenCRE
that referenced
this pull request
Aug 8, 2026
…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 8, 2026
…ions as auto-links
_proposed_links stamped link_type "Automatically linked to" on every ProposedLink,
including a ReviewItem's suggested_links — but those are candidates for a human to
consider, not auto-links. Parametrize the link type: LinkProposal.links use
AUTO_LINK_TYPE, ReviewItem.suggested_links use SUGGESTED_LINK_TYPE ("Related",
mirrors cre_defs.LinkTypes.Related). Test asserts a review suggestion is never
labelled as an auto-link.
PRAteek-singHWY
added a commit
to PRAteek-singHWY/OpenCRE
that referenced
this pull request
Aug 8, 2026
…in per-row failures Two nitpicks on pipeline.py, both aimed at the wiring rather than the dry-run behaviour: - The injected seams were the only untyped parameters in the signature, next to an annotated threshold/pipeline_run_id and fully-typed decision_engine/emitter. Each is structurally one method, so they are now Protocols — KnowledgeSource, Retriever, Reranker, Scaler — which keeps stubs as trivial as before while the call sites are checked. pipeline.py is clean under the --strict mypy the coding guidelines ask for; KnowledgeSource.items() is typed to what section_from_queue_row actually accepts, not a looser Mapping. - Only the C.0 boundary was guarded, so a failure from retrieve / rerank / confidence / decide / emit propagated and abandoned the whole batch along with every envelope already built. Those stages are stubs today but become live DB, embedding, and cross-encoder calls in W8, where one timeout must not cost the run. Failures are now contained per row: the row is logged with its chunk and artifact id, counted in a new RunStats.errored, and the run continues. errored is separate from skipped on purpose — a boundary rejection is expected input hygiene, an error is a fault worth chasing. It defaults to 0, so existing RunStats callers are unaffected. Adds five containment tests: each seam failing in isolation, one bad row in a batch of three leaving the other two linked, and errored not being conflated with skipped. Verified all five fail if the containment is removed. 153 librarian tests pass; the hermetic harness run still exits 0.
PRAteek-singHWY
force-pushed
the
gsocmodule_C_week_6b
branch
from
August 8, 2026 21:07
596142a to
332db47
Compare
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/utils/librarian/__init__.py`:
- Around line 21-24: Update the C.4 scope statement in the module documentation
to remove the claim that the envelope emitter and pipeline glue are unbuilt,
since emitter.py and pipeline.py now provide them. Keep the statement that only
the W8 queue and graph writers remain unbuilt.
🪄 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: 7adc8589-344b-416e-bcef-3bfb9538345b
📒 Files selected for processing (5)
application/tests/librarian/evaluate_harness_test.pyapplication/tests/librarian/pipeline_test.pyapplication/utils/librarian/__init__.pyapplication/utils/librarian/pipeline.pyscripts/evaluate_librarian.py
PRAteek-singHWY
added a commit
to PRAteek-singHWY/OpenCRE
that referenced
this pull request
Aug 8, 2026
…ions as auto-links
_proposed_links stamped link_type "Automatically linked to" on every ProposedLink,
including a ReviewItem's suggested_links — but those are candidates for a human to
consider, not auto-links. Parametrize the link type: LinkProposal.links use
AUTO_LINK_TYPE, ReviewItem.suggested_links use SUGGESTED_LINK_TYPE ("Related",
mirrors cre_defs.LinkTypes.Related). Test asserts a review suggestion is never
labelled as an auto-link.
PRAteek-singHWY
added a commit
to PRAteek-singHWY/OpenCRE
that referenced
this pull request
Aug 8, 2026
…in per-row failures Two nitpicks on pipeline.py, both aimed at the wiring rather than the dry-run behaviour: - The injected seams were the only untyped parameters in the signature, next to an annotated threshold/pipeline_run_id and fully-typed decision_engine/emitter. Each is structurally one method, so they are now Protocols — KnowledgeSource, Retriever, Reranker, Scaler — which keeps stubs as trivial as before while the call sites are checked. pipeline.py is clean under the --strict mypy the coding guidelines ask for; KnowledgeSource.items() is typed to what section_from_queue_row actually accepts, not a looser Mapping. - Only the C.0 boundary was guarded, so a failure from retrieve / rerank / confidence / decide / emit propagated and abandoned the whole batch along with every envelope already built. Those stages are stubs today but become live DB, embedding, and cross-encoder calls in W8, where one timeout must not cost the run. Failures are now contained per row: the row is logged with its chunk and artifact id, counted in a new RunStats.errored, and the run continues. errored is separate from skipped on purpose — a boundary rejection is expected input hygiene, an error is a fault worth chasing. It defaults to 0, so existing RunStats callers are unaffected. Adds five containment tests: each seam failing in isolation, one bad row in a batch of three leaving the other two linked, and errored not being conflated with skipped. Verified all five fail if the containment is removed. 153 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 8, 2026
…ement The package docstring still listed the envelope emitter and the C.0->C.4 pipeline glue as not built, but this is the branch that builds them — the same staleness the W6 scope line was corrected for on OWASP#990. Adds the W6b line for the emitter and the glue (noting the pipeline is dry-run), and narrows the not-yet marker to what is genuinely still missing: the live queue drain and the graph / review-queue writers in W8.
PRAteek-singHWY
force-pushed
the
gsocmodule_C_week_6b
branch
from
August 8, 2026 23:22
332db47 to
5f2d75b
Compare
northdpole
approved these changes
Aug 9, 2026
northdpole
left a comment
northdpole
left a comment
Collaborator
There was a problem hiding this comment.
Re-review (stacked on #990)
Prior rebase/black blocker cleared. Confirmed this tip is ahead of #990 (proper stack, no duplicate W5 copies) and CI is green.
Verified
- Emitter builds RFC
LinkProposal/ReviewItemwith injected timestamps; review suggestions useRelated(not auto-link labelling) — good. - Pipeline wires C.0→C.4 dry-run with typed Protocol seams; per-row error containment + skipped-vs-errored stats are tested.
- Package docstring now lists W6b emitter/pipeline; “not built” marker correctly moved to W8 writers.
- Prior notes on SafetyGuard flags / B↔C queue shape remain pre-W8 — fine to leave for later.
Approve. Merge with rebase after #990 lands (stack order #990 → #991).
northdpole
pushed a commit
that referenced
this pull request
Aug 9, 2026
…ve 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.
…l / ReviewItem) The decision engine (week_6) yields a verdict; this turns it into the wire contract Module D consumes. - emitter.py: `emit(section, audit, result, *, pipeline_run_id, at)` dispatches on the verdict — `linked` -> RFC LinkProposal, `review` -> RFC ReviewItem — plus the two explicit builders. Pure and timestamp-injected (no clock read) so it is hermetically testable; only builds the envelope (persistence is W8). Auto-links carry link_type "Automatically linked to" (mirrors cre_defs.LinkTypes) and the calibrated confidence; reviews carry the reason_code and a deterministic review_id derived from the chunk id. update_detection defaults to the declared degraded value (is_update=False) until the SafetyGuard lands. - emitter_test.py: 9 hermetic tests — both envelopes end-to-end, audit passthrough, degraded update_detection, no-candidates review has no suggestions, the verdict/reason guards, and link_type == cre_defs single source of truth. Stacked on week_6. Pipeline glue (C.0->C.4) follows next.
Wires the librarian end to end: section_from_queue_row (C.0) -> retriever (C.1) -> reranker (C.2) -> scaler.confidence (C.3) -> decide + emit (C.4), one envelope per valid knowledge_queue row. - pipeline.py: LibrarianPipeline.run(at=...) -> RunResult(envelopes, RunStats). Every stage is an injected seam (source/retriever/reranker/scaler), so the whole pipeline runs hermetically with stubs. Inherently dry-run — builds envelopes, never persists (queue write-back + graph writes are W8). pipeline_run_id and the timestamp are injected, never read from the clock, so a run is reproducible. Rows rejected at the C.0 boundary (e.g. UNCERTAIN) are skipped and counted, not linked. - pipeline_test.py: 5 hermetic tests — confident row auto-links, low confidence reviews (below_threshold), empty shortlist reviews (no_candidates), UNCERTAIN row skipped at the boundary, and mixed-batch counts. Stacked on the week_6b emitter.
…ions as auto-links
_proposed_links stamped link_type "Automatically linked to" on every ProposedLink,
including a ReviewItem's suggested_links — but those are candidates for a human to
consider, not auto-links. Parametrize the link type: LinkProposal.links use
AUTO_LINK_TYPE, ReviewItem.suggested_links use SUGGESTED_LINK_TYPE ("Related",
mirrors cre_defs.LinkTypes.Related). Test asserts a review suggestion is never
labelled as an auto-link.
…in per-row failures Two nitpicks on pipeline.py, both aimed at the wiring rather than the dry-run behaviour: - The injected seams were the only untyped parameters in the signature, next to an annotated threshold/pipeline_run_id and fully-typed decision_engine/emitter. Each is structurally one method, so they are now Protocols — KnowledgeSource, Retriever, Reranker, Scaler — which keeps stubs as trivial as before while the call sites are checked. pipeline.py is clean under the --strict mypy the coding guidelines ask for; KnowledgeSource.items() is typed to what section_from_queue_row actually accepts, not a looser Mapping. - Only the C.0 boundary was guarded, so a failure from retrieve / rerank / confidence / decide / emit propagated and abandoned the whole batch along with every envelope already built. Those stages are stubs today but become live DB, embedding, and cross-encoder calls in W8, where one timeout must not cost the run. Failures are now contained per row: the row is logged with its chunk and artifact id, counted in a new RunStats.errored, and the run continues. errored is separate from skipped on purpose — a boundary rejection is expected input hygiene, an error is a fault worth chasing. It defaults to 0, so existing RunStats callers are unaffected. Adds five containment tests: each seam failing in isolation, one bad row in a batch of three leaving the other two linked, and errored not being conflated with skipped. Verified all five fail if the containment is removed. 153 librarian tests pass; the hermetic harness run still exits 0.
…ement The package docstring still listed the envelope emitter and the C.0->C.4 pipeline glue as not built, but this is the branch that builds them — the same staleness the W6 scope line was corrected for on OWASP#990. Adds the W6b line for the emitter and the glue (noting the pipeline is dry-run), and narrows the not-yet marker to what is genuinely still missing: the live queue drain and the graph / review-queue writers in W8.
northdpole
force-pushed
the
gsocmodule_C_week_6b
branch
from
August 9, 2026 20:22
5f2d75b to
9baa415
Compare
Collaborator
|
Maintainer note: #990 is merged. I rebased this branch onto latest |
northdpole
pushed a commit
that referenced
this pull request
Aug 9, 2026
…as auto-links
_proposed_links stamped link_type "Automatically linked to" on every ProposedLink,
including a ReviewItem's suggested_links — but those are candidates for a human to
consider, not auto-links. Parametrize the link type: LinkProposal.links use
AUTO_LINK_TYPE, ReviewItem.suggested_links use SUGGESTED_LINK_TYPE ("Related",
mirrors cre_defs.LinkTypes.Related). Test asserts a review suggestion is never
labelled as an auto-link.
northdpole
pushed a commit
that referenced
this pull request
Aug 9, 2026
…r-row failures Two nitpicks on pipeline.py, both aimed at the wiring rather than the dry-run behaviour: - The injected seams were the only untyped parameters in the signature, next to an annotated threshold/pipeline_run_id and fully-typed decision_engine/emitter. Each is structurally one method, so they are now Protocols — KnowledgeSource, Retriever, Reranker, Scaler — which keeps stubs as trivial as before while the call sites are checked. pipeline.py is clean under the --strict mypy the coding guidelines ask for; KnowledgeSource.items() is typed to what section_from_queue_row actually accepts, not a looser Mapping. - Only the C.0 boundary was guarded, so a failure from retrieve / rerank / confidence / decide / emit propagated and abandoned the whole batch along with every envelope already built. Those stages are stubs today but become live DB, embedding, and cross-encoder calls in W8, where one timeout must not cost the run. Failures are now contained per row: the row is logged with its chunk and artifact id, counted in a new RunStats.errored, and the run continues. errored is separate from skipped on purpose — a boundary rejection is expected input hygiene, an error is a fault worth chasing. It defaults to 0, so existing RunStats callers are unaffected. Adds five containment tests: each seam failing in isolation, one bad row in a batch of three leaving the other two linked, and errored not being conflated with skipped. Verified all five fail if the containment is removed. 153 librarian tests pass; the hermetic harness run still exits 0.
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 6b of Module C, stacked on the Week-6 decision engine. Week 6 produced the verdict (auto-link vs. review); this PR turns that verdict into the wire envelopes Module D consumes, and wires the whole C.0→C.4 pipeline end to end (dry-run).
Overview
Week 6 gave us
decide()→ aDecisionResult. Two things were deliberately left out of that PR to keep it a clean, provable unit: emitting the RFC envelope, and wiring the stages together. This PR adds both.This PR's role:
The emitter (
emitter.py) -emit(section, audit, result, *, pipeline_run_id, at)dispatches on the verdict:linked→ an RFCLinkProposal,review→ an RFCReviewItem. It only builds the envelope (persisting it is W8). Pure and timestamp-injected (no clock read), so every branch is hermetically testable. Auto-links carrylink_type"Automatically linked to" (mirrorscre_defs.LinkTypes) and the calibrated confidence; reviews carry thereason_codeand a deterministicreview_idderived from the chunk id.update_detectiondefaults to the declared-degraded value (is_update=False) until the SafetyGuard lands.The pipeline (
pipeline.py) -LibrarianPipeline.run(at=...)runs C.0→C.4 over a knowledge source:section_from_queue_row(C.0) →retrieve(C.1) →rerank(C.2) →scaler.confidence(C.3) →decide+emit(C.4), one envelope per valid row. Every stage is an injected seam, so the whole pipeline runs hermetically with stubs. Inherently dry-run - builds envelopes, never persists.pipeline_run_idand the timestamp are injected, never read from the clock, so a run is reproducible.Scope: 2 new modules + 2 new tests. No frontend, no migration, no behaviour change to OpenCRE proper.
What changed
emitter.py(new)emit()+build_link_proposal/build_review_item.DecisionResult→ RFCLinkProposal/ReviewItem, snapshotting the chunk (KnowledgeSnapshot) and passing the C.1/C.2RetrievalAuditthrough untouched. Pure, timestamp-injected, customEmitterError; auto-linklink_typemirrorscre_defs.LinkTypes.AutomaticallyLinkedTo; degradedupdate_detectiondefault; deterministicreview_id.pipeline.py(new)LibrarianPipeline.run(at=...) -> RunResult(envelopes, RunStats). Wires all five stages via injected seams (source/retriever/reranker/scaler), inherently dry-run. Rows rejected at the C.0 boundary (e.g.UNCERTAIN) are skipped and counted, not linked.emitter_test.py(new),pipeline_test.py(new)update_detection, no-candidates review has no suggestions, the verdict/reason guards, andlink_type == cre_defssingle source of truth. Pipeline (5): confident row auto-links, low confidence reviews (below_threshold), empty shortlist reviews (no_candidates),UNCERTAINrow skipped at the boundary, and mixed-batch counts.How the pieces connect
flowchart TB row["knowledge_queue row"] subgraph PIPE["LibrarianPipeline.run (this PR)"] c0["C.0 section_from_queue_row"] c1["C.1 retriever.retrieve"] c2["C.2 reranker.rerank"] c3["C.3 scaler.confidence"] c4["C.4 decide()"] emit["emit()"] c0 --> c1 --> c2 --> c3 --> c4 --> emit end row --> c0 emit --> lp["LinkProposal (linked)"] emit --> ri["ReviewItem (review + reason_code)"] skip["UNCERTAIN / invalid row -> skipped, counted"] c0 -.-> skipResults
The emitter and pipeline are pure and dry-run - every branch is covered by the hermetic tests above, no live key required. The end-to-end demo on the golden set (populated envelopes for a full slice) is the midterm deliverable; this PR lands the machinery it runs on.
What is intentionally not here
ood/conformal/update_detector) that would populate theadversarial/update_ambiguousflags and realupdate_detection.cre_maindispatch and persistence / queue write-back / graph writes (W8) - the pipeline stays dry-run; nothing is written to OpenCRE.KnowledgeQueueItemmirror over the golden fixture, not a live connection to Module B.How to verify locally