feat(migration): versioned schema_version tracking and atomic vouch m…#163
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements VEP-0004: schema versioning and on-disk migration for vouch KBs. It introduces a ChangesVersioned KB migration with atomic rollback
Sequence DiagramsequenceDiagram
participant KB as KB Directory<br/>(.vouch/)
participant migrate_kb
participant migrate_fn as migrate_kb()
participant temp as Temp<br/>Directory
participant validate as _validate_<br/>migrated
participant swap as _atomic_<br/>swap
KB->>migrate_kb: discover root, read config
migrate_kb->>migrate_kb: compute migration steps
migrate_kb->>temp: copy KB & apply transforms
temp->>validate: load YAML & Markdown
alt validation succeeds
validate-->>migrate_kb: artifacts valid
migrate_kb->>swap: temp dir ready
swap->>KB: atomic rename (temp → .vouch)
swap-->>migrate_kb: success
else validation fails
validate-->>migrate_kb: errors collected
migrate_kb->>temp: clean up temp dir
migrate_kb->>KB: original untouched, schema_version unchanged
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/vouch/migration.py (1)
153-168: 💤 Low value
.vouch-pre-migrateis not cleaned up after successful swap.After a successful atomic swap, the
.vouch-pre-migratedirectory (containing the pre-migration KB) persists indefinitely. While this could serve as a manual recovery option, it:
- Wastes disk space (potentially duplicating the entire KB)
- Could confuse users who discover the directory
Consider cleaning it up after successful swap, or documenting this behavior as an intentional rollback mechanism.
♻️ Option A: Clean up after successful swap
def _atomic_swap(kb_dir: Path, tmp_dir: Path) -> None: """Replace kb_dir with tmp_dir atomically. Renames kb_dir → kb_dir/../.vouch-pre-migrate, then tmp_dir → kb_dir. On POSIX this pair of renames is as atomic as the filesystem allows. """ pre = kb_dir.parent / ".vouch-pre-migrate" if pre.exists(): shutil.rmtree(pre) kb_dir.rename(pre) try: tmp_dir.rename(kb_dir) + shutil.rmtree(pre) except Exception: # Best-effort rollback of the first rename. pre.rename(kb_dir) raise🤖 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/migration.py` around lines 153 - 168, The function _atomic_swap currently leaves the pre-migration folder (pre = kb_dir.parent / ".vouch-pre-migrate") behind after a successful swap; modify _atomic_swap so that after tmp_dir.rename(kb_dir) completes successfully it removes the pre directory (e.g., call shutil.rmtree(pre) or shutil.rmtree(pre, ignore_errors=True)); keep the existing rollback behavior in the except branch (pre.rename(kb_dir) then raise) and ensure the cleanup runs only on success (not on exception) to avoid losing the rollback snapshot.src/vouch/server.py (1)
54-55: 💤 Low valueConsider including migration hint in the error message.
The
SchemaMismatchErrorclass docstring mentions "Runvouch migrate", butstr(e)only returns the first line of the exception message. UnlikeKBNotFoundErrorhandling (lines 50-53) which appends a helpful hint, this handler doesn't provide actionable guidance.However, reviewing the upstream contract in
storage.py:68-72, the exception message itself should include the migration instruction, so this is likely fine. If the base message doesn't include the hint, consider adding it explicitly like theKBNotFoundErrorcase.🤖 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/server.py` around lines 54 - 55, The SchemaMismatchError except block currently raises RuntimeError(str(e)) without the migration hint; update the handler in server.py (the except SchemaMismatchError as e block) to append the migration instruction (e.g. "Run `vouch migrate`") to the error message—mirroring the KBNotFoundError handling—while preserving exception chaining (raise RuntimeError(f"{str(e)} — Run `vouch migrate`") from e) so callers get both the original message and the actionable migration hint.src/vouch/cli.py (1)
734-739: 💤 Low valueBackup is created even during dry-run.
When
--backupand--dry-runare both specified, the backup is still created (line 734-739 executes before the dry-run check at line 741). This may be surprising since dry-run typically implies no disk writes. Consider either:
- Documenting this behavior in the help text
- Skipping backup creation during dry-run
🤖 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/cli.py` around lines 734 - 739, The backup block currently runs unconditionally when backup is True (uses ts, backup_dir, shutil.copytree, click.echo), causing writes even for dry-run; change the logic so backup is only created when not dry_run (e.g., guard the backup code with "if backup and not dry_run:" or move the backup block to run after the dry-run check), or alternatively update the CLI help text for the backup option to state that backups occur during dry-run; reference the variables/operations kb_dir, root, shutil.copytree, backup, and dry_run when making the change.
🤖 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 `@proposals/VEP-0004-migration.md`:
- Around line 147-148: Update the VEP wording to match the implementation:
change the statement about health.doctor() emitting a `critical`-severity
finding for schema version mismatches to say it emits an `error`-severity
finding, referencing the `schema_version_mismatch` finding produced by
health.doctor() (compare to VOUCH_SCHEMA_VERSION) so the VEP and the
implementation use the same severity vocabulary.
- Around line 85-87: The fenced code block containing the CLI usage line `vouch
migrate [--dry-run] [--backup] [--from VERSION] [--to VERSION]` lacks a language
tag and triggers markdownlint MD040; update the fence to include an appropriate
language identifier such as "bash" or "sh" (e.g., replace the opening ``` with
```bash) so the block is correctly annotated for CLI/shell content.
- Around line 129-133: The atomic swap steps are inconsistent because
`.vouch/.migrate-tmp/` is moved when `.vouch/` is renamed; update the spec so
the temp output lives outside `.vouch/` (e.g., `.vouch-migrate-tmp/` or
`.migrate-tmp/` as a sibling) and then perform the swap as: write all migrated
artifacts to the sibling temp dir, validate via the target Pydantic models,
rename `.vouch/` → `.vouch-pre-migrate/`, then rename the sibling temp dir →
`.vouch/`; alternatively keep the current temp name but reverse/adjust the
rename ordering to avoid moving the temp directory.
In `@src/vouch/bundle.py`:
- Around line 261-269: The comparison of bundle_schema to VOUCH_SCHEMA_VERSION
in import_check is unsafe because bundle_schema is untrusted and may be
non-string or a dotted version string; change import_check to normalize and
validate the incoming schema: coerce bundle_schema to str only if not None,
parse both bundle_schema and VOUCH_SCHEMA_VERSION with a robust version parser
(e.g., packaging.version.parse) and compare Version objects, catching any
exceptions (TypeError, ValueError) from parsing; on parse/compare failure return
an ImportCheckResult with a clear error message instead of raising, referencing
the bundle_schema variable, the import_check function, the VOUCH_SCHEMA_VERSION
constant, and the ImportCheckResult constructor so callers (vouch import-check /
kb_import_check / _h_import_check) receive a safe result.
In `@src/vouch/cli.py`:
- Around line 716-718: The call to discover_root(Path(path)) can raise
KBNotFoundError and will produce an unhelpful traceback; wrap the call in a
try/except that catches KBNotFoundError and emits the same friendly hint/exit
behavior used by _load_store() (or simply call and use _load_store() instead of
discover_root here), then proceed to compute kb_dir and cfg_path only after
recovering the root; reference the discover_root function and KBNotFoundError in
the handler so the user sees a clear message and the process exits cleanly.
In `@src/vouch/migration.py`:
- Around line 129-140: The validate loop uses p.parent.name in (s for s in
["sources"]) which mismatches the skip logic in _copy_tree_for_migration that
checks the loop variable sub == "sources", causing nested files like
sources/abc/some.yaml to be validated incorrectly; change the skip condition in
the validator loop to the same approach as the copier (check sub == "sources")
and simplify the expression (remove the generator expression), i.e., skip
validation when sub == "sources" or equivalent, keeping the rest of the flow
(use VALIDATORS.get(sub) and the existing try/except) unchanged.
In `@tests/test_bundle.py`:
- Around line 357-383: The test should assert the explicit acceptance path
instead of only checking for absence of the substring "newer": replace the final
assertion with a direct acceptance check (e.g., assert result.ok) and optionally
assert no issues (e.g., assert not result.issues) so the test validates
bundle.import_check returns an accepted result for the pre-schema manifest;
update the assertion in test_import_check_accepts_bundle_without_schema_version
to check result.ok (and/or empty result.issues) against the manifest built for
the test.
In `@tests/test_migration.py`:
- Around line 225-232: The test asserts that the temp directory ".migrate-tmp"
does not exist, but the migration code creates/cleans ".vouch-migrate-tmp" (see
src.vouch.migration usage); update the assertion in the test (around the
on_disk/tmp-dir checks referencing kb_dir) to assert that (kb_dir /
".vouch-migrate-tmp").exists() is False (or use .exists() and negate) so the
test actually verifies that the real migration temp directory used by the
migration logic was removed. Also keep the surrounding checks (on_disk content
and config schema_version) unchanged.
---
Nitpick comments:
In `@src/vouch/cli.py`:
- Around line 734-739: The backup block currently runs unconditionally when
backup is True (uses ts, backup_dir, shutil.copytree, click.echo), causing
writes even for dry-run; change the logic so backup is only created when not
dry_run (e.g., guard the backup code with "if backup and not dry_run:" or move
the backup block to run after the dry-run check), or alternatively update the
CLI help text for the backup option to state that backups occur during dry-run;
reference the variables/operations kb_dir, root, shutil.copytree, backup, and
dry_run when making the change.
In `@src/vouch/migration.py`:
- Around line 153-168: The function _atomic_swap currently leaves the
pre-migration folder (pre = kb_dir.parent / ".vouch-pre-migrate") behind after a
successful swap; modify _atomic_swap so that after tmp_dir.rename(kb_dir)
completes successfully it removes the pre directory (e.g., call
shutil.rmtree(pre) or shutil.rmtree(pre, ignore_errors=True)); keep the existing
rollback behavior in the except branch (pre.rename(kb_dir) then raise) and
ensure the cleanup runs only on success (not on exception) to avoid losing the
rollback snapshot.
In `@src/vouch/server.py`:
- Around line 54-55: The SchemaMismatchError except block currently raises
RuntimeError(str(e)) without the migration hint; update the handler in server.py
(the except SchemaMismatchError as e block) to append the migration instruction
(e.g. "Run `vouch migrate`") to the error message—mirroring the KBNotFoundError
handling—while preserving exception chaining (raise RuntimeError(f"{str(e)} —
Run `vouch migrate`") from e) so callers get both the original message and the
actionable migration hint.
🪄 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: d8321cc3-5583-4714-9a47-22178dc48202
📒 Files selected for processing (12)
proposals/VEP-0004-migration.mdsrc/vouch/bundle.pysrc/vouch/cli.pysrc/vouch/health.pysrc/vouch/jsonl_server.pysrc/vouch/migration.pysrc/vouch/models.pysrc/vouch/server.pysrc/vouch/storage.pytests/test_bundle.pytests/test_migration.pytests/test_storage.py
| def test_import_check_accepts_bundle_without_schema_version(store: KBStore, tmp_path: Path) -> None: | ||
| """Pre-VEP-0004 bundles (no schema_version field) must still import.""" | ||
| import hashlib as _hl | ||
| bundle_path = tmp_path / "old.tar.gz" | ||
| payload = b"id: c1\n" | ||
| manifest = { | ||
| "spec": bundle.SPEC_VERSION, | ||
| "bundle_id": "abc", | ||
| "files": [ | ||
| {"path": "claims/c1.yaml", "size": len(payload), | ||
| "sha256": _hl.sha256(payload).hexdigest()} | ||
| ], | ||
| "counts": {}, | ||
| "safety": {}, | ||
| } | ||
| with tarfile.open(bundle_path, "w:gz") as tar: | ||
| mf_bytes = json.dumps(manifest).encode() | ||
| mf_info = tarfile.TarInfo(bundle.MANIFEST_NAME) | ||
| mf_info.size = len(mf_bytes) | ||
| tar.addfile(mf_info, io.BytesIO(mf_bytes)) | ||
| info = tarfile.TarInfo("claims/c1.yaml") | ||
| info.size = len(payload) | ||
| tar.addfile(info, io.BytesIO(payload)) | ||
|
|
||
| result = bundle.import_check(store.kb_dir, bundle_path) | ||
| # No schema version rejection issue | ||
| assert not any("newer" in i for i in result.issues) |
There was a problem hiding this comment.
Strengthen the pre-schema compatibility test assertion.
This test currently passes as long as no issue contains "newer", even if missing schema_version is effectively rejected for another reason. Please assert the acceptance path directly (e.g., result.ok) using a structurally valid manifest payload.
🤖 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_bundle.py` around lines 357 - 383, The test should assert the
explicit acceptance path instead of only checking for absence of the substring
"newer": replace the final assertion with a direct acceptance check (e.g.,
assert result.ok) and optionally assert no issues (e.g., assert not
result.issues) so the test validates bundle.import_check returns an accepted
result for the pre-schema manifest; update the assertion in
test_import_check_accepts_bundle_without_schema_version to check result.ok
(and/or empty result.issues) against the manifest built for the test.
…er error handling
|
Hi, @plind-junior |
|
The atomic-swap pattern for One thing to double-check in Non-blocking: Small process note: the PR body says Thanks for taking this on — |
|
PR didn't target test branch will be auto-closed by an agent |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b9dc42843
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| def status(store: KBStore) -> dict: | ||
| def status(store: KBStore) -> dict[str, Any]: |
There was a problem hiding this comment.
Adding this second def status immediately after the existing one leaves the first function definition without an indented body, so src/vouch/health.py raises an IndentationError on import. Because cli.py, the JSONL server, and the MCP server all import health, this prevents the main entry points from starting at all.
Useful? React with 👍 / 👎.
| def _copy_tree_for_migration(kb_dir: Path, tmp_dir: Path, steps: list[Migration]) -> list[str]: | ||
| """Copy and transform every migratable file from kb_dir into tmp_dir.""" | ||
| changed: list[str] = [] | ||
|
|
||
| for sub in MIGRATABLE_YAML_SUBDIRS: |
There was a problem hiding this comment.
When a real migration has at least one step, this copy routine seeds the replacement directory only from the whitelisted artifact subdirectories plus pages/config, and _atomic_swap then replaces the entire live .vouch/ directory and deletes the original. That drops existing files outside this whitelist such as .gitignore, audit.log.jsonl, state.db, and pending proposed/ files; a user running vouch migrate with pending proposals or custom ignores loses data. Seed the temp directory from the full KB tree before transforming artifacts, or explicitly carry over the non-migrated files.
Useful? React with 👍 / 👎.
| def migrate(dry_run: bool, backup: bool, from_version: str | None, | ||
| to_version: str | None, path: str) -> None: |
There was a problem hiding this comment.
This adds a second migrate callback decorated with @cli.command() using the same command name as the existing migrate command earlier in this file. Click stores commands by name, so the later registration replaces the command that supported --check, --to-version, --json, and the legacy KB-format migration path; for example, existing vouch migrate --check users now hit an unknown option instead of the dry-run check. Merge the new schema-version behavior into the existing command or register it under a distinct name.
Useful? React with 👍 / 👎.
| _audit.log_event( | ||
| kb_dir, event="migration.rollback", actor="vouch-migrate", | ||
| reversible=False, dry_run=dry_run, | ||
| data={"from_version": from_v, "to_version": to_v, "errors": errors}, |
There was a problem hiding this comment.
If validation fails while the user is running vouch migrate --dry-run, this branch appends a migration.rollback event to the live KB before returning the error. That violates the dry-run contract described by migrate_kb() and the CLI help: a user checking an invalid migration gets their audit.log.jsonl mutated even though no migration was requested to run. Skip audit writes when dry_run is true, or record this only after an actual write attempt.
Useful? React with 👍 / 👎.
| )) | ||
| else: | ||
| stored = _stored_schema_version(store) | ||
| if stored is not None and stored != VOUCH_SCHEMA_VERSION: |
There was a problem hiding this comment.
When config.yaml contains an unquoted schema_version: 0.1, YAML loads it as a float; KBStore.assert_schema_ok() accepts that by comparing str(stored) to VOUCH_SCHEMA_VERSION, but doctor compares the raw value here and emits a schema_version_mismatch error for a KB the storage guard considers current. Cast stored to str before this comparison to keep doctor consistent with the loader.
Useful? React with 👍 / 👎.
| except SchemaMismatchError as e: | ||
| click.echo(f"error: {e}", err=True) | ||
| click.echo("hint: run `vouch migrate` to upgrade the on-disk layout.", err=True) | ||
| sys.exit(3) |
There was a problem hiding this comment.
With this early exit in _load_store, vouch doctor --json and vouch status --json on a mismatched KB terminate before the health code can emit the new schema_version_mismatch finding or any machine-readable payload. The only command that should bypass normal store loading is migrate; health commands need a way to construct a store without enforcing the schema guard so users and automation can diagnose the mismatch.
Useful? React with 👍 / 👎.
| raw = _yaml_load(p.read_text()) or {} | ||
| baseline_text = _yaml_dump(raw) | ||
| transformed = _apply_transforms(dict(raw), sub, steps) | ||
| new_text = _yaml_dump(transformed) | ||
| dst.write_text(new_text) |
There was a problem hiding this comment.
This writes _yaml_dump(raw) for every YAML artifact in every migration step, even when none of the transforms changed that artifact. In migrations where a step only bumps the schema or touches another subdirectory, a real run silently rewrites unrelated committed YAML files, dropping comments/formatting and creating noisy diffs that dry-run does not report because changed compares against the dumped baseline. Copy the original bytes unless the transformed data actually differs from the loaded input.
Useful? React with 👍 / 👎.
| pre = kb_dir.parent / ".vouch-pre-migrate" | ||
| if pre.exists(): | ||
| shutil.rmtree(pre) |
There was a problem hiding this comment.
Before every migration this deletes any existing sibling named .vouch-pre-migrate. If a previous failed run left the rollback directory behind, or a user created that path as a manual backup, invoking vouch migrate removes it before the new migration has succeeded. Use a unique temporary name or fail when the path already exists rather than recursively deleting possible recovery data.
Useful? React with 👍 / 👎.
2 participants
What changed
Closes #162
Adds versioned on-disk format tracking and an atomic
vouch migratecommand. A newVOUCH_SCHEMA_VERSIONconstant inmodels.pyis the single source of truth for the schema the installed vouch expects.KBStore.init()writesschema_versionintoconfig.yaml;KBStore.__init__()reads it back and raisesSchemaMismatchErrorwhen there is a mismatch instead of letting Pydantic throw a crypticValidationErrorat read time. A newsrc/vouch/migration.pymodule provides aMigrationregistry, aTransformprotocol, andmigrate_kb()— which writes all migrated artifacts to a sibling temp directory, validates every file under the target Pydantic models, then atomically renames the directory into place (or rolls back completely on any failure).bundle.pystampsschema_versioninto every exported manifest and rejects bundles built by a newer vouch.health.doctor()surfaces acriticalfinding on mismatch.Why
Without a migration path, any breaking change to a Pydantic model (a new non-defaulted field, a renamed field) silently corrupts every existing KB at read time. There is no
vouch migrate, no version header in any artifact, and no recovery short of manually patching every YAML file. This is the last hard blocker before 1.0 can freeze the on-disk format. Closes the migration story listed under0.1in ROADMAP.md.What might break
KBStore.__init__()now raisesSchemaMismatchErrorwhenconfig.yamlcontains aschema_versionthat does not match the installed vouch. Any code that was silently succeeding despite a schema mismatch will now get a loud error. The fix isvouch migrate.schema_versioninconfig.yaml(all existing KBs before this PR) are treated as"0.1"and pass the guard without requiring migration — no action needed for current users.bundle.import_checknow rejects bundles whoseschema_versionis newer than the installed vouch. Bundles with noschema_versionfield (pre-this-PR) are accepted unchanged.health.status()gains aschema_versionkey — additive, no existing callers break.VEP
proposals/VEP-0004-migration.md (draft, included in this PR per the surface-change requirement).
Tests
make checkpasses locally (lint + mypy + pytest)tests/test_migration.py— round-trip with transforms, dry-run, rollback on validation failure, no-op, transform factories, missing-KB errortests/test_storage.py—SchemaMismatchErrorfires on version mismatch, missingschema_versionis allowed,initwrites the fieldtests/test_bundle.py— export stampsschema_version, future-version bundle is rejected, pre-VEP bundle is acceptedCHANGELOG.mdupdated under## [Unreleased]Summary by CodeRabbit
Release Notes
New Features
vouch migratecommand to upgrade KB format between versions, with--backupand--dry-runoptionsDocumentation