Skip to content

fix(config): fall back to defaults on malformed numeric config values - #686

Merged
plind-junior merged 3 commits into
vouchdev:testfrom
joaovictor91123:fix/config-numeric-coercion
Jul 31, 2026
Merged

fix(config): fall back to defaults on malformed numeric config values#686
plind-junior merged 3 commits into
vouchdev:testfrom
joaovictor91123:fix/config-numeric-coercion

Conversation

@joaovictor91123

Copy link
Copy Markdown
Contributor

What changed

recall.load_config()'s max_chars and capture.load_config()'s
min_observations/dedup_window_seconds now go through a new shared
coerce_numeric() helper in config_coerce.py instead of bare
int()/float() calls. compile.py's local _coerce() — which already
implemented this exact fail-soft pattern for its own numeric fields — is
removed in favor of the shared helper.

Why

Both load_config() functions promise "fall back to defaults" on
malformed config, and their boolean enabled field already honors that
via coerce_bool, but their numeric fields didn't: a config typo like
max_chars: "12,000" raised ValueError straight out of load_config.
recall.load_config backs the SessionStart hook with no exception
handling around the call, so one bad numeric value crashed recall-digest
injection on every new session; capture.load_config backs session-split
capture and codex rollout ingestion the same way.

Confirmed with a repro: recall.load_config(store) on a KB with
max_chars: "12,000" raises ValueError: invalid literal for int() with base 10: '12,000' instead of falling back to DEFAULT_MAX_CHARS.

Fixes #685

What might break

Nothing for users with an existing .vouch/ directory — no on-disk
shape, kb.* method, or object model change. Behaviorally: a malformed
numeric config value now degrades to the default instead of raising —
strictly the documented, intended contract. A valid numeric value parses
identically to before.

VEP

Not applicable — no object model, kb.* method, on-disk layout, bundle
format, or audit-log shape change. A config-parsing robustness fix, plus
consolidating a helper that already existed in one module into the
shared one two other modules already use for the boolean case.

Prior art

This exact defect and fix were previously submitted as #488
(fix(config): fall back to defaults on malformed numeric config values, CodeRabbit-reviewed with no substantive objections) but closed
unmerged on 2026-07-29 purely for going stale against test
(src/vouch/capture.py and src/vouch/recall.py conflicts) — the
maintainer's closing comment explicitly said this wasn't a judgment on
the change. Re-verified independently against current test HEAD: only
the boolean coercion was separately fixed since; the numeric gap is
still live. This PR reintroduces the fix, freshly rebased, structured as
a shared coerce_numeric() (matching the existing coerce_bool()
pattern) rather than three separate local copies.

Tests

  • Local make check-equivalent: ruff clean (src + tests); mypy
    clean on all four changed source files; 83 tests across
    tests/test_recall.py, tests/test_capture.py, and the relevant
    tests/test_compile.py case pass — two pre-existing, unrelated
    failures in test_compile.py (test_jsonl_kb_compile_files_ proposals, test_two_phase_compile_drafts_planned_pages) are a
    Windows-only stub-LLM python3 invocation issue, confirmed
    identical via git stash comparison against unmodified test
    HEAD; CI runs on Linux
  • New / changed behaviour has a test —
    test_load_config_malformed_max_chars_falls_back (recall),
    test_load_config_malformed_numeric_falls_back (capture);
    compile.py's existing test_load_config_bad_values_fall_back_to_defaults
    continues to pass unchanged against the now-shared helper
  • CHANGELOG.md updated under ## [Unreleased]

recall.load_config and capture.load_config passed YAML-sourced numeric
fields (max_chars, min_observations, dedup_window_seconds) straight
through bare int()/float(), raising ValueError on a config typo like
max_chars: "12,000" instead of degrading to the default the way the
same modules' enabled boolean already does via coerce_bool.
recall.load_config backs the SessionStart hook with no exception
handling around the call, so one bad numeric value took down recall
injection on every new session; capture.load_config backs
session-split capture and codex rollout ingestion the same way.

compile.py already implemented this fail-soft contract for its own
numeric fields via a local _coerce(value, default, cast) helper.
promote it into the shared config_coerce.py module as coerce_numeric,
alongside the existing coerce_bool, and use it in recall.py and
capture.py's load_config functions, removing the now-redundant local
copy in compile.py.

new tests confirm a malformed numeric value degrades to the default
in both recall and capture rather than raising.

this exact defect and fix were previously submitted as vouchdev#488
(CodeRabbit-reviewed, no substantive objections), but that PR was
closed unmerged for going stale against a fast-moving test branch, not
for anything wrong with the change; the maintainer's closing comment
explicitly invited a fresh PR.

Fixes vouchdev#685
@github-actions github-actions Bot added docs documentation, specs, examples, and repo guidance tests tests and fixtures size: S 50-199 changed non-doc lines labels Jul 30, 2026
@plind-junior

Copy link
Copy Markdown
Member

the consolidation is the right shape: compile._coerce already was coerce_numeric under another name, and moving it next to coerce_bool in config_coerce.py means the next module that reads a numeric config key finds the fail-soft version first instead of reaching for bare int(). deleting the local copy rather than leaving both is what makes it a consolidation.

the recall.load_config case is the one that justifies the change on its own — it backs the SessionStart hook with no exception handling around the call, so a single typo'd max_chars took recall-digest injection down on every new session, silently, for a value that has a perfectly good default sitting right there.

two things worth a look, neither blocking:

coerce_numeric(value, default, int) will happily accept a bool, since int(True) is 1 — so min_observations: yes in yaml parses to True and lands as 1 rather than falling back to the default. it is a strict improvement over raising, but it is also the one malformed shape that still passes through silently instead of degrading to the documented default. an isinstance(value, bool) guard before the cast would close it, if you think that is worth the line.

and int(12.9) truncating to 12 is the same story — arguably fine, but it means min_observations: 2.9 is honoured as 2 rather than treated as malformed. worth a sentence in the helper's docstring saying which of these are deliberate, since "fall back to defaults on malformed values" currently reads as stricter than the implementation is.

the prior-art note on #488 is useful context and matches what i see — the boolean half landed separately since, the numeric gap did not.

@plind-junior
plind-junior merged commit 4f730bb into vouchdev:test Jul 31, 2026
8 of 11 checks passed
@github-actions github-actions Bot added the ci: failing ci is red label Jul 31, 2026
galuis116 added a commit to galuis116/vouch that referenced this pull request Jul 31, 2026
a6c6862 (vouchdev#686) fixed capture.load_config's min_observations and
dedup_window_seconds to fall back to their defaults on a malformed
config value via the new coerce_numeric() helper, instead of raising
ValueError straight out of load_config. 47eaf56 (vouchdev#645, realtime
opt-in) branched off the pre-fix capture.py and reintroduced the bare
int()/float() calls when it merged into test - the coerce_numeric
import survived (nothing else referenced it), but the two call sites
it fed didn't, silently reverting the fix and leaving
test_load_config_malformed_numeric_falls_back red on `test` HEAD
itself, currently failing this PR's CI via ruff's unused-import gate.

restore the coerce_numeric() calls, matching recall.load_config's
still-intact equivalent.

unrelated to this PR's own change (fsck delete-proposal handling);
needed only to get CI green on top of a currently-broken `test`.
philluiz2323 added a commit to philluiz2323/vouch that referenced this pull request Jul 31, 2026
a6c6862 (vouchdev#686) fixed capture.load_config's min_observations and
dedup_window_seconds to fall back to their defaults on a malformed
config value via the coerce_numeric() helper, instead of raising
ValueError straight out of load_config. 47eaf56 (vouchdev#645, realtime
opt-in) branched off the pre-fix capture.py and reintroduced the bare
int()/float() calls when it merged into test - the coerce_numeric
import survived (nothing else referenced it), but the two call sites
it fed didn't, silently reverting the fix and breaking ruff's
unused-import gate for every PR built on top of `test`.

restore the coerce_numeric() calls, matching recall.load_config's
still-intact equivalent.

unrelated to this PR's own change (extract.py segmentation); needed
only to get CI green on top of a currently-broken `test`.
joaovictor91123 added a commit to joaovictor91123/vouch that referenced this pull request Jul 31, 2026
a6c6862 (vouchdev#686) fixed capture.load_config's min_observations and
dedup_window_seconds to fall back to their defaults on a malformed
config value via the coerce_numeric() helper, instead of raising
ValueError straight out of load_config. 47eaf56 (vouchdev#645, realtime
opt-in) branched off the pre-fix capture.py and reintroduced the bare
int()/float() calls when it merged into test - the coerce_numeric
import survived (nothing else referenced it), but the two call sites
it fed didn't, silently reverting the fix and breaking ruff's
unused-import gate for every PR built on top of `test`.

restore the coerce_numeric() calls, matching recall.load_config's
still-intact equivalent.

unrelated to this PR's own change (graph.py edge leak); needed only
to get CI green on top of a currently-broken `test`.
joaovictor91123 added a commit to joaovictor91123/vouch that referenced this pull request Jul 31, 2026
a6c6862 (vouchdev#686) fixed capture.load_config's min_observations and
dedup_window_seconds to fall back to their defaults on a malformed
config value via the coerce_numeric() helper, instead of raising
ValueError straight out of load_config. 47eaf56 (vouchdev#645, realtime
opt-in) branched off the pre-fix capture.py and reintroduced the bare
int()/float() calls when it merged into test - the coerce_numeric
import survived (nothing else referenced it), but the two call sites
it fed didn't, silently reverting the fix and breaking ruff's
unused-import gate for every PR built on top of `test`.

restore the coerce_numeric() calls, matching recall.load_config's
still-intact equivalent.

unrelated to this PR's own change (experts.py viewer scoping); needed
only to get CI green on top of a currently-broken `test`.
minion1227 added a commit to minion1227/vouch that referenced this pull request Jul 31, 2026
vouchdev#686 added coerce_numeric and routed capture.py's two boolean fields
through coerce_bool, but left min_observations and dedup_window_seconds
on bare int()/float(). a typo'd value raised out of load_config instead
of falling back to the default, which is the exact case the helper's own
docstring cites (`min_observations: "three"`), and the resulting unused
import tripped ruff F401.

surfaced by merging test into this branch: the branch-push workflows on
test don't run pytest/mypy/ruff, so the gate never ran on the merge that
landed it.

Co-authored-by: Cursor <cursoragent@cursor.com>
plind-junior pushed a commit that referenced this pull request Jul 31, 2026
#686 added coerce_numeric and routed capture.py's two boolean fields
through coerce_bool, but left min_observations and dedup_window_seconds
on bare int()/float(). a typo'd value raised out of load_config instead
of falling back to the default, which is the exact case the helper's own
docstring cites (`min_observations: "three"`), and the resulting unused
import tripped ruff F401.

surfaced by merging test into this branch: the branch-push workflows on
test don't run pytest/mypy/ruff, so the gate never ran on the merge that
landed it.

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci: failing ci is red docs documentation, specs, examples, and repo guidance size: S 50-199 changed non-doc lines tests tests and fixtures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(config): recall/capture crash on malformed numeric config values instead of falling back to defaults

2 participants