fix(repo-finder): notes-only preferences fall back to defaults; overrides read '## filters'#30
Merged
Merged
Conversation
…ead '## filters' too Two follow-up fixes to the user-preferences feature (#29): - A preferences.md with no ## Filters content (only ## Notes, or an empty block) compiled to a catch-all and aborted repo-finder with exit 10 and a misleading "topics: any with no languages" error the user never wrote, leaving /contribute with an empty shortlist. It now falls back to DEFAULT_PROFILE ("default filters, plus advisory notes"). A ## Filters block with a real typo still fails loud, so the fallback never swallows a malformed filter. New prefs_has_filters helper in scripts/lib/preferences.sh. - The /repo-finder override path (--lang/--topic/--min-stars) inherited saved axes with an exact, case-sensitive '## Filters' matcher while preferences.sh accepts '## filters' and spacing variants, so an override on a hand-edited lowercase-header profile silently dropped the saved languages/stars and searched with defaults. Both readers now use the same detector. Tests: extend test_build_queries.sh (notes-only + empty-block cases), add test_repo_finder_override_inherit.sh. Full suite 64/64 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesPreferences filter handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Override as repo-finder override
participant Preferences as preferences.sh
participant Compiler as build_queries.sh
Override->>Compiler: Validate inherited preferences
Compiler->>Preferences: Detect and parse Filters
Preferences-->>Compiler: Parsed filters or default fallback
Compiler-->>Override: Compiled override query
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
tests/scripts/test_repo_finder_override_inherit.sh (1)
20-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the full header-normalization contract.
This regression test exercises only
## filters. Add## FILTERS, mixed-case, and whitespace variants so the test catches incomplete case-insensitive matching.🤖 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/scripts/test_repo_finder_override_inherit.sh` around lines 20 - 28, Expand the saved-profile fixture in the test covering the override inheritance path to include separate headers for ## FILTERS, mixed-case ## Filters, and whitespace variants around the header. Keep the existing preference values and assertions, ensuring each variant is processed identically by the header parser.
🤖 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 `@commands/repo-finder.md`:
- Around line 123-126: Update the saved-profile handling around the awk
extraction to validate the full non-empty $SAVED filter block with the canonical
parser before constructing the temporary override. Ensure malformed lines such
as misspelled axis keys are rejected explicitly rather than silently omitted,
and preserve normal extraction for valid saved filters.
In `@scripts/lib/preferences.sh`:
- Around line 58-60: Normalize the `## Filters` header case-insensitively in
both readers: update `_prefs_filter_lines` used by `prefs_has_filters` in
scripts/lib/preferences.sh at lines 58-60, and apply equivalent `tolower(...)`
normalization when inheriting saved axes for overrides in
commands/repo-finder.md at lines 119-122. Preserve the existing filtering and
override behavior.
---
Nitpick comments:
In `@tests/scripts/test_repo_finder_override_inherit.sh`:
- Around line 20-28: Expand the saved-profile fixture in the test covering the
override inheritance path to include separate headers for ## FILTERS, mixed-case
## Filters, and whitespace variants around the header. Keep the existing
preference values and assertions, ensuring each variant is processed identically
by the header parser.
🪄 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: f7c7dd6e-b368-48b9-9e58-bda8b9d0e8e5
📒 Files selected for processing (6)
CHANGELOG.mdcommands/repo-finder.mdscripts/lib/preferences.shscripts/repo-finder/build_queries.shtests/scripts/test_build_queries.shtests/scripts/test_repo_finder_override_inherit.sh
…ile, full case-insensitive header Two follow-ups from the PR #30 review: - Validate the saved profile before inheriting axes in an override run. The override extraction greps the saved ## Filters block for exact keys, so a typo'd `langauges: go` (which the normal path rejects with exit 10) was silently dropped, letting `/repo-finder --topic cli` proceed on a broken profile. When any axis is inherited, $SAVED is now validated with the same compiler the normal path uses; a full --lang/--topic/--min-stars override skips the check (it reads nothing from the saved file). - Match the ## Filters header fully case-insensitively (tolower) in both readers. Only `Filters`/`filters` were recognized before; a `## FILTERS` block was silently ignored, and with the notes-only fallback that emitted the default profile and dropped the user's filters with no error. Tests: add the uppercase-header case to test_build_queries.sh; extend test_repo_finder_override_inherit.sh with malformed-saved (must fail loud) and full-override (must not be blocked) contracts. Full suite 64/64 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/scripts/test_repo_finder_override_inherit.sh (1)
45-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the uppercase header variant.
This only exercises
## filters; add## FILTERSto protect the stated fully case-insensitive contract.Proposed test addition
+SAVED_UC='## FILTERS +languages: go +topics: backend +stars: 3000 +' +OUT=$(run_override "$SAVED_UC" "" "cli" "") \ + || fail "override on an uppercase Filters header errored: $OUT" +case "$OUT" in *"languages: go"*"stars: 3000"*) ;; *) + fail "saved axes not inherited from '## FILTERS': +$OUT" ;; +esac🤖 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/scripts/test_repo_finder_override_inherit.sh` around lines 45 - 50, Extend the Contract 1 test fixture in test_repo_finder_override_inherit.sh to include an uppercase ## FILTERS header alongside the existing lowercase ## filters case. Ensure the test verifies unnamed-axis inheritance remains successful under the uppercase header, preserving the fully case-insensitive behavior.
🤖 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 `@CHANGELOG.md`:
- Line 22: Polish the changelog entry by replacing “fails loud” with “fails
loudly” (or “fails explicitly”) and “typo'd” with “mistyped,” leaving the
surrounding technical explanation unchanged.
---
Nitpick comments:
In `@tests/scripts/test_repo_finder_override_inherit.sh`:
- Around line 45-50: Extend the Contract 1 test fixture in
test_repo_finder_override_inherit.sh to include an uppercase ## FILTERS header
alongside the existing lowercase ## filters case. Ensure the test verifies
unnamed-axis inheritance remains successful under the uppercase header,
preserving the fully case-insensitive behavior.
🪄 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: 0f274086-0116-4437-a753-a3d7bd2c16b3
📒 Files selected for processing (5)
CHANGELOG.mdcommands/repo-finder.mdscripts/lib/preferences.shtests/scripts/test_build_queries.shtests/scripts/test_repo_finder_override_inherit.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/scripts/test_build_queries.sh
- scripts/lib/preferences.sh
Address CodeRabbit nitpick on #30: use 'mistyped' over "typo'd" and 'fails loudly' over 'fails loud' in the two new Fixed entries.
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.
Two follow-up fixes to the user-preferences feature in #29. Stacked on #29 — its commits ride along in this diff until it lands; once #29 is merged this collapses to a single commit. The files this PR owns are
scripts/lib/preferences.sh,scripts/repo-finder/build_queries.sh,commands/repo-finder.md,CHANGELOG.md, and the two tests.1. A
preferences.mdwith no## Filtersblock dead-ended the whole scanA file carrying only
## Notes(advisory guidance, keep default filters) — or an empty## Filtersblock — compiled to a catch-all, sobuild_queries.shabortedrepo-finderwithexit 10and:…an error naming
topics: any, which the user never wrote. Downstream,/contributethen hit an empty shortlist and told the user to run/repo-finder— a dead end for someone who just wanted to add notes on top of the default filters.Fix: a file with no
## Filterscontent now means "default filters, plus advisory notes" and falls back toDEFAULT_PROFILE. A## Filtersblock with content (even a typo'd key likelangauges: go) still goes to the parser and fails loud — the fallback never swallows a malformed filter. Implemented with a newprefs_has_filtershelper that reuses the single existing Filters-block reader (_prefs_filter_lines), so there's no third parser to drift.2.
/repo-finderoverrides silently ignored a saved## filters(lowercase) headerThe override path (
--lang/--topic/--min-stars) inherits the axes you don't name from your saved profile. It read that profile with an exact, case-sensitive## Filtersmatcher, whilescripts/lib/preferences.shaccepts## filtersand spacing variants. So on a hand-edited lowercase-header profile,/repo-finder --topic clisilently dropped the savedlanguages/starsand searched with defaults — the exact silent-wrong-result this feature exists to prevent.Fix: the override path now detects the block with the same matcher
preferences.shuses.Tests
tests/scripts/test_build_queries.sh— added the notes-only and empty-## Filterscases (both must fall back to the golden default profile); the existing malformed-abort case guards that a real typo still exits 10.tests/scripts/test_repo_finder_override_inherit.sh(new) — an override on a lowercase## filtersprofile must inherit the savedlanguages/stars.Both were written test-first and watched fail before the fix. Full suite 64/64 green on local bash; network-free and bash-3.2-clean so they gate on ubuntu and macOS.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
/repo-findernow treats notes-only or empty## Filtersinpreferences.mdas “default filters + advisory notes,” avoiding failures and empty shortlists.## Filtersheaders are matched case-insensitively (and tolerate header/whitespace variations), so valid axes aren’t dropped.Tests
## FILTERS)./repo-findersaved-profile override inheritance, including failure scenarios.