Log a WARN instead of silently dropping invalid ScopeKnownError config - #355
Draft
rubberduck203 wants to merge 2 commits into
Draft
Log a WARN instead of silently dropping invalid ScopeKnownError config#355rubberduck203 wants to merge 2 commits into
rubberduck203 wants to merge 2 commits into
Conversation
…ing it Config::new (now FoundConfig::new) discarded any raw config document that failed to convert into its typed model, with zero logging. A ScopeKnownError using a reserved capture group name, or shipping a broken fix template, would silently never register, giving false confidence scope was watching for it. ScopeKnownError conversion failures now abort config loading with a diagnosable error naming the file and cause (scope exits 2; scope-intercept falls back to an empty config after logging the error). Other kinds, chiefly ScopeDoctorGroup, keep the existing tolerant behavior from #68/PR #69 but now actually warn instead of failing silently — including a ScopeKnownError from an apiVersion this binary doesn't recognize, which is treated the same as any other not-yet-understood config rather than a hard failure. Also fixes ParsedConfig::try_from silently swallowing real deserialization errors behind a generic, typo'd message, and scope.rs's error log dropping the underlying cause by using Display instead of the alternate/chained form. Fixes #344
Revisited the design from the previous commit after further discussion: a recognized-but-invalid ScopeKnownError now warns and is dropped, the same as any other kind, rather than aborting the entire config load. Aborting the whole load meant a single broken known-error file anywhere in a fleet-wide shared config directory would silently disable known-error matching (and scope-intercept's auto-fix) for every unrelated known error too, and would block scope doctor/list/report outright — too large a blast radius for one bad file. This also removes the hand-rolled kind/apiVersion matching that only existed to special-case ScopeKnownError, and consolidates the four config-load tests onto a shared found_config_from_files test helper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #344.
Config::new(nowFoundConfig::new) converted each raw parsedconfig document into its typed model via
try_into()and silently discardedthe
Errcase with zero logging — aScopeKnownErrorusing a reservedcapture group name, or shipping a broken
{{ }}fix template, would justvanish with no signal, and the tool would report "No known errors found" as
if the config didn't exist at all.
The issue thread initially discussed making a
ScopeKnownErrorconversionfailure a hard failure that aborts config loading entirely, since a
recognized-but-invalid known error can never work as written. After
implementing that, we reconsidered: aborting the whole config load meant a
single broken known-error file anywhere in a fleet-wide shared
.scopedirectory would disable known-error matching (and
scope-intercept'sauto-fix) for every unrelated known error too, and would block
scope doctor/list/reportoutright with exit code 2 — too large a blastradius for one bad file.
So the final behavior: every kind (
ScopeKnownError,ScopeDoctorGroup,etc.) now logs a
warn!(target: "user", ...)naming the file and the causewhen it fails to convert, and is dropped — the config load itself always
succeeds, same as before #344, just no longer silently. This preserves the
existing tolerant philosophy from #68/PR #69 uniformly across all kinds,
while finally giving the "at minimum, log it" signal the issue asked for.
Also fixed along the way, since silence had been hiding it:
ParsedConfig::try_from(src/shared/models/internal/mod.rs) usedif let Ok(Some(known)) = ...known_type(...), silently swallowing realdeserialization errors (e.g. a missing required field) behind a generic,
typo'd
"Error was know a known type"message. Real errors now propagatevia
?, so the new warn logs actually show the useful underlying cause.scope.rs's config-load error log used{}instead of{:#}, which fora
.context()-wrapped anyhow error prints only the outermost message anddrops the real cause. Fixed for the "unable to get a working dir" path
that can still fail there.
Test plan
cargo test— full suite passes, including 2 new unit tests insrc/shared/config_load.rs(via a sharedfound_config_from_filestest helper) covering: a bad known error warning-and-dropping while a
valid known error in the same directory still loads, and the same for
a bad doctor group alongside a valid one.
cargo clippy --all-targets -- -D warningscleancargo fmt --checkcleanscope-intercept: abroken known-error file now logs
WARN Unable to load ScopeKnownError/bad-reserved from <path> because known error '...' has a pattern with a capture group named 'captures', which is reserved for template substitution, while a second, validknown error in the same directory still matches and reports correctly.
🤖 Generated with Claude Code