fix(config): merge ignore.files and index_skip_files instead of overwriting - #477
Merged
Merged
Conversation
…riting Config merge used clone_from/clone for ignore.files and index_skip_files, which silently overwrote default patterns when a user specified any custom value. This caused build artifacts (target/**, dist/**, node_modules/**, .git/**) and framework config files (*.config.ts, *.config.js) to leak into review context, scan walks, and index-based scanner output. Fix: extend defaults with user values instead of replacing, with deduplication to avoid redundant entries. Impact: - cora review: context_chain now correctly filters default-ignored paths - cora scan: walk_project exclude patterns now include defaults - cora index: index-based scanner skip files preserved - cora config show: effective config displays merged patterns Tests: 3 new tests (merge_ignore updated, dedup, index_skip_files). 776 unit + 22 integration tests pass. Clippy clean.
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
1 participant
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.
What
Config merge in
CoraFile::merge_intousedclone_from/clonefor two Vec fields with non-empty defaults:ignore.files(line 530) — defaults:node_modules/**,dist/**,target/**,.git/**rules_config.index_skip_files(line 620) — defaults:*.config.ts,*.config.js,*.config.mjs, etc.When a user specified any custom value in
.cora.yaml, all defaults were silently overwritten — not merged.Fix: extend defaults with user values, with deduplication to avoid redundant entries.
Why
Bug impact across all cora commands:
cora reviewcontext_chain→ignore_patternstarget/debug/*,node_modules/) leak into review context, wasting tokens and confusing the LLMcora scanwalk_project→exclude_patternscora index(scanner)index_skip_filesvite.config.ts,next.config.js) generate false-positive findingscora config showRoot cause:
clone_from(v)replaces the entire Vec. Should beextend(v)with dedup check.Testing
cargo test --bin cora— 776 passed, 0 failedcargo test --test *— 22 passed (cli_basic + config_loading)cargo clippy --bin cora -- -D warnings— cleancora reviewpre-commit hook — No issues found ✅.cora.yamlwithignore.files: [vendor/**, *.lock]→cora config showcorrectly displaysnode_modules/**, dist/**, target/**, .git/**, vendor/**, *.lockNew tests added:
merge_ignore(updated) — asserts defaults preserved + user value presentmerge_ignore_files_dedup— verifies no duplicate when user specifies a default patternmerge_index_skip_files_preserves_defaults— asserts default skip files preserved + user value presentNotes
ignore.rulesmerge left asclone_from— default is empty[], so clone is correct behaviorfocusmerge left asclone_from— intentionally overwrite (focus narrowing is a valid use case, not a safety issue)--configflag does not load file (separate issue, not fixed in this PR)