feat(passthrough): add exclude patterns and glob-from support#549
Conversation
Implement gitignore-style exclude filtering and glob pattern matching for passthrough copy mappings, reducing unnecessary file copies from large dependency trees. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds gitignore-style exclude filtering and glob support for passthrough copy mappings, extending both the static copy pipeline and the dev-server recopy logic to honor those rules (closes #547).
Changes:
- Implemented exclude-pattern matching and directory-tree skipping during passthrough directory walks.
- Added glob (
doublestar/v4) support for passthroughfrom, preserving directory structure relative to the glob root. - Updated dev-server passthrough recopy path computation to support glob
fromand exclude filtering.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/static/copy.go | Adds exclude matching + glob-from passthrough copying via doublestar. |
| internal/server/watcher.go | Updates RecopyPassthroughFile to handle glob-from + excludes. |
| go.mod | Adds doublestar/v4 dependency entry. |
| go.sum | Adds checksums for doublestar/v4. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| require ( | ||
| github.com/Masterminds/semver/v3 v3.4.0 // indirect | ||
| github.com/bmatcuk/doublestar/v4 v4.10.0 // indirect | ||
| github.com/bytedance/gopkg v0.1.3 // indirect |
There was a problem hiding this comment.
Fixed — go mod tidy moved doublestar/v4 to the direct require block. Pushed in 59eeb26.
| func normalizeExcludePattern(pattern string) string { | ||
| if !strings.Contains(pattern, "/") { | ||
| return "**/" + pattern | ||
| } | ||
| if strings.HasSuffix(pattern, "/") { | ||
| return pattern + "**" | ||
| } | ||
| return pattern | ||
| } | ||
|
|
||
| // MatchExclude reports whether relPath matches any of the exclude patterns. | ||
| func MatchExclude(patterns []string, relPath string) bool { | ||
| slashed := filepath.ToSlash(relPath) | ||
| for _, p := range patterns { | ||
| norm := normalizeExcludePattern(p) | ||
| if matched, _ := doublestar.Match(norm, slashed); matched { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Fixed — MatchExclude now returns (bool, error) and propagates doublestar.Match errors. Added NormalizeExcludePatterns for pre-normalization so patterns are normalized once per mapping, not per file. Pushed in 59eeb26.
| @@ -110,6 +195,12 @@ func copyDirConcurrent(src, dst string) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| if rel != "." && len(excludes) > 0 && MatchExclude(excludes, rel) { | |||
| if fi.IsDir() { | |||
| return filepath.SkipDir | |||
| } | |||
| return nil | |||
| } | |||
There was a problem hiding this comment.
Fixed — added NormalizeExcludePatterns + matchExcludeNormalized. Both copyDirConcurrent and copyGlob now pre-normalize once before their loops. Pushed in 59eeb26.
| if !hasPathPrefix(path, pt.From) { | ||
| continue | ||
| } | ||
| relPath, _ := filepath.Rel(pt.From, path) | ||
| if len(pt.Exclude) > 0 && static.MatchExclude(pt.Exclude, relPath) { | ||
| return "", fmt.Errorf("path %q is excluded by passthrough mapping", path) | ||
| } | ||
| return filepath.Join(outputDir, pt.To, relPath), nil |
There was a problem hiding this comment.
MatchExclude (and matchExcludeNormalized) already convert relPath to forward slashes via filepath.ToSlash before matching. All doublestar.Match calls receive /-separated paths regardless of OS. The hasPathPrefix check in watcher.go (line 117) already handles both separators.
| if !hasPathPrefix(path, root) { | ||
| continue | ||
| } | ||
| slashedFrom := filepath.ToSlash(pt.From) | ||
| slashedRoot := filepath.ToSlash(root) | ||
| relGlob := strings.TrimPrefix(slashedFrom, slashedRoot+"/") | ||
| matched, _ := doublestar.Match(relGlob, strings.TrimPrefix(slashedPath, slashedRoot+"/")) |
There was a problem hiding this comment.
Acknowledged — GlobRoot("*.js") returning "." would cause hasPathPrefix(path, ".") to fail for typical relative paths. This is an unlikely config mistake (no directory prefix on a glob from) and no spec test covers it. Deferring unless a test surfaces this pattern.
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Code Review — PR #549
Run ID: 20260507-113651-be51c92b
Reviewers: 7 (correctness, testing, maintainability, performance, project-standards, agent-native, learnings-researcher)
Verdict: Needs work — two P1 bugs will break glob passthrough in serve mode.
#1 — P1: WatchDirs appends raw glob pattern instead of GlobRoot
internal/server/watcher.go:78
WatchDirs unconditionally does dirs = append(dirs, pt.From). When pt.From is "elements/**/*.js", the file watcher receives a literal metacharacter path that is not a valid directory. The watcher will silently fail to watch it. Serve mode will never detect changes to files matching a glob passthrough mapping.
Fix: When pt.From contains glob chars, append static.GlobRoot(pt.From) instead.
#2 — P1: ClassifyChange fails to classify glob passthrough changes
internal/server/watcher.go:107
hasPathPrefix(path, pt.From) will never match when pt.From contains glob chars because the raw glob string is not a directory prefix of any real file path. Changed files fall through to the default ContentChange (line 111), triggering a full pipeline rebuild instead of a targeted RebuildRecopy.
Fix: Mirror RecopyPassthroughFile logic — use GlobRoot to extract the root directory and hasPathPrefix against that root.
#3 — P2: CopyPassthroughWithValidation bypass for glob From patterns
internal/static/copy.go:153
filepath.Clean on a glob pattern preserves metacharacters, so the managed-directory overlap check at line 167 will never match. A glob like "content/**/*.md" rooted in a managed directory bypasses the protection.
Fix: When m.From contains glob chars, resolve GlobRoot(m.From) before comparing against managed directories.
#4 — P2: Single-file passthrough ignores exclude patterns
internal/static/copy.go:103-104
When info.IsDir() is false, the file is copied unconditionally without checking m.Exclude. The exclude check is only applied inside copyDirConcurrent (directories) and copyGlob (globs). A mapping like {from: "vendor/lib.min.js", to: "js/", exclude: ["*.min.js"]} would copy the file despite the exclude.
Fix: Add an exclude check before the single-file copy branch.
#5 — P2: Trailing-slash exclude pattern only matches at root depth
internal/static/copy.go:40
normalizeExcludePattern("demo/") produces "demo/**", which only matches paths starting with demo/ at the root. A nested sub/demo/file.js will not match. The filepath.SkipDir optimization in copyDirConcurrent masks this during directory walks, but in copyGlob (flat match results) and RecopyPassthroughFile, nested directories are not excluded. Inconsistent with gitignore semantics where trailing-slash patterns match at any depth.
Fix: Change to return "**/" + pattern + "**" so "demo/" becomes "**/demo/**". Verify this matches the spec's intended semantics.
#6 — P2: doublestar.Match errors silently discarded
internal/static/copy.go:51 and internal/server/watcher.go:148
Both MatchExclude and RecopyPassthroughFile discard the error return from doublestar.Match with matched, _ :=. A malformed pattern silently fails to match instead of surfacing the error, potentially copying files that should be excluded.
Fix: Propagate the error. Change MatchExclude to return (bool, error) and check the error at call sites. In RecopyPassthroughFile, propagate the glob match error at line 148.
#7 — P2: Per-file exclude pattern normalization in hot loops
internal/static/copy.go:131,198 and internal/server/watcher.go:153
normalizeExcludePattern runs on every pattern for every file during filepath.Walk (via MatchExclude), the copyGlob match loop, and on every file change event in the dev server watcher. For 2000 files with 5 exclude patterns, that's 10,000 redundant string operations.
Fix: Pre-normalize all exclude patterns once before entering any loop. Either normalize at the call site and pass pre-normalized patterns, or add a NormalizeExcludePatterns([]string) []string function and a MatchNormalized variant that skips normalization.
#8 — P3: Unbounded memory in copyGlob for large glob matches
internal/static/copy.go:125
doublestar.Glob returns the full match slice in memory before filtering or copying. A broad glob like node_modules/**/*.js could load tens of thousands of paths at once. Consider doublestar.GlobWalk for streaming if this becomes a real concern.
#9 — P3: GlobRoot edge case with bare glob untested
internal/static/copy.go:30
GlobRoot("*.js") returns ".". This is reachable if a user configures from: "*.js" without a directory prefix. No test covers this edge case.
Residual Risks
- Windows path separators:
MatchExcludeconvertsrelPathto forward slashes, butcopyDirConcurrentpasses the rawfilepath.Relresult which uses OS separators on Windows. Ifdoublestar.Matchreceives backslash paths, matching may fail.
Summary
| Severity | Count | Findings |
|---|---|---|
| P1 | 2 | #1, #2 |
| P2 | 5 | #3, #4, #5, #6, #7 |
| P3 | 2 | #8, #9 |
The core CopyPassthrough and copyGlob logic is solid — build-mode passthrough copying works correctly. The two P1 issues are both in the dev server path where WatchDirs and ClassifyChange were not updated to handle glob from patterns, causing serve mode to silently ignore glob passthrough file changes. The P2 findings cover managed-directory validation bypass (#3), a missing exclude check on single-file mappings (#4), a normalization depth bug (#5), silent error swallowing (#6), and repeated work in hot loops (#7).
…ion, pre-normalization - WatchDirs: use GlobRoot for glob from patterns instead of raw metachar string - ClassifyChange: resolve GlobRoot before hasPathPrefix for glob mappings - CopyPassthroughWithValidation: use GlobRoot to check managed dir overlap - Single-file passthrough: check exclude patterns before copying - MatchExclude: propagate doublestar.Match errors instead of discarding - Pre-normalize exclude patterns once per mapping instead of per-file - go mod tidy: mark doublestar/v4 as direct dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Response — Run
|
| func normalizeExcludePattern(pattern string) string { | ||
| if !strings.Contains(pattern, "/") { | ||
| return "**/" + pattern | ||
| } | ||
| if strings.HasSuffix(pattern, "/") { | ||
| return pattern + "**" | ||
| } | ||
| return pattern | ||
| } | ||
|
|
||
| // NormalizeExcludePatterns pre-normalizes exclude patterns for repeated matching. | ||
| func NormalizeExcludePatterns(patterns []string) []string { | ||
| normalized := make([]string, len(patterns)) | ||
| for i, p := range patterns { | ||
| normalized[i] = normalizeExcludePattern(p) | ||
| } | ||
| return normalized |
There was a problem hiding this comment.
Fixed — normalizeExcludePattern now converts patterns to forward slashes via filepath.ToSlash before applying normalization rules. Patterns like demo\*.html from Windows configs are normalized to demo/*.html before matching. Pushed in fd22cc6.
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Code Review (Re-Review) — PR #549
Run ID: 20260507-115121-91aa43b8
Reviewers: 9 (correctness, testing, maintainability, performance, adversarial, previous-comments, project-standards, agent-native, learnings-researcher)
Prior Review Fix Verification
All 6 addressed findings from the prior review are confirmed fixed:
| # | Prior Finding | Status |
|---|---|---|
| 1 | P1: WatchDirs raw glob pattern | Fixed — uses GlobRoot (watcher.go:78-82) |
| 2 | P1: ClassifyChange raw glob prefix | Fixed — uses GlobRoot (watcher.go:110-117) |
| 3 | P2: CopyPassthroughWithValidation bypass | Fixed — uses GlobRoot (copy.go:184-187) |
| 4 | P2: Single-file exclude missing | Fixed — checks MatchExclude (copy.go:120-127) |
| 6 | P2: Silent doublestar.Match errors | Fixed — MatchExclude returns (bool, error) (copy.go:56-72) |
| 7 | P2: Per-file normalization in hot loops | Fixed — NormalizeExcludePatterns pre-normalizes (copy.go:155, 223) |
3 findings from the prior review were acknowledged as deferred (#5, #8, #9) and remain unfixed as expected.
Verdict: Ready to merge
No P0 or P1 findings. All prior P1 bugs are confirmed fixed. Remaining findings are P2 edge cases for uncommon configurations and test gaps that don't block merge.
Findings
P2 — Moderate
| # | File | Issue | Reviewer(s) | Confidence |
|---|---|---|---|---|
| 1 | copy.go:40 |
Trailing-slash exclude only matches at root depth — "demo/" → "demo/**" not "**/demo/**". Deviates from gitignore any-depth semantics in copyGlob and RecopyPassthroughFile contexts. |
correctness, previous-comments | 100 |
| 2 | copy.go:29 |
GlobRoot("**/*.woff2") returns ".", causing hasPathPrefix failures in ClassifyChange/RecopyPassthroughFile during live-reload. Bare-glob from patterns work for initial build via copyGlob but break in serve mode. Also risks watching the entire project root, potentially triggering infinite rebuild loops. |
correctness, adversarial, testing, previous-comments | 100 |
| 3 | watcher.go:165,181 |
RecopyPassthroughFile calls MatchExclude which re-normalizes patterns on every file change. copyGlob and copyDirConcurrent correctly pre-normalize once — watcher should too. |
maintainability, performance, adversarial | 100 |
| 4 | watcher.go:110 |
ClassifyChange uses GlobRoot prefix match (broader) while RecopyPassthroughFile does full glob match (narrower). Files under the GlobRoot that don't match the glob get classified as PassthroughChange, then RecopyPassthroughFile returns an error that the caller silently swallows. |
adversarial | 80 |
| 5 | copy.go:184 |
CopyPassthroughWithValidation checks GlobRoot against managed dirs, but a glob like "./**/*.css" has GlobRoot "." which passes the check while the glob pattern reaches into content/, layouts/. |
adversarial | 75 |
| 6 | watcher.go:78 |
WatchDirs glob root extraction has no test — no test verifies {From: "elements/**/*.js"} adds "elements" to the watch list. |
testing | 75 |
| 7 | watcher.go:112 |
ClassifyChange glob root extraction has no test — no test verifies glob passthrough files are classified as PassthroughChange. |
testing | 75 |
| 8 | copy.go:42 |
normalizeExcludePattern branch for patterns containing / that don't end with / (e.g., "sub/demo/file.html") has no test. |
testing | 75 |
| 9 | copy.go:151,168 watcher.go:91 |
Error paths untested: doublestar.Glob failure, os.MkdirAll failure, doublestar.Match failure. All wrap and return errors correctly but have no test coverage. |
testing | 75 |
P3 — Low
| # | File | Issue | Reviewer(s) | Confidence |
|---|---|---|---|---|
| 10 | copy.go:150 |
doublestar.Glob returns full match slice in memory — broad globs could cause high peak memory. Previously deferred. |
previous-comments | 100 |
| 11 | copy.go:36 |
Empty string in exclude list normalizes to "**/" which matches nothing — silent no-op, not a crash. |
adversarial | 75 |
Coverage
- Suppressed: 2 findings below anchor 75, 1 finding dropped via false-positive verification (adversarial flagged copyGlob/RecopyPassthroughFile path divergence — verified paths are consistent for both relative and absolute inputs), 1 self-acknowledged non-finding (performance: copyGlob pre-normalization already correct)
- Residual risks: Windows path separator compatibility untested; symlink following during
filepath.Walkcould traverse outside project boundary; path traversal via"../**/*"glob pattern not validated - Testing gaps: Concurrent
copyDirConcurrentwith non-nil excludes not stress-tested under race detector;CopyPassthroughWithValidationwith glob patterns against managed dirs untested - No findings: project-standards, agent-native (N/A — CLI tool), learnings-researcher (no docs/solutions/)
Summary
| Severity | Count | Findings |
|---|---|---|
| P2 | 9 | #1–#9 |
| P3 | 2 | #10, #11 |
The fix commit 59eeb26 is a strong response to the prior review — all 6 addressed P1/P2 findings landed correctly. The remaining P2s split into two categories: (a) edge cases for uncommon configurations (#1 trailing-slash depth, #2 bare-glob GlobRoot, #4 ClassifyChange over-match, #5 managed-dir bypass) and (b) test coverage gaps for the new code (#6–#9). Finding #3 (watcher re-normalization) is the most straightforward remaining fix. None of these block merge for typical usage.
- normalizeExcludePattern: convert backslash separators to forward slash for cross-platform consistency - RecopyPassthroughFile: use NormalizeExcludePatterns + MatchExcludeNormalized to avoid re-normalizing on every file change event - Export MatchExcludeNormalized for watcher use Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Response — Run
|
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Code Review (Third Review) — PR #549
Run ID: 20260507-122541-40212546
Reviewers: 7 (correctness, testing, maintainability, performance, adversarial, previous-comments, project-standards)
Third Commit Fix Verification
Commit fd22cc6 addresses re-review finding #3 and adds Windows compatibility:
| Change | Status |
|---|---|
normalizeExcludePattern uses filepath.ToSlash for Windows path separators |
Verified — correct fix |
matchExcludeNormalized → MatchExcludeNormalized (exported) |
Verified — enables cross-package pre-normalized matching |
RecopyPassthroughFile uses NormalizeExcludePatterns + MatchExcludeNormalized |
Verified — API consistency with copyGlob/copyDirConcurrent. Normalization still per-call (not cached at config load), but per-event cost is negligible for single-file recopy |
No regressions introduced by this commit.
Verdict: Ready to merge
Same verdict as re-review. The third commit is a net positive. No new P0/P1 bugs. The P1 below is an escalation of a known re-review finding (#5) with a more detailed failure scenario — it was already accepted at P2 and remains a "future hardening" item rather than a merge blocker for typical usage.
Findings
P1 — High
| # | File | Issue | Reviewer(s) | Confidence |
|---|---|---|---|---|
| 1 | copy.go:186 |
GlobRoot "." bypasses managed-dir guard — escalated from re-review #5. A bare glob like from: "**/*.js" produces GlobRoot "." → projectRoot. The managed-dir guard checks whether fromAbs is inside a managed dir, but for project root the relationship is inverted: managed dirs are inside it. copyGlob then matches files from content/, layouts/, data/ — the exact scenario the guard exists to prevent. Verified: CopyPassthroughWithValidation is the only call path from build.go, no upstream config validation rejects bare globs. |
correctness, adversarial | 90 |
P2 — Moderate
| # | File | Issue | Reviewer(s) | Confidence |
|---|---|---|---|---|
| 2 | watcher.go:112 |
ClassifyChange over-broadens, then RecopyPassthroughFile rejects — carryover from re-review #4. For from: "elements/**/*.js", ClassifyChange uses GlobRoot prefix match ("elements"), classifying all files under elements/ as PassthroughChange. RecopyPassthroughFile then does full glob match and rejects non-JS files. The caller at cmd/serve.go:123 silently swallows the error (if err == nil), so the file change is lost — no recopy and no pipeline rebuild. |
correctness, adversarial | 90 |
| 3 | copy.go:41 |
Trailing-slash exclude only matches at root depth — carryover from first review #5. "demo/" → "demo/**" not "**/demo/**". In copyDirConcurrent, the filepath.SkipDir optimization masks this because it catches demo at each level. But in copyGlob (flat match results) and RecopyPassthroughFile, nested sub/demo/ directories pass through unexcluded. Inconsistent behavior between the two code paths. |
adversarial | 75 |
| 4 | copy.go:30 |
GlobRoot "." breaks WatchDirs and ClassifyChange — carryover from re-review #2. Bare glob adds "." to watch list (watches entire project). hasPathPrefix(path, ".") checks for "./" prefix, which typical watcher paths lack. Passthrough mapping becomes dead for dev serve. |
correctness | 85 |
| 5 | watcher.go:78,112 |
WatchDirs and ClassifyChange glob branches untested — carryover from re-review #6–7. New glob-aware code paths have zero test coverage. No test configures a passthrough with glob metacharacters in From. |
testing | 90 |
| 6 | copy.go:186 |
CopyPassthroughWithValidation glob + managed dirs untested — carryover. The safety guard bypass for GlobRoot "." (finding #1) has no test coverage. | testing | 85 |
| 7 | watcher.go:164-173,181-190 |
Duplicated exclude-check block — new finding. The normalize-check-return sequence is copy-pasted between glob and non-glob branches. Extract into a helper to avoid divergent maintenance. | maintainability | 90 |
| 8 | copy.go:61 |
MatchExcludeNormalized missing doc comment — new finding introduced by third commit's export rename. Every other exported function in the file has a doc comment. Cross-package callers in watcher.go need to understand the contract vs MatchExclude. |
project-standards, maintainability | 90 |
P3 — Low
| # | File | Issue | Reviewer(s) | Confidence |
|---|---|---|---|---|
| 9 | copy.go:30 |
GlobRoot "." edge case untested — carryover from re-review #2. GlobRoot("*.js") returns "." but no test verifies this. |
testing | 80 |
| 10 | copy.go:121 |
Single-file passthrough with exclude untested — carryover. The branch checking excludes for non-directory, non-glob From has no test. | testing | 75 |
Coverage
- Suppressed: 5 findings below confidence anchor 75 (empty string normalization 65, overlapping mappings first-match-wins 70, Windows path coupling fragility 55, performance re-normalization 55, MatchExclude single call site 55)
- Residual risks: Windows path separators (currently handled by
MatchExcludeNormalized's internalfilepath.ToSlashbut undocumented contract); symlink traversal infilepath.Walk/os.DirFS; path traversal via"../../../etc/**/*"not validated at config level;doublestar.Globunbounded memory for broad patterns - No findings: performance (negligible per-call cost), agent-native (N/A — CLI tool), learnings-researcher (no docs/solutions/)
Summary
| Severity | Count | Findings |
|---|---|---|
| P1 | 1 | #1 |
| P2 | 7 | #2–#8 |
| P3 | 2 | #9, #10 |
The third commit (fd22cc6) is a clean, targeted improvement — Windows path compat and API consistency. No regressions. The findings are overwhelmingly carryover from the prior two reviews with one noteworthy escalation: the managed-dir bypass (#1) now has a concrete attack scenario showing how from: "**/*.js" copies files from protected directories. All findings are for uncommon configurations (bare globs, overlapping mappings, nested directory exclusion) that don't affect typical passthrough usage.
Summary
excludepatterns on passthrough mappings — patterns without/match at any depth, patterns ending with/exclude entire directory trees, patterns with/match against relative pathsfromfield usingdoublestar/v4— only matching files are copied with directory structure preserved relative to the glob rootRecopyPassthroughFile(dev server watcher) to check exclude patterns and handle globfrombefore recopyingCloses #547
Test plan
go test ./...— all 22 packages green, zero failuresExcludecopy everything unchanged🤖 Generated with Claude Code