fix: crystallize exits non-zero and warns on approval failures - #58
Closed
Tet-9 wants to merge 63 commits into
Closed
fix: crystallize exits non-zero and warns on approval failures#58Tet-9 wants to merge 63 commits into
Tet-9 wants to merge 63 commits into
Conversation
…nd-label-14 fix: track backend label separately in JSONL search handler (vouchdev#14)
…ion-12 fix: add existence guards to all put_* methods (vouchdev#12)
…nt-artifact-guard fix(proposals): refuse to overwrite existing artifact on approve
kb.register_source_from_path read the contents of any file the process could access, letting an agent register /etc/passwd, ~/.ssh/id_rsa, ~/.aws/credentials etc. as a "source" and then retrieve the bytes via kb.cite or kb.list_sources. Resolve the path (following symlinks) and require it to be inside the KB root before reading. Adds KBStore.resolve_under_root() so both the JSONL handler and the MCP tool share one containment check. Fixes vouchdev#10
The CLI handlers for approve and reject caught only (ArtifactNotFoundError, ValueError) but proposals.approve() / proposals.reject() raise ProposalError -- a RuntimeError subclass. The four propose-* shortcuts caught nothing at all. Result: a double- approve, an empty rejection reason, an empty claim text, or an unknown source id surfaced a raw Python traceback instead of the one-line `Error: ...` the rest of the CLI emits. Add a small `_cli_errors` context manager that translates ArtifactNotFoundError / ValueError / ProposalError / LifecycleError into click.ClickException, and apply it to every command that calls into proposals, lifecycle, sessions, or storage. The MCP and JSONL servers already do the equivalent in their own envelopes; this brings the human-facing surface in line. Adds tests/test_cli.py with regressions for approve, reject, propose- claim, propose-entity, and show.
ruff B904 requires `raise ... from err` inside an except block so the original exception is preserved on the chained __cause__. Without it, the CI lint step rejects the file. The seven sites are pre-existing (put_claim, put_page, put_entity, put_relation, put_evidence, put_session, put_proposal); this PR inherited the failure from main rather than introducing it, but the path traversal fix cannot land until lint is green, so the cleanup happens here. No behavior change: the chained exception carries .__cause__ but the public ValueError message and type are unchanged.
verify_source() caught FileNotFoundError, but store.read_source_content() raises ArtifactNotFoundError (a KeyError subclass) when the content blob is missing -- so the "stored content missing" graceful path never ran and a single broken source crashed the entire verify_all() sweep, breaking `vouch source verify` and `vouch doctor`. The same call can also raise OSError (permission denied, TOCTOU race between exists() and read_bytes(), underlying I/O error) which was likewise unhandled. Catch both: the existing "missing" path keeps its note, and OSError surfaces as "stored content unreadable: <reason>". Adds three regression tests: - missing-content blob -> graceful per-source failure - unreadable stored content (monkeypatched PermissionError) -> graceful per-source failure with the underlying reason in the note - mixed sweep with one good + one broken source -> verify_all returns both results instead of aborting at the first failure Fixes vouchdev#30
The CI workflow runs `ruff check`, `mypy`, then `pytest`, and the fix/cli-clean-domain-errors branch was failing all three on pre-existing main-branch issues unrelated to the CLI change itself. * ruff: add `from e` to the seven `raise ValueError(...) from FileExistsError` re-raises added by the recent exclusive-create guards in storage.put_* (B904), and let ruff re-sort the cli.py import block (I001). * mypy: narrow the `kind` value flowing into `ContextItem(type=...)` with a `Literal` cast so the strict-typed field accepts what the search backends actually return. * pytest: `session_end()` mutates a session that `session_start()` has already written, but `put_session()` now uses exclusive create and rejects the second write. Add `KBStore.update_session()` mirroring `update_claim` and have `session_end` call it; existing test_sessions coverage now passes again. No behavior change for the CLI surface — these are infrastructure fixes so the existing test_sessions / lint / type assertions pass on this branch.
The B904 lint failures were already addressed in 2a439a5, but the CI matrix still fails on two pre-existing main-branch issues: * mypy: context.py:64 passed `kind: str` into ContextItem.type, which is Literal["claim","page","entity","relation","source"]. Narrow it with a typing.cast so strict mode accepts the search-backend output. * pytest: session_end() mutates a session that session_start() already wrote, but put_session() switched to exclusive create and rejects the second write. Add KBStore.update_session() mirroring update_claim and have session_end call it; test_sessions coverage passes again. No behavior change for the path-traversal fix itself.
CodeRabbit on PR vouchdev#28 noted that resolve_under_root() only validated a pathname snapshot. Callers then re-opened the same name with .is_file() and .read_bytes(), so an attacker who can swap the resolved path for a symlink between the containment check and the read can still exfiltrate an out-of-root file via kb.register_source_from_path. Collapse the validate-then-read into a single trusted helper `KBStore.read_under_root(path)` that returns `(resolved, bytes)`: 1. Path.resolve() chases pre-existing symlinks and the resulting target is checked for containment (existing behaviour -- legitimate in-root symlinks still work). 2. The read goes through `os.open(resolved, O_RDONLY | O_NOFOLLOW)` so a fresh symlink placed at the resolved name *after* the check fails with ELOOP rather than following the swap. 3. `fstat` + `S_ISREG` rejects directories / device nodes / pipes atomically on the same fd, replacing the racy `is_file()` test the callers used to do. Both register_source_from_path handlers (MCP + JSONL) switch to the new helper and drop their now-redundant follow-up checks. Adds tests: - symlink swapped into the resolved name -> rejected via ELOOP - directory at a valid path -> rejected via S_ISREG Existing "outside the root" and "inside the root" tests still pass.
…path-traversal fix(server): block path traversal in register_source_from_path
ruff B904 requires `raise ... from err` inside an except block so the original exception is preserved on the chained __cause__. Without it, the CI lint step rejects the file. The seven sites are pre-existing (put_claim, put_page, put_entity, put_relation, put_evidence, put_session, put_proposal); this PR inherited the failure from main rather than introducing it, but the path traversal fix cannot land until lint is green, so the cleanup happens here. No behavior change: the chained exception carries .__cause__ but the public ValueError message and type are unchanged.
The B904 lint failures were already addressed in 2a439a5, but the CI matrix still fails on two pre-existing main-branch issues: * mypy: context.py:64 passed `kind: str` into ContextItem.type, which is Literal["claim","page","entity","relation","source"]. Narrow it with a typing.cast so strict mode accepts the search-backend output. * pytest: session_end() mutates a session that session_start() already wrote, but put_session() switched to exclusive create and rejects the second write. Add KBStore.update_session() mirroring update_claim and have session_end call it; test_sessions coverage passes again. No behavior change for the path-traversal fix itself.
Resolve conflicts in context.py, sessions.py, storage.py (keep main). fix: block bad writes in import_apply per review feedback (vouchdev#13) import_apply now captures schema validation issues and skips the file instead of passing a throwaway list.
…rors fix(cli): translate domain errors into clean ClickException output
…raversal fix(bundle): reject path traversal in import (CVE-2007-4559, vouchdev#9)
…ception fix(verify): catch ArtifactNotFoundError on missing stored content
fix: catch ArtifactNotFoundError in verify (vouchdev#30) and validate bundle content on import (vouchdev#13)
Approved design for feat/semantic-search. Embedding (sentence- transformers all-mpnet-base-v2) becomes the primary search backend with FTS5 as fallback. Synchronous-at-write indexing across all six artifact types (claim, page, source, entity, relation, evidence). Maximally functional scope (~3000 LOC): pluggable model adapter registry, sqlite-vec ANN with NumPy fallback, cross-encoder rerank, HyDE query expansion, ingest-time duplicate detection, model-identity migration, recall/MRR/nDCG eval harness, and full CLI/MCP/JSONL parity. The writing-plans step will turn the rollout order in section 16 into concrete phased tasks.
32-task TDD plan for the semantic-search feature: foundation, storage, write hooks across all six artifact types, semantic-primary search integration in MCP/JSONL/CLI, RRF fusion + hybrid, cross-encoder rerank, HyDE expansion, ingest-time duplicate detection, model-identity migration, recall/MRR/nDCG scorer, end-to-end integration test, and user docs. Each task: failing test, minimal implementation, passing test, commit. MockEmbedder test double keeps the unit suite fast; the real model is exercised only under @pytest.mark.integration. The evaluation module is named scorer.py rather than eval.py to avoid shadowing the Python builtin and to keep static analysers quiet; the user-facing CLI subcommand remains `vouch eval embedding` (the Click group is registered under the name "eval", with the Python identifier eval_group).
CI ran ruff which flagged SIM105 on the three try/except ImportError guard blocks in src/vouch/embeddings/__init__.py. Replace each with `contextlib.suppress(ImportError)` -- same semantics, satisfies ruff. Also add mypy overrides for numpy / sqlite_vec / sentence_transformers / fastembed so the type check passes in the base [dev] CI install where those optional extras aren't present. The embedding code paths that import these libraries are only reached when the extras are installed at runtime; for the static type check the missing stubs are noise.
CI runs `pip install -e '.[dev]'` which deliberately excludes the
optional `[embeddings]` extras. tests/embeddings/test_*.py modules
import numpy at top level, so pytest collection fails with
ModuleNotFoundError before any test can be deselected.
Add tests/embeddings/conftest.py with `pytest.importorskip("numpy")`
so the entire embeddings test directory skips gracefully when numpy
is absent. Once `pip install vouch[embeddings]` (or numpy itself) is
present, the skip is a no-op and the tests run normally.
`_validate_content` in bundle.py uses the path's first directory component to look up a Pydantic validator. For sources, both `sources/<sha>/meta.yaml` (the Source model) and `sources/<sha>/content` (raw opaque bytes) hit the same "sources" key, so the validator was being run on the raw content bytes and failing with "1 validation error for Source". Add an early return for any non-meta.yaml path under `sources/` so opaque content bytes are not Pydantic-validated. Only the Source metadata file is checked, which matches the original intent of the PR vouchdev#13 validation. Unblocks three pre-existing test_bundle.py failures inherited from upstream main.
…embedding Per CodeRabbit Critical on PR vouchdev#39/40/43: SQLite does not allow a column alias defined in the SELECT list to be referenced in the WHERE clause of the same query. The old form SELECT ... 1.0 - vec_distance_cosine(vec, ?) AS score ... WHERE ... AND score >= ? raised `no such column: score` at runtime; the catch-all `except sqlite3.OperationalError` silently swallowed it and made every call fall through to the NumPy brute-force path, which works but defeats the whole point of installing sqlite-vec. Wrap the projection in a subquery so the WHERE clause sees the alias. No behavior change for callers that have already been using the fallback (NumPy still produces correct results); the ANN code path now actually runs when sqlite-vec is loaded.
The hybrid backend branch in kb_search (server.py) and _h_search (jsonl_server.py) lazily imports vouch.embeddings.fusion (added in Phase 5). On Phase 4's branch that module doesn't exist yet, so mypy emits import-untyped. Mark each import with `# type: ignore` and let ruff auto-format the line.
The KBStore._embed_and_store helper lazily imports vouch.embeddings.dedup (added in Phase 6). On Phase 3's branch that module does not yet exist, so mypy treats it as untyped and errors out. Mark the import with `# type: ignore[import-not-found,import-untyped, unused-ignore]` so the type check passes here and stays silent once Phase 6 lands.
Feat/semantic search
…ase-2-storage feat(embeddings): storage layer — state.db schema + vector search + query cache
…ase-3-write-hooks feat(embeddings): write-time embedding hooks across all six artifact kinds
…ase-4-read-integration feat(embeddings): semantic-primary kb.search and JSONL parity
The CLI propose-* commands and most actor sites called _whoami(), which only checked VOUCH_USER and the OS user — VOUCH_AGENT was silently ignored. The MCP and JSONL servers (server.py, jsonl_server.py) and session_start already honour VOUCH_AGENT, so multi-agent attribution broke as soon as you used the CLI to propose. Prefer VOUCH_AGENT over VOUCH_USER over the OS user so the recorded proposed_by / audit actor matches across all three transports.
Real propose -> review -> commit -> retrieve loop captured from a sandbox run on 2026-05-21. Includes on-disk YAML, audit log lines, and an honest note about the literal substring backend limitation pre-embeddings.
… hook Bundles the substantive correctness fixes flagged by CodeRabbit on PR vouchdev#41. CI was already green; these are latent bugs the review caught. storage._embed_and_store - Re-embed when the active embedder model changes, not only when the content hash changes. The previous short-circuit kept stale vectors on disk after a model swap, mixing document embeddings from one model with queries from another. - Truly best-effort: catch any exception during encode/put/meta/dedup so a hook failure can't bubble back to the caller and leave an artifact persisted-but-API-errored. index_db.reset - Also clears embedding_index, query_embedding_cache, embedding_dupes, and embedding_* keys from index_meta. Otherwise `vouch index` left orphaned hits behind after artifacts were removed. index_db.search_embedding (Python fallback) - Normalize the stored vector by its own L2 norm before scoring, so rankings match the sqlite-vec `1 - vec_distance_cosine` path. The previous raw dot product made magnitude leak into the score. index_db.search_semantic - Invalidate the query-vector cache entry when the embedder dim no longer matches what's on disk; otherwise a model swap returned stale vectors living in the wrong space. server.kb_search + jsonl_server._h_search (hybrid branch) - Hybrid now passes min_score into search_semantic (consistent with embedding/auto branches) and wraps the FTS lookup in try/except so an FTS5 error doesn't fail the whole request. - JSONL transport rejects unknown backend values with a clear error instead of silently returning []; matches MCP transport behavior. embeddings.fusion - rrf_fuse validates limit >= 0 and k >= 0 (k = -rank would divide by zero). weighted_fuse validates limit >= 0. Tests - Three new fusion guard tests cover the negative-limit and negative-k paths.
…ase-5-fusion feat(embeddings): hybrid fusion strategies (RRF, weighted, normalized)
crystallize() swallows every approve() exception into a failures list and the CLI printed the JSON result and exited 0 regardless of outcome. A complete failure — all proposals failing to approve — was indistinguishable from success without carefully inspecting the JSON. After emitting the JSON result, check result[failures]: - All failed (approved == 0): print error to stderr and sys.exit(1) - Partial failure: print warning to stderr, exit 0 Fixes vouchdev#57
📝 WalkthroughWalkthroughThis PR implements embedding-based semantic search with intelligent backend fallback, hardens security against path traversal and TOCTOU attacks, centralizes CLI error handling with improved crystallize exit codes, and documents the complete vouch protocol specification with versioned snapshots. The changes span ~4,700 lines across storage, indexing, server routing, security, CLI, and comprehensive specification/documentation artifacts. ChangesEmbedding Foundation & Semantic Search
Search Integration & Write-Time Persistence
Security Hardening & Defensive Logic
CLI & Operational Improvements
Comprehensive Specification & Documentation
Supporting Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This PR introduces substantial new functionality (embedding-based semantic search with multiple adapters, SQLite vector storage, hybrid fusion), significant security hardening (path safety, TOCTOU defense, approval guards), comprehensive specification documentation, and extensive test coverage across multiple domains. While individual layers are well-isolated and changes follow consistent patterns, the breadth of integration points (storage hooks, index db, server routing, CLI error handling), semantic density (embedding math, search fusion, state machine logic), and variety of file changes (Python source, YAML specs, markdown docs) demand careful reasoning across multiple functional areas and architectures. Possibly related issues
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
|
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 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 `@CHANGELOG.md`:
- Around line 9-18: There are two duplicate "### Fixed" headers in the
Unreleased section; consolidate them into a single "### Fixed" header and move
both bullet points under that one header so the bullets for "`vouch crystallize`
now exits..." and "Bundle import rejects tar members whose path escapes
`kb_dir`..." appear together under a single "### Fixed" section, removing the
extra header so the changelog conforms to MD024.
In `@docs/superpowers/specs/2026-05-20-semantic-search-design.md`:
- Around line 33-47: The fenced code block that lists files under
"src/vouch/embeddings/" is missing a language tag, triggering MD040; update the
opening fence from ``` to a labeled fence such as ```text (or ```bash) for that
block so the linter accepts it—find the block that begins with the line
"src/vouch/embeddings/" and change its opening fence accordingly while leaving
the file list (__init__.py, base.py, st_mpnet.py, etc.) unchanged.
In `@README.md`:
- Around line 233-249: The README currently contradicts itself about embeddings:
update the "Retrieval" table row and the later "Status" section so they say the
same thing (e.g., clarify that semantic embedding backends are optional and
available only behind install extras or that vector embeddings are not included
in the base implementation), by editing the "Retrieval" bullet in the "What
ships today" table and the "Status" section text to use identical wording about
availability, prerequisites, and which models (e.g., all-mpnet-base-v2,
MiniLM-L6, fastembed-BGE) are optional extras.
In `@spec/2026-05-21/methods.md`:
- Line 19: Links in spec/2026-05-21/methods.md use "../schemas/..." which
resolves to spec/schemas instead of the repository-level schemas; update the
references by replacing "../schemas/" with "../../schemas/" (e.g., the link on
line containing "**Result:**
[Capabilities](../schemas/capabilities.schema.json)" and the similar link around
line 51) so they point to the repo root schemas folder.
- Around line 201-205: Update the documented result shape for kb.crystallize to
match the CLI contract: add a failures field (array of failure objects or IDs
describing which claims failed and why) and an approval indicator (e.g.,
approved: boolean or outcome: "approved"|"rejected") alongside the existing
page_id and claims_promoted; ensure the spec references the exact symbols used
by the CLI (kb.crystallize, failures, approved/outcome) so consumers and the CLI
agree on the contract.
In `@spec/2026-05-21/retrieval.md`:
- Around line 5-7: The spec text incorrectly says embeddings are “not yet”
supported; update the retrieval spec sections referencing FTS5 and embeddings
(the paragraphs mentioning FTS5 as the only backend and the block around lines
describing embeddings) to state that vector embeddings are now implemented as an
optional, additional retrieval backend that complements — not replaces — FTS5,
and adjust any example/behavior notes and test pointers in the same sections to
reflect current implementation and test coverage; search for the phrases "FTS5
(SQLite full-text) as the only retrieval backend" and "embeddings" to locate and
edit the relevant paragraphs.
- Line 117: Update the Markdown link target in the line "decision. Making this
opt-in in 0.1 (see [ROADMAP.md](../ROADMAP.md))" so it points to the
repository-root ROADMAP; replace the incorrect "../ROADMAP.md" with the correct
"../../ROADMAP.md" (i.e., change the link target string "../ROADMAP.md" to
"../../ROADMAP.md").
In `@spec/2026-05-21/review-gate.md`:
- Around line 135-138: Update the spec text for kb.approve to match tested
behavior: when a durable artifact already exists for a proposal, kb.approve MUST
refuse to overwrite the existing artifact, must NOT move the proposal into
decided/, and must leave the proposal state as PENDING (i.e., do not perform the
durable write nor duplicate it). Update the sentence referencing "If the durable
artifact was written but the proposal not yet moved to `decided/`" to state the
rejection/leave-PENDING semantics for kb.approve and reference the durable
artifact, `decided/`, and PENDING state explicitly.
- Line 14: The fenced code block using triple backticks in the markdown lacks a
language tag and triggers MD040; update that fence to include an explicit
language identifier (for example change ``` to ```text or another appropriate
language) so the markdown linter recognizes the block language.
In `@spec/2026-05-21/SPEC.md`:
- Line 35: Several fenced code blocks in SPEC.md are missing a language tag
which triggers markdownlint MD040; update the triple-backtick fences that
currently read "```" for the diagram/tree listing examples to include an
explicit language (use "```text") so each of the three diagram/treelist blocks
(the fenced blocks containing the ASCII diagrams/tree listings) are changed to
use "```text" instead of plain "```".
- Around line 7-23: The dated SPEC.md contains relative links that are written
as if the file is at repo root; update each broken link in
spec/2026-05-21/SPEC.md (and the other ranges noted: 200-201, 252-253, 260-261,
318-319, 334-335, 365-366, 426) to be correct from the spec/2026-05-21/
directory by either prefixing with the proper relative path (e.g., ../ for
repo-root targets) or converting to root-absolute paths (starting with "/") so
references like the current "[spec/2026-05-21/SPEC.md]" and other in-file links
resolve on GitHub when viewed from the dated spec folder.
In `@spec/2026-05-21/transports.md`:
- Around line 101-103: The sentence conflates two rules about response ordering;
separate them into (1) a requirement that each response echo the same request
`id` (i.e., responses MUST include the request `id` they correspond to) and (2)
a distinct rule that if a server requires in-order processing, responses MUST be
emitted in the same order as request arrival (i.e., when the server processes
requests strictly in arrival order, responses MUST preserve that order); update
the text around "Server response order MUST match the request `id`" to split
these two normative statements and use explicit MUST language for both the
id-echoing requirement and the in-order-emission requirement.
In `@src/vouch/embeddings/cache.py`:
- Around line 15-16: The cache key currently only hashes the raw query in
_query_key which can collide across different embedding models/dimensions;
update _query_key to incorporate a stable embedder identity (e.g., model name,
backend id, and/or dimension) into the hash input so keys are namespaced by
embedder; likewise update the other cache key generators referenced around lines
19-21 and 46-47 to include the same embedder identifier so vectors produced by
different EmbeddingProvider instances cannot collide or produce shape
mismatches. Use the embedder's unique identifier (method/property from your
embedder/EmbeddingProvider) rather than mutable runtime state to construct the
key.
In `@src/vouch/index_db.py`:
- Around line 262-285: The cache keyed by id(conn) in _sqlite_vec_loaded can
falsely hit when object IDs are reused; change the cache to a weak-reference set
of connection objects (use weakref.WeakSet) and update _load_sqlite_vec to check
membership with "if conn in _sqlite_vec_loaded" and add the actual conn object
after successful sqlite_vec.load(conn); also replace the set[int] declaration
with a WeakSet[sqlite3.Connection] and import weakref so the cache automatically
drops closed/garbage-collected connections and avoids stale hits while keeping
the existing enable_load_extension/
sqlite_vec.load/conn.enable_load_extension(False) logic intact.
In `@src/vouch/sessions.py`:
- Around line 44-47: The current assignment to sess.proposal_ids includes
proposals in any status; change the comprehension that builds sess.proposal_ids
(the call to store.list_proposals() and the filter p.session_id == sess.id) to
also restrict to only pending proposals at session end (e.g., p.status ==
ProposalStatus.PENDING or p.status in {ProposalStatus.PENDING,
ProposalStatus.OPEN} depending on your enum names) before sorting, then call
store.update_session(sess) as before; ensure you reference the ProposalStatus
enum or the model's status values that the codebase uses so only unresolved
proposals are recorded.
In `@src/vouch/storage.py`:
- Around line 119-148: The read_under_root function currently unguardedly uses
os.O_NOFOLLOW which is not present on native Windows; update read_under_root to
check hasattr(os, "O_NOFOLLOW") before OR-ing that flag into os.open and handle
the Windows case explicitly: if O_NOFOLLOW exists keep the current
open+fstat+S_ISREG logic, otherwise either implement a Windows-safe alternative
(e.g. open with os.open without O_NOFOLLOW and perform an equivalent atomic
check if available) or raise a clear, specific ValueError/NotImplementedError
indicating the operation is not supported on native Windows; reference
read_under_root, resolved, and the os.open(...) call so the change is applied
around that open/fstat/close flow.
In `@tests/embeddings/test_search.py`:
- Around line 24-28: The autouse fixture _use_mock_embedder mutates the global
embedder registry by calling register(DEFAULT_MODEL_NAME, ...) without restoring
it; change the fixture to snapshot the current registry entry for
DEFAULT_MODEL_NAME (or the whole registry if needed) before calling register,
yield to run the test, and then restore the original registry state in a
finally/teardown step so register and DEFAULT_MODEL_NAME return to their prior
values; reference the _use_mock_embedder fixture, register function and
DEFAULT_MODEL_NAME symbol to locate where to save and restore the registry.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 51d860d2-67ad-4f45-bf48-ce613a1fc20c
⛔ Files ignored due to path filters (2)
docs/banner.svgis excluded by!**/*.svgdocs/demo.gifis excluded by!**/*.gif
📒 Files selected for processing (57)
.vouch/.gitignore.vouch/audit.log.jsonl.vouch/claims/vouch-uses-a-review-gated-proposal-workflow-agents-propose-c.yaml.vouch/config.yaml.vouch/decided/20260521-055206-7d6d92d6.yaml.vouch/sources/06d8519f8dcf4149d23c8a48984541b2e9365ec364e7e58192e28ed149a2c47c/content.vouch/sources/06d8519f8dcf4149d23c8a48984541b2e9365ec364e7e58192e28ed149a2c47c/meta.yaml.vouch/sources/67478e72acfb8fac3a059143e95c95f5cc6f7e8d4dccc05fbcea8dbccb8a4eba/content.vouch/sources/67478e72acfb8fac3a059143e95c95f5cc6f7e8d4dccc05fbcea8dbccb8a4eba/meta.yamlCHANGELOG.mdREADME.mdSPEC.mddocs/demo.tapedocs/example-session.mddocs/superpowers/plans/2026-05-20-semantic-search.mddocs/superpowers/specs/2026-05-20-semantic-search-design.mdpyproject.tomlschemas/README.mdscripts/gen_schemas.pyspec/2026-05-21/README.mdspec/2026-05-21/SPEC.mdspec/2026-05-21/audit-vocabulary.mdspec/2026-05-21/methods.mdspec/2026-05-21/retrieval.mdspec/2026-05-21/review-gate.mdspec/2026-05-21/transports.mdspec/README.mdsrc/vouch/bundle.pysrc/vouch/cli.pysrc/vouch/context.pysrc/vouch/embeddings/__init__.pysrc/vouch/embeddings/base.pysrc/vouch/embeddings/cache.pysrc/vouch/embeddings/fastembed_bge.pysrc/vouch/embeddings/fusion.pysrc/vouch/embeddings/st_minilm.pysrc/vouch/embeddings/st_mpnet.pysrc/vouch/index_db.pysrc/vouch/jsonl_server.pysrc/vouch/proposals.pysrc/vouch/server.pysrc/vouch/sessions.pysrc/vouch/storage.pysrc/vouch/verify.pytests/embeddings/__init__.pytests/embeddings/_fakes.pytests/embeddings/conftest.pytests/embeddings/test_core.pytests/embeddings/test_fusion.pytests/embeddings/test_integration.pytests/embeddings/test_search.pytests/embeddings/test_storage.pytests/test_bundle.pytests/test_cli.pytests/test_jsonl_server.pytests/test_storage.pytests/test_verify.py
Comment on lines
+9
to
+18
| ### Fixed | ||
| - `vouch crystallize` now exits with code 1 and prints an error when all proposals fail to approve, and prints a warning on partial failures. | ||
|
|
||
| ### Fixed | ||
| - Bundle import rejects tar members whose path escapes `kb_dir` | ||
| (CVE-2007-4559, #9). Previously a crafted `.tar.gz` with a member | ||
| named `../../evil.txt` could write outside `.vouch/`; the manifest | ||
| allow-list did not prevent this because the manifest lives inside | ||
| the same tarball. `import_apply`, `import_check`, and `export_check` | ||
| now validate every member path and raise on unsafe names. |
There was a problem hiding this comment.
Consolidate duplicate "### Fixed" sections.
The Unreleased section contains two separate "### Fixed" headers (lines 9 and 12), which violates changelog formatting conventions and triggers the markdownlint MD024 rule. Merge these into a single "### Fixed" section with both bullet points.
📝 Proposed fix
## [Unreleased]
### Fixed
- `vouch crystallize` now exits with code 1 and prints an error when all proposals fail to approve, and prints a warning on partial failures.
-
-### Fixed
- Bundle import rejects tar members whose path escapes `kb_dir`
(CVE-2007-4559, `#9`). Previously a crafted `.tar.gz` with a member
named `../../evil.txt` could write outside `.vouch/`; the manifest🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 12-12: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 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 `@CHANGELOG.md` around lines 9 - 18, There are two duplicate "### Fixed"
headers in the Unreleased section; consolidate them into a single "### Fixed"
header and move both bullet points under that one header so the bullets for
"`vouch crystallize` now exits..." and "Bundle import rejects tar members whose
path escapes `kb_dir`..." appear together under a single "### Fixed" section,
removing the extra header so the changelog conforms to MD024.
Comment on lines
+33
to
+47
| ``` | ||
| src/vouch/embeddings/ | ||
| __init__.py # public API: encode, search, register | ||
| base.py # Embedder ABC + adapter registry | ||
| st_mpnet.py # default impl (sentence-transformers all-mpnet-base-v2) | ||
| st_minilm.py # alternative impl | ||
| fastembed_bge.py # alternative impl (no-torch path via fastembed) | ||
| cache.py # query embedding LRU + content-hash skip cache | ||
| rerank.py # cross-encoder reranker (ms-marco-MiniLM-L6-v2) | ||
| hyde.py # Hypothetical Document Embedding query expansion | ||
| dedup.py # cosine-threshold duplicate detection at ingest | ||
| fusion.py # RRF, weighted-sum, normalized-cosine fusion strategies | ||
| eval.py # recall@k / MRR / nDCG harness | ||
| migration.py # model-identity check + backfill orchestration | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced code block.
This block is missing a fence language (MD040), which can trip markdown lint in CI.
Suggested fix
-```
+```text
src/vouch/embeddings/
__init__.py # public API: encode, search, register
base.py # Embedder ABC + adapter registry
@@
eval.py # recall@k / MRR / nDCG harness
migration.py # model-identity check + backfill orchestration</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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/superpowers/specs/2026-05-20-semantic-search-design.md around lines 33
- 47, The fenced code block that lists files under "src/vouch/embeddings/" is
missing a language tag, triggering MD040; update the opening fence fromto a labeled fence such astext (or ```bash) for that block so the linter accepts
it—find the block that begins with the line "src/vouch/embeddings/" and change
its opening fence accordingly while leaving the file list (init.py, base.py,
st_mpnet.py, etc.) unchanged.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+233
to
+249
| ## What ships today | ||
|
|
||
| | Area | Current support | | ||
| |------|-----------------| | ||
| | Knowledge base | `.vouch/` folder, YAML claims/entities/relations/evidence/sessions, markdown pages with frontmatter, JSONL audit log, content-addressed sources | | ||
| | CLI | `init`, `discover`, `capabilities`, `status`, `lint`, `doctor`, `pending`, `show`, `approve`, `reject`, `propose-{claim,page,entity,relation}`, `source add`, `source verify`, `supersede`, `contradict`, `archive`, `confirm`, `cite`, `session {start,end}`, `crystallize`, `search`, `context`, `index`, `audit`, `export`, `export-check`, `import-check`, `import-apply`, `serve` | | ||
| | Tool servers | MCP over stdio + JSONL over stdin/stdout, same `kb.*` surface across both transports, capabilities + knowledge-capability descriptor | | ||
| | Schemas | 13 JSON Schemas (Draft 2020-12) generated from pydantic in [schemas/](schemas/), plus hand-maintained `bundle.manifest` and `jsonl-envelope` schemas | | ||
| | Write safety | review-gated writes via [proposed/](spec/review-gate.md), `dry_run:true` previews, host trust required for `approve`/`reject`, atomic exclusive-create storage, path-traversal blocked on source intake and bundle import | | ||
| | Retrieval | SQLite FTS5 + substring fallback; optional semantic backends (`all-mpnet-base-v2`, `MiniLM-L6`, fastembed-BGE) behind install extras; context packs with citations + quality gate | | ||
| | Lifecycle | `supersede`, `contradict`, `archive`, `confirm`, `cite` — direct mutations, all audited | | ||
| | Portability | tar.gz bundles with per-file sha256 `manifest.json`, `export-check`, `import-check`, `import-apply` with skip/overwrite/fail conflict modes | | ||
| | Audit | append-only `audit.log.jsonl`, per-event actor (`VOUCH_AGENT`), object ids, dry-run flag, reversible flag | | ||
| | Adapters | Claude Code wiring documented via `.mcp.json` + `VOUCH_AGENT` env; per-runtime adapter templates not yet shipped | | ||
| | Validation | pytest suite (storage, FTS5, audit, source-verify, review-gate, bundle, JSONL), ruff + mypy gates, GitHub CI | | ||
| | Specification | dated snapshots under [spec/](spec/), JSON Schemas in [schemas/](schemas/), generator script at [scripts/gen_schemas.py](scripts/gen_schemas.py) | | ||
|
|
There was a problem hiding this comment.
Resolve README contradiction on embeddings availability.
Line 242 says semantic embedding backends ship today, but the later Status section still says vector embeddings are not in this implementation. Please reconcile those statements so users get a single, accurate capability picture.
🤖 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 `@README.md` around lines 233 - 249, The README currently contradicts itself
about embeddings: update the "Retrieval" table row and the later "Status"
section so they say the same thing (e.g., clarify that semantic embedding
backends are optional and available only behind install extras or that vector
embeddings are not included in the base implementation), by editing the
"Retrieval" bullet in the "What ships today" table and the "Status" section text
to use identical wording about availability, prerequisites, and which models
(e.g., all-mpnet-base-v2, MiniLM-L6, fastembed-BGE) are optional extras.
| ### `kb.capabilities` | ||
|
|
||
| **Params:** none. | ||
| **Result:** [Capabilities](../schemas/capabilities.schema.json). |
There was a problem hiding this comment.
Fix broken relative links to schema files.
From spec/2026-05-21/methods.md, ../schemas/... resolves to spec/schemas/... instead of repository schemas/....
Suggested fix
-**Result:** [Capabilities](../schemas/capabilities.schema.json).
+**Result:** [Capabilities](../../schemas/capabilities.schema.json).
@@
-**Result:** [ContextPack](../schemas/context-pack.schema.json).
+**Result:** [ContextPack](../../schemas/context-pack.schema.json).Also applies to: 51-51
🤖 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 `@spec/2026-05-21/methods.md` at line 19, Links in spec/2026-05-21/methods.md
use "../schemas/..." which resolves to spec/schemas instead of the
repository-level schemas; update the references by replacing "../schemas/" with
"../../schemas/" (e.g., the link on line containing "**Result:**
[Capabilities](../schemas/capabilities.schema.json)" and the similar link around
line 51) so they point to the repo root schemas folder.
Comment on lines
+201
to
+205
| ### `kb.crystallize` | ||
|
|
||
| **Params:** `{ "session_id": str, "no_page": bool? }`. | ||
| **Result:** `{ "page_id": str?, "claims_promoted": int }`. | ||
|
|
There was a problem hiding this comment.
kb.crystallize result shape is missing failure/approval fields used by CLI behavior.
The documented result omits fields that the CLI now relies on (failures and approved outcome), so this spec snapshot no longer matches the actual contract.
Suggested fix
-**Result:** `{ "page_id": str?, "claims_promoted": int }`.
+**Result:** `{ "page_id": str?, "claims_promoted": int, "approved": [str], "failures": [{"proposal_id": str, "error": str}] }`.📝 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
| ### `kb.crystallize` | |
| **Params:** `{ "session_id": str, "no_page": bool? }`. | |
| **Result:** `{ "page_id": str?, "claims_promoted": int }`. | |
| ### `kb.crystallize` | |
| **Params:** `{ "session_id": str, "no_page": bool? }`. | |
| **Result:** `{ "page_id": str?, "claims_promoted": int, "approved": [str], "failures": [{"proposal_id": str, "error": str}] }`. |
🤖 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 `@spec/2026-05-21/methods.md` around lines 201 - 205, Update the documented
result shape for kb.crystallize to match the CLI contract: add a failures field
(array of failure objects or IDs describing which claims failed and why) and an
approval indicator (e.g., approved: boolean or outcome: "approved"|"rejected")
alongside the existing page_id and claims_promoted; ensure the spec references
the exact symbols used by the CLI (kb.crystallize, failures, approved/outcome)
so consumers and the CLI agree on the contract.
Comment on lines
+15
to
+16
| def _query_key(query: str) -> str: | ||
| return hashlib.sha256(query.encode("utf-8")).hexdigest() |
There was a problem hiding this comment.
Namespace the cache key by embedder identity.
Line 15 currently keys only on query, so the same text can return stale vectors across different models/backends (or dimensions). That can silently degrade ranking or raise shape errors downstream.
Suggested direction
-def _query_key(query: str) -> str:
- return hashlib.sha256(query.encode("utf-8")).hexdigest()
+def _query_key(query: str, *, namespace: str) -> str:
+ material = f"{namespace}\n{query}".encode("utf-8")
+ return hashlib.sha256(material).hexdigest()
def cache_query_vec(
- kb_dir: Path, *, query: str, vec: np.ndarray, max_entries: int = 1024,
+ kb_dir: Path, *, query: str, vec: np.ndarray, namespace: str, max_entries: int = 1024,
) -> None:
- h = _query_key(query)
+ h = _query_key(query, namespace=namespace)
def lookup_query_vec(kb_dir: Path, *, query: str) -> np.ndarray | None:
- h = _query_key(query)
+ h = _query_key(query, namespace=namespace)Also applies to: 19-21, 46-47
🤖 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 `@src/vouch/embeddings/cache.py` around lines 15 - 16, The cache key currently
only hashes the raw query in _query_key which can collide across different
embedding models/dimensions; update _query_key to incorporate a stable embedder
identity (e.g., model name, backend id, and/or dimension) into the hash input so
keys are namespaced by embedder; likewise update the other cache key generators
referenced around lines 19-21 and 46-47 to include the same embedder identifier
so vectors produced by different EmbeddingProvider instances cannot collide or
produce shape mismatches. Use the embedder's unique identifier (method/property
from your embedder/EmbeddingProvider) rather than mutable runtime state to
construct the key.
Comment on lines
+262
to
+285
| _sqlite_vec_loaded: set[int] = set() | ||
|
|
||
|
|
||
| def _load_sqlite_vec(conn: sqlite3.Connection) -> bool: | ||
| """Best-effort load of the sqlite-vec extension.""" | ||
| if id(conn) in _sqlite_vec_loaded: | ||
| return True | ||
| try: | ||
| conn.enable_load_extension(True) | ||
| except (AttributeError, sqlite3.OperationalError): | ||
| return False | ||
| try: | ||
| import sqlite_vec # type: ignore[import-not-found] | ||
| except ImportError: | ||
| return False | ||
| try: | ||
| sqlite_vec.load(conn) | ||
| except sqlite3.OperationalError: | ||
| return False | ||
| finally: | ||
| with suppress(sqlite3.OperationalError): | ||
| conn.enable_load_extension(False) | ||
| _sqlite_vec_loaded.add(id(conn)) | ||
| return True |
There was a problem hiding this comment.
Cache keyed by id(conn) can produce stale hits after connection close/reopen.
Python reuses object IDs after garbage collection, so a newly opened connection may receive the same id() as a previously closed one. The cache would then incorrectly indicate sqlite_vec is loaded when it isn't (extensions must be loaded per-connection).
Consider using a WeakSet keyed by connection object instead, or simply remove caching since extension loading is cheap and connections are typically short-lived in this codebase.
Proposed fix using WeakSet
-_sqlite_vec_loaded: set[int] = set()
+import weakref
+_sqlite_vec_loaded: weakref.WeakSet[sqlite3.Connection] = weakref.WeakSet()
def _load_sqlite_vec(conn: sqlite3.Connection) -> bool:
"""Best-effort load of the sqlite-vec extension."""
- if id(conn) in _sqlite_vec_loaded:
+ if conn in _sqlite_vec_loaded:
return True
try:
conn.enable_load_extension(True)
except (AttributeError, sqlite3.OperationalError):
return False
try:
import sqlite_vec # type: ignore[import-not-found]
except ImportError:
return False
try:
sqlite_vec.load(conn)
except sqlite3.OperationalError:
return False
finally:
with suppress(sqlite3.OperationalError):
conn.enable_load_extension(False)
- _sqlite_vec_loaded.add(id(conn))
+ _sqlite_vec_loaded.add(conn)
return True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/index_db.py` around lines 262 - 285, The cache keyed by id(conn) in
_sqlite_vec_loaded can falsely hit when object IDs are reused; change the cache
to a weak-reference set of connection objects (use weakref.WeakSet) and update
_load_sqlite_vec to check membership with "if conn in _sqlite_vec_loaded" and
add the actual conn object after successful sqlite_vec.load(conn); also replace
the set[int] declaration with a WeakSet[sqlite3.Connection] and import weakref
so the cache automatically drops closed/garbage-collected connections and avoids
stale hits while keeping the existing enable_load_extension/
sqlite_vec.load/conn.enable_load_extension(False) logic intact.
Comment on lines
44
to
+47
| sess.proposal_ids = sorted({ | ||
| p.id for p in store.list_proposals() if p.session_id == sess.id | ||
| }) | ||
| path = store._session_path(sess.id) | ||
| path.write_text(_yaml_dump(sess.model_dump(mode="json"))) | ||
| store.update_session(sess) |
There was a problem hiding this comment.
Filter proposal_ids to pending proposals at session end.
Line 45 currently scans all statuses, so proposal_ids includes already resolved proposals. That inflates session-end counts and contradicts the expected semantics for issue #57.
Proposed fix
- sess.proposal_ids = sorted({
- p.id for p in store.list_proposals() if p.session_id == sess.id
- })
+ sess.proposal_ids = sorted({
+ p.id
+ for p in store.list_proposals(ProposalStatus.PENDING)
+ if p.session_id == sess.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 `@src/vouch/sessions.py` around lines 44 - 47, The current assignment to
sess.proposal_ids includes proposals in any status; change the comprehension
that builds sess.proposal_ids (the call to store.list_proposals() and the filter
p.session_id == sess.id) to also restrict to only pending proposals at session
end (e.g., p.status == ProposalStatus.PENDING or p.status in
{ProposalStatus.PENDING, ProposalStatus.OPEN} depending on your enum names)
before sorting, then call store.update_session(sess) as before; ensure you
reference the ProposalStatus enum or the model's status values that the codebase
uses so only unresolved proposals are recorded.
Comment on lines
+119
to
+148
| def read_under_root(self, path: str | Path) -> tuple[Path, bytes]: | ||
| # Guard against arbitrary-file-read primitives exposed by the MCP / | ||
| # JSONL `register_source_from_path` entrypoints, and against a TOCTOU | ||
| # race between the containment check and the read: an attacker who | ||
| # can swap the resolved name for a symlink after Path.resolve() has | ||
| # validated it could otherwise still exfiltrate an out-of-root file. | ||
| # | ||
| # Path.resolve() chases any pre-existing symlinks first (so legitimate | ||
| # in-root symlinks still work, then their target is the thing checked | ||
| # for containment). O_NOFOLLOW on the open then rejects a fresh | ||
| # symlink swapped into the resolved name after the containment check. | ||
| # fstat + S_ISREG rejects directories / special files atomically. | ||
| resolved = Path(path).resolve() | ||
| if not resolved.is_relative_to(self.root): | ||
| raise ValueError( | ||
| f"path must be inside project root ({self.root}): {resolved}" | ||
| ) | ||
| try: | ||
| fd = os.open(resolved, os.O_RDONLY | os.O_NOFOLLOW) | ||
| except OSError as e: | ||
| raise ValueError(f"cannot read {resolved}: {e}") from e | ||
| try: | ||
| if not stat.S_ISREG(os.fstat(fd).st_mode): | ||
| os.close(fd) | ||
| raise ValueError(f"not a regular file: {resolved}") | ||
| except OSError: | ||
| os.close(fd) | ||
| raise | ||
| with os.fdopen(fd, "rb") as f: | ||
| return resolved, f.read() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any Windows compatibility consideration in the codebase
rg -l -i "windows|win32|platform" --type=pyRepository: vouchdev/vouch
Length of output: 1935
🌐 Web query:
Python os.O_NOFOLLOW Windows availability
💡 Result:
Python’s os.O_NOFOLLOW is not available/portable on Windows: it’s a Unix feature, and the Python os module only provides it when the underlying C library defines it [1]. In practice, on Windows you should not expect os.O_NOFOLLOW to exist (i.e., attribute may be missing), and any code using it must be guarded with a feature check or use a Windows-specific approach [2][1]. How to check in code: - On Windows, test whether the attribute exists: hasattr(os, "O_NOFOLLOW") before using it [2]. - The official os documentation notes that the availability of these O_* constants depends on what the platform C library defines (“The above constants are extensions and not present if they are not defined by the C library”) [1]. Bottom line (for 2026-05-22): on native Windows (not WSL), treat os.O_NOFOLLOW as unsupported/absent and don’t rely on it for symlink “no follow” behavior [1].
Citations:
- 1: https://docs.python.org/3/library/os.html
- 2: https://runebook.dev/en/docs/python/library/os/os.O_NOFOLLOW
Make read_under_root handle native Windows where os.O_NOFOLLOW is missing
os.O_NOFOLLOW is a Unix-only constant; on native Windows it may be undefined, so os.open(resolved, os.O_RDONLY | os.O_NOFOLLOW) can raise AttributeError and break this security path. Guard with hasattr(os, "O_NOFOLLOW") and either fall back to a Windows-appropriate approach or raise a clear “not supported on Windows” 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 `@src/vouch/storage.py` around lines 119 - 148, The read_under_root function
currently unguardedly uses os.O_NOFOLLOW which is not present on native Windows;
update read_under_root to check hasattr(os, "O_NOFOLLOW") before OR-ing that
flag into os.open and handle the Windows case explicitly: if O_NOFOLLOW exists
keep the current open+fstat+S_ISREG logic, otherwise either implement a
Windows-safe alternative (e.g. open with os.open without O_NOFOLLOW and perform
an equivalent atomic check if available) or raise a clear, specific
ValueError/NotImplementedError indicating the operation is not supported on
native Windows; reference read_under_root, resolved, and the os.open(...) call
so the change is applied around that open/fstat/close flow.
Comment on lines
+24
to
+28
| @pytest.fixture(autouse=True) | ||
| def _use_mock_embedder() -> None: | ||
| from vouch.embeddings.base import DEFAULT_MODEL_NAME | ||
| register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8)) | ||
|
|
There was a problem hiding this comment.
Restore global embedder registry state in the autouse fixture.
Line [27] mutates shared global registry state and never restores it, which can leak mock as the default embedder into later modules and make test outcomes order-dependent.
Proposed fix
`@pytest.fixture`(autouse=True)
-def _use_mock_embedder() -> None:
- from vouch.embeddings.base import DEFAULT_MODEL_NAME
+def _use_mock_embedder(request: pytest.FixtureRequest) -> None:
+ from vouch.embeddings.base import DEFAULT_MODEL_NAME, _REGISTRY
+ prev = _REGISTRY.get(DEFAULT_MODEL_NAME)
register(DEFAULT_MODEL_NAME, lambda: MockEmbedder(dim=8))
+
+ def _restore() -> None:
+ if prev is None:
+ _REGISTRY.pop(DEFAULT_MODEL_NAME, None)
+ else:
+ _REGISTRY[DEFAULT_MODEL_NAME] = prev
+
+ request.addfinalizer(_restore)🤖 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_search.py` around lines 24 - 28, The autouse fixture
_use_mock_embedder mutates the global embedder registry by calling
register(DEFAULT_MODEL_NAME, ...) without restoring it; change the fixture to
snapshot the current registry entry for DEFAULT_MODEL_NAME (or the whole
registry if needed) before calling register, yield to run the test, and then
restore the original registry state in a finally/teardown step so register and
DEFAULT_MODEL_NAME return to their prior values; reference the
_use_mock_embedder fixture, register function and DEFAULT_MODEL_NAME symbol to
locate where to save and restore the registry.
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
crystallize()insessions.pycatches everyapprove()exception into afailureslist. The CLI then printed the JSON result and exited 0 regardless — a complete failure where every proposal failed to approve was indistinguishable from a successful crystallize without carefully reading the JSON output.Fix
After emitting the JSON result, inspect
result["failures"]:approved == []): print a clear error to stderr andsys.exit(1)The JSON output is unchanged so any downstream tooling parsing it is unaffected. The error and warning go to stderr only.
Testing
108 tests passing, no regressions.
Fixes #57
Summary by CodeRabbit
New Features
Bug Fixes
crystallizecommand now exits with error status when all proposals fail approvalDocumentation