week_8b: Module C (The Librarian) — docs, final metrics, and the regression gate - #1012
Closed
PRAteek-singHWY wants to merge 11 commits into
Closed
week_8b: Module C (The Librarian) — docs, final metrics, and the regression gate#1012PRAteek-singHWY wants to merge 11 commits into
PRAteek-singHWY wants to merge 11 commits into
Conversation
…dule B's merged table Module B's `knowledge_queue` (OWASP#989) is substantially richer than the flat mirror C was built against. C modelled `source_repo` / `source_path` / `source_commit_sha` and nothing else, so against the merged table it failed validation on 100% of real rows, had no `chunk_id` or `artifact_id` to carry, and could not represent an rss row at all. `schemas.KnowledgeQueueItem` now mirrors B's table column for column — all 23, nothing dropped and nothing invented — and `section_from_queue_row` maps both provenance branches: github : source_repo / source_commit_sha / source_committed_at, locator repo_path rss : feed_url / post_guid, locator feed_item The identity columns are the point. `chunk_id` and `artifact_id` originate in Module A and travel through B untouched; C now uses them verbatim instead of synthesising its own, so an envelope refers to the same chunk every other module means. `DbKnowledgeSource` reads B's own SQLAlchemy model rather than redeclaring the table — C owns no schema and ships no migration. A row B wrote that C cannot model is logged with its ids and skipped rather than aborting the batch: a contract breach is worth seeing, but it must not cost the run.
…ates consumption
Retiring a queue row tells Module B never to offer that chunk again. Doing that
while the envelope built from it goes nowhere destroys the chunk outright: B will
not re-offer the row and nothing downstream ever saw the decision. So consumption
is defined against a sink, not against the pipeline having finished.
JsonlEnvelopeSink — appends one RFC envelope per line; fsynced, append-only so
several runs can share a file. `persists=True`.
NullEnvelopeSink — counts and discards, for dry runs. `persists=False`, and
the runner refuses to consume behind it.
The graph / review-queue writers (W8b) become further implementations of the same
protocol, so the consumption rule does not have to change when they land.
Envelopes are serialised with `exclude_none=True`, which is not cosmetic. The
vendored RFC schemas type their optional fields as plain "string" and leave them
out of `required`, so an absent value must be an absent *key*. Emitting
`"repo": null` fails validation against the very schema Module D reads — every
rss envelope trips it on repo/commit_sha, every github one on feed_url/post_guid.
No data is lost by omitting them: those are exactly the columns B itself stores
NULL per row type, which its own contract documents as `github only; NULL for
rss`.
Two tests pin this, because the old tests could not have caught it — asserting
"one JSON object per line" passes happily on a file whose every record is
invalid. One validates a written envelope against the vendored schema, resolved
in `$id` space so link-proposal's local `#/$defs` pointers resolve; the other
asserts no null survives anywhere in the tree. Both fail if `exclude_none` is
removed.
The mirror image of Module B's queue_writer. B inserts rows and C retires them; `db.KnowledgeQueueItem`'s own docstring assigns this side to C. This is the only place Module C writes to Module B's table, and it writes exactly one column. It never deletes a row: the queue doubles as the audit trail of what B handed over and what C did with it. Idempotent by construction — the update is filtered on `consumed_at IS NULL`, so replaying a run cannot move a timestamp that is already set. The returned count is rows actually stamped, which is why it can be lower than the number of ids passed in: a concurrent run got there first, or the id no longer exists. That is reported rather than raised. Ids are chunked before the `IN (...)`, because Postgres has a bind-parameter ceiling and a large IN plans badly. The caller owns the transaction, matching `run_noise_filter` on B's side.
…de() `decide()` has accepted `adversarial` / `update_ambiguous` since W6, but no caller ever passed them, so ADVERSARIAL_FLAG and UPDATE_AMBIGUOUS could not fire from the pipeline. That was flagged on OWASP#991 as something that must be wired before any write-back, since an auto-link that skips the safety path is exactly the failure the flags exist to prevent. This is that wiring. What is deliberately *not* here is a detector. The real guard — out-of-distribution scoring, conformal prediction, update detection — is later work. So the seam ships with NullSafetyGuard, which evaluates nothing and says so: its verdict carries `evaluated=False`, the pipeline counts those rows, and the runner reports the count. That distinction is the whole point. W5's review turned on a gate that skipped and still reported success; an unevaluated safety path that looks identical to a clean one is the same failure wearing a different hat. A clean verdict from a guard that never ran is a default, not a finding, and the run log now says which one it is. The rule this establishes for W8b: a writer that commits links into the graph must refuse to run behind a guard reporting `evaluated=False`. Retiring a queue row without the safety path is recoverable — the envelope is still on disk. Committing a link into a graph other tools read as truth is not.
…ator Until now the only place that knew how to construct a real retriever, reranker and scaler was `cre_main.run_librarian`, inline. That is why the OIE orchestrator (OWASP#996) could not drive `LibrarianPipeline`: it had the pipeline class but no way to produce the components it takes. This module is that seam. Everything here is construction only — no rows are read, no run is executed. Callers that want a run use `queue_runner.run_librarian_queue`. The DB and embedding imports are deliberately function-local. The rest of the librarian package is hermetically testable precisely because it never imports the database at module scope, and this is the one boundary where that stops being true; keeping the imports inside the call preserves it for everyone else. `load_config` now warns when `CRE_LIBRARIAN_TEMPERATURE` is still the 1.0 default, because 1.0 is the identity transform — an uncalibrated softmax. Thresholding τ against an uncalibrated confidence is not a smaller version of the right thing, it is a different thing, so the run says so out loud rather than looking fine.
Module C's live entry point: knowledge_queue -> C.0..C.4 -> consumed. Deliberately the same shape as Module B's `run_noise_filter` — `(session, pipeline_run_id, ..., dry_run) -> RunSummary` with a `to_json()` the CLI prints for the orchestrator to read. B reads harvest_input and fills knowledge_queue; C drains knowledge_queue and stamps consumed_at. What this adds over LibrarianPipeline is only the two DB-facing ends. The pipeline stays persistence-free and hermetic; this wraps it with a live source on one side and the write-back on the other. Consumption is gated on persistence. The runner refuses to stamp anything unless it was given a sink reporting `persists=True` and that sink accepted the batch. Graph writes are still W8b; JsonlEnvelopeSink is what makes a live drain lossless in the meantime. Which rows get retired: everything the pipeline finished with — rows that linked, rows that routed to review, and rows refused at the C.0 boundary, which is a definitive refusal (re-reading a malformed row forever helps nobody). Rows that *errored* mid-pipeline are left unconsumed, because those are the transient failures — an embedding timeout, a cross-encoder hiccup — the next run should retry. `RowOutcome` carries that distinction out of the pipeline, and `RunStats.errored` keeps it separate from `skipped`. UNCERTAIN rows are not touched. They are Module D's queue, per the B->C contract: C reads `consumed_at IS NULL AND llm_label = 'KNOWLEDGE'`. CLI: `--run_librarian` with `--run_id` drains the live queue; without it, walks a JSONL fixture. `--librarian_envelopes_out` is required for a real run, because a run with nowhere to put envelopes cannot safely retire anything. Combining `--librarian_source` with `--run_id` is a usage error rather than one silently winning.
Found while measuring this branch, and it is the same schema drift the rest of the PR fixes — just on the harness side. Golden rows are adapted into a synthetic knowledge_queue row before C.0 validates them. When Module B's table shape moved in OWASP#989, `queue_row_from_golden` kept minting the old flat row, so every one of the 319 rows was rejected at the boundary: positive 0/292 (0%) explicit 0/5 (0%) ... And the run still exited 0. With nothing validated, `explicit_total` stayed 0, the `if explicit_total:` gate skipped itself, and a harness that graded absolutely nothing reported success. That is precisely the skipped-gate-reports-success failure the C.3 calibration gate was fixed for on OWASP#974. Two changes: - `queue_row_from_golden` mints the full v0.2 shape, with deterministic ids derived from the row id (the live reports key their shared audits off them, so two runs must produce the same ids). - A boundary that rejects the whole dataset now fails the run with a message naming the likely cause, and an explicit slice that never reaches the resolver fails rather than vanishing. Restored: 319/319 validate, explicit gate 5/5 PASS, hub-firewall strips 319 leaking entries. Three tests cover it, including one that regresses the adapter to the flat shape and asserts exit code 1.
PRAteek-singHWY
requested review from
Pa04rth,
northdpole,
paoga87 and
robvanderveer
as code owners
August 12, 2026 14:19
Contributor
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Summary by CodeRabbit
WalkthroughChangesQueue contract and RFC adaptation
Live queue orchestration and persistence
Safety evaluation and row outcomes
Evaluation boundary and regression gate
Module C documentation and project metadata
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 10
🧹 Nitpick comments (5)
application/tests/librarian/envelope_sink_test.py (1)
76-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRelease the temporary directory and the schema file handles.
setUpcreates a directory withtempfile.mkdtemp()and notearDownremoves it, so every test method leaves a directory behind. Line 137 also passesopen(...)straight intojson.load, so those file objects stay unclosed.♻️ Proposed cleanup
def setUp(self) -> None: - self.dir = tempfile.mkdtemp() + self._tmp = tempfile.TemporaryDirectory() + self.addCleanup(self._tmp.cleanup) + self.dir = self._tmp.name self.path = os.path.join(self.dir, "envelopes.jsonl")And for the schema load at lines 136-140:
- schemas = [ - json.load(open(os.path.join(schema_dir, name), encoding="utf-8")) - for name in sorted(os.listdir(schema_dir)) - if name.endswith(".json") - ] + schemas = [] + for name in sorted(os.listdir(schema_dir)): + if not name.endswith(".json"): + continue + with open(os.path.join(schema_dir, name), encoding="utf-8") as fh: + schemas.append(json.load(fh))🤖 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/envelope_sink_test.py` around lines 76 - 78, Update the test fixture around setUp to add tearDown cleanup that removes the temporary directory created by tempfile.mkdtemp(), and revise the schema-loading code around json.load to use a context-managed file handle so it is closed after reading. Preserve the existing test behavior and schema contents.application/tests/librarian/factory_test.py (1)
94-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIsolate the environment in this test.
load_config()on line 106 reads the ambient environment. The other tests in this file clear it withmock.patch.dict(os.environ, {}, clear=True). If a developer or CI runner exportsCRE_LIBRARIAN_RETRIEVER_BACKEND=pgvector,build_componentstakes the pgvector branch and callscan_use_pgvector_similarityon_FakeDatabase, which does not define it. The test then fails for an unrelated reason.♻️ Proposed fix
- with mock.patch( - "application.utils.librarian.cross_encoder." "build_cross_encoder_score_fn", - return_value=lambda pairs: [0.0 for _ in pairs], - ): - components = build_components( - _FakeDatabase(), config=load_config(), embed_fn=embed - ) + with mock.patch( + "application.utils.librarian.cross_encoder." "build_cross_encoder_score_fn", + return_value=lambda pairs: [0.0 for _ in pairs], + ): + with mock.patch.dict(os.environ, {}, clear=True): + components = build_components( + _FakeDatabase(), config=load_config(), embed_fn=embed + )🤖 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/factory_test.py` around lines 94 - 109, Isolate configuration loading in test_embed_fn_is_injectable_so_no_paid_call_is_made by clearing the ambient environment with mock.patch.dict(os.environ, {}, clear=True) around load_config() and build_components. Preserve the existing embed injection and assertion behavior while ensuring CRE_LIBRARIAN_RETRIEVER_BACKEND cannot select the pgvector path.application/utils/librarian/knowledge_source.py (1)
90-104: 🗄️ Data Integrity & Integration | 🔵 TrivialTwo concurrent drains of the same run can produce duplicate envelopes.
The read applies no row lock. Two runners that read the same
pipeline_run_idat the same time select the same rows.mark_consumedfilters onconsumed_at IS NULL, so only one runner stamps each row, but both runners already wrote an envelope for it. The JSONL sink appends, so the file then holds two envelopes for onechunk_id.The CLI is documented as manual and opt-in, so this is not reachable today. When the orchestrator drives this path, add
with_for_update(skip_locked=True)on the read, or deduplicate envelopes bychunk_idin the consumer.🤖 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/knowledge_source.py` around lines 90 - 104, The _query method must prevent concurrent drains from selecting the same unconsumed rows. Apply a row-level lock with skip_locked enabled to the KnowledgeQueueRow query before ordering, limiting, and returning it, preserving the existing run-id, label, and consumed-at filters.application/utils/librarian/factory.py (1)
96-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider routing
run_librarianthroughbuild_componentsto remove duplicated wiring.
application/cmd/cre_main.pylines 1160-1201 still construct the retriever and the reranker inline: the same pgvector guard, the sameCandidatePool.from_mapping(cre_embeddings), the samebuild_retriever(...)arguments, and the sameCrossEncoderReranker(...)arguments.build_componentsnow performs that work and also exposesknown_cre_ids, which is theknown_idsset the fixture path needs. Two copies of this wiring will drift when a retriever or reranker argument changes in only one place.The fixture path can call
build_components(database, config=cfg)and readcomponents.retriever,components.reranker, andcomponents.known_cre_ids.🤖 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/factory.py` around lines 96 - 148, Update run_librarian in application/cmd/cre_main.py to call build_components(database, config=cfg) instead of constructing the retriever and CrossEncoderReranker inline. Reuse components.retriever, components.reranker, and components.known_cre_ids for the fixture path, removing the duplicated pgvector guard, embedding pool, and wiring arguments while preserving the existing behavior.cre.py (1)
334-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider the symmetric check for
--librarian_envelopes_out.This check rejects
--librarian_sourcewith--run_id, because the fixture path would be silently ignored. The mirror case is not checked:--librarian_envelopes_outwithout--run_idruns the fixture walk-through, which writes no envelopes, so the flag is silently ignored in the same way.♻️ Proposed additional check
if args.librarian_source and args.run_id.strip(): parser.error( "--librarian_source reads a fixture and cannot be combined " "with --run_id (which drains the live knowledge_queue)" ) + if args.librarian_envelopes_out and not args.run_id.strip(): + parser.error( + "--librarian_envelopes_out only applies to the live queue path; " + "add --run_id <pipeline_run_id>" + )🤖 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 `@cre.py` around lines 334 - 340, Extend the argument validation near the existing librarian_source/run_id check to reject --librarian_envelopes_out when --run_id is absent. Ensure the error clearly states that envelope output requires the live knowledge_queue path, while preserving the existing validation and fixture walk-through behavior otherwise.
🤖 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 @.github/workflows/librarian_regression.yml:
- Around line 37-38: Update the actions/checkout@v4 step in the “Check out code”
workflow action to set persist-credentials to false, preventing the checkout
from storing the GITHUB_TOKEN in local Git configuration.
In `@application/utils/librarian/queue_runner.py`:
- Around line 66-73: Update the run summary flow around run_librarian_queue and
QueueSummary so status is derived from the completed counts before to_json
serializes it: mark runs with errored > 0 or safety_unevaluated > 0 as
degraded/non-OK, while keeping status "ok" only when both counts are zero.
In `@application/utils/librarian/README.md`:
- Around line 9-12: Update the opening fences for both ASCII diagrams to use the
text language tag: application/utils/librarian/README.md lines 9-12 and
docs/gsoc_2026_module_c/final_report.md lines 21-27 should each use ```text,
with the diagram contents unchanged.
- Around line 45-56: Update LibrarianPipeline and queue_runner handling for C.0
rejections so a skipped RowOutcome is durably persisted before consumed_at is
set; if persistence is unavailable or fails, leave the row unconsumed. Preserve
the distinction between rejected rows and mid-pipeline errors in RunStats.
In `@docs/gsoc_2026_module_c/final_metrics.md`:
- Around line 119-125: Update the Week 9 selective-reranking comparison around
the reported 250/319 and shipped 238 results to explicitly define the evaluation
slice, metric, and denominator for both values. Either label the differing
populations clearly or recompute the baseline on the same 319-row population
before retaining the conclusion about the reranker versus corpus quality.
- Around line 3-4: Revise the provenance statement in final_metrics.md so it
applies only to the reproducible result tables generated by the command shown
below, rather than claiming every number in the document is command-derived.
Preserve the historical experiment and corpus-observation sections, and add
their provenance only if it is already known.
In `@docs/gsoc_2026_module_c/final_report.md`:
- Line 51: Use “no external database service” consistently in
docs/gsoc_2026_module_c/final_report.md lines 51-51 and replace “no DB” with “no
external DB service” in docs/gsoc_2026_module_c/runbook.md lines 168-178, while
retaining the documented local SQLAlchemy sessions and row inserts.
In `@docs/gsoc_2026_module_c/runbook.md`:
- Around line 46-48: Revise the C.3 exit-status statement in the runbook to
explicitly limit it to live evaluation. Preserve the existing explanation that
failed or skipped ECE causes live runs to return non-zero, while clarifying that
hermetic explicit-gate and boundary failures are also handled by the harness and
that C.4 remains informational.
- Around line 150-152: Update the troubleshooting section headed “A real run
reports rows decided but nothing consumed” to describe only sink refusal after
rows were processed. Add a separate preflight-error entry explaining that
application/cmd/cre_main.py raises SystemExit before processing when a non-dry
run omits --librarian_envelopes_out, so it cannot report “rows decided.”
In `@scripts/evaluate_librarian.py`:
- Around line 543-550: Update the evaluation flow around the rows selection and
the validated_total check so an empty rows result immediately fails before
reporting success, covering both an empty --dataset and a non-matching --slice.
Preserve the existing C.0 validation behavior for non-empty selections, and add
regression tests for both empty-selection cases.
---
Nitpick comments:
In `@application/tests/librarian/envelope_sink_test.py`:
- Around line 76-78: Update the test fixture around setUp to add tearDown
cleanup that removes the temporary directory created by tempfile.mkdtemp(), and
revise the schema-loading code around json.load to use a context-managed file
handle so it is closed after reading. Preserve the existing test behavior and
schema contents.
In `@application/tests/librarian/factory_test.py`:
- Around line 94-109: Isolate configuration loading in
test_embed_fn_is_injectable_so_no_paid_call_is_made by clearing the ambient
environment with mock.patch.dict(os.environ, {}, clear=True) around
load_config() and build_components. Preserve the existing embed injection and
assertion behavior while ensuring CRE_LIBRARIAN_RETRIEVER_BACKEND cannot select
the pgvector path.
In `@application/utils/librarian/factory.py`:
- Around line 96-148: Update run_librarian in application/cmd/cre_main.py to
call build_components(database, config=cfg) instead of constructing the
retriever and CrossEncoderReranker inline. Reuse components.retriever,
components.reranker, and components.known_cre_ids for the fixture path, removing
the duplicated pgvector guard, embedding pool, and wiring arguments while
preserving the existing behavior.
In `@application/utils/librarian/knowledge_source.py`:
- Around line 90-104: The _query method must prevent concurrent drains from
selecting the same unconsumed rows. Apply a row-level lock with skip_locked
enabled to the KnowledgeQueueRow query before ordering, limiting, and returning
it, preserving the existing run-id, label, and consumed-at filters.
In `@cre.py`:
- Around line 334-340: Extend the argument validation near the existing
librarian_source/run_id check to reject --librarian_envelopes_out when --run_id
is absent. Ensure the error clearly states that envelope output requires the
live knowledge_queue path, while preserving the existing validation and fixture
walk-through behavior otherwise.
🪄 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: c525282c-aa23-4d78-b0f3-fbff5fee39aa
📒 Files selected for processing (32)
.github/workflows/librarian_regression.yml.gitignoreapplication/cmd/cre_main.pyapplication/tests/librarian/config_loader_test.pyapplication/tests/librarian/envelope_sink_test.pyapplication/tests/librarian/evaluate_harness_test.pyapplication/tests/librarian/factory_test.pyapplication/tests/librarian/fixtures/sample_knowledge_queue.jsonlapplication/tests/librarian/knowledge_source_test.pyapplication/tests/librarian/pipeline_test.pyapplication/tests/librarian/queue_consumer_test.pyapplication/tests/librarian/queue_runner_test.pyapplication/tests/librarian/safety_guard_test.pyapplication/tests/librarian/schemas_test.pyapplication/tests/librarian/section_validator_test.pyapplication/utils/librarian/README.mdapplication/utils/librarian/__init__.pyapplication/utils/librarian/config_loader.pyapplication/utils/librarian/envelope_sink.pyapplication/utils/librarian/factory.pyapplication/utils/librarian/knowledge_source.pyapplication/utils/librarian/pipeline.pyapplication/utils/librarian/queue_consumer.pyapplication/utils/librarian/queue_runner.pyapplication/utils/librarian/safety_guard.pyapplication/utils/librarian/schemas.pyapplication/utils/librarian/section_validator.pycre.pydocs/gsoc_2026_module_c/final_metrics.mddocs/gsoc_2026_module_c/final_report.mddocs/gsoc_2026_module_c/runbook.mdscripts/evaluate_librarian.py
Comment on lines
+45
to
+56
| **1. A row is only retired if its envelope survived.** Marking a queue row | ||
| consumed tells Module B never to offer that chunk again. Doing that while the | ||
| envelope goes nowhere destroys the chunk outright. So `queue_runner` refuses to | ||
| stamp anything unless it was given a sink that reports `persists=True` *and* that | ||
| sink accepted the batch. Dry runs use `NullEnvelopeSink`, which reports `False`, | ||
| so a dry run can never consume. | ||
|
|
||
| **2. Errors and refusals are different.** A row rejected at the C.0 boundary is | ||
| consumed — re-reading a malformed row forever helps nobody. A row that *errored* | ||
| mid-pipeline (an embedding timeout, a cross-encoder hiccup) is left unconsumed, | ||
| because the next run should retry it. `RunStats` counts them separately on | ||
| purpose. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect: C.0 rejections have a durable outcome before consumed_at.
rg -n -C 8 \
'\b(consumed_at|rejected|skipped|errored|persists|envelope)\b' \
application/utils/librarian application/tests/librarianRepository: OWASP/OpenCRE
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- queue_runner structure ---'
ast-grep outline application/utils/librarian/queue_runner.py
printf '%s\n' '--- queue_runner implementation ---'
sed -n '1,280p' application/utils/librarian/queue_runner.py
printf '%s\n' '--- C.0 validator exceptions and pipeline handling ---'
rg -n -C 10 'SectionValidationError|section_from_queue|section_from_knowledge|skipped|consumed' \
application/utils/librarian/pipeline.py \
application/utils/librarian/section_validator.py \
application/utils/librarian/queue_runner.pyRepository: OWASP/OpenCRE
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- pipeline result and retirement selection ---'
sed -n '120,155p' application/utils/librarian/pipeline.py
sed -n '245,285p' application/utils/librarian/pipeline.py
printf '%s\n' '--- queue consumer ---'
cat -n application/utils/librarian/queue_consumer.py
printf '%s\n' '--- focused retirement tests ---'
sed -n '245,285p' application/tests/librarian/queue_runner_test.py
printf '%s\n' '--- focused behavioral verifier ---'
python3 - <<'PY'
from pathlib import Path
pipeline = Path("application/utils/librarian/pipeline.py").read_text()
runner = Path("application/utils/librarian/queue_runner.py").read_text()
checks = {
"C.0 rejection emits no envelope": "outcomes.append(RowOutcome(row_id, None, RowStatus.skipped))" in pipeline
and "continue" in pipeline[pipeline.index("except SectionValidationError"):pipeline.index("except SectionValidationError") + 300],
"retirement uses finished row ids": "finished = result.finished_row_ids()" in runner
and "mark_consumed(session, finished" in runner,
"sink writes only pipeline envelopes": "summary.persisted = sink.write(result.envelopes)" in runner,
}
for name, passed in checks.items():
print(f"{name}: {'YES' if passed else 'NO'}")
PYRepository: OWASP/OpenCRE
Length of output: 6991
Persist C.0 rejection outcomes before retiring rows.
LibrarianPipeline records C.0 rejections only as RowOutcome(status=skipped) and emits no envelope. queue_runner then retires those rows. Emit a durable rejection outcome before consumed_at, or leave the row unconsumed.
🤖 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/README.md` around lines 45 - 56, Update
LibrarianPipeline and queue_runner handling for C.0 rejections so a skipped
RowOutcome is durably persisted before consumed_at is set; if persistence is
unavailable or fails, leave the row unconsumed. Preserve the distinction between
rejected rows and mid-pipeline errors in RunStats.
Comment on lines
+3
to
+4
| Every number here comes from one command over the committed golden set. Nothing | ||
| is hand-copied from a notebook. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Limit the one-command provenance claim.
Line [3] says every number in this file comes from the command at Lines [6]-[10]. Lines [108]-[125] also report historical experiments and corpus observations. Add provenance for those values, or limit the statement to the reproducible result tables.
Proposed wording
-Every number here comes from one command over the committed golden set. Nothing
-is hand-copied from a notebook.
+The result tables below come from one command over the committed golden set.
+Historical experiments and corpus observations are identified in their sections.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Every number here comes from one command over the committed golden set. Nothing | |
| is hand-copied from a notebook. | |
| The result tables below come from one command over the committed golden set. | |
| Historical experiments and corpus observations are identified in their sections. |
🤖 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 `@docs/gsoc_2026_module_c/final_metrics.md` around lines 3 - 4, Revise the
provenance statement in final_metrics.md so it applies only to the reproducible
result tables generated by the command shown below, rather than claiming every
number in the document is command-derived. Preserve the historical experiment
and corpus-observation sections, and add their provenance only if it is already
known.
Comment on lines
+119
to
+125
| Week 9's selective-reranking experiment — gating C.2 to fire only where it helps | ||
| — reached 250/319 top-1 against the shipped 238, holding precision at τ=0.80. It | ||
| is deliberately not shipped: it needs two calibrators and held-out validation | ||
| that there was not time to do honestly. | ||
|
|
||
| **The path to ≥0.90 runs through populating CRE descriptions, not through a | ||
| better reranker.** That is a corpus problem and belongs upstream of Module C. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Define the slice for the selective-reranking comparison.
Line [60] reports C.2 on 292 positive rows. Lines [119]-[120] report 250/319 versus 238, but do not define the metric or slice for either value. Label both denominators and metrics, or compare the baseline on the same population before drawing the conclusion at Lines [124]-[125].
🤖 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 `@docs/gsoc_2026_module_c/final_metrics.md` around lines 119 - 125, Update the
Week 9 selective-reranking comparison around the reported 250/319 and shipped
238 results to explicitly define the evaluation slice, metric, and denominator
for both values. Either label the differing populations clearly or recompute the
baseline on the same 319-row population before retaining the conclusion about
the reranker versus corpus quality.
| | 7 | — | *analysis* | Auto-link threshold sweep; τ held at 0.80 | | ||
| | 8 | — | *this PR* | Live B→C integration — queue drain, sink, write-back | | ||
|
|
||
| **223 tests**, all hermetic — no database, API key, or model download required. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use “no external database service” consistently.
docs/gsoc_2026_module_c/final_report.md and docs/gsoc_2026_module_c/runbook.md describe the test suite as database-free, while docs/gsoc_2026_module_c/runbook.md documents real SQLAlchemy sessions and row inserts.
docs/gsoc_2026_module_c/final_report.md#L51-L51: replace “no database” with “no external database service”.docs/gsoc_2026_module_c/runbook.md#L168-L178: replace “no DB” with “no external DB service” and retain the local SQLAlchemy test detail.
📍 Affects 2 files
docs/gsoc_2026_module_c/final_report.md#L51-L51(this comment)docs/gsoc_2026_module_c/runbook.md#L168-L178
🤖 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 `@docs/gsoc_2026_module_c/final_report.md` at line 51, Use “no external
database service” consistently in docs/gsoc_2026_module_c/final_report.md lines
51-51 and replace “no DB” with “no external DB service” in
docs/gsoc_2026_module_c/runbook.md lines 168-178, while retaining the documented
local SQLAlchemy sessions and row inserts.
Comment on lines
+46
to
+48
| **Only the C.3 gate sets the exit status.** A failed *or skipped* ECE gate | ||
| returns non-zero, so a live run cannot pass without calibration having actually | ||
| run. C.4 is informational. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the C.3 exit-status statement.
Lines [23]-[25] state that the hermetic harness also fails for explicit-gate and boundary failures. Line [46] says only C.3 sets the exit status. Scope this statement to live evaluation so operators do not misread the other gates.
Proposed wording
-**Only the C.3 gate sets the exit status.**
+**For live evaluation, C.3 adds the calibration exit-status gate.**📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Only the C.3 gate sets the exit status.** A failed *or skipped* ECE gate | |
| returns non-zero, so a live run cannot pass without calibration having actually | |
| run. C.4 is informational. | |
| **For live evaluation, C.3 adds the calibration exit-status gate.** A failed *or skipped* ECE gate | |
| returns non-zero, so a live run cannot pass without calibration having actually | |
| run. C.4 is informational. |
🤖 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 `@docs/gsoc_2026_module_c/runbook.md` around lines 46 - 48, Revise the C.3
exit-status statement in the runbook to explicitly limit it to live evaluation.
Preserve the existing explanation that failed or skipped ECE causes live runs to
return non-zero, while clarifying that hermetic explicit-gate and boundary
failures are also handled by the harness and that C.4 remains informational.
Comment on lines
+150
to
+152
| **A real run reports rows decided but nothing consumed** | ||
| The sink refused the batch, or you passed no `--librarian_envelopes_out`. | ||
| Consumption is gated on persistence — this is the safe failure, not a bug. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate the preflight error from a sink refusal.
The supplied application/cmd/cre_main.py entrypoint raises SystemExit before processing when a non-dry run omits --librarian_envelopes_out. Therefore that case cannot report “rows decided.” Document the preflight error separately from a sink that refuses an already-processed batch.
Proposed troubleshooting split
**A real run reports rows decided but nothing consumed**
-The sink refused the batch, or you passed no `--librarian_envelopes_out`.
+The sink refused the batch after processing. Check sink persistence and logs.
+
+**A real run fails before processing**
+If a non-dry run omits `--librarian_envelopes_out`, the CLI raises before
+the queue runner starts.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **A real run reports rows decided but nothing consumed** | |
| The sink refused the batch, or you passed no `--librarian_envelopes_out`. | |
| Consumption is gated on persistence — this is the safe failure, not a bug. | |
| **A real run reports rows decided but nothing consumed** | |
| The sink refused the batch after processing. Check sink persistence and logs. | |
| **A real run fails before processing** | |
| If a non-dry run omits `--librarian_envelopes_out`, the CLI raises before | |
| the queue runner starts. |
🤖 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 `@docs/gsoc_2026_module_c/runbook.md` around lines 150 - 152, Update the
troubleshooting section headed “A real run reports rows decided but nothing
consumed” to describe only sink refusal after rows were processed. Add a
separate preflight-error entry explaining that application/cmd/cre_main.py
raises SystemExit before processing when a non-dry run omits
--librarian_envelopes_out, so it cannot report “rows decided.”
Comment on lines
+543
to
+550
| if rows and not validated_total: | ||
| print( | ||
| f"validation (C.0): 0/{len(rows)} rows validated — the whole golden " | ||
| "set was rejected at the boundary, so no report below this line " | ||
| "graded anything; FAILED (gates did not run). Most likely the " | ||
| "synthetic row shape has drifted from Module B's knowledge_queue" | ||
| ) | ||
| return 1 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail empty selections before reporting success.
If --dataset is empty, or --slice matches no rows, rows is empty. Line 543 bypasses this failure path. The command then returns 0 although C.0 and C.0.5 did not run.
Reject an empty selection before the total-validation check. Add regression coverage for an empty dataset and a non-matching slice.
Proposed fix
validated_total = sum(validated_per_slice.values())
- if rows and not validated_total:
+ if not rows:
+ print("validation (C.0): 0 rows selected; FAILED (gates did not run)")
+ return 1
+ if not validated_total:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if rows and not validated_total: | |
| print( | |
| f"validation (C.0): 0/{len(rows)} rows validated — the whole golden " | |
| "set was rejected at the boundary, so no report below this line " | |
| "graded anything; FAILED (gates did not run). Most likely the " | |
| "synthetic row shape has drifted from Module B's knowledge_queue" | |
| ) | |
| return 1 | |
| validated_total = sum(validated_per_slice.values()) | |
| if not rows: | |
| print("validation (C.0): 0 rows selected; FAILED (gates did not run)") | |
| return 1 | |
| if not validated_total: | |
| print( | |
| f"validation (C.0): 0/{len(rows)} rows validated — the whole golden " | |
| "set was rejected at the boundary, so no report below this line " | |
| "graded anything; FAILED (gates did not run). Most likely the " | |
| "synthetic row shape has drifted from Module B's knowledge_queue" | |
| ) | |
| return 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 `@scripts/evaluate_librarian.py` around lines 543 - 550, Update the evaluation
flow around the rows selection and the validated_total check so an empty rows
result immediately fails before reporting success, covering both an empty
--dataset and a non-matching --slice. Preserve the existing C.0 validation
behavior for non-empty selections, and add regression tests for both
empty-selection cases.
…run status **`consumed_at` was written timezone-aware into a naive column.** The runner passes an aware `at`, and `consumed_at` is a plain `DateTime`. That is dialect-dependent — SQLite keeps the offset in the string, Postgres drops it — and the B->C contract is explicit that the UTC wall clock is "stored and read back timezone-naive". `mark_consumed` now converts to UTC and strips tzinfo, so the stored instant is correct under both and matches the `created_at` values B writes beside it. Only ever tested on SQLite, which is exactly why this was invisible. **`RunSummary.status` was the constant "ok".** The orchestrator reads this JSON, so a run that dropped rows to errors, or decided them without the safety path, still reported success in the one field a consumer branches on. That is the same failure this module keeps fixing elsewhere: a field that looks like a verdict while measuring nothing. `finalize_status()` now derives it from the counts and names the reason. Note this makes every real run today report `degraded: N decided without the safety path`, because `NullSafetyGuard` is what ships and no row is ever evaluated. That is accurate rather than noisy — C genuinely runs without a safety path until the detector lands, and W8b's graph writer is required to refuse on exactly this signal. Also: - `envelope_sink_test` removes its temp directory; each method wrote envelopes containing chunk text and left the directory behind. - the harness contract test validates every golden row rather than the first 25; `main()` validates the whole selection, so a row past the cap could fail C.0 while the test still passed.
…the regression gate The last of the Module C plan that is not code: the package README, a runbook, the measured final metrics, the GSoC final report, and the CI gate that keeps the golden-set numbers from rotting. `.gitignore` blanket-ignores `*.md`, so the new docs need explicit negations — the same pattern `docs/Mid_eval_blog_gsoc2026/module_B_mideval_blog.md` already uses. Nothing else is un-ignored; gsoc-notes and other local markdown stay out. The regression workflow is deliberately hermetic: no DB, no key, no model download. The semantic reports (C.1 recall, C.2 top-1, the C.3 ECE gate, C.4 decision accuracy) need live CRE vectors, and seeding a candidate pool from golden text offline is exactly the leakage the hub firewall exists to strip — a CI job that "measured" them would be measuring nothing. What it does catch is the failure that actually bit this branch: a Module B schema change silently rejecting every row at the C.0 boundary. `final_metrics.md` records the numbers as measured, including the two that were missed. Top-1 is 75% against a planned 90%, and the write-up says why rather than rounding it away: W8's investigation tried thirteen reranker levers and all thirteen regressed, because 427 of 428 CREs have empty descriptions — a cross-encoder cannot cross-attend to a bare title. C.1 still reaches 98% recall over that same corpus, which localises the problem precisely. The path to 90% runs through populating CRE descriptions, not through a better reranker, and that is a corpus problem upstream of Module C. W9's selective reranking (250/319 vs the shipped 238) is documented as measured but deliberately unshipped: it needs two calibrators and held-out validation. Shipping it on in-sample numbers would be the same greenwashing this module spent four separate fixes removing.
The job failed on its first run: `virtualenv: not found`. `make install-python` builds a venv — which needs an apt `virtualenv` the other workflows install first — and then runs `playwright install`. Neither is used by a hermetic librarian run, and worse, the venv is never activated by the steps that follow, so they would have executed against a bare interpreter even had it built. Installs `requirements-dev.txt` straight into the runner's interpreter instead, which is what the two steps actually need, and drops the browser download. Also switched the unit step from pytest to `unittest discover`, matching how `make test` runs the suite in the existing Test job.
…gram fences - `actions/checkout` now sets `persist-credentials: false`. The job only reads the tree, so leaving the token in `.git/config` would expose it to anything the test run executes. - Tagged both ASCII pipeline diagrams as ```text (markdownlint MD040).
PRAteek-singHWY
force-pushed
the
gsocmodule_C_week_8b
branch
from
August 12, 2026 14:39
0038b6e to
63faf0b
Compare
Contributor
Author
|
Folding this into #1011 rather than reviewing it as a stack. The split was mine and it was not worth the extra review round: this is one week's work, and separating the integration from its own docs meant two PRs, two CI runs and two review cycles for a change that reads better in one place. #1011 now carries all 11 commits — the live B→C integration plus the package README, runbook, final metrics, final report, and the regression-gate workflow. Nothing is dropped; #1011 is a strict superset of what was here. Closing. Sorry for the churn, @northdpole — please review #1011 only. |
This was referenced Aug 16, 2026
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
1 participant
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 8b of Module C, and the last piece of the plan that is not code: package docs, the runbook, measured final metrics, the GSoC final report, and the CI gate.
Stacked on #1011 (week_8). Until that merges the diff shows its commits too; I'll rebase onto
mainas it lands, shrinking this to the docs-only surface. Stack order: #1011 → this.No application code changes. One
.gitignoreedit, one workflow, four markdown files.Overview
Module C is complete through C.4 and now drains B's live queue (#1011). What was missing was everything a maintainer needs to operate and trust it: how to run it, what each knob means, what the numbers actually are, and a gate that stops those numbers rotting.
What changed
application/utils/librarian/README.mddocs/gsoc_2026_module_c/runbook.mdCRE_LIBRARIAN_*variables, the pgvector cache migration, and a troubleshooting section keyed to the actual error strings.docs/gsoc_2026_module_c/final_metrics.mddocs/gsoc_2026_module_c/final_report.md.github/workflows/librarian_regression.yml.gitignore*.mdis blanket-ignored, so the new docs need explicit negations — the same patterndocs/Mid_eval_blog_gsoc2026/module_B_mideval_blog.mdalready uses. Nothing else is un-ignored.The measured numbers
Read C.4 by direction. Review recall is 5/5 — every chunk that should reach a human does, and the engine never wrongly auto-links something that needed review. For a gate whose job is protecting a shared graph, that is the number that matters. Auto-link recall of 56% is the cost: at τ=0.80 many correct-but-close positives fall under the bar and route to review, which is the safe direction.
On the missed top-1 target, stated plainly rather than rounded away. Week 8's investigation tried thirteen reranker levers and all thirteen regressed — C.2 as shipped scores net −7 against plain cosine on the same shortlist. The cause is not the model: 427 of 428 CREs have empty
descriptionfields, and a cross-encoder cannot cross-attend to a bare title. C.1 still reaches 98% recall over that same corpus, which localises the problem precisely — the information needed to rank within a shortlist largely is not present. The path to 90% runs through populating CRE descriptions, and that is a corpus problem upstream of Module C.W9's selective reranking (250/319 top-1 vs the shipped 238, precision held at τ=0.80) is documented as measured but deliberately unshipped: it needs two calibrators and held-out validation. Shipping it on in-sample numbers would be the same greenwashing this module spent four separate fixes removing.
Why the CI gate is hermetic
No DB, key, or model download. The semantic reports need live CRE vectors, and seeding a candidate pool from golden text offline is exactly the leakage the hub firewall exists to strip — a CI job that "measured" C.2 offline would be measuring nothing.
What it does catch is the failure that actually bit #1011: a Module B schema change silently rejecting every row at the C.0 boundary, leaving the explicit gate with nothing to count while the run still exited 0.
What the report records as not built
It also records that A → B is not yet connected (Module A does not write
harvest_input), so the end-to-end chain is not demonstrable outside a seeded record. That belongs in the report rather than in a claim that the pipeline is whole.How to verify locally