[exclude] add support for globs in exclude#6
Merged
Conversation
🤖 Pull request artifacts
|
WalkthroughAdds a doublestar-based exclude.Matcher, validates glob patterns, and wires the matcher into CLI, watcher, and syncer to skip matched paths. Pins Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI
participant Exclude as "exclude.Matcher"
participant Watcher
participant Syncer
participant FS as "Filesystem / fsnotify"
CLI->>Exclude: New(cfg.Excludes, cfg.Source)
Exclude-->>CLI: matcher / error
CLI->>Watcher: New(rootPath, matcher, logger)
CLI->>Syncer: New(rootPath, client, matcher, logger)
FS->>Watcher: fsnotify event (path)
Watcher->>Exclude: MatchRule(fullpath)
Exclude-->>Watcher: matched? (rule)
alt matched
Watcher-->>Watcher: log debug + skip
else not matched
Watcher->>Syncer: notify event/path
Syncer->>Exclude: MatchRule(absPath)
Exclude-->>Syncer: matched? (rule)
alt matched
Syncer-->>Syncer: log debug + skip/return
else not matched
Syncer->>FS: upload/remove/reconcile actions
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
🧹 Nitpick comments (10)
internal/syncer/syncer.go (1)
159-169: SimplifyisExcluded– fallback is unnecessary.The matcher's
normalizeInputPathalready handles both absolute and relative paths (converting absolute paths to relative internally). CallingMatchRuletwice with bothabsPathandrelPathis redundant—passing justabsPathshould suffice.♻️ Suggested simplification
func (s *Syncer) isExcluded(absPath, relPath string, isDir bool) (bool, string) { if s.matcher == nil { return false, "" } - if matched, rule := s.matcher.MatchRule(absPath, isDir); matched { - return true, rule - } - - return s.matcher.MatchRule(relPath, isDir) + return s.matcher.MatchRule(absPath, isDir) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/syncer/syncer.go` around lines 159 - 169, The isExcluded method redundantly calls s.matcher.MatchRule twice; simplify by only using the absolute path once. Keep the nil check for s.matcher, then call s.matcher.MatchRule(absPath, isDir) and return its (matched, rule) result directly instead of calling MatchRule(relPath...). Update the function body in Syncer.isExcluded to remove the fallback call and return the single MatchRule result.internal/exclude/matcher.go (3)
61-61: UnusedisDirparameter.The
isDirparameter is ignored (blank identifier_ bool). If directory-specific matching behavior is planned (e.g., patterns ending with/), consider implementing it; otherwise, remove the parameter from the API to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` at line 61, The MatchRule method currently declares an unused isDir parameter (signature: Matcher.MatchRule(filePath string, _ bool) (bool, string)); remove the unused boolean parameter from the method signature (change to Matcher.MatchRule(filePath string) (bool, string)) and update all callers and any tests to use the new signature, or alternatively implement directory-aware logic inside Matcher.MatchRule to respect patterns ending with '/' by using the isDir value — pick one approach consistently and adjust references to the Matcher.MatchRule symbol accordingly.
12-16: Consider simplifying therulestruct.Both
literalandpatternfields are always assigned the same normalized value (lines 44-45). You could consolidate into a singlevaluefield to reduce redundancy.♻️ Suggested simplification
type rule struct { - literal string - pattern string + value string isPattern bool }Then update usages accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 12 - 16, The rule struct has redundant fields literal and pattern that are always the same; replace them with a single field (e.g., value string) and keep isPattern bool to indicate whether value is a glob/pattern or a literal, then update every place that constructs or reads rule.literal or rule.pattern (e.g., parsers/constructors and matchers) to use rule.value instead and preserve the existing isPattern semantics so matching logic in functions that reference rule.literal/pattern continues to work correctly.
125-127: Consider documenting the intentional omission of[and].The
hasMetafunction only checks for*and?, meaning bracket expressions like[abc]are treated as literals. This appears intentional (per the tests), but a brief comment would clarify this design choice for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 125 - 127, The hasMeta function currently checks only for '*' and '?' via strings.ContainsAny(pattern, "*?") and intentionally treats bracket expressions like '[' and ']' as literal characters; add a brief comment above hasMeta explaining this design decision (i.e., that '[' and ']' are intentionally omitted so character classes are not considered meta) and reference the function name hasMeta and the pattern parameter so future maintainers understand why bracket expressions are not treated as meta patterns.internal/watcher/watcher.go (1)
227-230: Consider threading context throughaddRecursive.Using
context.Background()here loses the ability to respect cancellation during initial directory traversal and prevents structured logging context from propagating. Consider passing the context from the caller.♻️ Suggested change
-func (w *Watcher) addRecursive(path string) error { - if w.isExcluded(context.Background(), path) { +func (w *Watcher) addRecursive(ctx context.Context, path string) error { + if w.isExcluded(ctx, path) { return nil }Then update callers to pass
ctx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/watcher/watcher.go` around lines 227 - 230, The call to context.Background() in addRecursive loses cancellation and logging context; change the signature of Watcher.addRecursive to accept a context (e.g., func (w *Watcher) addRecursive(ctx context.Context, path string) error) and use that ctx when calling w.isExcluded and any other context-using helpers, then update all callers of addRecursive to pass through their ctx (or derive one) so cancellation and structured logging propagate correctly.docs/implementation-plan-exclude-patterns.md (5)
54-60: Clarify matching order and literal-vs-pattern precedence.The statement "Every
--excludeentry is treated as: 1. literal path match... 2. glob pattern match" is ambiguous. Does this mean:
- Both checks are performed for every rule (literal first, then pattern)?
- Or that each rule is classified as either literal OR pattern?
If both are attempted, what's the performance impact? If a rule contains glob metacharacters, should literal matching be skipped? Clarify the evaluation order and short-circuit behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/implementation-plan-exclude-patterns.md` around lines 54 - 60, Clarify that each --exclude entry is evaluated deterministically: first test for an exact literal path equality (short-circuit and treat as a match if true), and only if the literal check fails then evaluate as a glob pattern match; explicitly note that entries containing glob metacharacters still undergo the literal check first but that implementations MAY skip the literal check for strings that are explicitly flagged/parsed as patterns to avoid extra cost, and document that a successful literal match prevents any subsequent glob evaluation (short-circuit) and that directory literal matches exclude all descendants.
43-67: Add cross-platform and edge-case considerations.The proposed behavior should explicitly address:
- Case sensitivity: Should pattern matching be case-sensitive? This varies by platform (Windows is case-insensitive, Unix is case-sensitive). Specify the intended behavior or document that it follows the underlying filesystem.
- Symbolic links: How should symbolic links be handled? Should they be followed for matching, or matched by their link name?
- Absolute paths: What happens if a user provides an absolute path in a pattern (e.g.,
/home/user/project/file.txt)? Should these be rejected, normalized, or supported?- Directory matching scope (line 59): Explicitly state whether "directory match excludes all descendants" applies to both literal matches (e.g.,
node_modules) and pattern matches (e.g.,**/tmp).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/implementation-plan-exclude-patterns.md` around lines 43 - 67, Update the "Proposed behavior" section to explicitly cover cross-platform and edge cases: state the intended case-sensitivity behavior (e.g., normalize decision or "follows underlying filesystem"), define symbolic link handling (e.g., whether matching follows symlinks or matches the link name), specify how absolute paths in patterns are treated (reject, normalize to source-root-relative, or support as-is), and clarify that the rule "Directory match excludes all descendants" in "Matching rules" applies to both literal matches and glob pattern matches (give examples like `node_modules` and `**/tmp` to illustrate). Ensure these clarifications appear alongside the existing "Pattern syntax" and "Matching rules" text so readers can find them with the same headings and examples (e.g., `--exclude=.git/**`, `--exclude=**/*.tmp`).
106-115: Expand test coverage to include additional edge cases.The minimum test cases are solid. Consider adding:
- Case sensitivity: Test behavior with patterns like
*.TMPvs*.tmpacross platforms.- Malformed patterns: Test validation of invalid patterns (e.g., unbalanced brackets if using extended globs later).
- Broad patterns: Test performance implications of overly broad patterns like
**/*or*.- Empty/whitespace rules: Test behavior with empty strings or whitespace-only exclude rules.
- Boundary cases: Test patterns matching the source root itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/implementation-plan-exclude-patterns.md` around lines 106 - 115, Add additional unit tests around the exclude-pattern matcher to cover edge cases: add tests for case-sensitivity (e.g., patterns like "*.TMP" vs "*.tmp"), malformed patterns (invalid glob syntax), broad patterns ("**/*" and "*" to observe matching and performance), empty or whitespace-only rules, and patterns that match the source root itself; target the existing exclude matcher tests (the test file covering the exclude logic / the function that evaluates excludes e.g., isExcluded or matchesExclude) and assert normalized path handling (Windows separators) and explicit validation errors for invalid patterns where applicable.
150-155: Optionally enhance acceptance criteria with validation tooling.The acceptance criteria are clear and measurable. Consider adding:
- Optional enhancement: "Users can run a
--validate-excludesor--list-excludedcommand to preview which files would be excluded without performing actual sync, reducing risk of unintended exclusions."This would strengthen the UX and make troubleshooting easier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/implementation-plan-exclude-patterns.md` around lines 150 - 155, Add an optional acceptance criterion that describes a dry-run/preview validation tool (e.g., a CLI flag like --validate-excludes or --list-excluded) that allows users to preview which files would be excluded without performing create/update/delete actions; update the "Acceptance criteria" section to include this bullet, clarify it is optional/enhancement, and ensure it specifies expected behavior (preview output, non-destructive, usable with glob and literal excludes).
137-147: Consider additional risks for completeness.The identified risks are good. Consider adding:
Unintended critical file exclusion: Users might write overly broad patterns (e.g.,
*.log) that accidentally exclude important files.
- Mitigation: Provide a
--dry-runor--list-excludedflag to preview what would be excluded before actual sync.Literal filenames with glob metacharacters: If a user has a file literally named
test[1].txtand wants to exclude it, the current plan treats all rules as potential patterns. A ruletest[1].txtmight be interpreted as a character class pattern rather than a literal filename.
- Mitigation: Document escaping mechanism (if supported by doublestar) or recommend a prefix syntax like
literal:to force literal matching.Production rollback: If severe issues arise post-release, what's the rollback strategy?
- Mitigation: Consider a feature flag or environment variable to temporarily disable pattern matching and fall back to literal-only mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/implementation-plan-exclude-patterns.md` around lines 137 - 147, Add three additional risk items under the "Risks and mitigations" section: 4) Unintended critical file exclusion — advise adding a preview flag like --dry-run or --list-excluded to show what would be excluded before syncing; 5) Literal filenames with glob metacharacters — document escaping rules for doublestar or introduce/describe a literal: prefix to force literal matching; and 6) Production rollback — specify a rollback strategy such as a feature flag or environment variable to disable pattern matching and fall back to literal-only mode in emergencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 6: Update the doublestar dependency from v4.8.1 to v4.10.0 in go.mod by
changing the module version string (the line containing
"github.com/bmatcuk/doublestar/v4 v4.8.1"), then run `go get
github.com/bmatcuk/doublestar/v4@v4.10.0` followed by `go mod tidy` to refresh
the lockfile and verify module resolution; finally run the test suite/build to
ensure no breaking changes affected code that uses doublestar functions.
In `@internal/exclude/matcher_test.go`:
- Around line 87-95: The test loop contains a redundant per-iteration copy `tt
:= tt` causing a copyloopvar lint; remove that line and use the ranged variable
`tt` directly in the t.Run closure, leaving the call to matcher.Match(tt.path,
false) and the t.Fatalf unchanged so each subtest uses the correct tt values
(i.e., update the loop that iterates over tests and the t.Run anonymous function
to drop the extra `tt := tt`).
- Around line 39-47: The test loop contains a redundant loop-variable copy `tt
:= tt` inside the range over `tests`; remove that assignment so each subtest
uses the iteration-scoped `tt` directly. Update the for loop that ranges over
`tests` and its t.Run closure (which calls matcher.Match) by deleting the `tt :=
tt` line to satisfy Go 1.22+ semantics and the linter warning.
In `@internal/syncer/syncer.go`:
- Around line 110-111: Replace path.Join with filepath.Join when constructing
childAbsPath and childRelPath (these variables are created using absPath and
relPath) so filesystem separators follow the OS semantics; update the same
usages of path.Join elsewhere in this function (the later construction of
sibling/descendant paths around where childAbsPath/childRelPath are set) to
filepath.Join to avoid mixing path and filepath operations.
In `@README.md`:
- Around line 217-222: The "Proposed next implementation" heading and its text
now conflict with the checklist state showing the `--exclude` patterns feature
as completed; update the README's "Proposed next implementation" section
(heading and the bullet referencing
`docs/implementation-plan-exclude-patterns.md`) to either (a) mark it as
completed and move or rename the section to something like "Completed: Support
for patterns in the `--exclude` option" with a brief status note, or (b) remove
the section entirely to avoid duplication; ensure you reference the same heading
text "Proposed next implementation" and the linked file
`docs/implementation-plan-exclude-patterns.md` when making the change.
---
Nitpick comments:
In `@docs/implementation-plan-exclude-patterns.md`:
- Around line 54-60: Clarify that each --exclude entry is evaluated
deterministically: first test for an exact literal path equality (short-circuit
and treat as a match if true), and only if the literal check fails then evaluate
as a glob pattern match; explicitly note that entries containing glob
metacharacters still undergo the literal check first but that implementations
MAY skip the literal check for strings that are explicitly flagged/parsed as
patterns to avoid extra cost, and document that a successful literal match
prevents any subsequent glob evaluation (short-circuit) and that directory
literal matches exclude all descendants.
- Around line 43-67: Update the "Proposed behavior" section to explicitly cover
cross-platform and edge cases: state the intended case-sensitivity behavior
(e.g., normalize decision or "follows underlying filesystem"), define symbolic
link handling (e.g., whether matching follows symlinks or matches the link
name), specify how absolute paths in patterns are treated (reject, normalize to
source-root-relative, or support as-is), and clarify that the rule "Directory
match excludes all descendants" in "Matching rules" applies to both literal
matches and glob pattern matches (give examples like `node_modules` and `**/tmp`
to illustrate). Ensure these clarifications appear alongside the existing
"Pattern syntax" and "Matching rules" text so readers can find them with the
same headings and examples (e.g., `--exclude=.git/**`, `--exclude=**/*.tmp`).
- Around line 106-115: Add additional unit tests around the exclude-pattern
matcher to cover edge cases: add tests for case-sensitivity (e.g., patterns like
"*.TMP" vs "*.tmp"), malformed patterns (invalid glob syntax), broad patterns
("**/*" and "*" to observe matching and performance), empty or whitespace-only
rules, and patterns that match the source root itself; target the existing
exclude matcher tests (the test file covering the exclude logic / the function
that evaluates excludes e.g., isExcluded or matchesExclude) and assert
normalized path handling (Windows separators) and explicit validation errors for
invalid patterns where applicable.
- Around line 150-155: Add an optional acceptance criterion that describes a
dry-run/preview validation tool (e.g., a CLI flag like --validate-excludes or
--list-excluded) that allows users to preview which files would be excluded
without performing create/update/delete actions; update the "Acceptance
criteria" section to include this bullet, clarify it is optional/enhancement,
and ensure it specifies expected behavior (preview output, non-destructive,
usable with glob and literal excludes).
- Around line 137-147: Add three additional risk items under the "Risks and
mitigations" section: 4) Unintended critical file exclusion — advise adding a
preview flag like --dry-run or --list-excluded to show what would be excluded
before syncing; 5) Literal filenames with glob metacharacters — document
escaping rules for doublestar or introduce/describe a literal: prefix to force
literal matching; and 6) Production rollback — specify a rollback strategy such
as a feature flag or environment variable to disable pattern matching and fall
back to literal-only mode in emergencies.
In `@internal/exclude/matcher.go`:
- Line 61: The MatchRule method currently declares an unused isDir parameter
(signature: Matcher.MatchRule(filePath string, _ bool) (bool, string)); remove
the unused boolean parameter from the method signature (change to
Matcher.MatchRule(filePath string) (bool, string)) and update all callers and
any tests to use the new signature, or alternatively implement directory-aware
logic inside Matcher.MatchRule to respect patterns ending with '/' by using the
isDir value — pick one approach consistently and adjust references to the
Matcher.MatchRule symbol accordingly.
- Around line 12-16: The rule struct has redundant fields literal and pattern
that are always the same; replace them with a single field (e.g., value string)
and keep isPattern bool to indicate whether value is a glob/pattern or a
literal, then update every place that constructs or reads rule.literal or
rule.pattern (e.g., parsers/constructors and matchers) to use rule.value instead
and preserve the existing isPattern semantics so matching logic in functions
that reference rule.literal/pattern continues to work correctly.
- Around line 125-127: The hasMeta function currently checks only for '*' and
'?' via strings.ContainsAny(pattern, "*?") and intentionally treats bracket
expressions like '[' and ']' as literal characters; add a brief comment above
hasMeta explaining this design decision (i.e., that '[' and ']' are
intentionally omitted so character classes are not considered meta) and
reference the function name hasMeta and the pattern parameter so future
maintainers understand why bracket expressions are not treated as meta patterns.
In `@internal/syncer/syncer.go`:
- Around line 159-169: The isExcluded method redundantly calls
s.matcher.MatchRule twice; simplify by only using the absolute path once. Keep
the nil check for s.matcher, then call s.matcher.MatchRule(absPath, isDir) and
return its (matched, rule) result directly instead of calling
MatchRule(relPath...). Update the function body in Syncer.isExcluded to remove
the fallback call and return the single MatchRule result.
In `@internal/watcher/watcher.go`:
- Around line 227-230: The call to context.Background() in addRecursive loses
cancellation and logging context; change the signature of Watcher.addRecursive
to accept a context (e.g., func (w *Watcher) addRecursive(ctx context.Context,
path string) error) and use that ctx when calling w.isExcluded and any other
context-using helpers, then update all callers of addRecursive to pass through
their ctx (or derive one) so cancellation and structured logging propagate
correctly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e909d99-0b8d-4891-a306-715d1d6e90af
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
README.mddocs/implementation-plan-exclude-patterns.mdgo.modinternal/cli/commands/sync/sync.gointernal/exclude/matcher.gointernal/exclude/matcher_test.gointernal/syncer/syncer.gointernal/watcher/watcher.gomain.go
capcom6
force-pushed
the
codex/plan-implementation-for-roadmap-feature
branch
from
241ece7 to
f399f7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exclude/matcher.go`:
- Around line 23-33: The New function currently calls normalizePath on each raw
rule which strips leading slashes and misinterprets absolute rules; update New
to detect absolute rules (using filepath.IsAbs) and handle them explicitly: if a
rule is absolute, convert it to a path relative to absSourceRoot (use
filepath.Rel(absSourceRoot, raw)) and reject with an error if the absolute rule
is outside sourceRoot, then pass the resulting relative path into normalizePath;
alternatively, if absolute rules should be unsupported, return a clear error
from New when filepath.IsAbs(raw) is true instead of silently normalizing them.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0262868-53d4-40d1-8da6-644379d53485
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
README.mdgo.modinternal/cli/commands/sync/sync.gointernal/exclude/matcher.gointernal/exclude/matcher_test.gointernal/syncer/syncer.gointernal/watcher/watcher.gomain.go
✅ Files skipped from review due to trivial changes (1)
- main.go
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- go.mod
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/exclude/matcher.go (3)
79-93: Minor: avoid building the ancestor list when no pattern rules exist, and skip the literal-prefix check for pattern rules.Two small tweaks on the hot path (every sync/watch event calls this):
pathCandidateswalks the path for every call, even when all rules are literal.- For a pattern rule like
**/*.tmp, thenormalized == r.literal || HasPrefix(normalized, r.literal+"/")check on line 81 can never succeed and is pure overhead.Proposed tweak
- ancestors := pathCandidates(normalized) - for _, r := range m.rules { - if normalized == r.literal || strings.HasPrefix(normalized, r.literal+"/") { - return true, r.literal - } - - if r.isPattern { - for _, candidate := range ancestors { - matched, err := doublestar.Match(r.pattern, candidate) - if err == nil && matched { - return true, r.pattern - } - } - } - } + var ancestors []string + for _, r := range m.rules { + if !r.isPattern { + if normalized == r.literal || strings.HasPrefix(normalized, r.literal+"/") { + return true, r.literal + } + continue + } + if ancestors == nil { + ancestors = pathCandidates(normalized) + } + for _, candidate := range ancestors { + if matched, err := doublestar.Match(r.pattern, candidate); err == nil && matched { + return true, r.pattern + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 79 - 93, The pathCandidates list is built unconditionally and the literal-prefix check runs for pattern rules; change the logic in the match loop to: first, detect whether any m.rules have r.isPattern and only call pathCandidates(normalized) when at least one pattern exists; second, for each rule skip the literal equality/prefix check when r.isPattern is true (only perform normalized == r.literal || strings.HasPrefix(normalized, r.literal+"/") for non-pattern rules), and keep the existing doublestar.Match loop for pattern rules (returning r.pattern on match) so literal rules still return r.literal immediately.
12-16: Redundantliteral/patternfields onrule.Both fields are always assigned the same
normalizedvalue, sorulecarries duplicated state. A singlevalue string(plusisPattern) keeps the struct smaller and the intent clearer.Proposed simplification
type rule struct { - literal string - pattern string - isPattern bool + value string + isPattern bool } @@ - compiled = append(compiled, rule{ - literal: normalized, - pattern: normalized, - isPattern: isPattern, - }) + compiled = append(compiled, rule{ + value: normalized, + isPattern: isPattern, + })Update
MatchRuleaccordingly (r.valuein both the literal-prefix check and thedoublestar.Matchcall).Also applies to: 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 12 - 16, The rule struct currently duplicates state with literal and pattern both set to the same normalized value; replace these two fields with a single value string plus the existing isPattern bool, update all usages (including the MatchRule function) to read r.value for both the literal-prefix check and the doublestar.Match call, and remove the redundant fields and any code that sets both literal and pattern so the struct and related logic only store and use r.value and r.isPattern.
73-73:isDirparameter is accepted but ignored.Both
MatchandMatchRuletake anisDir bool, callers ininternal/syncer/syncer.goandinternal/watcher/watcher.gopass it, but the implementation discards it via_ bool. Either:
- drop the parameter from the public API to avoid misleading callers into thinking directory-specific rules (e.g., trailing-slash rules à la
.gitignore) are honored, or- actually use it — e.g., only allow a rule ending in
/to match whenisDiris true, which also restores the "match directories only" semantics lost tofilepath.Cleanon line 31.Given the watcher currently hard-codes
falseon line 261 ofinternal/watcher/watcher.go, whichever path you pick, the caller contract should be made explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` at line 73, Matcher.Match and Matcher.MatchRule currently accept an isDir bool but ignore it (signature uses `_ bool`), so either remove the parameter from both APIs or honor it; to fix, decide on one approach and make caller contract explicit: either remove isDir from Matcher.Match and Matcher.MatchRule signatures and update all callers to stop passing it, or implement directory-only matching (e.g., treat rules ending with '/' as matching only when isDir is true and preserve other semantics lost by filepath.Clean) by using the passed isDir in both Matcher.Match and Matcher.MatchRule and update the caller that currently passes a hard-coded false to pass the actual directory flag. Ensure you update both methods (Matcher.Match, Matcher.MatchRule) and all callers to be consistent with the chosen contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exclude/matcher.go`:
- Line 31: The code uses filepath.Clean on strings.TrimSpace(raw) which silently
removes a trailing slash and thus loses directory-only intent; preserve that
intent by detecting whether the original raw string (before TrimSpace/Clean)
ends with a path separator (e.g. strings.HasSuffix(raw, "/") or
os.PathSeparator) and carry that flag forward instead of letting filepath.Clean
drop it; update the logic around the cleanedRaw assignment (the
filepath.Clean(strings.TrimSpace(raw)) expression) and downstream handling that
currently ignores the isDir parameter (see the comment near line 73) so that
directory-only matches are honored when the original pattern had a trailing
slash, and ensure any canonicalization still uses cleanedRaw for normalization
while relying on the new flag for directory-only semantics.
---
Nitpick comments:
In `@internal/exclude/matcher.go`:
- Around line 79-93: The pathCandidates list is built unconditionally and the
literal-prefix check runs for pattern rules; change the logic in the match loop
to: first, detect whether any m.rules have r.isPattern and only call
pathCandidates(normalized) when at least one pattern exists; second, for each
rule skip the literal equality/prefix check when r.isPattern is true (only
perform normalized == r.literal || strings.HasPrefix(normalized, r.literal+"/")
for non-pattern rules), and keep the existing doublestar.Match loop for pattern
rules (returning r.pattern on match) so literal rules still return r.literal
immediately.
- Around line 12-16: The rule struct currently duplicates state with literal and
pattern both set to the same normalized value; replace these two fields with a
single value string plus the existing isPattern bool, update all usages
(including the MatchRule function) to read r.value for both the literal-prefix
check and the doublestar.Match call, and remove the redundant fields and any
code that sets both literal and pattern so the struct and related logic only
store and use r.value and r.isPattern.
- Line 73: Matcher.Match and Matcher.MatchRule currently accept an isDir bool
but ignore it (signature uses `_ bool`), so either remove the parameter from
both APIs or honor it; to fix, decide on one approach and make caller contract
explicit: either remove isDir from Matcher.Match and Matcher.MatchRule
signatures and update all callers to stop passing it, or implement
directory-only matching (e.g., treat rules ending with '/' as matching only when
isDir is true and preserve other semantics lost by filepath.Clean) by using the
passed isDir in both Matcher.Match and Matcher.MatchRule and update the caller
that currently passes a hard-coded false to pass the actual directory flag.
Ensure you update both methods (Matcher.Match, Matcher.MatchRule) and all
callers to be consistent with the chosen contract.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ead20beb-99eb-4dfa-93ca-b31c0a4032fe
📒 Files selected for processing (4)
internal/exclude/errors.gointernal/exclude/matcher.gointernal/exclude/matcher_test.gointernal/syncer/syncer.go
✅ Files skipped from review due to trivial changes (1)
- internal/exclude/matcher_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/syncer/syncer.go
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/exclude/matcher.go (1)
147-149:hasMetamisses doublestar's character-class and brace metacharacters.
doublestartreats[,],{,}as glob metacharacters, buthasMetaonly detects*?. A rule likefile[0-9].logor{a,b}/creturns false fromhasMeta, causing the pattern to skip validation and be treated as a literal match—silently failing to match as a glob pattern.♻️ Suggested change
func hasMeta(pattern string) bool { - return strings.ContainsAny(pattern, "*?") + // Mirror the metacharacters recognized by doublestar: *, ?, [..], {..}. + return strings.ContainsAny(pattern, "*?[{") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 147 - 149, hasMeta currently only checks for "*" and "?" using strings.ContainsAny, but doublestar also treats "[", "]", "{", "}" as glob metacharacters; update hasMeta to detect those additional characters so patterns like "file[0-9].log" or "{a,b}/c" are recognized as globs. Modify the function hasMeta to include the bracket and brace characters in the character set checked (keep using strings.ContainsAny or equivalent) so it returns true for any of "*?[]{}". Ensure you update the code paths that call hasMeta (e.g., the glob validation/branching logic) so they will now treat those patterns as globs rather than literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exclude/matcher.go`:
- Line 164: The file internal/exclude/matcher.go is not correctly formatted
(EOF/newline or import grouping) causing golangci-lint to fail; fix by running
goimports -w internal/exclude/matcher.go (or gofmt -s -w
internal/exclude/matcher.go) to normalize imports and add the trailing newline,
then re-run the linter/CI; ensure the final commit includes the formatted
matcher.go changes.
- Around line 85-103: The directory-only guard in the loop over m.rules (in
matcher.go) is too broad: the check "if r.isDir && !isDir { continue }" prevents
descendant literal-prefix matches (e.g., r.literal "node_modules" should exclude
"node_modules/foo.js"); change the logic in the rule iteration so the r.isDir &&
!isDir branch only skips when the current path is an exact match of the rule
(compare normalized == r.literal), but still allow the literal-prefix check
(strings.HasPrefix(normalized, r.literal+"/")) to run for descendants; keep the
existing pattern handling (r.isPattern with doublestar.Match over ancestors)
unchanged.
---
Nitpick comments:
In `@internal/exclude/matcher.go`:
- Around line 147-149: hasMeta currently only checks for "*" and "?" using
strings.ContainsAny, but doublestar also treats "[", "]", "{", "}" as glob
metacharacters; update hasMeta to detect those additional characters so patterns
like "file[0-9].log" or "{a,b}/c" are recognized as globs. Modify the function
hasMeta to include the bracket and brace characters in the character set checked
(keep using strings.ContainsAny or equivalent) so it returns true for any of
"*?[]{}". Ensure you update the code paths that call hasMeta (e.g., the glob
validation/branching logic) so they will now treat those patterns as globs
rather than literals.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b991017-5b05-4c99-b283-f8f676a85632
📒 Files selected for processing (1)
internal/exclude/matcher.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/exclude/matcher.go (1)
29-44:⚠️ Potential issue | 🟠 MajorAbsolute exclude rules still silently misbehave.
filepath.ToSlashpreserves the leading/, so a rule like/repo/dist/**is stored as/repo/dist/**and is then matched against cleaned relative paths likedist/foo.js— it will never match. Either relativize absolute rules againstsourceRootup front (rejecting rules outside the root), or reject absolute rules with a clear error fromNew. Also considerstrings.TrimSpaceand skipping empty entries so accidental blank lines in config don't produce a no-op rule.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 29 - 44, Trim each raw rule (use strings.TrimSpace) and skip empty entries before calling filepath.ToSlash in the code that builds the compiled []rule (the constructor New or the function that creates compiled). After normalizing, detect absolute rules (leading "/") and either relativize them against the package's sourceRoot parameter (rejecting and returning an error if the absolute path is outside sourceRoot) or return a clear ErrInvalidPattern-style error for absolute rules; update the compiled append logic for rule{value: normalized, isPattern: hasMeta(normalized)} accordingly so absolute rules no longer silently fail. Ensure validation still uses doublestar.ValidatePattern and keep references to ErrInvalidPattern, rule, hasMeta, and the New constructor for locating changes.
🧹 Nitpick comments (3)
internal/exclude/matcher.go (1)
36-38: Remove the commented-out**prefixing block.Dead/commented code should not ship — either re-enable it with a rationale or delete it. Leaving it in invites drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 36 - 38, Remove the dead commented block that checks for a "**" prefix (the lines using strings.HasPrefix(normalized, "**") and normalized = "/" + normalized) in internal/exclude/matcher.go; either delete these commented lines entirely so no commented-out logic remains around the normalized variable, or restore them as active code with a short rationale comment if the "**" prefix handling is required (update any related tests in functions referencing normalized accordingly).internal/syncer/syncer.go (2)
101-138: Deduplicate the per-entry exclusion block.
childAbsPath/childRelPathare constructed twice and the exclusion-check + debug-log pattern is copy-pasted across the dir and file branches. Hoisting it above thefile.IsDir()switch tightens the loop and keeps the log/skip behavior consistent. Same pattern also appears inSync(lines 49-55) and could be extracted into a small helper likeskipIfExcluded(ctx, abs, rel string) boolthat returns true after logging.Proposed refactor
for _, file := range files { select { case <-ctx.Done(): return nil default: } - if file.IsDir() { - childAbsPath := filepath.Join(absPath, file.Name()) - childRelPath := filepath.Join(relPath, file.Name()) - if matched, rule := s.isExcluded(childAbsPath, childRelPath); matched { - s.logger.Debug(ctx, "Excluded path skipped", logger.Fields{ - "path": childRelPath, - "rule": rule, - }) - continue - } - - if dErr := s.syncDir(ctx, childAbsPath, childRelPath); dErr != nil { - return dErr - } - - continue - } - childAbsPath := filepath.Join(absPath, file.Name()) childRelPath := filepath.Join(relPath, file.Name()) if matched, rule := s.isExcluded(childAbsPath, childRelPath); matched { s.logger.Debug(ctx, "Excluded path skipped", logger.Fields{ "path": childRelPath, "rule": rule, }) continue } + if file.IsDir() { + if dErr := s.syncDir(ctx, childAbsPath, childRelPath); dErr != nil { + return dErr + } + continue + } + if fErr := s.syncFile(ctx, childAbsPath, childRelPath); fErr != nil { return fErr } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/syncer/syncer.go` around lines 101 - 138, The loop duplicates construction of childAbsPath/childRelPath and the exclusion-check+debug log in both branches; hoist childAbsPath := filepath.Join(absPath, file.Name()) and childRelPath := filepath.Join(relPath, file.Name()) to the top of the per-file loop, call s.isExcluded(childAbsPath, childRelPath) once and continue if matched (logging the rule), and then call s.syncDir(ctx, childAbsPath, childRelPath) or s.syncFile(ctx, childAbsPath, childRelPath) based on file.IsDir(); also extract the same pattern used in Sync into a small helper like skipIfExcluded(ctx, abs, rel string) bool that calls s.isExcluded and s.logger.Debug so both syncer.go loop and Sync use that helper to avoid duplication.
160-170: TherelPathfallback is likely redundant.
MatchRulealready convertsabsPathto a path relative tosourceRootviafilepath.Relinternally, so tryingabsPathfirst and thenrelPathdouble-normalizes. If the absolute-path call succeeds in relativization, the second call just repeats the same work; if it fails (path outside source), retrying withrelPathwon't produce a different match. Consider passing onlyabsPathand dropping the fallback — or, if the fallback exists to paper over the Windows separator bug flagged inmatcher.go, fixing that root cause is preferable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/syncer/syncer.go` around lines 160 - 170, The fallback to calling s.matcher.MatchRule(relPath) in Syncer.isExcluded is redundant because MatchRule already relativizes absPath; remove the second call and have isExcluded only call and return the result of s.matcher.MatchRule(absPath) (i.e., change isExcluded to call MatchRule(absPath) and return that bool/string), remove the now-unused relPath parameter from isExcluded and update all callers accordingly; if the fallback was intended to address the Windows-separator issue in matcher.go, instead fix that normalization in matcher.MatchRule so relPath fallback is unnecessary, then remove the fallback call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exclude/matcher.go`:
- Around line 3-11: Replace the stdlib log usage and import with the injected
logger by adding a logger.Logger parameter to New (the matcher constructor used
by the watcher/syncer) and store it on the matcher struct, remove the "log"
import from internal/exclude/matcher.go, and change the noisy log.Printf calls
in MatchRule (and any other places) to use the stored logger at Debug level (or
remove them entirely) so linting passes and structured logging is used
consistently.
- Around line 57-67: The MatchRule function uses filepath.ToSlash on filePath
before calling filepath.Rel, causing separator mismatch with m.sourceRoot on
Windows; fix by first computing relPath := filepath.Rel(m.sourceRoot, filePath)
(use original OS separators), only if err == nil set normalized = relPath, then
convert normalized to forward slashes via filepath.ToSlash and then call
path.Clean; update the logic in Matcher.MatchRule (references:
Matcher.MatchRule, m.sourceRoot, normalized, filepath.Rel, filepath.ToSlash,
path.Clean) so relative paths are computed with OS separators and normalized
afterwards.
---
Duplicate comments:
In `@internal/exclude/matcher.go`:
- Around line 29-44: Trim each raw rule (use strings.TrimSpace) and skip empty
entries before calling filepath.ToSlash in the code that builds the compiled
[]rule (the constructor New or the function that creates compiled). After
normalizing, detect absolute rules (leading "/") and either relativize them
against the package's sourceRoot parameter (rejecting and returning an error if
the absolute path is outside sourceRoot) or return a clear
ErrInvalidPattern-style error for absolute rules; update the compiled append
logic for rule{value: normalized, isPattern: hasMeta(normalized)} accordingly so
absolute rules no longer silently fail. Ensure validation still uses
doublestar.ValidatePattern and keep references to ErrInvalidPattern, rule,
hasMeta, and the New constructor for locating changes.
---
Nitpick comments:
In `@internal/exclude/matcher.go`:
- Around line 36-38: Remove the dead commented block that checks for a "**"
prefix (the lines using strings.HasPrefix(normalized, "**") and normalized = "/"
+ normalized) in internal/exclude/matcher.go; either delete these commented
lines entirely so no commented-out logic remains around the normalized variable,
or restore them as active code with a short rationale comment if the "**" prefix
handling is required (update any related tests in functions referencing
normalized accordingly).
In `@internal/syncer/syncer.go`:
- Around line 101-138: The loop duplicates construction of
childAbsPath/childRelPath and the exclusion-check+debug log in both branches;
hoist childAbsPath := filepath.Join(absPath, file.Name()) and childRelPath :=
filepath.Join(relPath, file.Name()) to the top of the per-file loop, call
s.isExcluded(childAbsPath, childRelPath) once and continue if matched (logging
the rule), and then call s.syncDir(ctx, childAbsPath, childRelPath) or
s.syncFile(ctx, childAbsPath, childRelPath) based on file.IsDir(); also extract
the same pattern used in Sync into a small helper like skipIfExcluded(ctx, abs,
rel string) bool that calls s.isExcluded and s.logger.Debug so both syncer.go
loop and Sync use that helper to avoid duplication.
- Around line 160-170: The fallback to calling s.matcher.MatchRule(relPath) in
Syncer.isExcluded is redundant because MatchRule already relativizes absPath;
remove the second call and have isExcluded only call and return the result of
s.matcher.MatchRule(absPath) (i.e., change isExcluded to call MatchRule(absPath)
and return that bool/string), remove the now-unused relPath parameter from
isExcluded and update all callers accordingly; if the fallback was intended to
address the Windows-separator issue in matcher.go, instead fix that
normalization in matcher.MatchRule so relPath fallback is unnecessary, then
remove the fallback call.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02fba150-69a4-4cdb-8413-71045c8dc19f
📒 Files selected for processing (4)
internal/exclude/matcher.gointernal/exclude/matcher_test.gointernal/syncer/syncer.gointernal/watcher/watcher.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exclude/matcher_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/syncer/syncer.go (1)
33-55:⚠️ Potential issue | 🟡 MinorCheck exclusions before statting the path.
Synccurrently callsfsInfobeforeisExcluded, so ignored paths can still fail the sync withos.Staterrors such as permission errors. SincerelPathcan be computed without the file existing, move the exclude check beforefsInfoto make excluded paths a true early return.🐛 Proposed fix
func (s *Syncer) Sync(ctx context.Context, absPath string) error { - exists, isDir, err := fsInfo(absPath) - if err != nil { - return fmt.Errorf("fsInfo: %w", err) - } - absRoot, err := filepath.Abs(s.rootPath) if err != nil { return fmt.Errorf("filepath.Abs: %w", err) } relPath, err := filepath.Rel(absRoot, absPath) if err != nil { return fmt.Errorf("filepath.Rel: %w", err) } if matched, rule := s.isExcluded(absPath); matched { s.logger.Debug(ctx, "Excluded path skipped", logger.Fields{ "path": relPath, "rule": rule, }) return nil } + + exists, isDir, err := fsInfo(absPath) + if err != nil { + return fmt.Errorf("fsInfo: %w", err) + } if !exists {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/syncer/syncer.go` around lines 33 - 55, In Sync, move the exclusion check to before any filesystem stat: compute absRoot = filepath.Abs(s.rootPath) and relPath = filepath.Rel(absRoot, absPath) first (keep the existing error handling for filepath.Abs and filepath.Rel), then call s.isExcluded(absPath) and if matched log using relPath and return early; only after that call fsInfo(absPath) to get exists/isDir/err. This ensures isExcluded runs before fsInfo and avoided os.Stat permission errors for excluded paths while preserving the existing log and error behavior.
♻️ Duplicate comments (1)
internal/exclude/matcher.go (1)
29-42:⚠️ Potential issue | 🟠 MajorNormalize rules the same way as candidate paths.
Rules are stored nearly raw, while candidates are cleaned and relativized before matching. This makes valid excludes like
./dist/**or absolute rules undersourceRootsilently miss because they are compared againstdist/...candidates. Handle absolute rules relative toabsSourceRoot, trim empty input, and clean slash-normalized relative rules before validation/compilation.🐛 Proposed fix
compiled := make([]rule, 0, len(rules)) for _, raw := range rules { - normalized := filepath.ToSlash(raw) + candidate := strings.TrimSpace(raw) + if candidate == "" { + continue + } + + if filepath.IsAbs(candidate) { + rel, err := filepath.Rel(absSourceRoot, candidate) + if err != nil { + return nil, fmt.Errorf("failed to relativize exclude rule %q: %w", raw, err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return nil, fmt.Errorf("%w: exclude rule %q is outside source root", ErrInvalidPattern, raw) + } + candidate = rel + } + + normalized := path.Clean(filepath.ToSlash(candidate)) if !doublestar.ValidatePattern(normalized) { return nil, fmt.Errorf("%w: exclude rule %q is invalid", ErrInvalidPattern, raw) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 29 - 42, The loop that compiles exclude rules currently uses the nearly-raw value `raw` and only applies filepath.ToSlash, causing mismatches with cleaned/relativized candidate paths; update the compilation in the function that iterates `for _, raw := range rules` to first trim whitespace/empty inputs, convert absolute rules to paths relative to `absSourceRoot` (when `raw` is abs and `absSourceRoot` is available), then run filepath.Clean + filepath.ToSlash on the resulting relative path before calling doublestar.ValidatePattern and appending the `rule{ value: ..., isPattern: hasMeta(...) }`; ensure validation uses the cleaned/normalized value and preserve existing ErrInvalidPattern error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/syncer/syncer.go`:
- Around line 33-55: In Sync, move the exclusion check to before any filesystem
stat: compute absRoot = filepath.Abs(s.rootPath) and relPath =
filepath.Rel(absRoot, absPath) first (keep the existing error handling for
filepath.Abs and filepath.Rel), then call s.isExcluded(absPath) and if matched
log using relPath and return early; only after that call fsInfo(absPath) to get
exists/isDir/err. This ensures isExcluded runs before fsInfo and avoided os.Stat
permission errors for excluded paths while preserving the existing log and error
behavior.
---
Duplicate comments:
In `@internal/exclude/matcher.go`:
- Around line 29-42: The loop that compiles exclude rules currently uses the
nearly-raw value `raw` and only applies filepath.ToSlash, causing mismatches
with cleaned/relativized candidate paths; update the compilation in the function
that iterates `for _, raw := range rules` to first trim whitespace/empty inputs,
convert absolute rules to paths relative to `absSourceRoot` (when `raw` is abs
and `absSourceRoot` is available), then run filepath.Clean + filepath.ToSlash on
the resulting relative path before calling doublestar.ValidatePattern and
appending the `rule{ value: ..., isPattern: hasMeta(...) }`; ensure validation
uses the cleaned/normalized value and preserve existing ErrInvalidPattern error
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae73e918-c266-4b33-a124-88a0e7ad1c38
📒 Files selected for processing (2)
internal/exclude/matcher.gointernal/syncer/syncer.go
capcom6
force-pushed
the
codex/plan-implementation-for-roadmap-feature
branch
from
23f2201 to
0228f7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/exclude/matcher.go (2)
29-42:⚠️ Potential issue | 🟠 MajorNormalize rules with the same pipeline used for candidates.
Line 30 only converts separators, while candidates are cleaned on Line 63. Rules like
./dist/**,dist//**, ornode_modules/won’t reliably match their cleaned candidate paths. Trim and clean rules before storing them; if trailing slash means “directory-only”, preserve that intent separately.Proposed normalization adjustment
for _, raw := range rules { - normalized := filepath.ToSlash(raw) + trimmed := strings.TrimSpace(raw) + if trimmed == "" { + continue + } + + normalized := path.Clean(filepath.ToSlash(trimmed)) + if normalized == "." { + continue + } if !doublestar.ValidatePattern(normalized) { return nil, fmt.Errorf("%w: exclude rule %q is invalid", ErrInvalidPattern, raw) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 29 - 42, The exclude rule normalization currently only converts path separators (variable normalized) but must mirror the candidate normalization used later (line ~63) by trimming and filepath.Clean-ing each raw rule before validation and storage; update the loop that builds compiled (and the rule struct usage) to compute a cleaned value := filepath.Clean(strings.TrimSpace(raw)) (while detecting and preserving a trailing-slash intent into a new boolean like isDirOnly if raw ended with a slash) then validate doublestar.ValidatePattern on the cleaned form, compute isPattern via hasMeta(cleaned), and append rule{value: cleaned, isPattern: ..., /* preserve isDirOnly */} so patterns like ./dist/**, dist//**, or node_modules/ match the cleaned candidates.
29-42:⚠️ Potential issue | 🟠 MajorRelativize or reject absolute exclude rules before compiling them.
MatchRuleconverts absolute candidate paths to paths relative tosourceRoot, butNewstores absolute rules as-is. An exclude like/repo/dist/**will be compared againstdist/bundle.jsand miss; outside-root absolute rules should fail fast.Proposed absolute-rule handling
for _, raw := range rules { - normalized := filepath.ToSlash(raw) + trimmed := strings.TrimSpace(raw) + if trimmed == "" { + continue + } + + normalized := filepath.ToSlash(trimmed) + if filepath.IsAbs(trimmed) { + rel, err := filepath.Rel(absSourceRoot, trimmed) + if err != nil { + return nil, fmt.Errorf("%w: exclude rule %q cannot be relativized: %v", ErrInvalidPattern, raw, err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return nil, fmt.Errorf("%w: exclude rule %q is outside source root", ErrInvalidPattern, raw) + } + normalized = filepath.ToSlash(rel) + } + + normalized = path.Clean(normalized) if !doublestar.ValidatePattern(normalized) { return nil, fmt.Errorf("%w: exclude rule %q is invalid", ErrInvalidPattern, raw) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 29 - 42, The code currently accepts absolute exclude rules which will not match because MatchRule compares candidates relative to sourceRoot; update the rule normalization in the New function (the loop that appends rule{...} using hasMeta and value) to detect absolute patterns (e.g., filepath.IsAbs(raw) or strings.HasPrefix(normalized, "/")); for such patterns either (preferably) return an error indicating absolute exclude rules are not allowed (ErrInvalidPattern-style) or else convert them to a relative pattern by stripping the leading "/" (and handling Windows drive prefixes) before constructing the rule so MatchRule will match correctly; ensure to reference and preserve hasMeta(normalized) and the rule.value assignment when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/exclude/matcher.go`:
- Around line 100-102: The hasMeta function doesn't detect character class
syntax (e.g., "[ab]") so such patterns aren't routed to doublestar.Match; update
hasMeta to also treat '[' as a meta-character (e.g., change the detection from
strings.ContainsAny(pattern, "*?{") to include '[' or explicitly check for '[')
so patterns like "logs/[ab].log" are considered globs and forwarded to
doublestar.Match; keep the function name hasMeta and ensure no other behavior
changes.
- Around line 57-63: The current relativization of absolute filePath using
filepath.Rel in the matcher (candidate := filePath; if filepath.IsAbs(candidate)
{ if rel, err := filepath.Rel(m.sourceRoot, candidate); err == nil { candidate =
rel } }) can produce "../" paths when the target is outside m.sourceRoot; after
filepath.Rel and before assigning candidate (and again after computing
normalized via path.Clean(filepath.ToSlash(candidate))), add a guard that
rejects/returns early when rel or normalized begins with ".." or contains "../"
(or equals ".."), ensuring files outside m.sourceRoot are not accepted for
pattern matching by methods that consume candidate/normalized in this matcher.
---
Duplicate comments:
In `@internal/exclude/matcher.go`:
- Around line 29-42: The exclude rule normalization currently only converts path
separators (variable normalized) but must mirror the candidate normalization
used later (line ~63) by trimming and filepath.Clean-ing each raw rule before
validation and storage; update the loop that builds compiled (and the rule
struct usage) to compute a cleaned value :=
filepath.Clean(strings.TrimSpace(raw)) (while detecting and preserving a
trailing-slash intent into a new boolean like isDirOnly if raw ended with a
slash) then validate doublestar.ValidatePattern on the cleaned form, compute
isPattern via hasMeta(cleaned), and append rule{value: cleaned, isPattern: ...,
/* preserve isDirOnly */} so patterns like ./dist/**, dist//**, or node_modules/
match the cleaned candidates.
- Around line 29-42: The code currently accepts absolute exclude rules which
will not match because MatchRule compares candidates relative to sourceRoot;
update the rule normalization in the New function (the loop that appends
rule{...} using hasMeta and value) to detect absolute patterns (e.g.,
filepath.IsAbs(raw) or strings.HasPrefix(normalized, "/")); for such patterns
either (preferably) return an error indicating absolute exclude rules are not
allowed (ErrInvalidPattern-style) or else convert them to a relative pattern by
stripping the leading "/" (and handling Windows drive prefixes) before
constructing the rule so MatchRule will match correctly; ensure to reference and
preserve hasMeta(normalized) and the rule.value assignment when making this
change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 641dd5f3-04b9-4d8e-82ea-b48454754fa0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
README.mdgo.modinternal/cli/commands/sync/sync.gointernal/exclude/errors.gointernal/exclude/matcher.gointernal/exclude/matcher_test.gointernal/syncer/syncer.gointernal/watcher/watcher.gomain.go
✅ Files skipped from review due to trivial changes (4)
- internal/exclude/errors.go
- main.go
- go.mod
- internal/exclude/matcher_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- internal/cli/commands/sync/sync.go
- internal/syncer/syncer.go
There was a problem hiding this comment.
♻️ Duplicate comments (3)
internal/exclude/matcher.go (3)
93-95:⚠️ Potential issue | 🟠 MajorRecognize bracket character classes as glob meta.
Doublestar supports bracket character classes, but
hasMetadoes not treat[as a meta character. Patterns likelogs/[ab].logare compiled as literals and will not matchlogs/a.log.Proposed fix
func hasMeta(pattern string) bool { - return strings.ContainsAny(pattern, "*?{") + return strings.ContainsAny(pattern, "*?{[") }github.com/bmatcuk/doublestar/v4 character class glob syntax🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 93 - 95, The hasMeta function currently only checks "*?{" and misses bracket character classes; update hasMeta to treat '[' as a meta character (e.g., include '[' in the strings.ContainsAny call used in hasMeta) so patterns like "logs/[ab].log" are recognized as globs; update any related tests to cover bracket class patterns if present.
29-38:⚠️ Potential issue | 🟠 MajorCanonicalize raw rules before storing them.
Rules are stored as only slash-normalized, but candidates are later cleaned and usually relativized. This breaks common inputs like
./build,build//out, andnode_modules/because the prefix check becomesnode_modules//.... Absolute rules also need to be relativized againstsourceRootor rejected explicitly.Proposed canonicalization
compiled := make([]rule, 0, len(rules)) for _, raw := range rules { - normalized := filepath.ToSlash(raw) + raw = strings.TrimSpace(raw) + if raw == "" { + continue + } + + cleaned := filepath.Clean(raw) + normalized := filepath.ToSlash(cleaned) + if filepath.IsAbs(cleaned) { + rel, err := filepath.Rel(absSourceRoot, cleaned) + if err != nil { + return nil, fmt.Errorf("failed to relativize exclude rule %q: %w", raw, err) + } + rel = filepath.Clean(rel) + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return nil, fmt.Errorf("exclude rule %q is outside source root", raw) + } + normalized = filepath.ToSlash(rel) + } + normalized = path.Clean(normalized) if !doublestar.ValidatePattern(normalized) { return nil, fmt.Errorf("%w: exclude rule %q is invalid", ErrInvalidPattern, raw) }Also applies to: 61-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 29 - 38, The loop that compiles exclude rules stores only slash-normalized values so inputs like "./build", "build//out", "node_modules/" and absolute paths don't canonicalize to the same form as later cleaned/relativized candidates; update the compile logic (inside the loop handling raw → normalized and where rule{...} is appended) to first run filepath.Clean(raw), strip any trailing "/" (except keep "." as ""), convert to slash form with filepath.ToSlash, reject or relativize absolute paths against sourceRoot (or return ErrInvalidPattern if not allowed), then validate with doublestar.ValidatePattern on that canonical form and set rule.value to the canonicalized value so prefix checks align with cleaned/relativized candidates (refer to variables/functions: raw, normalized, compiled, rule{}, hasMeta, doublestar.ValidatePattern, ErrInvalidPattern, sourceRoot).
52-60:⚠️ Potential issue | 🔴 CriticalReject absolute candidates outside
sourceRoot.
filepath.Relcan return../...for paths outside the source root. The current code accepts that result, so broad rules like**can match paths that should be outside the matcher boundary.Proposed root-boundary guard
func (m *Matcher) MatchRule(filePath string) (bool, string) { candidate := filePath if filepath.IsAbs(candidate) { - if rel, err := filepath.Rel(m.sourceRoot, candidate); err == nil { - candidate = rel + rel, err := filepath.Rel(m.sourceRoot, candidate) + if err != nil { + return false, "" + } + rel = filepath.Clean(rel) + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return false, "" } + candidate = rel } normalized := path.Clean(filepath.ToSlash(candidate)) + if normalized == ".." || strings.HasPrefix(normalized, "../") { + return false, "" + }Go path/filepath Rel documentation outside base path returns dot-dot components🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 52 - 60, MatchRule currently accepts filepath.Rel results that begin with ".." (paths outside m.sourceRoot), allowing rules like "**" to match outside the boundary; update Matcher.MatchRule to detect when rel indicates going above m.sourceRoot (e.g., rel == ".." or strings.HasPrefix(rel, ".."+string(os.PathSeparator))) and treat that as outside the sourceRoot by rejecting the match (return false, "") or otherwise not using the rel as candidate; reference the MatchRule method, the candidate variable, and m.sourceRoot when making this change and ensure normalized is only computed for valid relative paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/exclude/matcher.go`:
- Around line 93-95: The hasMeta function currently only checks "*?{" and misses
bracket character classes; update hasMeta to treat '[' as a meta character
(e.g., include '[' in the strings.ContainsAny call used in hasMeta) so patterns
like "logs/[ab].log" are recognized as globs; update any related tests to cover
bracket class patterns if present.
- Around line 29-38: The loop that compiles exclude rules stores only
slash-normalized values so inputs like "./build", "build//out", "node_modules/"
and absolute paths don't canonicalize to the same form as later
cleaned/relativized candidates; update the compile logic (inside the loop
handling raw → normalized and where rule{...} is appended) to first run
filepath.Clean(raw), strip any trailing "/" (except keep "." as ""), convert to
slash form with filepath.ToSlash, reject or relativize absolute paths against
sourceRoot (or return ErrInvalidPattern if not allowed), then validate with
doublestar.ValidatePattern on that canonical form and set rule.value to the
canonicalized value so prefix checks align with cleaned/relativized candidates
(refer to variables/functions: raw, normalized, compiled, rule{}, hasMeta,
doublestar.ValidatePattern, ErrInvalidPattern, sourceRoot).
- Around line 52-60: MatchRule currently accepts filepath.Rel results that begin
with ".." (paths outside m.sourceRoot), allowing rules like "**" to match
outside the boundary; update Matcher.MatchRule to detect when rel indicates
going above m.sourceRoot (e.g., rel == ".." or strings.HasPrefix(rel,
".."+string(os.PathSeparator))) and treat that as outside the sourceRoot by
rejecting the match (return false, "") or otherwise not using the rel as
candidate; reference the MatchRule method, the candidate variable, and
m.sourceRoot when making this change and ensure normalized is only computed for
valid relative paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20e1cb99-3e94-47c6-aca6-d749062abce4
📒 Files selected for processing (1)
internal/exclude/matcher.go
capcom6
force-pushed
the
codex/plan-implementation-for-roadmap-feature
branch
from
5d7ddf0 to
ef8c4b4
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
internal/exclude/matcher.go (3)
29-38:⚠️ Potential issue | 🟠 MajorCanonicalize exclude rules before storing them.
Rules are stored almost raw, while candidates are cleaned in
MatchRule. Inputs likedist/,./dist, or absolute rules won’t match consistently; absolute rules should be relativized againstsourceRootor rejected explicitly.Proposed normalization fix
compiled := make([]rule, 0, len(rules)) for _, raw := range rules { - normalized := filepath.ToSlash(raw) + cleanedRaw := strings.TrimSpace(raw) + if cleanedRaw == "" { + continue + } + + if filepath.IsAbs(cleanedRaw) { + rel, err := filepath.Rel(absSourceRoot, filepath.Clean(cleanedRaw)) + if err != nil { + return nil, fmt.Errorf("failed to relativize exclude rule %q: %w", raw, err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return nil, fmt.Errorf("%w: exclude rule %q is outside source root", ErrInvalidPattern, raw) + } + cleanedRaw = rel + } + + normalized := path.Clean(filepath.ToSlash(cleanedRaw)) if !doublestar.ValidatePattern(normalized) { return nil, fmt.Errorf("%w: exclude rule %q is invalid", ErrInvalidPattern, raw) }Go path/filepath Rel documentation outside base path returns dot-dot components🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 29 - 38, The exclude rules are stored inconsistently with how candidates are normalized in MatchRule, causing mismatches for inputs like "dist/", "./dist" or absolute paths; in the loop that builds compiled rules (the block that appends to compiled as rule{value: normalized, isPattern: hasMeta(normalized)}), canonicalize each raw rule before validation and storage by using filepath.Clean, remove a leading "./", strip trailing path separators, convert Windows paths to forward slashes (filepath.ToSlash) and then either relativize absolute rules against sourceRoot or return ErrInvalidPattern if they lie outside sourceRoot; keep using the same hasMeta(normalized) and doublestar.ValidatePattern on the canonicalized value so MatchRule and the stored rule values match.
93-95:⚠️ Potential issue | 🟠 MajorRecognize bracket character classes as glob meta.
Patterns like
logs/[ab].logare valid doublestar globs, buthasMetatreats them as literals, so they won’t be matched viadoublestar.Match.Proposed fix
func hasMeta(pattern string) bool { - return strings.ContainsAny(pattern, "*?{") + return strings.ContainsAny(pattern, "*?{[") }github.com/bmatcuk/doublestar/v4 character class glob syntax [abc]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 93 - 95, The hasMeta function currently only checks for "*?{" so it misses character class globs like [abc]; update hasMeta to treat '[' (and optionally ']' if you prefer explicit pairs) as a meta character as well (e.g., include '[' in the strings.ContainsAny call inside hasMeta) so patterns such as logs/[ab].log are recognized as globs for doublestar.Match.
52-59:⚠️ Potential issue | 🔴 CriticalReject absolute candidates outside
sourceRoot.
filepath.Relcan return../...without error. With the current fallback, broad rules like**may match paths outside the matcher root.Proposed boundary guard
func (m *Matcher) MatchRule(filePath string) (bool, string) { candidate := filePath if filepath.IsAbs(candidate) { - if rel, err := filepath.Rel(m.sourceRoot, candidate); err == nil { - candidate = rel + rel, err := filepath.Rel(m.sourceRoot, candidate) + if err != nil { + return false, "" } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return false, "" + } + candidate = rel } normalized := path.Clean(filepath.ToSlash(candidate)) + if normalized == ".." || strings.HasPrefix(normalized, "../") { + return false, "" + }Go path/filepath Rel documentation outside base path returns dot-dot components🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exclude/matcher.go` around lines 52 - 59, MatchRule currently accepts the result of filepath.Rel even when that result points outside m.sourceRoot (it can start with “..”), allowing rules like ** to match files outside the intended root; update MatchRule to only replace candidate with rel when filepath.Rel returns no error AND rel does not contain any leading ".." path segment (e.g. check for rel == ".." or strings.HasPrefix(rel, ".."+string(os.PathSeparator)) or inspect path segments) so that absolute candidates outside m.sourceRoot keep their original path; reference the MatchRule method, the candidate variable, filepath.Rel call, and m.sourceRoot when making the change.
🧹 Nitpick comments (2)
internal/watcher/watcher.go (1)
227-253: Honor cancellation during recursive registration.
addRecursivenow receivesctx, but it still walks the whole tree after cancellation. Checkctx.Done()before adding and before descending to keep shutdown responsive on large trees.Proposed cancellation check
func (w *Watcher) addRecursive(ctx context.Context, path string) error { + select { + case <-ctx.Done(): + return nil + default: + } + if w.isExcluded(ctx, path) { return nil } err := w.fswatcher.Add(path) @@ for _, entry := range entries { + select { + case <-ctx.Done(): + return nil + default: + } + if !entry.IsDir() { continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/watcher/watcher.go` around lines 227 - 253, The addRecursive function must stop work when the provided ctx is cancelled: before calling w.fswatcher.Add(path) and before iterating/descending into entries check ctx.Done()/ctx.Err() and return ctx.Err() (or context.Canceled) if cancelled; likewise check ctx before calling os.ReadDir and before each recursive call to w.addRecursive to avoid walking the rest of the tree after cancellation. Use the existing addRecursive, isExcluded and fswatcher.Add symbols to locate where to insert these early ctx checks and return the context error instead of continuing.internal/cli/commands/sync/sync.go (1)
71-84: Consider validating excludes before creating the remote client.Invalid exclude patterns are parameter errors and should fail fast. However,
client.New()does not perform network I/O—it only allocates a struct with the connection deferred to lazy initialization. Moving matcher creation earlier is still a reasonable architectural improvement for parameter validation, but the impact is primarily about clean error-handling order rather than avoiding remote operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/commands/sync/sync.go` around lines 71 - 84, Move validation of exclude patterns to run before creating the remote client: call exclude.New(cfg.Excludes, cfg.Source) and handle its error (returning cli.Exit with codes.ParamsError and logging via log.Error) before calling client.New(cfg.Dest, log). This ensures parameter errors from exclude.New are surfaced as parameter validation failures (not client errors) and keeps the existing watcher.New(cfg.Source, excludeMatcher, log) and syncer.New(cfg.Source, remote, excludeMatcher, log) creation order consistent once excludeMatcher is valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/exclude/matcher.go`:
- Around line 29-38: The exclude rules are stored inconsistently with how
candidates are normalized in MatchRule, causing mismatches for inputs like
"dist/", "./dist" or absolute paths; in the loop that builds compiled rules (the
block that appends to compiled as rule{value: normalized, isPattern:
hasMeta(normalized)}), canonicalize each raw rule before validation and storage
by using filepath.Clean, remove a leading "./", strip trailing path separators,
convert Windows paths to forward slashes (filepath.ToSlash) and then either
relativize absolute rules against sourceRoot or return ErrInvalidPattern if they
lie outside sourceRoot; keep using the same hasMeta(normalized) and
doublestar.ValidatePattern on the canonicalized value so MatchRule and the
stored rule values match.
- Around line 93-95: The hasMeta function currently only checks for "*?{" so it
misses character class globs like [abc]; update hasMeta to treat '[' (and
optionally ']' if you prefer explicit pairs) as a meta character as well (e.g.,
include '[' in the strings.ContainsAny call inside hasMeta) so patterns such as
logs/[ab].log are recognized as globs for doublestar.Match.
- Around line 52-59: MatchRule currently accepts the result of filepath.Rel even
when that result points outside m.sourceRoot (it can start with “..”), allowing
rules like ** to match files outside the intended root; update MatchRule to only
replace candidate with rel when filepath.Rel returns no error AND rel does not
contain any leading ".." path segment (e.g. check for rel == ".." or
strings.HasPrefix(rel, ".."+string(os.PathSeparator)) or inspect path segments)
so that absolute candidates outside m.sourceRoot keep their original path;
reference the MatchRule method, the candidate variable, filepath.Rel call, and
m.sourceRoot when making the change.
---
Nitpick comments:
In `@internal/cli/commands/sync/sync.go`:
- Around line 71-84: Move validation of exclude patterns to run before creating
the remote client: call exclude.New(cfg.Excludes, cfg.Source) and handle its
error (returning cli.Exit with codes.ParamsError and logging via log.Error)
before calling client.New(cfg.Dest, log). This ensures parameter errors from
exclude.New are surfaced as parameter validation failures (not client errors)
and keeps the existing watcher.New(cfg.Source, excludeMatcher, log) and
syncer.New(cfg.Source, remote, excludeMatcher, log) creation order consistent
once excludeMatcher is valid.
In `@internal/watcher/watcher.go`:
- Around line 227-253: The addRecursive function must stop work when the
provided ctx is cancelled: before calling w.fswatcher.Add(path) and before
iterating/descending into entries check ctx.Done()/ctx.Err() and return
ctx.Err() (or context.Canceled) if cancelled; likewise check ctx before calling
os.ReadDir and before each recursive call to w.addRecursive to avoid walking the
rest of the tree after cancellation. Use the existing addRecursive, isExcluded
and fswatcher.Add symbols to locate where to insert these early ctx checks and
return the context error instead of continuing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 25a678fd-2bf2-421c-84bb-d76212c0f431
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
README.mdgo.modinternal/cli/commands/sync/sync.gointernal/exclude/errors.gointernal/exclude/matcher.gointernal/exclude/matcher_test.gointernal/syncer/syncer.gointernal/watcher/watcher.gomain.go
✅ Files skipped from review due to trivial changes (4)
- go.mod
- internal/exclude/errors.go
- main.go
- internal/exclude/matcher_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
capcom6
changed the title
--exclude and integrate matcher into watcher/syncer
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/watcher/watcher.go (1)
227-260: Consider returningctx.Err()on cancellation and checking ctx at function entry.Two small refinements to the recursive add:
- On cancellation the function returns
nil, which callers (e.g.Watchat line 55 andupdateObserversat line 188) treat as success. Current behavior is acceptable — the watcher goroutine will promptly exit viactx.Done()and the deferred close tears downfswatcher— but propagatingctx.Err()would make the partial-setup case explicit to callers and logs.- The
selectis only evaluated between child entries, so an already-cancelled context still performs onefswatcher.Add(path)+os.ReadDir(path)at every recursion level. A top-of-function check (or folding it into the existingisExcludedpreamble) would make cancellation more prompt on deep trees.Neither is a correctness bug; feel free to defer.
♻️ Optional tweak
func (w *Watcher) addRecursive(ctx context.Context, path string) error { + select { + case <-ctx.Done(): + return ctx.Err() + default: + } + if w.isExcluded(ctx, path) { return nil } @@ for _, entry := range entries { select { case <-ctx.Done(): - return nil + return ctx.Err() default: }Note: if you adopt this,
Watch()at line 55 will start surfacingcontext.Canceledas an error fromaddRecursive; you may want to treat that as a non-error there (e.g.errors.Is(addErr, context.Canceled)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/watcher/watcher.go` around lines 227 - 260, addRecursive currently continues work after ctx is cancelled and returns nil on cancellation; change it to check ctx at the top (e.g., immediate ctx.Err() check before isExcluded/fswatcher.Add) and during the loop return ctx.Err() when ctx.Done() is observed so cancellation is propagated; update callers (Watch and updateObservers) to treat context.Canceled/ctx.Err() as a non-fatal condition if desired (e.g., errors.Is(addErr, context.Canceled)) and keep referencing addRecursive and isExcluded to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/watcher/watcher.go`:
- Around line 227-260: addRecursive currently continues work after ctx is
cancelled and returns nil on cancellation; change it to check ctx at the top
(e.g., immediate ctx.Err() check before isExcluded/fswatcher.Add) and during the
loop return ctx.Err() when ctx.Done() is observed so cancellation is propagated;
update callers (Watch and updateObservers) to treat context.Canceled/ctx.Err()
as a non-fatal condition if desired (e.g., errors.Is(addErr, context.Canceled))
and keep referencing addRecursive and isExcluded to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4397a705-5869-4d19-a95d-f3755bd7bf0c
📒 Files selected for processing (1)
internal/watcher/watcher.go
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.
Motivation
*,**,?) so users can ignore groups of files likenode_modules/**or**/*.tmpinstead of enumerating paths.Description
internal/exclude/matcher.gowhich implementsNewandMatch/MatchRuleusinggithub.com/bmatcuk/doublestar/v4for doublestar-style glob matching and path normalization.internal/cli/commands/sync/sync.goand updatingmain.goand flag help text to mention glob support.internal/watcherandinternal/syncerso watcher registration and sync operations early-return for excluded paths and log which rule caused the skip.README.md, adddocs/implementation-plan-exclude-patterns.md, updatego.modwith the new dependency, and include unit tests ininternal/exclude/matcher_test.gocovering literal and pattern cases plus normalization and absolute-path boundaries.Testing
go test ./internal/exclude -vand passed.go test ./...andgo build ./...and succeeded.Codex Task
Summary by CodeRabbit
New Features
Documentation
Tests
Chores