Fix/verify source containment#158
Closed
philluiz2323 wants to merge 83 commits into
Closed
Conversation
…ouchdev#76) crystallize wrote a durable Page directly through store.put_page, embedding sess.task, sess.note, and sess.agent verbatim into the rendered markdown body. The body was never gated by propose_page + approve, so an agent calling kb.session_start(task=<payload>) and getting any one claim approved via crystallize could land arbitrary content into pages/. The page surfaces in kb.read_page, kb.list_pages, kb.context, and (once vouchdev#60 is fixed) kb.search. Restrict the summary body to fields the proposing agent cannot influence: session id (server-generated), timestamps (server clock), and the list of approved artifact ids. The agent-controlled fields remain on the Session model itself and are still queryable, but no longer promoted into a durable Page. Also include summary_page_id in the session.crystallize audit event's object_ids when a page is written, so vouch audit truthfully attributes the write. Adds two regression tests: - test_crystallize_summary_page_does_not_leak_agent_controlled_fields - test_crystallize_audit_event_records_summary_page_id
Leftover from the merge of main (which brought PR vouchdev#62's test_sessions.py changes); ruff F401 was failing CI on PR vouchdev#77.
…index FTS5 on update (vouchdev#78) Two compounding bugs let archived / superseded / redacted claims keep flowing back to agents through kb.context: 1. build_context_pack had no claim.status filter, so any retrieval hit was appended to the pack regardless of subsequent lifecycle mutations. 2. store.update_claim refreshed the embedding cache but never re-indexed the FTS5 row, so claims_fts.status stayed frozen at first-index time. Every lifecycle op (archive, supersede, contradict, confirm) routes through update_claim and was therefore invisible to FTS5 / semantic search. This made ClaimStatus.{ARCHIVED, SUPERSEDED, REDACTED} purely decorative on the read side — agents kept quoting retracted knowledge as if it were live. Fix: - storage.update_claim now also calls index_db.index_claim under a short-lived sqlite connection (mirroring proposals.approve's first-index pattern). FTS5 errors are logged-and-skipped so a lifecycle op never fails because the embedding/FTS5 layer is misconfigured. - context.build_context_pack resolves each claim hit, drops it if the on-disk status is in {ARCHIVED, SUPERSEDED, REDACTED}, and drops it if the claim YAML has been deleted between index time and now. CONTESTED claims keep surfacing so contradictions remain visible. Tests: - test_context_pack_excludes_archived_claims - test_context_pack_excludes_superseded_claims - test_update_claim_refreshes_fts5_status (asserts the FTS5 status column via direct SQL against state.db) No on-disk-layout, schema, or bundle-format change.
Resolves conflicts in CHANGELOG.md, tests/test_sessions.py, and auto-merges src/vouch/sessions.py with the recently-merged: - vouchdev#61 (FTS5 indexing of crystallize summary page) — adds index_db.index_page(...) right after store.put_page(...) inside crystallize. Composes cleanly with this PR's strict-derivation _build_summary_body and the summary_page_id audit fix. - vouchdev#62 (test_crystallize_collects_approval_failures) — preserved as a sibling test. - vouchdev#75 (bundle import sha256 verification) — CHANGELOG entry only. - vouchdev#73 (bundle POSIX separators) — CHANGELOG entry only. Both regression tests added by this PR (test_crystallize_summary_page_does_not_leak_agent_controlled_fields and test_crystallize_audit_event_records_summary_page_id) still pass against the merged sessions.crystallize.
vouchdev#81) The 'claims must cite sources' guarantee (README §'Why this exists' point 3; CONTRIBUTING §'Things we won't merge') used to live only in proposals.propose_claim, so every other write path silently accepted Claim(evidence=[]) and landed an uncited claim: - store.put_claim direct: existence-check loop iterates zero times. - store.update_claim: writes the YAML without re-validating. - bundle.import_apply via _validate_content: defers to Claim.model_validate, which accepted evidence=[] because the model had no min-length constraint. Add @field_validator('evidence') on Claim — raises ValueError when the list is empty. Closes all three bypass paths in one place. store.update_claim additionally re-validates via Claim.model_validate(claim.model_dump()) before persisting, so in-place mutation (c.evidence = []; store.update_claim(c)) raises before the YAML hits disk — the field validator only fires at construction time, not on attribute assignment. Four regression tests: - test_claim_model_rejects_empty_evidence (tests/test_storage.py) — Claim(evidence=[]) raises pydantic.ValidationError. - test_put_claim_rejects_empty_evidence — store.put_claim raises; no claims/<id>.yaml is written. - test_update_claim_rejects_empty_evidence — in-place mutation + update_claim raises; the on-disk YAML is unchanged. - test_import_rejects_uncited_claim (tests/test_bundle.py) — a schema-valid bundle whose claim YAML has evidence: [] is rejected by import_check (schema validation issue) and import_apply raises before writing. The existing guard in proposals.propose_claim becomes a redundant user-facing error message and is left in place for the friendlier CLI/JSONL error string. No on-disk-layout, schema, or bundle-format change; data the model never should have accepted now raises.
…ing from tarball (vouchdev#80) import_check previously only verified that tar members listed in the manifest had matching hashes, but never checked the reverse: whether every manifest entry had a corresponding tar member. A bundle whose manifest.json referenced claims/c1.yaml but whose tarball contained only manifest.json would pass import_check with ok=True, and import_apply would silently write nothing — no exception, no audit event indicating data loss. Add the missing-member pass (mirroring the existing check in export_check) so that manifest entries without a matching tar member produce a "manifest lists missing file" issue. import_apply then refuses to import because check.issues is non-empty.
read-only `vouch diff <old> <new>` that shows what changed between two claim or two page revisions: field-level changes plus a line-diff of the long text/body. auto-detects kind, hides churning metadata.
new read-only `vouch diff <old> <new>` compares two claims or two pages by id and shows what changed: scalar/list fields as old → new, plus a line-diff of the long text/body. auto-detects kind, hides churning metadata (timestamps, approved_by), supports --json. closes a roadmap 0.1 item.
mypy inferred old/new as Claim from the first branch, so the page branch's reassignment tripped an incompatible-assignment error in CI. annotate the union up front.
feat(diff): add `vouch diff` for claim/page revisions
…hdev#82 review) The new Claim.evidence min-citation validator (vouchdev#81) also fires when claims are read back from disk. A KB that has a pre-existing uncited claims/<id>.yaml from before the fix would otherwise crash vouch lint / vouch doctor with a bare pydantic.ValidationError deep in store.list_claims(). Add _load_claims_for_lint(), a per-file iteration that catches pydantic.ValidationError (and any other load error) and surfaces each bad file as a Finding with code='invalid_claim' and an explicit repair hint: 'edit the YAML to add a citation, or delete the file'. lint() also stops calling status() to populate counts — status() calls the strict store.list_claims() which would re-raise on the same files — and builds the counts dict inline from the safely-loaded valid claims. Regression test in tests/test_health.py: - test_lint_surfaces_legacy_uncited_claim_yaml_without_crashing hand-crafts a claims/legacy.yaml with evidence: [] (matches the on-disk shape an older buggy write path would have left), asserts vouch lint runs to completion, surfaces invalid_claim in findings with the repair-hint message, and that the well-formed sibling claim is still discovered. CHANGELOG migration note expanded to describe the repair hint.
The new inline-built counts in lint() (from 5c881de) is a literal dict with mixed value types (str/int/bool), which mypy correctly inferred as dict[str, object] and rejected against the HealthReport.counts: dict[str, int] annotation. The original status() returned the same mixed dict via an untyped 'dict' return, which masked the mismatch — the narrow type was effectively never checked at the call site. Widen counts to dict[str, Any] to match runtime reality. Also tighten status()'s return annotation from 'dict' to 'dict[str, Any]' for consistency. No caller does arithmetic on counts values; they just echo or pass through, so the widening is risk-free.
feat: add guided proposal review CLI
Feat/pending json
…ch-sync Feat/deterministic vouch sync
…ummary-page-bypass fix(sessions): close crystallize review-gate bypass via summary page
…s-citation fix(models): require Claim.evidence to be non-empty at the model layer
…nd-config fix(context): honor retrieval.backend config instead of hardcoding embeddings
feat(cli): vouch approve accepts multiple ids for scriptable bulk approval
generalize init seeding into a named-template registry and ship a gittensor (SN74) starter pack as the first non-default template. complements gittensory rather than cloning its live-data signals.
generalize init seeding into a small in-code template registry. default 'starter' is unchanged; new 'gittensor' template seeds a cited, approved pack (1 source, 1 entity, 4 claims) about SN74 contribution scoring so a fresh kb in a gittensor repo has retrievable context immediately. unknown --template gives a clean error listing available packs. complements gittensory's live-data signals rather than cloning them.
# Conflicts: # src/vouch/health.py # tests/test_cli.py
# Conflicts: # src/vouch/cli.py
feat(init): add --template with a Gittensor (SN74) starter pack
…late feat(init): add --template with a gittensor seed pack
…chdev#97) Adds a `vouch.logging_config` module with a `JsonFormatter` and an idempotent `configure_logging()` that the CLI, MCP server, and JSONL server entry points call at startup. When `VOUCH_LOG_FORMAT=json`, the `vouch` logger emits one JSON object per line with `level`, `logger`, `event`, and any structured extras (`actor`, `object_ids`, ...) passed via the stdlib `extra=` argument. Any other value (including unset) is a no-op - vouch's loggers keep stdlib default behaviour, so callers who don't opt in see no change. Scoped to the `vouch` logger namespace with propagate=False in JSON mode, so this never reformats logs emitted by libraries vouch happens to depend on.
# Conflicts: # CHANGELOG.md
…tion-docs docs(gittensor): adoption guide (init, MCP wiring, decision workflow)
…le-proposals Feat/vouch expire stale proposals
the var was already documented in adapters/generic-mcp/README ('force a
specific KB root, skip discovery') but never wired into the code. now
`discover_root()` reads VOUCH_KB_PATH first and short-circuits the upward
walk, with a clear error if the value isn't an existing .vouch directory.
closes the doc-vs-code drift and removes the per-host 'cwd: ...' ceremony
needed by Claude Desktop (and any other host that launches the stdio
server from a default working directory).
fix(storage): honour VOUCH_KB_PATH env var in discover_root
Address review on vouchdev#112. - fsck() now loads claims via _load_claims_for_lint() like lint(), so a single invalid YAML (e.g. a legacy uncited claim from before vouchdev#81) becomes an `invalid_claim` finding instead of aborting the whole check with a traceback — the exact inconsistency a deep checker should surface. Counts are built from the safely-loaded claims via a shared _safe_counts() helper so the report can't re-trip the strict loader. - Add _drift_findings(kind, indexed_ids, on_disk_ids, findings) to emit the orphan + missing-row findings, collapsing the three near-identical blocks in _check_index_drift. Codes and messages are unchanged; adding a kind is now a one-liner. Add test_fsck_surfaces_invalid_claim_yaml_without_crashing.
Replace the _vouch_managed attribute (and its type: ignore[attr-defined]) with a _VouchManagedHandler(StreamHandler) subclass. configure_logging() identifies its handler by isinstance for install/reuse/remove, which reads cleaner and drops the attribute hack. Tests updated to match.
# Conflicts: # CHANGELOG.md
feat(health): vouch fsck — deep consistency checks beyond doctor
…t-json feat: structured JSON logging via VOUCH_LOG_FORMAT=json
…ent-addressing fix: enforce Source content-addressing on bundle/sync import
Co-authored-by: Cursor <cursoragent@cursor.com>
feat: add vouch migrate for safe on-disk KB upgrades
tiered adoption story under adapters/claude-code/: .mcp.json (T1), fenced CLAUDE.md snippet (T2), four slash commands (T3), settings.json with SessionStart hook + read-only auto-allow (T4). new vouch install-mcp claude-code [--tier T1..T4] writes them idempotently into a project.
docs: plan for Claude Code adapter (4 tiers + install-mcp CLI)
|
Warning Review limit reached
More reviews will be available in 39 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (43)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
ReviewSummary: Routes What works
Suggestions
Verdictapprove — The core fix is correct, minimal, and well-tested. The two new validators ( |
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
10 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.
What changed
verify.verify_source(src/vouch/verify.py) now reads the external file atsource.locatorthroughKBStore.read_under_rootinstead ofPath(source.locator).read_bytes(). The new path inherits the same containment guard the write side (register_source_from_path→read_under_root) has had since #28:Path.resolve()+is_relative_to(self.root)+O_NOFOLLOW(POSIX) +fstat S_ISREG. Off-root locators surface asexternal_status="missing"with a note attached; in-project locators behave exactly as before (drift detection unchanged). Adds aSource._locator_non_empty@field_validatorso the model layer rejects empty / whitespace-only locators (parallel to theClaim._text_non_empty/Entity._name_non_empty/Page._title_non_emptyvalidators in the sibling fix that closes #155). No new public method, no on-disk-layout or MCP/JSONL surface change.Fixes #157
Why
Fixes [BUG].
Source.locatoris set verbatim from caller-supplied input on both transports —kb.register_source(content, url=..., source_type="file")routesurlstraight intolocator(src/vouch/server.py:267,src/vouch/jsonl_server.py:205), and bundle / sync import accepts anylocatorstring because theSourcemodel has no locator-format validator. The verify-side read atsrc/vouch/verify.py:44-56then didPath(source.locator).read_bytes()with no containment, no symlink defence, and no size cap. Three paths to plant an off-tree locator, one path to read it:kb.register_source(content="x", url="/etc/passwd", source_type="file")— the URL string lands aslocatorverbatim._validate_contentrunsSource.model_validate(...); validation gap: bundle/sync import doesn't enforce Source content-addressing (sha256(content) == id) #125's content-addressing check only constrained the<sha>/contentmember's hash, not the locator.put_source(or a hand-writtenmeta.yaml).Then
vouch source verify/vouch doctor/kb.source_verifyopens the planted path, hashes it, and returnsmatch/drift/missing. That's a usable file-existence + file-readability + hash-confirmation side channel against any path the vouch process can read — composable into filesystem-layout probing (does/etc/cron.d/Xexist?), specific-file-content confirmation (registerid=<known-target-hash>,locator=<target-path>;external_status="match"is the oracle), and unbounded-read latency / memory amplification via/dev/urandomor large files.The matching write surface —
register_source_from_path— was hardened in #28 with exactly this pattern (read_under_root, post-CVE-2007-4559). This PR closes the parallel read surface. Same structural lane as #81 / #82 (claim citations), #123 / #124 (graph integrity), #125 / #126 (source content-addressing), and #149 (artifact ids): an invariant articulated at one boundary, missing at the parallel boundary, with a fix that just routes the latter through the helper the former already uses.What might break
Nothing on-disk, schema, bundle-format, MCP/JSONL surface, or audit-log shape — strictly tightening read-time containment.
kb.register_source_from_pathalready pass throughread_under_root, so their locators are scoped to the project root by design. They continue to drift-detect normally.locatorpoints outsidestore.root(planted via the un-scopedkb.register_sourceURL field, a malicious bundle, or hand-editing) now reportsexternal_status="missing"with note"unreadable: path must be inside project root (...)"instead of"match"/"drift". The off-tree file is no longer opened.locatoris empty / whitespace-only now raisespydantic.ValidationErrorat construction /model_validatetime. Bundle / sync import paths surface this asschema validation failed: ...locator must be a non-empty...via_validate_content's existing pydantic path (same shape as bug: Claim model has no min-evidence validator — uncited claims land via bundle import, put_claim, update_claim #81 / fix(models): require Claim.evidence to be non-empty at the model layer #82 surface uncited claims).vouch lintsurfaces a legacy pre-fix on-disk source with an empty locator as aninvalid_source-style finding rather than crashing the sweep.test_verify_detects_external_drift,test_verify_handles_missing_stored_content,test_verify_handles_unreadable_stored_content,test_verify_all_continues_after_one_failure— all usetmp_path/<file>locators wheretmp_pathIS the KBStore root, so they flow throughread_under_rootexactly as they used to flow throughPath.read_bytes().VEP
Not required. No change to the object model field set,
kb.*method surface, on-disk layout, bundle format, or audit-log shape — this only tightens read-time containment inverify_sourceto match the containmentregister_source_from_pathandread_under_rootalready document. The new locator validator is the same@field_validatorpattern the codebase already uses forClaim._at_least_one_citation.Tests
make checkpasses locally —ruff check src testsclean,mypy srcreportsSuccess: no issues found in 33 source files, fullpytestgreen on Windows / Python 3.13 except the pre-existingtests/test_jsonl_server.py::test_read_under_root_rejects_symlink_at_resolved_pathwhich needs symlink-create privilege (unrelated; same env-only skip noted in fix(models): require Claim.evidence to be non-empty at the model layer #82 / fix: reject dangling Relation/Page references at every write path #124 / fix: enforce Source content-addressing on bundle/sync import #126).New / changed behaviour has tests — 4 new regressions in
tests/test_verify.py:test_verify_refuses_to_read_off_project_locator— plants an off-tree file whose content sha256 matches the registered source'sid. If the buggyPath(locator).read_bytes()path were still alive,external_statuswould be"match"(proving the off-tree read happened — exactly the hash-confirmation oracle). After the fix,external_status="missing"with"must be inside project root"in the note. The hash-equality framing means the test detects any future regression that re-introduces the off-tree read, regardless of whether the mocking story changes.test_verify_refuses_to_read_absolute_system_path—/etc/passwdon POSIX /C:\Windows\win.inion Windows. Same containment guarantee, no dependency on whether the target file exists (the guard fires onis_relative_to(root)before the open).test_verify_in_project_locator_still_detects_drift— positive guard. An in-project locator continues to drift-detect; rules out the containment fix accidentally breaking honestregister_source_from_pathflows.test_verify_empty_locator_caught_by_model—Source(locator="")andSource(locator=" ")raiseValidationErrormatching"locator must be a non-empty".CHANGELOG.mdupdated under## [Unreleased]— add before merge (suggested entry):