Skip to content

feat(config): typed Config model + validation for .vouch/config.yaml#297

Open
minion1227 wants to merge 8 commits into
vouchdev:testfrom
minion1227:minion_243
Open

feat(config): typed Config model + validation for .vouch/config.yaml#297
minion1227 wants to merge 8 commits into
vouchdev:testfrom
minion1227:minion_243

Conversation

@minion1227

@minion1227 minion1227 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What changed

.vouch/config.yaml is now parsed once into a validated pydantic Config
(with nested ReviewConfig / RetrievalConfig), exposed as a cached
KBStore.config. the two readers the issue calls out — proposals'
self-approval / expire-window checks and the retrieval backend selector —
switch to store.config.<field> and drop their ad-hoc yaml.safe_load +
nested .get() + except: {} parsing. vouch doctor gains a config check.

Why

config was read defensively at each call site, so the schema was spread
across the codebase, a mistyped key (reveiw:) was silently ignored, and a
malformed value (retrieval.default_limit: "ten") silently fell back to a
default instead of surfacing. a single typed model makes config a
load-bearing, validated shape — consistent with the "prefer pydantic models
for any persisted shape" rule in CONTRIBUTING. closes #243.

acceptance from the issue:

  • a mistyped top-level key surfaces a config_unknown_key warning in
    vouch doctor (preserved, not dropped).
  • a malformed value fails fast with the offending key path
    (config.yaml: retrieval.default_limit: ...) instead of a silent fallback.
  • an existing .vouch/ with no retrieval: / review: blocks loads with
    the documented defaults — covered by round-trip tests.

What might break

nothing on disk. config.yaml's format is unchanged, the legacy
retrieval.backends list still resolves exactly as before, and a missing
file still yields all-defaults. the one behavior change is intentional and
per the issue: a genuinely malformed config value now raises ConfigError
(and vouch doctor reports config_invalid) rather than silently
defaulting. no kb.* method, on-disk layout, or audit-log shape changes.

VEP

not a surface change — internal parsing of an existing file plus two
additive vouch doctor finding codes. no VEP needed.

Tests

  • make check passes locally (ruff clean; mypy clean on touched files;
    pytest green — the only local failures are pre-existing and
    environment-only: fastapi/[web] not installed and embeddings
    installed, both of which also fail on main)
  • new behaviour has a test — tests/test_config_model.py (15 cases:
    defaults, partial configs, unknown-key warning, malformed-value
    failure, legacy-backends resolution, doctor integration)
  • CHANGELOG.md updated under ## [Unreleased]

Summary by CodeRabbit

  • New Features
    • Added advisory triage scoring for pending-review proposals (configurable).
    • Introduced typed .vouch/config.yaml parsing with validation/defaulting, exposed via KBStore.config.
    • Retrieval backend resolution now uses the typed config (supports backend and legacy backends with fallback to auto).
  • Bug Fixes
    • Config loading and doctor diagnostics now fail fast with clear path-based errors.
    • doctor now warns on unknown top-level and nested keys and reports invalid configs as errors.
  • Tests
    • Added unit coverage for typed config behavior, caching, backend resolution, and doctor reporting.

`.vouch/config.yaml` was read as an untyped dict at each call site, with
nested `.get()` guards and silent `except: {}` fallbacks — the schema was
spread across the codebase and typos (`reveiw:`) were silently dropped.

add a validated pydantic `Config` (with nested `ReviewConfig` and
`RetrievalConfig`), parsed once and exposed as a cached `KBStore.config`.
migrate the readers the issue names — proposals' self-approval /
expire-window checks and the retrieval backend selector — onto it, and
delete their ad-hoc parsing. a malformed value (e.g.
`retrieval.default_limit: "ten"`) now fails fast with the offending key
path instead of falling back, and `vouch doctor` surfaces unknown
top-level keys as likely typos.

sections owned by their own readers (serve, volunteer, mcp) stay loose so
they neither break nor trip the unknown-key check, and the legacy
`retrieval.backends` list still resolves as before, so an existing
`.vouch/` loads with documented defaults and no on-disk change.

closes vouchdev#243
@github-actions github-actions Bot added docs documentation, specs, examples, and repo guidance storage kb storage, migrations, schemas, and proposals retrieval context, search, synthesis, and evaluation tests tests and fixtures size: M 200-499 changed non-doc lines labels Jul 1, 2026
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Introduces a typed Pydantic Config model with nested review/retrieval settings, caches parsed .vouch/config.yaml on KBStore, switches config consumers to store.config, and adds doctor() diagnostics plus tests and changelog entries.

Changes

Typed Config Model

Layer / File(s) Summary
Config models
src/vouch/models.py
Adds typed configuration models, defaults, backend resolution, validation errors, and unknown-key reporting.
KBStore.config loading
src/vouch/storage.py
Caches configuration per store, returns defaults when missing, and raises ConfigError for read or YAML parse failures.
Backend and proposal config reads
src/vouch/context.py, src/vouch/proposals.py
Uses typed backend and review settings through store.config.
doctor() diagnostics
src/vouch/health.py
Reports invalid configurations and unknown keys.
Validation and documentation
tests/test_models.py, tests/test_storage.py, tests/test_health.py, CHANGELOG.md
Adds coverage for defaults, validation, caching, diagnostics, and documents the configuration changes.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Retrieval
  participant Proposals
  participant KBStore
  participant Config
  participant Doctor

  Retrieval->>KBStore: access store.config
  KBStore->>Config: Config.load(raw)
  Config-->>KBStore: cached typed Config
  KBStore-->>Retrieval: retrieval.resolved_backend()
  Proposals->>KBStore: access review settings
  KBStore-->>Proposals: approver role and expiration
  Doctor->>KBStore: load config
  KBStore-->>Doctor: ConfigError or unknown_keys()
Loading

Possibly related PRs

Suggested labels: cli

Suggested reviewers: plind-junior

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title clearly summarizes the typed config model and validation work in this PR.
Linked Issues check ✅ Passed The changes implement the typed Config model, cached store.config, validation, and doctor warnings requested by #243.
Out of Scope Changes check ✅ Passed All described code changes support the config-schema and validation goals; no unrelated additions are evident.
✨ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
src/vouch/models.py (1)

477-477: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Nested review:/retrieval: typos are silently swallowed, unlike top-level ones.

ReviewConfig and RetrievalConfig both declare extra="allow", but Config.unknown_keys() (Line 559-561) only inspects the top-level __pydantic_extra__. A typo like review: {expier_pending_after_days: 1} lands as an untracked extra field on ReviewConfig — never raised as an error, never surfaced by unknown_keys(), and never shown by vouch doctor. This reproduces, one level deeper, exactly the silent-typo problem this PR is meant to close.

Consider extending unknown_keys() to also walk review.__pydantic_extra__ / retrieval.__pydantic_extra__ (prefixed, e.g. "review.foo"), or set extra="forbid" on ReviewConfig/RetrievalConfig if forward-compat isn't needed there.

♻️ Sketch: surface nested unknown keys too
     def unknown_keys(self) -> list[str]:
         """Top-level keys outside the known schema (likely typos)."""
-        return sorted(self.__pydantic_extra__ or {})
+        keys = set(self.__pydantic_extra__ or {})
+        keys |= {f"review.{k}" for k in (self.review.__pydantic_extra__ or {})}
+        keys |= {f"retrieval.{k}" for k in (self.retrieval.__pydantic_extra__ or {})}
+        return sorted(keys)

Also applies to: 489-489, 526-526, 559-561

🤖 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/models.py` at line 477, Nested `review:`/`retrieval:` typos are
still being hidden because `Config.unknown_keys()` only checks the top-level
`__pydantic_extra__`. Update `unknown_keys()` in `Config` to also inspect
`review.__pydantic_extra__` and `retrieval.__pydantic_extra__`, prefixing nested
entries like `review.foo` and `retrieval.bar`, or alternatively tighten
`ReviewConfig` and `RetrievalConfig` by changing their `model_config` from
`extra="allow"` to `extra="forbid"` if nested forward-compatibility is not
needed. Use the existing `Config`, `ReviewConfig`, and `RetrievalConfig` symbols
to keep the change localized.
tests/test_config_model.py (2)

1-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Test file name doesn't mirror the module it targets.

Guidelines require test files to mirror module names as tests/test_<module>.py. This file exercises vouch.models.Config (and integration with storage.py/health.py) but is named test_config_model.py rather than test_models.py.

As per coding guidelines, "Test file names must mirror module names using the tests/test_<module>.py convention."

🤖 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_config_model.py` around lines 1 - 138, Rename the test file so it
mirrors the target module name using the required tests/test_<module>.py
convention; this suite primarily covers vouch.models.Config, so the filename
should match the models module rather than the current config_model name. Update
the test file name consistently and keep the existing Config, KBStore, and
doctor tests in place.

Source: Coding guidelines


98-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider adding a same-instance caching test for KBStore.config.

The upstream KBStore.config docstring notes it's a cached_property, but no test here asserts that repeated access on the same store instance returns the same cached value (only cross-instance behavior via fresh KBStore(tmp_path) is exercised). A quick assert store.config is store.config would lock in the caching contract.

🤖 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_config_model.py` around lines 98 - 116, Add a same-instance
caching assertion for KBStore.config to cover the cached_property contract. In
the existing KBStore config tests, extend the relevant test around
KBStore.config so it verifies repeated access on the same store instance returns
the identical object (for example, by checking the same-instance identity on
store.config). Keep the current cross-instance checks and malformed-config
coverage unchanged.
src/vouch/health.py (1)

222-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider merging with the existing "Config sanity" block.

This new block and the pre-existing missing_config check (Lines 257-265) both gate on store.config_path.exists() but are now split apart by the unrelated "Source integrity" section. Consolidating all config-related findings (missing_config, config_invalid, config_unknown_key) into one block would make the config-diagnostics logic easier to follow and maintain.

♻️ Proposed consolidation
-    # Config validity — a malformed value is an error; an unknown top-level
-    # key is a likely typo silently ignored before `#243`, now surfaced.
-    if store.config_path.exists():
-        try:
-            cfg = store.config
-        except ConfigError as e:
-            report.findings.append(Finding("error", "config_invalid", str(e)))
-        else:
-            for key in cfg.unknown_keys():
-                report.findings.append(Finding(
-                    "warning", "config_unknown_key",
-                    f"unknown config key {key!r} — possible typo, ignored",
-                ))
-
     # Source integrity (content hash).
     ...
-    # Config sanity.
-    if not store.config_path.exists():
+    # Config sanity — missing file is fine (defaults apply); a malformed
+    # value is an error; an unknown top-level key is a likely typo silently
+    # ignored before `#243`, now surfaced.
+    if not store.config_path.exists():
         report.findings.append(
             Finding(
                 "error",
                 "missing_config",
                 "config.yaml is missing",
             )
         )
+    else:
+        try:
+            cfg = store.config
+        except ConfigError as e:
+            report.findings.append(Finding("error", "config_invalid", str(e)))
+        else:
+            for key in cfg.unknown_keys():
+                report.findings.append(Finding(
+                    "warning", "config_unknown_key",
+                    f"unknown config key {key!r} — possible typo, ignored",
+                ))
🤖 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/health.py` around lines 222 - 235, The config diagnostics in
health.py are split across separate sections even though they all depend on
store.config_path.exists(); merge the new config validity logic with the
existing Config sanity block so missing_config, config_invalid, and
config_unknown_key are handled together. Update the report-building flow in the
health check to keep all config-related findings adjacent and use the existing
store.config and ConfigError handling within that single block.
🤖 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 `@src/vouch/storage.py`:
- Around line 237-253: The config parsing in KBStore.config only converts
FileNotFoundError and OSError, so malformed YAML from _yaml_load can escape as
yaml.YAMLError and crash doctor(). Update the config property to catch
yaml.YAMLError around the _yaml_load(text) / Config.load path and re-raise it as
ConfigError with the config path context, so doctor() continues to handle
invalid config.yaml via its existing ConfigError path.

---

Nitpick comments:
In `@src/vouch/health.py`:
- Around line 222-235: The config diagnostics in health.py are split across
separate sections even though they all depend on store.config_path.exists();
merge the new config validity logic with the existing Config sanity block so
missing_config, config_invalid, and config_unknown_key are handled together.
Update the report-building flow in the health check to keep all config-related
findings adjacent and use the existing store.config and ConfigError handling
within that single block.

In `@src/vouch/models.py`:
- Line 477: Nested `review:`/`retrieval:` typos are still being hidden because
`Config.unknown_keys()` only checks the top-level `__pydantic_extra__`. Update
`unknown_keys()` in `Config` to also inspect `review.__pydantic_extra__` and
`retrieval.__pydantic_extra__`, prefixing nested entries like `review.foo` and
`retrieval.bar`, or alternatively tighten `ReviewConfig` and `RetrievalConfig`
by changing their `model_config` from `extra="allow"` to `extra="forbid"` if
nested forward-compatibility is not needed. Use the existing `Config`,
`ReviewConfig`, and `RetrievalConfig` symbols to keep the change localized.

In `@tests/test_config_model.py`:
- Around line 1-138: Rename the test file so it mirrors the target module name
using the required tests/test_<module>.py convention; this suite primarily
covers vouch.models.Config, so the filename should match the models module
rather than the current config_model name. Update the test file name
consistently and keep the existing Config, KBStore, and doctor tests in place.
- Around line 98-116: Add a same-instance caching assertion for KBStore.config
to cover the cached_property contract. In the existing KBStore config tests,
extend the relevant test around KBStore.config so it verifies repeated access on
the same store instance returns the identical object (for example, by checking
the same-instance identity on store.config). Keep the current cross-instance
checks and malformed-config coverage unchanged.
🪄 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: 2380c7a9-8ea3-414f-956d-30f189b46447

📥 Commits

Reviewing files that changed from the base of the PR and between 94e84b1 and 692415a.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • src/vouch/context.py
  • src/vouch/health.py
  • src/vouch/models.py
  • src/vouch/proposals.py
  • src/vouch/storage.py
  • tests/test_config_model.py

Comment thread src/vouch/storage.py
address review on vouchdev#297:

- `KBStore.config` now catches `yaml.YAMLError` from a syntactically
  broken config.yaml and re-raises it as `ConfigError`, so `vouch doctor`
  reports `config_invalid` instead of crashing on malformed YAML.
- `Config.unknown_keys()` walks the `review` and `retrieval` blocks too and
  returns dotted paths, so a typo one level deep
  (`review.expier_pending_after_days`) is flagged rather than silently
  swallowed — the same problem this change closes at the top level.
- consolidate the config diagnostics in `vouch doctor` into the single
  "config sanity" block.
- rename the test module to `tests/test_models.py` per the mirror-the-module
  convention, and add cases for malformed-yaml handling, nested-typo
  surfacing, and the cached_property contract.
@minion1227

Copy link
Copy Markdown
Contributor Author

thanks for the review — pushed 930a866 addressing all of it:

  • malformed yaml crash (actionable): KBStore.config now catches
    yaml.YAMLError and re-raises as ConfigError, so vouch doctor reports
    config_invalid rather than crashing. added a test that feeds broken yaml
    syntax through doctor().
  • nested typos: unknown_keys() now walks the review/retrieval blocks
    and returns dotted paths (review.expier_pending_after_days), so a typo one
    level deep is surfaced too. new test covers it.
  • test filename: renamed to tests/test_models.py.
  • caching contract: added assert store.config is store.config.
  • health.py: folded the config-validity checks into the single config
    sanity block.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/test_models.py (1)

128-190: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Cross-module tests placed in test_models.py.

Tests at Lines 128-141 (KBStore.config caching/ConfigError) and Lines 172-190 (doctor() diagnostics) exercise storage.py and health.py behavior respectively, not models.py. Per coding guidelines, test file names must mirror the module under test. Consider splitting these into tests/test_storage.py and tests/test_health.py, keeping test_models.py scoped to Config/ReviewConfig/RetrievalConfig unit tests.

As per coding guidelines, "Test file names must mirror module names using the tests/test_<module>.py convention."

🤖 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_models.py` around lines 128 - 190, The tests in test_models.py are
covering KBStore.config behavior and doctor() health checks from other modules,
so move the KBStore-related cases into the storage test module and the doctor()
cases into the health test module. Keep test_models.py focused on the models
classes it actually exercises (such as Config, ReviewConfig, and
RetrievalConfig), and preserve the existing assertions while relocating the
relevant test functions by their names.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@tests/test_models.py`:
- Around line 128-190: The tests in test_models.py are covering KBStore.config
behavior and doctor() health checks from other modules, so move the
KBStore-related cases into the storage test module and the doctor() cases into
the health test module. Keep test_models.py focused on the models classes it
actually exercises (such as Config, ReviewConfig, and RetrievalConfig), and
preserve the existing assertions while relocating the relevant test functions by
their names.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b95b459e-3531-4018-8aea-627113d158cd

📥 Commits

Reviewing files that changed from the base of the PR and between 692415a and 930a866.

📒 Files selected for processing (4)
  • src/vouch/health.py
  • src/vouch/models.py
  • src/vouch/storage.py
  • tests/test_models.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/vouch/health.py
  • src/vouch/models.py
  • src/vouch/storage.py

address the re-review caution on vouchdev#297: renaming to test_models.py left
`KBStore.config` and `doctor()` cases in the wrong module's test file.

- keep tests/test_models.py scoped to Config/ReviewConfig/RetrievalConfig
  unit tests
- move the KBStore.config accessor cases into tests/test_storage.py
- move the vouch doctor config-diagnostic cases into tests/test_health.py

no production code change; all assertions preserved.
@minion1227

Copy link
Copy Markdown
Contributor Author

addressed the re-review caution in bb7b079 — the rename to test_models.py
had left cross-module cases behind. split them to mirror the module under
test: test_models.py now holds only the Config/ReviewConfig/
RetrievalConfig unit tests, the KBStore.config cases moved to
test_storage.py, and the vouch doctor diagnostics to test_health.py.
no production code change; assertions preserved.

minion1227 added a commit to minion1227/vouch that referenced this pull request Jul 6, 2026
resolves conflicts from vouchdev#297 after main advanced to 1.2.1.

- context.py / proposals.py: keep the typed `store.config` readers, drop
  the ad-hoc yaml parsing main still carried at those call sites.
- proposals.py `_approval_block_reason`: keep main's protected-page-kind
  self-approval guard, but source `approver_role` from the typed config.
- health.py: import both `ConfigError` (vouchdev#243) and `Source` (main).
- test_storage.py: keep both the typed-config tests and main's
  corrupt-file resilience tests.
- changelog: move the vouchdev#243 entry into `[Unreleased]`; main already
  relocated the duplicate auto-labeling entry into [1.1.0].
- models.py: register `capture` / `recall` / `compile` as loose known
  config sections so main's new starter-config blocks don't trip the
  vouchdev#243 unknown-key check.
@plind-junior
plind-junior changed the base branch from main to test July 6, 2026 08:54
resolves the vouchdev#297 conflicts against its actual base branch (test, not
main) after test advanced to vouchdev#395.

- context.py / proposals.py: keep the typed `store.config` readers, drop
  the ad-hoc yaml parsing test still carried at those call sites.
- proposals.py `_approval_block_reason`: keep test's protected-page-kind
  self-approval guard, source `approver_role` from the typed config.
- health.py: import both `ConfigError` (vouchdev#243) and `Source` (test).
- test_storage.py: keep both the typed-config tests and test's
  corrupt-file resilience tests.
- changelog: move the vouchdev#243 entry into `[Unreleased]`; the auto-labeling
  dup test already carries in [1.1.0].
- models.py: register `capture` / `recall` / `compile` / `triage` as
  loose known config sections so test's starter-config and own-reader
  sections don't trip the vouchdev#243 unknown-key check.
…package

the vouchdev#237 host-compat drift check read openclaw.compat.pluginApi from
openclaw.plugin.json, but the 2026.6 loader repackage (47ac72f) moved
that floor into the new loader-facing package.json — the current openclaw
parser ignores openclaw.plugin.json's openclaw.* fields entirely. so
_load_host_compat() read a key that no longer existed, host_compat
silently degraded to {}, and the two drift tests failed on test.

point _load_host_compat() and the drift tests at package.json, where the
pluginApi floor now lives. the graceful-degradation and malformed-json
tests move with it. no behaviour change beyond reading the right file.
@github-actions github-actions Bot added the mcp mcp, jsonl, and http surfaces label Jul 6, 2026
# Conflicts:
#	CHANGELOG.md
#	src/vouch/capabilities.py
#	src/vouch/context.py
#	tests/test_capabilities.py
@github-actions github-actions Bot removed the mcp mcp, jsonl, and http surfaces label Jul 13, 2026
@plind-junior

Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 28-36: Update the changelog entry describing configuration
validation and diagnostics to also mention that malformed YAML syntax is
converted into ConfigError for vouch doctor, and that unknown nested
configuration keys are reported using dotted paths. Preserve the existing
details about validation, defaults, and top-level unknown-key reporting.

In `@src/vouch/context.py`:
- Around line 55-58: The `_configured_rerank` function still parses config.yaml
directly instead of using the typed configuration. Replace its YAML loading and
manual parsing with `store.config.retrieval.rerank`, and add the `rerank` field
to `RetrievalConfig` if it is missing so the key is schema-recognized and
doctor() does not warn about it.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e833a968-72f3-4084-af65-4ed5cd02d515

📥 Commits

Reviewing files that changed from the base of the PR and between bdfeadc and 52aff99.

📒 Files selected for processing (9)
  • CHANGELOG.md
  • src/vouch/context.py
  • src/vouch/health.py
  • src/vouch/models.py
  • src/vouch/proposals.py
  • src/vouch/storage.py
  • tests/test_health.py
  • tests/test_models.py
  • tests/test_storage.py
🚧 Files skipped from review as they are similar to previous changes (6)
  • tests/test_storage.py
  • src/vouch/storage.py
  • tests/test_models.py
  • src/vouch/models.py
  • src/vouch/proposals.py
  • tests/test_health.py

Comment thread CHANGELOG.md
Comment on lines +28 to +36
- typed `Config` model for `.vouch/config.yaml`: the file is parsed once into a
validated pydantic `Config` (with nested `ReviewConfig` / `RetrievalConfig`)
exposed as `KBStore.config`, replacing the per-call-site untyped `dict` reads
in `proposals.py` and the retrieval backend selector. a malformed value
(e.g. `retrieval.default_limit: "ten"`) now fails fast with the offending key
path instead of silently falling back, and `vouch doctor` surfaces unknown
top-level keys as likely typos instead of dropping them. existing KBs with
partial or absent `retrieval:` / `review:` blocks load with the documented
defaults — no on-disk change (#243).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

document the complete configuration-diagnostics behavior

the entry omits two user-visible behaviors added in this stack: malformed yaml syntax is converted into ConfigError for vouch doctor, and nested unknown keys are reported with dotted paths. please mention both so the changelog accurately reflects the shipped diagnostics.

🤖 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 28 - 36, Update the changelog entry describing
configuration validation and diagnostics to also mention that malformed YAML
syntax is converted into ConfigError for vouch doctor, and that unknown nested
configuration keys are reported using dotted paths. Preserve the existing
details about validation, defaults, and top-level unknown-key reporting.

Comment thread src/vouch/context.py
Comment on lines +55 to +58
by taking its first recognised entry. Anything unrecognised falls back to
"auto". Parsing + validation now lives in the typed `Config` model (#243).
"""
try:
loaded = yaml.safe_load(store.config_path.read_text(encoding="utf-8"))
except (OSError, yaml.YAMLError):
return "auto"
if not isinstance(loaded, dict):
return "auto"
retrieval = loaded.get("retrieval")
if not isinstance(retrieval, dict):
return "auto"
backend = retrieval.get("backend")
if isinstance(backend, str) and backend in _VALID_BACKENDS:
return backend
legacy = retrieval.get("backends")
if isinstance(legacy, list):
for entry in legacy:
if isinstance(entry, str) and entry in _VALID_BACKENDS:
return entry
return "auto"
return store.config.retrieval.resolved_backend()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

migrate _configured_rerank to the typed config model.

while _configured_backend was updated to use store.config, the adjacent _configured_rerank function (lines 61-90) still manually reads and parses config.yaml using yaml.safe_load. this contradicts the pr objective to replace untyped yaml parsing and update existing configuration readers.

additionally, if rerank was not added to the new RetrievalConfig model, doctor() will incorrectly flag retrieval.rerank as a warning for an unknown key. please update _configured_rerank to consume store.config.retrieval.rerank (adding it to the model if missing) to eliminate redundant i/o and enforce the new schema.

🤖 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/context.py` around lines 55 - 58, The `_configured_rerank` function
still parses config.yaml directly instead of using the typed configuration.
Replace its YAML loading and manual parsing with
`store.config.retrieval.rerank`, and add the `rerank` field to `RetrievalConfig`
if it is missing so the key is schema-recognized and doctor() does not warn
about it.

# Conflicts:
#	CHANGELOG.md
#	src/vouch/models.py
#	src/vouch/proposals.py
@minion1227
minion1227 requested a review from plind-junior as a code owner July 16, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs documentation, specs, examples, and repo guidance retrieval context, search, synthesis, and evaluation size: M 200-499 changed non-doc lines storage kb storage, migrations, schemas, and proposals tests tests and fixtures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(config): typed Config model + validation for .vouch/config.yaml

2 participants