feat(config): typed Config model + validation for .vouch/config.yaml#297
feat(config): typed Config model + validation for .vouch/config.yaml#297minion1227 wants to merge 3 commits into
Conversation
`.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
Bot
added
docs
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntroduces a typed pydantic ChangesTyped Config Model
Sequence Diagram(s)sequenceDiagram
participant Context as context.py
participant Proposals as proposals.py
participant KBStore
participant Config
Context->>KBStore: store.config
KBStore->>Config: Config.load(yaml)
Config-->>KBStore: typed Config
KBStore-->>Context: cached Config
Context->>Config: retrieval.resolved_backend()
Proposals->>KBStore: store.config
Proposals->>Config: review.approver_role / expire_pending_after_days
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
🧹 Nitpick comments (4)
src/vouch/models.py (1)
477-477: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNested
review:/retrieval:typos are silently swallowed, unlike top-level ones.
ReviewConfigandRetrievalConfigboth declareextra="allow", butConfig.unknown_keys()(Line 559-561) only inspects the top-level__pydantic_extra__. A typo likereview: {expier_pending_after_days: 1}lands as an untracked extra field onReviewConfig— never raised as an error, never surfaced byunknown_keys(), and never shown byvouch doctor. This reproduces, one level deeper, exactly the silent-typo problem this PR is meant to close.Consider extending
unknown_keys()to also walkreview.__pydantic_extra__/retrieval.__pydantic_extra__(prefixed, e.g."review.foo"), or setextra="forbid"onReviewConfig/RetrievalConfigif 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 valueTest file name doesn't mirror the module it targets.
Guidelines require test files to mirror module names as
tests/test_<module>.py. This file exercisesvouch.models.Config(and integration withstorage.py/health.py) but is namedtest_config_model.pyrather thantest_models.py.As per coding guidelines, "Test file names must mirror module names using the
tests/test_<module>.pyconvention."🤖 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 valueConsider adding a same-instance caching test for
KBStore.config.The upstream
KBStore.configdocstring notes it's acached_property, but no test here asserts that repeated access on the same store instance returns the same cached value (only cross-instance behavior via freshKBStore(tmp_path)is exercised). A quickassert store.config is store.configwould 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 winConsider merging with the existing "Config sanity" block.
This new block and the pre-existing
missing_configcheck (Lines 257-265) both gate onstore.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
📒 Files selected for processing (7)
CHANGELOG.mdsrc/vouch/context.pysrc/vouch/health.pysrc/vouch/models.pysrc/vouch/proposals.pysrc/vouch/storage.pytests/test_config_model.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.
|
thanks for the review — pushed 930a866 addressing all of it:
|
There was a problem hiding this comment.
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 winCross-module tests placed in
test_models.py.Tests at Lines 128-141 (
KBStore.configcaching/ConfigError) and Lines 172-190 (doctor()diagnostics) exercisestorage.pyandhealth.pybehavior respectively, notmodels.py. Per coding guidelines, test file names must mirror the module under test. Consider splitting these intotests/test_storage.pyandtests/test_health.py, keepingtest_models.pyscoped toConfig/ReviewConfig/RetrievalConfigunit tests.As per coding guidelines, "Test file names must mirror module names using the
tests/test_<module>.pyconvention."🤖 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
📒 Files selected for processing (4)
src/vouch/health.pysrc/vouch/models.pysrc/vouch/storage.pytests/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.
|
addressed the re-review caution in bb7b079 — the rename to |
1 participant
What changed
.vouch/config.yamlis now parsed once into a validated pydanticConfig(with nested
ReviewConfig/RetrievalConfig), exposed as a cachedKBStore.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-hocyaml.safe_load+nested
.get()+except: {}parsing.vouch doctorgains 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 amalformed value (
retrieval.default_limit: "ten") silently fell back to adefault 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:
config_unknown_keywarning invouch doctor(preserved, not dropped).(
config.yaml: retrieval.default_limit: ...) instead of a silent fallback..vouch/with noretrieval:/review:blocks loads withthe documented defaults — covered by round-trip tests.
What might break
nothing on disk.
config.yaml's format is unchanged, the legacyretrieval.backendslist still resolves exactly as before, and a missingfile still yields all-defaults. the one behavior change is intentional and
per the issue: a genuinely malformed config value now raises
ConfigError(and
vouch doctorreportsconfig_invalid) rather than silentlydefaulting. 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 doctorfinding codes. no VEP needed.Tests
make checkpasses 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 embeddingsinstalled, both of which also fail on
main)tests/test_config_model.py(15 cases:defaults, partial configs, unknown-key warning, malformed-value
failure, legacy-backends resolution, doctor integration)
CHANGELOG.mdupdated under## [Unreleased]Summary by CodeRabbit
New Features
Bug Fixes
Tests