Ci/diff coverage gate - #619
Merged
Merged
Conversation
repo-wide line coverage of src/vouch goes 82.96% -> 90.14% (3172 -> 1841 uncovered statements) and 18 modules reach 100%. 571 new tests. three setup problems were doing most of the damage, and none of them were visible from the coverage number alone: the embedder registry in embeddings/base is a module-global dict, and test_context, test_propose_similarity, test_clear_claims and test_triage each register a MockEmbedder into it and never restore it. the leak is inert while numpy is absent, so nobody noticed; install numpy and it flips later tests onto the embedding backend -- the fts5 backend-label assertions in test_cli, the deindex assertions in test_delete, and the salience cases in test_hot_memory all fail for reasons unrelated to what they test. tests/embeddings/conftest already isolated the registry for its own directory; tests/conftest now lifts that to the whole suite. nothing installed numpy, so tests/embeddings was skipped everywhere via `importorskip` and ~470 statements of shipped code never ran in any ci job. numpy alone unlocks it -- MockEmbedder is a pure-python fake -- so it joins the dev extra rather than requiring the embeddings extra and torch. `--cov=vouch` matches every importable copy of the package, and test_wheel_contents installs the built wheel into a pytest temp dir. those 58 duplicate modules landed in the report at ~0% and dragged the total from 85% to 43%, which is the number ci has been uploading. omitted in both [run] and [report] -- the [run] copy governs collection, but the temp-wheel modules still reach the data file and only [report] omit keeps them out of the totals. fail_under is a ratchet at 90: raise it as gaps close, never lower it to make a red build green. the two hard modules are done. strategy.py sandbox child needed sys.addaudithook swapped for a collector, so _install_audit_hook runs to completion and hands back the closure without permanently arming a write-blocking hook on the test interpreter; its child_main needed the fd primitives stubbed because os.dup2(devnull, 1) would silence pytest for every later test, and the real child is spawned with a stripped env that drops COVERAGE_PROCESS_START, so it is unmeasurable by design. volunteer_context needed a real event loop on a background thread for the mcp notification push, a closed loop for the RuntimeError path, and poll-until-logged assertions because the push resolves after the test's event fires. embeddings/similarity.py went 0% -> 100%. it is live code reached from proposals.py for propose-time duplicate warnings and had no tests at all. three findings pinned as tests rather than fixed, since changing them is behaviour and belongs in its own pr: `vouch capabilities` outside a kb cannot reach its except-Exception fallback because _load_store raises SystemExit; `vouch source add --url` silently discards the url because put_source only folds url into locator when locator is unset; kb_cite leaks ArtifactNotFoundError where every kb_read_* converts to ValueError. index_db.search_embeddings (plural) has no callers left in src or tests. covered so the number is honest, but deleting it is the better fix.
every python line a pr adds or changes under src/vouch must be executed by a test. the repo-wide fail_under ratchet stops regressions; this is the per-pr bar, so new code arrives covered instead of adding to the debt. a pr that touches no python under src/vouch passes trivially -- diff-cover reports "no lines with coverage information in this diff" and exits 0 -- so docs-only and workflow-only prs are unaffected. verified the gate actually gates rather than decorates: added uncovered lines exit 1, added covered lines exit 0, a docs-only diff exits 0, --fail-under 0 on the same failing diff exits 0 (so the flag is genuinely read), and an unknown flag errors out. rehearsed end to end in a worktree against a deliberately uncovered helper in admission.py: the gate failed, the bot named `src/vouch/admission.py -- line(s) 235-237` at 25%, and adding the covering test flipped it to 100% and exit 0. the first rehearsal was invalid and worth recording: the editable install made coverage measure the main checkout's src instead of the worktree's, so the gate passed a deliberately uncovered line. if coverage.xml describes different files than the diff, diff-cover silently skips them -- the job therefore reads the xml produced by ci's own test run in the same commit. when the gate fails, diff-coverage-comment posts the uncovered lines on the pr. it follows ci-label's pattern: workflow_run on ci completion, pr resolved via commits/<sha>/pulls so fork prs work, base-branch checkout only, never the pr head. the body is rendered by vouch.pr_bot, not by yaml, because file paths in the report come from the pr's own diff and belong in tested code; it is written to a file and posted with --body-file so it is never interpolated into a shell word. one marker-keyed comment per pr, updated in place and flipped to a resolved note when coverage goes green. auto-merge now reads the diff-coverage check-run conclusion for the head sha before arming, and on failure strips the label and comments why. it reads a conclusion rather than recomputing coverage: that workflow holds a write token and must never execute pr code. full coverage is a necessary condition for arming, not a sufficient one -- arming still needs the owner's label, because making a passing test suite sufficient would let any contributor merge non-core code unreviewed. the gate binds only once the check is required. `test` currently has no branch protection and `main` has no required contexts, so native auto-merge has nothing to wait for and merges as soon as a pr is conflict-free. adding "diff coverage (100% of changed python)" as a required status check is a repo-settings change and is not part of this pr. actionlint could not be run locally (no go toolchain); zizmor --persona regular reports no findings across all 23 workflows.
github-actions
Bot
added
ci
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThis pull request adds a 100% diff-coverage CI gate, PR coverage comments, and auto-merge enforcement. It also configures overall coverage thresholds and adds broad tests for embedding, CLI, server, migration, session, sandbox, caching, and volunteer-context behavior. ChangesDiff coverage automation
Regression surface tests
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 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 |
actionlint runs shellcheck over every `run:` block, and the new diff-coverage-comment workflow tripped three findings in its resolve-the-PR step. `base=test` reads as an attempt to capture the output of the `test` command (SC2209), so quote the branch name; the three consecutive `$GITHUB_OUTPUT` appends become one grouped redirect (SC2129). both edits are semantics-preserving -- the fail-closed base ref and the step outputs are unchanged.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
tests/test_jsonl_server_surface.py (1)
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winuse
itertools.countinstead of a bounded range iterator.
iter(range(1, 10_000))raisesStopIterationonce the module's tests exceed 9999 dispatches, which surfaces as an unrelated failure rather than a clear signal.♻️ suggested change
-_counter = iter(range(1, 10_000)) +_counter = itertools.count(1)plus the import:
+import itertools from pathlib import Path🤖 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 `@tests/test_jsonl_server_surface.py` at line 54, Replace the bounded _counter iterator with itertools.count starting at 1, adding the required itertools import, so dispatch identifiers remain available beyond 9,999 test calls.tests/test_index_db_embeddings.py (3)
246-250: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winthe hedged assertion is hiding a probable wrong
kindargument.every other call in this file passes the singular
"claim"; here it's"claims".assert snippet == "c1" or "\n" not in snippetthen passes either way, including the fallback-to-id path — so the test never proves the yaml artifact was read. use the same kind as_put/get_embeddingand assert the text.♻️ suggested change
- snippet = index_db._snippet_for(store.kb_dir, "claims", "c1") - assert snippet == "c1" or "\n" not in snippet + snippet = index_db._snippet_for(store.kb_dir, "claim", "c1") + assert "snippet source text" in snippetworth confirming which spelling
_snippet_forexpects before settling the fix.#!/bin/bash # how does _snippet_for resolve the `kind` argument, and what do callers pass? fd -t f 'index_db.py' --exec ast-grep outline {} --items all rg -nP -C10 'def _snippet_for' --type=py rg -nP -C2 '_snippet_for\s*\(' --type=py🤖 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 `@tests/test_index_db_embeddings.py` around lines 246 - 250, Update test_snippet_reads_the_yaml_artifact to pass the singular "claim" kind expected by _snippet_for, matching _put and get_embedding callers, then replace the hedged assertion with an exact assertion for "snippet source text" so the test verifies the YAML artifact is read.
185-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
all()over a possibly-empty list makes this pass without exercising anything.if the default
min_scorefilters out zero-cosine rows,hitsis[]and the assertion is vacuous. pin the intent by asserting the row is present (or that the result is empty, whichever is the real contract).♻️ suggested change
hits = index_db.search_embedding( - store.kb_dir, query_vec=np.zeros(8, dtype=np.float32) + store.kb_dir, query_vec=np.zeros(8, dtype=np.float32), min_score=0.0 ) + assert [h[1] for h in hits] == ["c1"] assert all(h[3] == pytest.approx(0.0) for h in hits)🤖 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 `@tests/test_index_db_embeddings.py` around lines 185 - 190, Update test_search_embedding_tolerates_a_zero_query_vector to assert the expected result count or matching row is present before validating its zero score, unless the intended contract is an empty result; ensure the test cannot pass vacuously when min_score filters out zero-cosine rows.
26-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_HashEmbedderis copy-pasted verbatim across four new test modules. same class, samename/version/dim, same sha256-normalise body — plus a marked variant intests/embeddings/test_similarity.py. one shared helper (or aregister-ing fixture intests/conftest.py, which now owns registry isolation) keeps the mock's dim and normalisation in one place.
tests/test_index_db_embeddings.py#L26-L37: move this class into a shared test helper and import it here.tests/test_cli_maintenance.py#L26-L39: import the shared class and drop the local copy (and its inlinehashlibimport).tests/test_jsonl_server_surface.py#L26-L39: import the shared class and drop the local copy.tests/test_server_tool_surface.py#L28-L41: import the shared class and drop the local copy.🤖 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 `@tests/test_index_db_embeddings.py` around lines 26 - 37, The _HashEmbedder implementation is duplicated across four test modules. Move the shared class into a common test helper, then import it in tests/test_index_db_embeddings.py (lines 26-37), tests/test_cli_maintenance.py (lines 26-39), tests/test_jsonl_server_surface.py (lines 26-39), and tests/test_server_tool_surface.py (lines 28-41); remove each local class definition and the now-unused inline hashlib import where applicable, preserving the existing name, version, dimension, hashing, and normalization behavior.tests/test_server_tool_surface.py (2)
245-248: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winpin the response key rather than accepting either.
out["proposal_id"] if "proposal_id" in out else out["id"]means the test passes under both shapes, so the tool's response contract — the thing MCP hosts parse — is never asserted. the sibling jsonl tests assertout["proposal_id"]directly; do the same here.♻️ suggested change
- assert store.get_proposal(out["proposal_id"] if "proposal_id" in out else out["id"]) + assert store.get_proposal(out["proposal_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 `@tests/test_server_tool_surface.py` around lines 245 - 248, The test test_kb_propose_claim_files_a_proposal should assert the canonical proposal_id response key directly. Remove the fallback to out["id"] and pass out["proposal_id"] to store.get_proposal, matching the sibling JSONL tests and pinning the tool response contract.
362-368: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winthis documents a real contract break in
vouch.server, worth tracking.the module docstring states the surface contract is that a missing artifact reaches the host as
ValueError;kb_citelettingArtifactNotFoundErrorthrough means an mcp host sees an unrecognised exception type instead. pinning it is fine for a coverage-only pr, but the wrapper inkb_citeshould be aligned with thekb_read_*family. want me to open an issue for it?🤖 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 `@tests/test_server_tool_surface.py` around lines 362 - 368, Update the server.kb_cite wrapper to catch ArtifactNotFoundError and convert it to ValueError, matching the documented surface contract and the existing kb_read_* behavior. Revise test_kb_cite_unknown_claim_raises to assert ValueError while preserving coverage for the missing “ghost” artifact case.tests/test_cli_read_list.py (1)
175-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick wingood to pin this, but the dead fallback in
cli.capabilitiesis still a latent bug.the comment correctly identifies that
_load_store()exits viaSystemExit, whichexcept Exceptioncannot catch, so the no-kb branch is unreachable. pinning current behaviour here is the right call for a coverage PR, but the fallback should either catchSystemExit/click.exceptions.Exitor be deleted. want me to open a tracking issue?🤖 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 `@tests/test_cli_read_list.py` around lines 175 - 186, Update the exception handling in cli.capabilities around _load_store() so the intended no-KB fallback is reachable by catching the exit type it actually raises, including SystemExit or click.exceptions.Exit as appropriate. Preserve the existing fallback behavior and keep the test test_capabilities_outside_a_kb_still_asks_for_init aligned with the corrected flow.tests/embeddings/test_similarity.py (1)
311-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winthe "ranked" half of this test is vacuous.
all five pending proposals share the same text, so every cosine is identical and
cosines == sorted(cosines, reverse=True)holds for any ordering. use texts with graded similarity if you want to pin the ranking.♻️ suggested shape
def test_pending_warnings_capped_at_three_and_ranked(store: KBStore) -> None: - text = "the same pending claim filed five times" - for i in range(5): - _pending(store, f"p{i}", text=text) - warnings = _codes(find_similar_on_propose(store, text), "similar_pending") + text = "the same pending claim filed five times" + _pending(store, "p0", text=text) + for i in range(1, 5): + # distinct texts -> distinct cosines, so the ordering is observable + _pending(store, f"p{i}", text=f"{text} variant {i}") + warnings = _codes( + find_similar_on_propose(store, text, threshold=0.0), "similar_pending" + ) assert len(warnings) == 3 cosines = [w["cosine"] for w in warnings] assert cosines == sorted(cosines, reverse=True) + assert len(set(cosines)) > 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 `@tests/embeddings/test_similarity.py` around lines 311 - 318, Update test_pending_warnings_capped_at_three_and_ranked so the five pending proposals use distinct texts with intentionally graded similarity to the queried text, while retaining the three-warning cap. Keep the cosine ordering assertion, ensuring the fixture data makes it validate meaningful descending ranking rather than equal values.tests/test_cli_bundle_and_misc.py (1)
197-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winseveral new tests are written to accept every outcome. the shared root cause is assertions that tolerate unknown behaviour (
A or Bon mutually exclusive branches, or a barereturnin anexcept) instead of pinning the one behaviour the setup produces — each of these sites reduces to an exit-code / no-crash check, which is exactly the kind of coverage that reads as green while asserting nothing.
tests/test_cli_bundle_and_misc.py#L197-L209: drop the"no experts found."alternative and assert the ranked-output branch that--min-claims 1produces.tests/test_cli_maintenance.py#L253-L255: assert the single deterministic bare-kb output instead of"no skills published" in out or "[" in out.tests/test_server_tool_surface.py#L495-L504: replace theexcept (ValueError, RuntimeError): returnescape with eitherpytest.raises(...)or an assertion on the returned shape.🤖 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 `@tests/test_cli_bundle_and_misc.py` around lines 197 - 209, Strengthen the three tests so they assert deterministic behavior instead of accepting any outcome: in tests/test_cli_bundle_and_misc.py:197-209, make test_experts_ranks_entities_by_evidence require the ranked output containing “claims=”; in tests/test_cli_maintenance.py:253-255, assert the expected single bare-kb output rather than either alternative; in tests/test_server_tool_surface.py:495-504, replace the exception-and-return escape with pytest.raises(...) or an assertion validating the returned shape.tests/test_pr_cache_helpers.py (1)
441-444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the timestamp parses rather than its length.
len(stamp) == 20encodes the format implicitly and fails opaquely if precision changes.♻️ Parse instead of measuring
def test_now_iso_is_a_utc_timestamp() -> None: stamp = pr_cache._now_iso() assert stamp.endswith("Z") - assert len(stamp) == 20 + parsed = datetime.fromisoformat(stamp.replace("Z", "+00:00")) + assert parsed.tzinfo is not NoneAdd
from datetime import datetimeto the imports.🤖 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 `@tests/test_pr_cache_helpers.py` around lines 441 - 444, Update test_now_iso_is_a_utc_timestamp to import datetime and validate stamp by parsing it with datetime.fromisoformat after converting the trailing UTC marker as needed, while retaining the UTC assertion. Remove the brittle len(stamp) == 20 check..github/workflows/ci.yml (1)
58-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDiff-cover version bound duplicated between
pyproject.tomland this workflow.
pip install 'diff-cover>=9,<10're-states the same bound already declared inpyproject.toml's dev extra. Installing justdiff-cover(rather than.[dev]) is reasonable for job speed, but the version range now has two sources of truth that can drift.🤖 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 @.github/workflows/ci.yml around lines 58 - 74, Update the install step near the Python setup to install diff-cover without repeating its version range, relying on the existing dev-extra declaration in pyproject.toml as the single source of truth. Keep the workflow’s direct diff-cover installation approach unchanged..github/workflows/diff-coverage-comment.yml (1)
22-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShellcheck warnings from the pipeline (SC2129, SC2209) in the "resolve the PR" step.
Two flagged issues:
- SC2129: the three sequential
echo ... >> "$GITHUB_OUTPUT"lines (lines 46-48) should be consolidated into a single redirect block.- SC2209 (lines 41, 44):
base=testreads as "did you meanbase=$(test)?" to shellcheck. It's intentional here (the fallback branch name is literallytest), but quoting silences the false-positive warning.🧹 Proposed fix
case "$base" in - ''|*' '*|*'..'*|-*) base=test ;; + ''|*' '*|*'..'*|-*) base="test" ;; esac if ! printf '%s' "$base" | grep -qE '^[A-Za-z0-9._/-]{1,100}$'; then - base=test + base="test" fi - echo "found=true" >> "$GITHUB_OUTPUT" - echo "number=$pr" >> "$GITHUB_OUTPUT" - echo "base=$base" >> "$GITHUB_OUTPUT" + { + echo "found=true" + echo "number=$pr" + echo "base=$base" + } >> "$GITHUB_OUTPUT"🤖 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 @.github/workflows/diff-coverage-comment.yml around lines 22 - 48, Update the “resolve the PR” step to address ShellCheck warnings: consolidate the three GITHUB_OUTPUT writes for found, number, and base into one grouped redirect block, and quote the literal fallback assignment in both validation branches so the intended branch name test is unambiguous without changing behavior.Source: Pipeline failures
🤖 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/auto-merge.yml:
- Around line 92-109: Update the check-run selection in the “require the
diff-coverage check to have passed” step to exclude in-progress runs before
sorting and selecting the latest conclusion. Filter matching runs to status
“completed” (or non-null completed_at), then retain the existing success
validation so auto-merge cannot arm from a stale completed success while a rerun
is still active.
In @.github/workflows/ci.yml:
- Around line 87-98: Update the diff-cover include pattern in
.github/workflows/ci.yml (lines 87-98) from src/vouch/* to src/vouch/** so
nested packages are covered; also update the local coverage repro command in
src/vouch/pr_bot.py (lines 258-307) to use the same src/vouch/** scope.
In `@tests/test_jsonl_server_surface.py`:
- Around line 63-72: Update the `_result` and `_error` helpers to validate the
complete JSONL response envelope: assert the echoed request `id` and `ok` value,
and ensure success responses contain `result` while failures contain `error`.
Preserve returning only `resp["result"]` and `resp["error"]` after these
assertions.
In `@tests/test_session_split_renarrate.py`:
- Around line 152-154: Make the assertion in the pending-topics test
unconditional: directly assert that “retrieval backends” is present in the
result of _pending_page_names and that the corresponding pending text appears in
prompt. Remove the surrounding conditional so the test fails when the pending
page is omitted.
In `@tests/test_strategy_sandbox_child.py`:
- Around line 123-134: Update test_hook_reads_the_guard_sets_from_closure_cells
to capture a real blocked event from strat._BLOCKED_EXACT before monkeypatching
the module globals, then use that captured event after disarming them. Remove
the dead conditional and hardcoded fallback, and wrap the updated assignment to
comply with the 88-character line limit.
In `@tests/test_volunteer_context_paths.py`:
- Around line 396-404: Move the _wait_for_log(caplog, "no event loop") assertion
inside the caplog.at_level("ERROR") context in
test_mcp_push_logs_when_the_loop_is_gone, so log polling remains active while
ERROR records are captured.
- Around line 451-459: Close the event loop created in
test_on_session_end_clears_everything after registering it, ensuring cleanup
occurs even if assertions fail; follow the existing test pattern for closing
newly created loops.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 58-74: Update the install step near the Python setup to install
diff-cover without repeating its version range, relying on the existing
dev-extra declaration in pyproject.toml as the single source of truth. Keep the
workflow’s direct diff-cover installation approach unchanged.
In @.github/workflows/diff-coverage-comment.yml:
- Around line 22-48: Update the “resolve the PR” step to address ShellCheck
warnings: consolidate the three GITHUB_OUTPUT writes for found, number, and base
into one grouped redirect block, and quote the literal fallback assignment in
both validation branches so the intended branch name test is unambiguous without
changing behavior.
In `@tests/embeddings/test_similarity.py`:
- Around line 311-318: Update test_pending_warnings_capped_at_three_and_ranked
so the five pending proposals use distinct texts with intentionally graded
similarity to the queried text, while retaining the three-warning cap. Keep the
cosine ordering assertion, ensuring the fixture data makes it validate
meaningful descending ranking rather than equal values.
In `@tests/test_cli_bundle_and_misc.py`:
- Around line 197-209: Strengthen the three tests so they assert deterministic
behavior instead of accepting any outcome: in
tests/test_cli_bundle_and_misc.py:197-209, make
test_experts_ranks_entities_by_evidence require the ranked output containing
“claims=”; in tests/test_cli_maintenance.py:253-255, assert the expected single
bare-kb output rather than either alternative; in
tests/test_server_tool_surface.py:495-504, replace the exception-and-return
escape with pytest.raises(...) or an assertion validating the returned shape.
In `@tests/test_cli_read_list.py`:
- Around line 175-186: Update the exception handling in cli.capabilities around
_load_store() so the intended no-KB fallback is reachable by catching the exit
type it actually raises, including SystemExit or click.exceptions.Exit as
appropriate. Preserve the existing fallback behavior and keep the test
test_capabilities_outside_a_kb_still_asks_for_init aligned with the corrected
flow.
In `@tests/test_index_db_embeddings.py`:
- Around line 246-250: Update test_snippet_reads_the_yaml_artifact to pass the
singular "claim" kind expected by _snippet_for, matching _put and get_embedding
callers, then replace the hedged assertion with an exact assertion for "snippet
source text" so the test verifies the YAML artifact is read.
- Around line 185-190: Update
test_search_embedding_tolerates_a_zero_query_vector to assert the expected
result count or matching row is present before validating its zero score, unless
the intended contract is an empty result; ensure the test cannot pass vacuously
when min_score filters out zero-cosine rows.
- Around line 26-37: The _HashEmbedder implementation is duplicated across four
test modules. Move the shared class into a common test helper, then import it in
tests/test_index_db_embeddings.py (lines 26-37), tests/test_cli_maintenance.py
(lines 26-39), tests/test_jsonl_server_surface.py (lines 26-39), and
tests/test_server_tool_surface.py (lines 28-41); remove each local class
definition and the now-unused inline hashlib import where applicable, preserving
the existing name, version, dimension, hashing, and normalization behavior.
In `@tests/test_jsonl_server_surface.py`:
- Line 54: Replace the bounded _counter iterator with itertools.count starting
at 1, adding the required itertools import, so dispatch identifiers remain
available beyond 9,999 test calls.
In `@tests/test_pr_cache_helpers.py`:
- Around line 441-444: Update test_now_iso_is_a_utc_timestamp to import datetime
and validate stamp by parsing it with datetime.fromisoformat after converting
the trailing UTC marker as needed, while retaining the UTC assertion. Remove the
brittle len(stamp) == 20 check.
In `@tests/test_server_tool_surface.py`:
- Around line 245-248: The test test_kb_propose_claim_files_a_proposal should
assert the canonical proposal_id response key directly. Remove the fallback to
out["id"] and pass out["proposal_id"] to store.get_proposal, matching the
sibling JSONL tests and pinning the tool response contract.
- Around line 362-368: Update the server.kb_cite wrapper to catch
ArtifactNotFoundError and convert it to ValueError, matching the documented
surface contract and the existing kb_read_* behavior. Revise
test_kb_cite_unknown_claim_raises to assert ValueError while preserving coverage
for the missing “ghost” artifact case.
🪄 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.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cd85c866-2baf-4bfa-a838-ac427eef8538
📒 Files selected for processing (21)
.github/workflows/auto-merge.yml.github/workflows/ci.yml.github/workflows/diff-coverage-comment.yml.gitignorepyproject.tomlsrc/vouch/pr_bot.pytests/conftest.pytests/embeddings/test_similarity.pytests/test_cli_bundle_and_misc.pytests/test_cli_lifecycle_surface.pytests/test_cli_maintenance.pytests/test_cli_read_list.pytests/test_index_db_embeddings.pytests/test_jsonl_server_surface.pytests/test_migrations_rewriter.pytests/test_pr_bot_diff_coverage.pytests/test_pr_cache_helpers.pytests/test_server_tool_surface.pytests/test_session_split_renarrate.pytests/test_strategy_sandbox_child.pytests/test_volunteer_context_paths.py
Comment on lines
+92
to
+109
| - name: require the diff-coverage check to have passed | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| REPO: ${{ github.repository }} | ||
| PR: ${{ github.event.pull_request.number }} | ||
| HEAD_SHA: ${{ github.event.pull_request.head.sha }} | ||
| run: | | ||
| conclusion=$(gh api "repos/$REPO/commits/$HEAD_SHA/check-runs" --paginate \ | ||
| --jq '[.check_runs[] | select(.name | startswith("diff coverage"))] | ||
| | sort_by(.completed_at) | last | .conclusion' 2>/dev/null || true) | ||
| if [ "$conclusion" != "success" ]; then | ||
| echo "::error::diff coverage is not green on $HEAD_SHA (conclusion=${conclusion:-missing}); refusing to arm auto-merge" | ||
| gh pr edit "$PR" --repo "$REPO" --remove-label auto-merge || true | ||
| gh pr comment "$PR" --repo "$REPO" --body \ | ||
| "auto-merge was not armed: the \`diff coverage\` check is not green on this head. every python line this PR changes under \`src/vouch/\` must be executed by a test. the bot has commented the uncovered lines; push tests and re-add the auto-merge label." | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
sed -n '1,150p' .github/workflows/auto-merge.yml 2>/dev/null || true
echo "== jq availability/version =="
jq --version 2>/dev/null || true
echo "== jq jq sort nulls behavior probe =="
python3 - <<'PY'
import json, subprocess, sys
checks = [
{"name": "diff coverage", "status": "completed", "completed_at": "2026-01-02T00:00:02Z", "conclusion": "success"},
{"name": "diff coverage", "status": "in_progress", "completed_at": None, "conclusion": None},
]
for lang in ["sort_by(.completed_at)", "sort_by(.status)", "select(.status == \"completed\") | sort_by(.completed_at)"]:
try:
out = subprocess.check_output(
["jq", lang],
input=json.dumps(checks),
text=True,
stderr=subprocess.STDOUT
)
last = out.strip().splitlines()[-1]
print(f"{lang}: conclusion={jq.loads(last).get('conclusion')}")
except Exception as e:
print(f"{lang}: ERROR {e}")
PY
echo "== jq comparison nulls and timestamps behavior probe =="
python3 - <<'PY'
import json, subprocess
samples = [
{"a": None, "label": "completed_at null"},
{"a": "2026-01-02T00:00:02Z", "label": "completed_at timestamp"},
]
for expr in ["sort_by(.a)", "sort_by(.status)", ".[:]", "if .[] != null then . else empty end"]:
out = subprocess.check_output(["jq", ".[]", "-n", f"--seq={json.dumps(samples)}", expr], text=True)
print("JQ not needed")
PYRepository: vouchdev/vouch
Length of output: 1914
🌐 Web query:
jq sort_by null value sort order null before other values documentation
💡 Result:
In jq, the sort_by function follows a strictly defined order for all data types. According to the official jq documentation, null values are consistently sorted before all other values [1][2]. The standard sorting order in jq is as follows: 1. null [1][2] 2. false [1][2] 3. true [1][2] 4. numbers [1][2] 5. strings (in alphabetical order by Unicode codepoint value) [1][2] 6. arrays (in lexical order) [1][2] 7. objects (compared by keys, then by values) [1][2] When you use sort_by(f), jq evaluates the filter f for each element and compares those results using this order [1][2]. Because null is the first element in the hierarchy, any items for which the filter returns null will appear at the beginning of the resulting array [1].
Citations:
Filter in-progress reruns before selecting the latest diff-coverage conclusion.
gh api .../check-runs includes null completed_at values for in_progress runs, and last can still pick the last list element rather than the latest by time. If a refreshed rerun starts before this job reads the check runs, arming may race behind a stale "success" conclusion. Select only completed runs before sorting, or require the selected check run to have .status == "completed" before accepting the conclusion.
🤖 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 @.github/workflows/auto-merge.yml around lines 92 - 109, Update the check-run
selection in the “require the diff-coverage check to have passed” step to
exclude in-progress runs before sorting and selecting the latest conclusion.
Filter matching runs to status “completed” (or non-null completed_at), then
retain the existing success validation so auto-merge cannot arm from a stale
completed success while a rerun is still active.
Comment on lines
+87
to
+98
| - name: diff coverage | ||
| id: gate | ||
| env: | ||
| BASE_REF: ${{ github.event.pull_request.base.ref }} | ||
| run: | | ||
| set +e | ||
| diff-cover coverage.xml \ | ||
| --compare-branch "origin/$BASE_REF" \ | ||
| --include 'src/vouch/*' \ | ||
| --fail-under 100 \ | ||
| --json-report diff-coverage.json \ | ||
| --markdown-report diff-coverage.md |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)(ci\.yml|pr_bot\.py|poetry\.lock|requirements|Pipfile|pyproject\.toml|setup\.cfg|tox\.ini)$' || true
echo
echo "== ci.yml relevant lines =="
if [ -f .github/workflows/ci.yml ]; then
sed -n '70,115p' .github/workflows/ci.yml | cat -n
fi
echo
echo "== pr_bot.py relevant lines =="
if [ -f src/vouch/pr_bot.py ]; then
sed -n '240,315p' src/vouch/pr_bot.py | cat -n
fi
echo
echo "== diff-cover dependency references =="
rg -n "diff[-_]?cover|diff-cover|include|coverage" pyproject.toml poetry.lock requirements*.txt setup.py setup.cfg tox.ini .github/workflows/ci.yml 2>/dev/null || trueRepository: vouchdev/vouch
Length of output: 1914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== diff-cover version if installed =="
python3 - <<'PY'
import subprocess
try:
out = subprocess.run(["diff-cover", "--version"], text=True, capture_output=True)
print(out.stdout + out.stderr)
except Exception as e:
print(f"diff-cover not available: {e}")
PY
echo
echo "== local glob behavior probe =="
python3 - <<'PY'
import glob
import os
for pattern in ["src/vouch/*", "src/vouch/**/*.py", "src/vouch/**/*"]:
try:
print(pattern, "=>", glob.glob(pattern))
except Exception as e:
print(pattern, "error", e)
PY
echo
echo "== inspect local requirements for diff-cover version if present =="
for f in $(git ls-files | rg '(^|/)(requirements.*\.txt|pyproject\.toml|poetry\.lock|setup\.cfg)$'); do
echo "--- $f"
rg -n "diff-cover|diff_cover|diffcover" "$f" || true
done
echo
echo "== search CI for checkout/pr/ref behavior =="
sed -n '1,130p' ./.github/workflows/ci.yml 2>/dev/null | cat -n || trueRepository: vouchdev/vouch
Length of output: 1914
Use src/vouch/** in the diff-coverage include pattern.
--include 'src/vouch/*' only matches files directly under src/vouch/; files under nested src/vouch/… packages are excluded from the gate. Change the workflow include pattern to src/vouch/** and update the repro guidance in src/vouch/pr_bot.py so the local command produces the same coverage scope.
📍 Affects 2 files
.github/workflows/ci.yml#L87-L98(this comment)src/vouch/pr_bot.py#L258-L307
🤖 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 @.github/workflows/ci.yml around lines 87 - 98, Update the diff-cover include
pattern in .github/workflows/ci.yml (lines 87-98) from src/vouch/* to
src/vouch/** so nested packages are covered; also update the local coverage
repro command in src/vouch/pr_bot.py (lines 258-307) to use the same
src/vouch/** scope.
Comment on lines
+63
to
+72
| def _result(method: str, **params: Any) -> Any: | ||
| resp = _call(method, **params) | ||
| assert "error" not in resp, resp | ||
| return resp["result"] | ||
|
|
||
|
|
||
| def _error(method: str, **params: Any) -> dict: | ||
| resp = _call(method, **params) | ||
| assert "error" in resp, resp | ||
| return resp["error"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
assert the full jsonl envelope shape here, not just the presence of result/error.
every test in this file funnels through these two helpers, so they're the natural place to pin the contract: {id, ok, result} on success and {id, ok: false, error} on failure. today neither the echoed id nor the ok flag is checked anywhere, so a regression that dropped ok or returned the wrong id would pass the whole suite.
♻️ suggested change
-def _call(method: str, **params: Any) -> dict:
- return handle_request(
- {"id": str(next(_counter)), "method": method, "params": params}
- )
+def _call(method: str, **params: Any) -> dict:
+ request_id = str(next(_counter))
+ resp = handle_request(
+ {"id": request_id, "method": method, "params": params}
+ )
+ assert resp["id"] == request_id, resp
+ return resp
def _result(method: str, **params: Any) -> Any:
resp = _call(method, **params)
assert "error" not in resp, resp
+ assert resp["ok"] is True, resp
return resp["result"]
def _error(method: str, **params: Any) -> dict:
resp = _call(method, **params)
assert "error" in resp, resp
+ assert resp["ok"] is False, resp
return resp["error"]as per coding guidelines, "For each new kb.* method, add a test that asserts the JSONL envelope shape: {id, ok, result} on success and {id, ok: false, error} on failure."
📝 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
| def _result(method: str, **params: Any) -> Any: | |
| resp = _call(method, **params) | |
| assert "error" not in resp, resp | |
| return resp["result"] | |
| def _error(method: str, **params: Any) -> dict: | |
| resp = _call(method, **params) | |
| assert "error" in resp, resp | |
| return resp["error"] | |
| def _call(method: str, **params: Any) -> dict: | |
| request_id = str(next(_counter)) | |
| resp = handle_request( | |
| {"id": request_id, "method": method, "params": params} | |
| ) | |
| assert resp["id"] == request_id, resp | |
| return resp | |
| def _result(method: str, **params: Any) -> Any: | |
| resp = _call(method, **params) | |
| assert "error" not in resp, resp | |
| assert resp["ok"] is True, resp | |
| return resp["result"] | |
| def _error(method: str, **params: Any) -> dict: | |
| resp = _call(method, **params) | |
| assert "error" in resp, resp | |
| assert resp["ok"] is False, resp | |
| return resp["error"] |
🤖 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 `@tests/test_jsonl_server_surface.py` around lines 63 - 72, Update the
`_result` and `_error` helpers to validate the complete JSONL response envelope:
assert the echoed request `id` and `ok` value, and ensure success responses
contain `result` while failures contain `error`. Preserve returning only
`resp["result"]` and `resp["error"]` after these assertions.
Source: Coding guidelines
Comment on lines
+152
to
+154
| pending = compile_mod._pending_page_names(store) | ||
| if "retrieval backends" in pending: | ||
| assert "- retrieval backends [pending]" in prompt |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Conditional assertion makes the pending-topics branch untestable.
If _pending_page_names stops returning the pending page, the assertion is skipped and the test still passes — exactly the regression this test exists to catch. Assert the pending set directly.
💚 Make the expectation unconditional
- pending = compile_mod._pending_page_names(store)
- if "retrieval backends" in pending:
- assert "- retrieval backends [pending]" in prompt
+ assert "retrieval backends" in compile_mod._pending_page_names(store)
+ assert "- retrieval backends [pending]" in prompt📝 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
| pending = compile_mod._pending_page_names(store) | |
| if "retrieval backends" in pending: | |
| assert "- retrieval backends [pending]" in prompt | |
| assert "retrieval backends" in compile_mod._pending_page_names(store) | |
| assert "- retrieval backends [pending]" in prompt |
🤖 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 `@tests/test_session_split_renarrate.py` around lines 152 - 154, Make the
assertion in the pending-topics test unconditional: directly assert that
“retrieval backends” is present in the result of _pending_page_names and that
the corresponding pending text appears in prompt. Remove the surrounding
conditional so the test fails when the pending page is omitted.
Comment on lines
+123
to
+134
| def test_hook_reads_the_guard_sets_from_closure_cells( | ||
| captured_hook: list[Any], monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| # the documented hardening: reassigning the module global must not disarm | ||
| # the installed hook, because it reads a closure cell instead | ||
| strat._install_audit_hook() | ||
| hook = captured_hook[0] | ||
| monkeypatch.setattr(strat, "_BLOCKED_EXACT", frozenset()) | ||
| monkeypatch.setattr(strat, "_BLOCKED_PREFIXES", ()) | ||
| event = next(iter(strat._BLOCKED_EXACT)) if strat._BLOCKED_EXACT else "socket.connect" | ||
| with pytest.raises(PermissionError): | ||
| hook(event, ()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Dead ternary: _BLOCKED_EXACT is already empty when the event is chosen.
The monkeypatch.setattr on line 130 runs first, so the conditional always falls through to the hardcoded "socket.connect", and the test quietly depends on that literal being in the real blocked set. Capture a real event before patching. (Line 132 is also 90 chars, over the 88-char limit.)
🐛 Capture the event before disarming the globals
strat._install_audit_hook()
hook = captured_hook[0]
+ event = next(iter(strat._BLOCKED_EXACT))
monkeypatch.setattr(strat, "_BLOCKED_EXACT", frozenset())
monkeypatch.setattr(strat, "_BLOCKED_PREFIXES", ())
- event = next(iter(strat._BLOCKED_EXACT)) if strat._BLOCKED_EXACT else "socket.connect"
with pytest.raises(PermissionError):
hook(event, ())📝 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
| def test_hook_reads_the_guard_sets_from_closure_cells( | |
| captured_hook: list[Any], monkeypatch: pytest.MonkeyPatch | |
| ) -> None: | |
| # the documented hardening: reassigning the module global must not disarm | |
| # the installed hook, because it reads a closure cell instead | |
| strat._install_audit_hook() | |
| hook = captured_hook[0] | |
| monkeypatch.setattr(strat, "_BLOCKED_EXACT", frozenset()) | |
| monkeypatch.setattr(strat, "_BLOCKED_PREFIXES", ()) | |
| event = next(iter(strat._BLOCKED_EXACT)) if strat._BLOCKED_EXACT else "socket.connect" | |
| with pytest.raises(PermissionError): | |
| hook(event, ()) | |
| def test_hook_reads_the_guard_sets_from_closure_cells( | |
| captured_hook: list[Any], monkeypatch: pytest.MonkeyPatch | |
| ) -> None: | |
| # the documented hardening: reassigning the module global must not disarm | |
| # the installed hook, because it reads a closure cell instead | |
| strat._install_audit_hook() | |
| hook = captured_hook[0] | |
| event = next(iter(strat._BLOCKED_EXACT)) | |
| monkeypatch.setattr(strat, "_BLOCKED_EXACT", frozenset()) | |
| monkeypatch.setattr(strat, "_BLOCKED_PREFIXES", ()) | |
| with pytest.raises(PermissionError): | |
| hook(event, ()) |
🤖 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 `@tests/test_strategy_sandbox_child.py` around lines 123 - 134, Update
test_hook_reads_the_guard_sets_from_closure_cells to capture a real blocked
event from strat._BLOCKED_EXACT before monkeypatching the module globals, then
use that captured event after disarming them. Remove the dead conditional and
hardcoded fallback, and wrap the updated assignment to comply with the
88-character line limit.
Comment on lines
+396
to
+404
| def test_mcp_push_logs_when_the_loop_is_gone( | ||
| caplog: pytest.LogCaptureFixture, | ||
| ) -> None: | ||
| dead = asyncio.new_event_loop() | ||
| dead.close() | ||
| vc.register_mcp_push("s1", _FakeSession(), dead) # type: ignore[arg-type] | ||
| with caplog.at_level("ERROR"): | ||
| vc._maybe_mcp_push(_offer()) | ||
| assert _wait_for_log(caplog, "no event loop") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Poll for the log record inside the caplog.at_level block.
The wait at line 404 runs after the level is restored, so a record arriving late can be filtered out and the test flakes. Line 393 does it correctly.
💚 Move the wait inside the context
with caplog.at_level("ERROR"):
vc._maybe_mcp_push(_offer())
- assert _wait_for_log(caplog, "no event loop")
+ assert _wait_for_log(caplog, "no event loop")📝 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
| def test_mcp_push_logs_when_the_loop_is_gone( | |
| caplog: pytest.LogCaptureFixture, | |
| ) -> None: | |
| dead = asyncio.new_event_loop() | |
| dead.close() | |
| vc.register_mcp_push("s1", _FakeSession(), dead) # type: ignore[arg-type] | |
| with caplog.at_level("ERROR"): | |
| vc._maybe_mcp_push(_offer()) | |
| assert _wait_for_log(caplog, "no event loop") | |
| def test_mcp_push_logs_when_the_loop_is_gone( | |
| caplog: pytest.LogCaptureFixture, | |
| ) -> None: | |
| dead = asyncio.new_event_loop() | |
| dead.close() | |
| vc.register_mcp_push("s1", _FakeSession(), dead) # type: ignore[arg-type] | |
| with caplog.at_level("ERROR"): | |
| vc._maybe_mcp_push(_offer()) | |
| assert _wait_for_log(caplog, "no event loop") |
🤖 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 `@tests/test_volunteer_context_paths.py` around lines 396 - 404, Move the
_wait_for_log(caplog, "no event loop") assertion inside the
caplog.at_level("ERROR") context in test_mcp_push_logs_when_the_loop_is_gone, so
log polling remains active while ERROR records are captured.
Comment on lines
+451
to
+459
| def test_on_session_end_clears_everything(store: KBStore) -> None: | ||
| hot_memory.register(session_id="s1", query="q", agent="a") | ||
| vc.enqueue_offer(_offer()) | ||
| vc.register_mcp_push("s1", _FakeSession(), asyncio.new_event_loop()) # type: ignore[arg-type] | ||
| vc.on_session_end("s1") | ||
| assert hot_memory.get("s1") is None | ||
| assert vc.drain_pending("s1", clear=False) == [] | ||
| with vc._state_lock: | ||
| assert "s1" not in vc._mcp_push |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unclosed event loop leaks a file descriptor.
asyncio.new_event_loop() here is never closed (unlike lines 399-400), which emits a ResourceWarning and leaks an fd per run.
🔒️ Close the loop
def test_on_session_end_clears_everything(store: KBStore) -> None:
hot_memory.register(session_id="s1", query="q", agent="a")
vc.enqueue_offer(_offer())
- vc.register_mcp_push("s1", _FakeSession(), asyncio.new_event_loop()) # type: ignore[arg-type]
+ loop = asyncio.new_event_loop()
+ loop.close()
+ vc.register_mcp_push("s1", _FakeSession(), loop) # type: ignore[arg-type]
vc.on_session_end("s1")📝 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
| def test_on_session_end_clears_everything(store: KBStore) -> None: | |
| hot_memory.register(session_id="s1", query="q", agent="a") | |
| vc.enqueue_offer(_offer()) | |
| vc.register_mcp_push("s1", _FakeSession(), asyncio.new_event_loop()) # type: ignore[arg-type] | |
| vc.on_session_end("s1") | |
| assert hot_memory.get("s1") is None | |
| assert vc.drain_pending("s1", clear=False) == [] | |
| with vc._state_lock: | |
| assert "s1" not in vc._mcp_push | |
| def test_on_session_end_clears_everything(store: KBStore) -> None: | |
| hot_memory.register(session_id="s1", query="q", agent="a") | |
| vc.enqueue_offer(_offer()) | |
| loop = asyncio.new_event_loop() | |
| loop.close() | |
| vc.register_mcp_push("s1", _FakeSession(), loop) # type: ignore[arg-type] | |
| vc.on_session_end("s1") | |
| assert hot_memory.get("s1") is None | |
| assert vc.drain_pending("s1", clear=False) == [] | |
| with vc._state_lock: | |
| assert "s1" not in vc._mcp_push |
🤖 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 `@tests/test_volunteer_context_paths.py` around lines 451 - 459, Close the
event loop created in test_on_session_end_clears_everything after registering
it, ensuring cleanup occurs even if assertions fail; follow the existing test
pattern for closing newly created loops.
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.
What changed
Why
What might break
VEP
Tests
make checkpasses locally (lint + mypy + pytest)CHANGELOG.mdupdated under## [Unreleased]Summary by CodeRabbit
New Features
src/vouch/*.Improvements