Skip to content

fix(core): name the file and key in config value type errors - #48

Merged
ElbertePlinio merged 2 commits into
mainfrom
fix/46-config-value-type-errors
Sep 30, 2026
Merged

ElbertePlinio merged 2 commits into
mainfrom
fix/46-config-value-type-errors

Conversation

@ElbertePlinio

@ElbertePlinio ElbertePlinio commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Closes #46

validate_keys in crates/core/src/config.rs now type-checks each known value in each config file before merging. It deserializes the value into the same type its Config field uses, so the accepted set is unchanged. A wrong type reports `<key>` in <file> must be <expected>, where the expected text is "a non-negative integer or null" for global and per-language limits, "a non-negative integer" for hook.max_blocks, and "a list of strings" for ignore, tests.patterns, and tests.exempt. The final deserialize of the merged config stays as a backstop. docs/spec.md states the rule, and the release draft gains a line for #46.

One behavior change goes beyond the message. Main type-checked only the merged config, so a bad value in the user config was silently dropped when the repo config replaced the same languages.<name> entry (QA 01 exits 0 on main). Each file is now checked on its own, the same way unknown keys already were, so that config now exits 2 and names the user file.

Requirement Verified by Scenario Result Evidence
A value type error names the config file and the full key path automated test + CLI QA string, float, boolean, negative, and out-of-range limits; per-language string limit in the user config; hook.max_blocks of -1 and null; ignore as a string or null; mixed tests.patterns; string tests.exempt; non-string tests.exempt entries [1] and ["lines",1]; bad value in the repo config with a valid user config pass, exit 2 with key and file string_global_limit_names_key_and_config_path, string_language_limit_names_full_path, float_limit_is_rejected, negative_max_blocks_names_key_and_config_path, string_ignore_is_rejected_as_non_list, non_string_test_pattern_is_rejected, string_test_exempt_is_rejected_as_non_list, non_string_test_exempt_entry_gets_list_message; QA 01 to 12, 17
Tests cover a wrong type in a global limit, a per-language limit, and hook.max_blocks automated test the tests above, asserting the whole message with the temp config path pass crates/core/src/config.rs
Valid configs and other errors unchanged automated test + CLI QA null global and per-language limits, zero limits, max_blocks 0, valid ignore, empty config, unknown limit key with a bad value, non-object per-language limits, tests.exempt of ["depth"] byte-identical to main, including exit codes null_limits_still_load and existing config tests; QA 13 to 16, 18 to 20
Tests, lint, format, coverage, PickCheck automated cargo test --workspace --locked --all-targets, cargo clippy ... -D warnings, cargo fmt --check, cargo run -- check crates, cargo llvm-cov --fail-under-lines 94 (94.92%), pickcheck check --base origin/main --fail-on new,worsened,unmatched (0 violations), on b48c1c5 pass CI on this PR

QA: a CLI pass ran release builds from main (213fbbd) and this branch over a scratch Git repo in 20 scenarios, with a user config supplied through XDG_CONFIG_HOME, plus --changed and --base HEAD --format json with a string limit. The 13 wrong-type cases now name the key and file, and the 7 valid-config and other-error cases are byte-identical to main. After the review fix, the tests.exempt cases were rerun: [1] now gets the list message instead of unsupported tests.exempt key `<non-string>` , while ["depth"] keeps its old message. The evidence stays local and outside the repo.

The coverage floor stays at 94, which is 94.92% rounded down.

Review: independent xhigh reviews by Astra and Fable at d2c873b. Astra found nothing. Fable found no blockers. Its minor finding was that a non-string tests.exempt entry kept the older <non-string> message. Its nits were a needless clone in the type check and a test that bundled three scenarios. b48c1c5 fixes all three, and the Fable recheck confirmed it. A nit to unify backtick style with the older shape messages was declined, because the new messages follow the existing unknown-key style and restyling older messages is outside this issue.

Each config file's values are now type-checked before merging, so a
wrong type reports its full key and file instead of a generic serde
error from the merged config.
@ElbertePlinio
ElbertePlinio merged commit f380d8c into main Sep 30, 2026
8 checks passed
@ElbertePlinio
ElbertePlinio deleted the fix/46-config-value-type-errors branch September 30, 2026 18:46
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.

Config value type errors do not name the file or key

1 participant