Skip to content

feat: vouch serve fails fast with clear hint when no KB is present - #122

Merged
plind-junior merged 2 commits into
vouchdev:testfrom
Tet-9:feat/95-serve-fail-fast-no-kb
Jun 12, 2026
Merged

feat: vouch serve fails fast with clear hint when no KB is present#122
plind-junior merged 2 commits into
vouchdev:testfrom
Tet-9:feat/95-serve-fail-fast-no-kb

Conversation

@Tet-9

@Tet-9 Tet-9 commented May 27, 2026

Copy link
Copy Markdown
Contributor

Problem

vouch serve in a directory without a .vouch/ KB silently starts
the server and then misbehaves — the stdio transport emits confusing
blank-line Invalid JSON noise and the JSONL transport errors on
every request. There is no upfront check and no actionable message.

Fix

Call _load_store() at the top of serve() before any transport is
started. _load_store() already handles the KBNotFoundError case
by printing a clear run vouch init hint and exiting non-zero, so
the fix is a single line.

A valid KB still starts normally — no behavior change for the happy
path.

Tests

test_serve_fails_fast_without_kb — invokes vouch serve --transport jsonl in a temp directory with no .vouch/ KB and asserts exit code
is non-zero and the output contains the vouch init hint.

198 tests pass, 5 deselected.

Fixes #95

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d36907de-bd77-45d4-b49c-190438188ad0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Tet-9

Tet-9 commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

@plind-junior , review this when you can 🫡

@Tet-9
Tet-9 force-pushed the feat/95-serve-fail-fast-no-kb branch from 05d8376 to 1af9521 Compare May 29, 2026 21:21
@Tet-9

Tet-9 commented May 29, 2026

Copy link
Copy Markdown
Contributor Author

@plind-junior , review this if you may.....would really be much obliged

@Tet-9

Tet-9 commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

@plind-junior , review time 🫡

@plind-junior

Copy link
Copy Markdown
Member

Fix the conflict

@plind-junior

Copy link
Copy Markdown
Member

Review

Summary: The one-liner fix for #95 is correct and minimal — _load_store() called at the top of serve() before any transport starts does exactly what the issue asks, reusing the existing KB-not-found guard. The PR is sound for its stated purpose, with one blocking concern about a dropped safety fixture and one target-branch question.

What works

  • src/vouch/cli.py:992_load_store() at the top of serve() is the right call. _load_store already catches KBNotFoundError, prints error: … + hint: run \vouch init`…to stderr, thensys.exit(2)`, so the fix is genuinely one line and reuses a proven path.
  • tests/test_cli.py:1489–1494test_serve_fails_fast_without_kb uses monkeypatch.chdir(tmp_path) to guarantee a KB-free environment, invokes via CliRunner, and asserts exit_code != 0 plus "vouch init" in output or stderr. The or is correct because CliRunner() with default mix_stderr=True folds stderr into result.output, so the check is resilient either way.
  • The PR has a single commit, keeping the history clean.

Suggestions

  • [blocking] tests/test_cli.py — The diff removes monkeypatch.setenv("VOUCH_AGENT", "human-reviewer") and its explanatory comment from test_crystallize_cli_partial_failures_shows_warning. That fixture was added deliberately (commit 7cc04b6, PR fix: allow crystallize to approve own session proposals #49) to break the self-approval guard collision when the OS login happens to be "a". Dropping it reintroduces environment-dependent flakiness on any machine where os.getlogin() returns "a". This removal appears to be an accidental artifact of the branch base, not intentional. The line should be restored before merge.

  • [non-blocking] The PR targets the test branch, not main. The resulting diff carries a large amount of apparent regressions (removal of on_progress callbacks, --json flags on lint/search/doctor, vouch init --template, colorised output, multi-id approve, docs/PROJECT-INDEX.md, tests/test_cli_output.py, tests/test_templates.py, version rollback to 0.0.1). If test is intentionally an earlier baseline these are non-issues, but if the intent is to land this on main/release/0.1.0, the base branch needs to be corrected — otherwise all those features get reverted on merge.

  • [non-blocking] tests/test_cli.py:1494 — The assertion "vouch init" in result.output or "vouch init" in (result.stderr or "") is slightly over-specified; with the default CliRunner() (no catch_exceptions=False, mix_stderr defaults to True in Click 8) result.output already contains stderr. A single assert "vouch init" in result.output would be cleaner and avoids the (result.stderr or "") dance. Minor.

Verdict

Request changes — The core fix is correct and the new test covers the happy/sad path well. The blocking item is the dropped monkeypatch.setenv("VOUCH_AGENT", "human-reviewer") in the existing crystallize partial-failure test, which reintroduces a known flakiness vector. Restore that line and confirm the target branch is intentional, then this is good to merge.

@plind-junior
plind-junior marked this pull request as draft June 9, 2026 06:49
@Tet-9

Tet-9 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Review

Summary: The one-liner fix for #95 is correct and minimal — _load_store() called at the top of serve() before any transport starts does exactly what the issue asks, reusing the existing KB-not-found guard. The PR is sound for its stated purpose, with one blocking concern about a dropped safety fixture and one target-branch question.

What works

  • src/vouch/cli.py:992_load_store() at the top of serve() is the right call. _load_store already catches KBNotFoundError, prints error: … + hint: run \vouch initto stderr, thensys.exit(2), so the fix is genuinely one line and reuses a proven path.
  • tests/test_cli.py:1489–1494test_serve_fails_fast_without_kb uses monkeypatch.chdir(tmp_path) to guarantee a KB-free environment, invokes via CliRunner, and asserts exit_code != 0 plus "vouch init" in output or stderr. The or is correct because CliRunner() with default mix_stderr=True folds stderr into result.output, so the check is resilient either way.
  • The PR has a single commit, keeping the history clean.

Suggestions

  • [blocking] tests/test_cli.py — The diff removes monkeypatch.setenv("VOUCH_AGENT", "human-reviewer") and its explanatory comment from test_crystallize_cli_partial_failures_shows_warning. That fixture was added deliberately (commit 7cc04b6, PR fix: allow crystallize to approve own session proposals #49) to break the self-approval guard collision when the OS login happens to be "a". Dropping it reintroduces environment-dependent flakiness on any machine where os.getlogin() returns "a". This removal appears to be an accidental artifact of the branch base, not intentional. The line should be restored before merge.
  • [non-blocking] The PR targets the test branch, not main. The resulting diff carries a large amount of apparent regressions (removal of on_progress callbacks, --json flags on lint/search/doctor, vouch init --template, colorised output, multi-id approve, docs/PROJECT-INDEX.md, tests/test_cli_output.py, tests/test_templates.py, version rollback to 0.0.1). If test is intentionally an earlier baseline these are non-issues, but if the intent is to land this on main/release/0.1.0, the base branch needs to be corrected — otherwise all those features get reverted on merge.
  • [non-blocking] tests/test_cli.py:1494 — The assertion "vouch init" in result.output or "vouch init" in (result.stderr or "") is slightly over-specified; with the default CliRunner() (no catch_exceptions=False, mix_stderr defaults to True in Click 8) result.output already contains stderr. A single assert "vouch init" in result.output would be cleaner and avoids the (result.stderr or "") dance. Minor.

Verdict

Request changes — The core fix is correct and the new test covers the happy/sad path well. The blocking item is the dropped monkeypatch.setenv("VOUCH_AGENT", "human-reviewer") in the existing crystallize partial-failure test, which reintroduces a known flakiness vector. Restore that line and confirm the target branch is intentional, then this is good to merge.

All right....I'm on it

@Tet-9
Tet-9 marked this pull request as ready for review June 12, 2026 02:04
@Tet-9

Tet-9 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@plind-junior , you may review this now

@plind-junior

Copy link
Copy Markdown
Member

Thanks for the contribution!
From now on, please focus on the desktop app release

@plind-junior
plind-junior merged commit b0dc9f8 into vouchdev:test Jun 12, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants