fix: P2 quick wins — version pinning, provider guard, micromatch glob#199
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a configurable FlagLint version to the composite action, restricts config provider support to ChangesConfigurable FlagLint version in composite action
Restrict provider to launchdarkly
Bootstrap glob matching via micromatch
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- action.yml: add `version` input (default "latest"); route through FLAGLINT_VERSION env var so npx flaglint@"$FLAGLINT_VERSION" works safely without shell injection risk - src/config.ts: replace z.enum() with z.string() + runtime assertSupportedProvider() that throws a clear exit-2 message for any provider other than "launchdarkly"; Zod still validates other fields - src/validator/index.ts: replace 30-line hand-rolled glob implementation in matchesBootstrapPattern() with micromatch.isMatch() — micromatch is already a direct dependency (used in migrator/apply.ts) - src/config/tests/config.test.ts: update two tests that previously expected provider:"unleash" to pass through; they now assert the new unsupported-provider error message Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
Krishan27
force-pushed
the
fix/p2-quick-wins
branch
from
0e943b2 to
4ef1421
Compare
![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 (2)
src/config.ts (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProvider type widened to plain
string.Switching from an enum to
z.string()meansFlagLintConfig["provider"]is now typed as a genericstringrather than a literal union, losing compile-time narrowing for any downstream code that switches on provider values. Runtime enforcement viaassertSupportedProvidercovers correctness, but consider a companion literal type (e.g.type SupportedProvider = "launchdarkly") if provider-specific branching grows later.🤖 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 `@src/config.ts` at line 19, The provider field in the config schema is now widened to a generic string, so downstream code loses compile-time narrowing for provider-specific branching. Update the config typing around the `FlagLintConfig`/`provider` schema in `config.ts` to preserve a literal union or companion `SupportedProvider` type while keeping the existing runtime validation via `assertSupportedProvider`, so future switches on provider remain type-safe.src/validator/index.ts (1)
46-49: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePattern cleaning is redundantly recomputed per usage.
cleanis redefined andpatterns.map(clean)re-executed on every call from theusagesloop (Lines 74-88), even thoughbootstrapExcludedoesn't change across iterations. Cheap to hoist the cleaned pattern list out of the per-usage loop.🤖 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 `@src/validator/index.ts` around lines 46 - 49, The pattern cleanup in matchesBootstrapPattern is being recomputed on every usage check, which is unnecessary because bootstrapExclude is stable across the loop. Hoist the cleaned pattern list out of the per-usage iteration and reuse it when calling matchesBootstrapPattern, keeping the existing clean normalization logic in matchesBootstrapPattern or a nearby helper so the comparison still uses the same symbols.
🤖 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 `@src/config.ts`:
- Around line 55-63: The unsupported-provider path in assertSupportedProvider is
still being treated like a generic failure by loadConfigOrExit, so it always
exits with code 1 instead of the intended code 2. Update assertSupportedProvider
to throw a typed error or otherwise mark this case distinctly, and adjust
loadConfigOrExit to recognize that specific error before the generic catch and
call process.exit(2) for it while leaving other errors unchanged.
In `@src/validator/index.ts`:
- Around line 46-49: Normalize the path before glob matching in
matchesBootstrapPattern so Windows backslash-separated relative paths are
handled consistently. Update the clean helper to also convert backslashes to
forward slashes, alongside stripping the leading ./, and keep using it for both
the file argument and the patterns passed to micromatch.isMatch. This should
align with the other path helpers and ensure bootstrapExclude matches usage.file
correctly.
---
Nitpick comments:
In `@src/config.ts`:
- Line 19: The provider field in the config schema is now widened to a generic
string, so downstream code loses compile-time narrowing for provider-specific
branching. Update the config typing around the `FlagLintConfig`/`provider`
schema in `config.ts` to preserve a literal union or companion
`SupportedProvider` type while keeping the existing runtime validation via
`assertSupportedProvider`, so future switches on provider remain type-safe.
In `@src/validator/index.ts`:
- Around line 46-49: The pattern cleanup in matchesBootstrapPattern is being
recomputed on every usage check, which is unnecessary because bootstrapExclude
is stable across the loop. Hoist the cleaned pattern list out of the per-usage
iteration and reuse it when calling matchesBootstrapPattern, keeping the
existing clean normalization logic in matchesBootstrapPattern or a nearby helper
so the comparison still uses the same symbols.
🪄 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: 24f89175-8aaf-458b-b396-41404009740f
📒 Files selected for processing (4)
action.ymlsrc/config.tssrc/config/tests/config.test.tssrc/validator/index.ts
| export function matchesBootstrapPattern(file: string, patterns: string[]): boolean { | ||
| // Normalise leading "./" | ||
| if (patterns.length === 0) return false; | ||
| const clean = (s: string): string => s.replace(/^\.\//, ""); | ||
| const cleanFile = clean(file); | ||
|
|
||
| return patterns.some((pattern) => { | ||
| const cleanPattern = clean(pattern); | ||
| if (cleanFile === cleanPattern) return true; | ||
|
|
||
| // Convert glob to a regular expression. | ||
| const regexStr = cleanPattern | ||
| // Escape regex-special chars except * and ? | ||
| .replace(/[.+^${}()|[\]\\]/g, "\\$&") | ||
| // Temporarily hide ** before processing single * | ||
| .replace(/\*\*/g, "\x00") | ||
| // Single * → match any sequence that does NOT cross a / | ||
| .replace(/\*/g, "[^/]*") | ||
| // ** → match anything including / | ||
| .replace(/\x00/g, ".*") | ||
| // ? → single non-separator char | ||
| .replace(/\?/g, "[^/]"); | ||
|
|
||
| try { | ||
| return new RegExp(`^${regexStr}$`).test(cleanFile); | ||
| } catch { | ||
| return false; | ||
| } | ||
| }); | ||
| return micromatch.isMatch(clean(file), patterns.map(clean)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
micromatch isMatch negation pattern array semantics vs match filter
💡 Result:
In micromatch, the handling of negation patterns in arrays differs significantly between boolean checks and list filtering due to how they resolve inclusion and exclusion [1][2]. Micromatch follows these established conventions regarding negation: 1. When a pattern is a string, patterns are inclusive by default [1]. 2. When an array of patterns is passed, patterns are exclusive by default [1]. Boolean Matching (isMatch) The isMatch(str, patterns) function returns true if any of the given glob patterns match the specified string [3][4]. When using an array of patterns, the behavior of negations (e.g., ['!pattern']) can be counterintuitive because a positive match from any non-negated pattern will still result in a true value [5]. Effectively, isMatch checks if the string matches any of the "include" patterns; it does not inherently act as a filter that must pass every negation constraint unless specifically structured to do so [5]. Array Filtering (match and filter) Methods like match(list, patterns) or the function returned by filter(patterns) are designed to perform set-based filtering [1][2]. These methods implement exclusive-by-default logic: - They start with a set of candidates and apply the patterns to filter the list [1]. - When an array of patterns includes both positive and negative glob patterns, the negative patterns (e.g., '!*.js') correctly exclude matching items from the final result set [1]. Key Differences - isMatch is designed for testing a single string against a set of rules and returns a boolean [3][4]. Because it is "any-match" based, simply providing negative patterns in an array may not exclude the string if other patterns also match it [5]. - filter/match is designed for processing lists [1][2]. It respects the "exclusive by default" behavior, where negative patterns are effectively used to subtract items from the result set [1]. If you require a boolean check that respects all negation patterns as "must not match," you may need to use a custom wrapper or check the string against match to see if it remains in the result set [5].
Citations:
- 1: https://npmx.dev/package/micromatch/v/~2.3.11
- 2: What is the difference between matcher and filter? micromatch/micromatch#40
- 3: https://github.com/micromatch/micromatch/
- 4: https://github.com/micromatch/micromatch/blob/master/README.md
- 5: Array with negation pattern micromatch/micromatch#9
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File under review ==\n'
sed -n '1,140p' src/validator/index.ts | cat -n
printf '\n== Micromatch dependency context ==\n'
for f in package.json package-lock.json pnpm-lock.yaml yarn.lock; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n 'micromatch' "$f" -n -C 2 || true
fi
done
printf '\n== Search for matching path normalization / negation handling ==\n'
rg -n 'replace\(/\\\\/g|isMatch\(|micromatch\(' src test . || trueRepository: flaglint/flaglint-js
Length of output: 8195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Path normalization in scanner/usage flow ==\n'
rg -n 'replace\(/\\\\/g, "/"\)|usage\.file|filePath|scannedFiles|usages' src -C 2
printf '\n== Scanner fingerprint helper ==\n'
sed -n '1,80p' src/scanner/fingerprint.ts | cat -n
printf '\n== Validation call sites / usage shape ==\n'
sed -n '1,140p' src/types.ts | cat -nRepository: flaglint/flaglint-js
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File source / path shape ==\n'
sed -n '1,220p' src/scanner/source.ts | cat -n 2>/dev/null || true
sed -n '1,220p' src/scanner/index.ts | awk 'NR>=640 && NR<=750 {print NR ":" $0}'
sed -n '1,220p' src/validator/tests/validate.test.ts | awk 'NR>=340 && NR<=390 {print NR ":" $0}'
printf '\n== Micromatch docs in repo ==\n'
rg -n 'bootstrapExclude|matchesBootstrapPattern|isMatch|micromatch' README.md src -C 2Repository: flaglint/flaglint-js
Length of output: 12050
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File source / path shape ==\n'
sed -n '1,220p' src/scanner/source.ts | cat -n 2>/dev/null || true
sed -n '640,750p' src/scanner/index.ts | cat -n
sed -n '340,390p' src/validator/tests/validate.test.ts | cat -n
printf '\n== Micromatch/docs references ==\n'
rg -n 'bootstrapExclude|matchesBootstrapPattern|isMatch|micromatch' README.md src -C 2Repository: flaglint/flaglint-js
Length of output: 18080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== FileSource / path contracts ==\n'
sed -n '1,140p' src/types.ts | cat -n
printf '\n== Source implementations ==\n'
fd -a 'source.ts' src
fd -a 'source.*' src
fd -a '*source*' src
printf '\n== Fast-glob / path normalization usage ==\n'
rg -n 'fast-glob|fg\.|listFiles\(' src -C 2Repository: flaglint/flaglint-js
Length of output: 4780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== local-source implementation ==\n'
sed -n '1,220p' src/scanner/local-source.ts | cat -n
printf '\n== any Windows-path normalization/tests ==\n'
rg -n '\\\\|replace\(/\\\\/g, "/"\)|file: .*\\|scanRoot' src test -C 2Repository: flaglint/flaglint-js
Length of output: 13381
Normalize usage.file before glob matching. LocalFileSource can feed backslash-separated relative paths on Windows, but clean() only strips ./, so bootstrapExclude can miss intended matches there. Add replace(/\\/g, "/") here to match the other path helpers.
🤖 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 `@src/validator/index.ts` around lines 46 - 49, Normalize the path before glob
matching in matchesBootstrapPattern so Windows backslash-separated relative
paths are handled consistently. Update the clean helper to also convert
backslashes to forward slashes, alongside stripping the leading ./, and keep
using it for both the file argument and the patterns passed to
micromatch.isMatch. This should align with the other path helpers and ensure
bootstrapExclude matches usage.file correctly.
2 participants
Summary
action.yml — adds
versioninput (default"latest") so callers can pin to a specific release:uses: flaglint/flaglint-js@v1 with: version: "0.8.0". Routes throughFLAGLINT_VERSIONenv var (no shell injection).src/config.ts — replaces
z.enum([...])forproviderwithz.string()+ a runtimeassertSupportedProvider()check. Any unsupported provider now throws a clear error message (exit 2) rather than silently accepting"unleash"/"growthbook"as valid configs.src/validator/index.ts — replaces the hand-rolled 30-line glob implementation in
matchesBootstrapPattern()withmicromatch.isMatch(). micromatch is already a direct dependency and handles edge cases ({a,b}brace expansion, negation, windows paths) that the DIY regex did not.src/config/tests/config.test.ts — updates two tests that previously expected
provider: "unleash"to be accepted; they now assert the new error message.Test plan
npm run typecheck— cleannpm run test:run— 520/520 passsrc/commands/tests/baseline.test.ts) already cover all--baseline/--fail-on-newscenariosaction.ymlversion input is safe: value routed through env var before shell sees itSummary by CodeRabbit
New Features
versioninput to the FlagLint GitHub Action (default:latest) to select the FlagLint release to run.Bug Fixes
providervalues now fail with a clearer “not supported” error.Tests