spec(config): watch directory config for extra watcher dirs (#530)#576
Conversation
Add spec tests for the `watch:` config key that registers extra directories for pipeline-triggering file watching during serve mode. Unlike passthrough (RebuildRecopy), watch dirs trigger RebuildPipeline by reusing existing ContentChange/LayoutChange/DataChange types. Config tests: parsing, validation (empty from, invalid type, array index in error), acceptance (all three types, no entries). Watcher tests: WatchDirs inclusion, glob root extraction, ClassifyChange mapping, RebuildPipeline scope confirmation. WatchMapping struct stub added to config.go for compilation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds the initial configuration surface area and spec/tests for “extra watch directories” (issue #530) so alloy serve can rebuild when plugin-read files change outside the standard content/ tree.
Changes:
- Introduces
Config.Watch []WatchMappingplus YAML testdata to parsewatch:entries. - Adds new Ginkgo tests specifying expected validation behavior (
watch[i].from/type) and watcher behavior (WatchDirs,ClassifyChange, rebuild scope). - Updates planning docs (
PLAN.md,IMPLEMENTATION.md) with the feature spec and implementation notes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plans/PLAN.md | Documents the new watch: config key behavior/validation. |
| plans/IMPLEMENTATION.md | Adds implementation guidance for watch mappings in config + watcher. |
| internal/server/watcher_test.go | Adds watcher specs for including/classifying watch: dirs. |
| internal/config/testdata/valid.yaml | Adds watch: examples used by config parsing tests. |
| internal/config/config.go | Adds Watch field to Config and defines WatchMapping. |
| internal/config/config_test.go | Adds parsing + validation specs for watch: entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Permalinks map[string]string `yaml:"permalinks" toml:"permalinks" json:"permalinks"` | ||
| Pagination PaginationConfig `yaml:"pagination" toml:"pagination" json:"pagination"` | ||
| Passthrough []PassthroughMapping `yaml:"passthrough" toml:"passthrough" json:"passthrough"` | ||
| Watch []WatchMapping `yaml:"watch" toml:"watch" json:"watch"` | ||
| Sources map[string]*SourceConfig `yaml:"sources" toml:"sources" json:"sources"` | ||
| Sitemap SitemapConfig `yaml:"sitemap" toml:"sitemap" json:"sitemap"` |
There was a problem hiding this comment.
This is a spec-first PR — tests are written to fail red and drive the implementation in a follow-up development PR. The validation logic, WatchDirs, and ClassifyChange changes are intentionally omitted. The WatchMapping struct stub is included only so the test file compiles.
| Describe("Watch directory watching (issue #530)", func() { | ||
| It("WatchDirs includes watch from: directories", func() { | ||
| cfg := &config.Config{ | ||
| Title: "Watch Site", | ||
| Watch: []config.WatchMapping{ | ||
| {From: "elements", Type: "content"}, |
There was a problem hiding this comment.
Same as above — spec-first workflow. Tests intentionally fail red to define the contract. The WatchDirs and ClassifyChange implementation is the developer's responsibility in the follow-up PR.
| Context("validation", func() { | ||
| It("rejects watch entry with empty from", func() { | ||
| cfg := &config.Config{ | ||
| Title: "Test", | ||
| BaseURL: "https://example.com", | ||
| Watch: []config.WatchMapping{{From: "", Type: "content"}}, | ||
| } | ||
| err := config.Validate(cfg) | ||
| Expect(err).To(HaveOccurred(), | ||
| "watch entry with empty from must fail validation (issue #530)") | ||
| Expect(err.Error()).To(ContainSubstring("from"), | ||
| "error must mention the invalid field (issue #530)") | ||
| }) | ||
|
|
||
| It("rejects watch entry with invalid type", func() { | ||
| cfg := &config.Config{ | ||
| Title: "Test", | ||
| BaseURL: "https://example.com", | ||
| Watch: []config.WatchMapping{{From: "elements", Type: "invalid"}}, | ||
| } | ||
| err := config.Validate(cfg) | ||
| Expect(err).To(HaveOccurred(), | ||
| "watch entry with invalid type must fail validation (issue #530)") | ||
| Expect(err.Error()).To(ContainSubstring("type"), | ||
| "error must mention the invalid field (issue #530)") | ||
| }) | ||
|
|
||
| It("rejects watch entry with empty type", func() { | ||
| cfg := &config.Config{ | ||
| Title: "Test", | ||
| BaseURL: "https://example.com", | ||
| Watch: []config.WatchMapping{{From: "elements", Type: ""}}, | ||
| } | ||
| err := config.Validate(cfg) | ||
| Expect(err).To(HaveOccurred(), | ||
| "empty type does not match content/layout/data (issue #530)") | ||
| }) | ||
|
|
||
| It("accepts valid watch entries for all three types", func() { | ||
| cfg := &config.Config{ | ||
| Title: "Test", | ||
| BaseURL: "https://example.com", | ||
| Watch: []config.WatchMapping{ | ||
| {From: "elements", Type: "content"}, | ||
| {From: "shared-layouts", Type: "layout"}, | ||
| {From: "external-data", Type: "data"}, | ||
| }, | ||
| } | ||
| err := config.Validate(cfg) | ||
| Expect(err).NotTo(HaveOccurred(), | ||
| "all three watch types must be valid (issue #530)") | ||
| }) | ||
|
|
||
| It("accepts config with no watch entries", func() { | ||
| cfg := &config.Config{ | ||
| Title: "Test", | ||
| BaseURL: "https://example.com", | ||
| } | ||
| err := config.Validate(cfg) | ||
| Expect(err).NotTo(HaveOccurred(), | ||
| "omitting watch entirely must be valid (issue #530)") | ||
| }) | ||
|
|
||
| It("includes index in validation error for second entry", func() { | ||
| cfg := &config.Config{ | ||
| Title: "Test", | ||
| BaseURL: "https://example.com", | ||
| Watch: []config.WatchMapping{ | ||
| {From: "elements", Type: "content"}, | ||
| {From: "", Type: "data"}, | ||
| }, | ||
| } | ||
| err := config.Validate(cfg) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("watch[1]"), | ||
| "validation error must include array index (issue #530)") | ||
| }) |
There was a problem hiding this comment.
Spec-first workflow — validation tests define the contract for the developer. The 4 red validation tests (empty from, invalid type, empty type, array index) will pass once Validate() is extended in the implementation PR.
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Review #1 — Ready to merge
Verdict: Ready to merge — 4 reviewers dispatched (correctness, testing, maintainability, project-standards).
Findings
| # | File | Issue | Reviewer | Conf | Route |
|---|---|---|---|---|---|
| 1 | config_test.go:~85 |
Empty type validation test has no ContainSubstring("type") assertion — peer tests ("empty from", "invalid type") both assert on error message content; this one only checks HaveOccurred() |
testing | 100 | advisory → human |
| 2 | watcher_test.go |
Watch/passthrough precedence untested — PLAN.md states "watch directories are checked before passthrough directories in ClassifyChange" but no test enforces this rule; two independent reviewers flagged this gap | testing, correctness | 75 | advisory → human |
Both findings are observations for the architect per project test immutability rules.
Testing Gaps
- Empty type test missing
ContainSubstring("type")— inconsistent with peer validation tests - No test for uppercase/mixed case watch type to verify case sensitivity
- Watch/passthrough precedence not tested (documented in PLAN.md but unenforced)
- No test for duplicate watch entries (same
from:with different types) - No test for watch
from:overlapping standard dirs (e.g.,from: "content" type: "layout") - No test for glob pattern watch dirs in ClassifyChange (only WatchDirs tests glob root extraction)
Correctness Verification
- Red/green split confirmed: config 4 FAIL / 3 PASS, watcher 5 FAIL / 5 PASS
WatchMappingstruct tags consistent withPassthroughMappingpatternWatch []WatchMappingplacement on Config correct (after Passthrough, before Sources)valid.yamlfixture matches config_test.go expectationsValidate()confirmed: no Watch validation yet (4 red tests drive this)WatchDirs()confirmed: no Watch dirs yet (3 red tests drive this)ClassifyChange()confirmed: no Watch handling yet (2 red tests drive this)RebuildScopeForChangeType()confirmed: Content/Layout/DataChange already map to RebuildPipeline- ClassifyChange content test passes via fallback
return ContentChange(watcher.go:119) — correct result, coincidental path
Verdict: Ready to merge. Well-structured spec PR with 17 tests, clear red/green expectations, minimal struct stub, and thorough plan documentation.
Run artifact: /tmp/compound-engineering/ce-code-review/20260512-aae07245/
…e test (#530) Add ContainSubstring("type") assertion to empty-type validation test for consistency with peer tests. Add ClassifyChange precedence test verifying watch dirs take priority over passthrough when both match the same directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressing review findings in 61e9289: Finding #1 — empty type test missing Finding #2 — watch/passthrough precedence untested: Fixed. Added Remaining testing gaps (case sensitivity, duplicates, standard dir overlap, glob in ClassifyChange) are valid observations but lower priority — the core contract is covered by the 12 tests. Happy to add any of these if you want them. |
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Review #2 — Ready to merge
Verdict: Ready to merge — 5 reviewers dispatched (correctness, testing, maintainability, project-standards, previous-comments).
Review #1 Findings Status
| # | Finding | Status |
|---|---|---|
| 1 | Empty type test missing ContainSubstring("type") |
Addressed — commit 61e9289 added the assertion, now consistent with peer tests |
| 2 | Watch/passthrough precedence untested | Addressed — commit 61e9289 added "ClassifyChange prefers watch over passthrough for same directory" test |
Findings
| # | Sev | File | Issue | Route |
|---|---|---|---|---|
| 1 | P3 | watcher_test.go:271-288 |
Three RebuildScopeForChangeType assertions omit (issue #530) while all 8 other assertions in the block include it — cosmetic inconsistency |
advisory → human |
Red/Green Verification (18 tests)
- Config: 4 FAIL red (validation), 3 PASS green (parsing + acceptance)
- Watcher: 6 FAIL red (WatchDirs ×3, ClassifyChange layout/data/precedence), 5 PASS green (ClassifyChange content via fallback, standard dirs, scope ×3)
- Precedence test fails red correctly: ClassifyChange checks passthrough first → returns PassthroughChange, not expected ContentChange
Remaining Testing Gaps
- Nonexistent watch directory behavior (validation vs runtime error)
- Duplicate watch entries (same
from:with different types) - Watch dir overlapping base directories (
from: "content" type: "layout") - Trailing slash in
from:path matching - File extension mismatch under watch directory
All are edge cases that don't block this spec PR.
Verdict: Ready to merge. Both review #1 findings addressed. Test count 17 → 18. Red/green split correct (10 red, 8 green). 1 P3 advisory (missing issue refs in scope assertions).
Run artifact: /tmp/compound-engineering/ce-code-review/20260512-b7f3c901/
Add tests for duplicate from: paths (ambiguous classification), from: overlapping base structure dirs (conflicting ChangeType), and trailing slash normalization (from: "elements/" must work). Add ClassifyChange trailing slash test in watcher. Update PLAN.md and IMPLEMENTATION.md with new validation rules. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Added edge case validation tests in 4f600dc: Duplicate Base dir overlap — Trailing slash normalization — Updated red/green totals: config 6 FAIL / 4 PASS, watcher 6 FAIL / 6 PASS. |
| changeType := server.ClassifyChange("elements/rh-button/docs/overview.md", cfg) | ||
| Expect(changeType).To(Equal(server.ContentChange), | ||
| "trailing slash in from: must not break path matching — "+ | ||
| "the slash should be normalized before ClassifyChange "+ | ||
| "compares prefixes (issue #530)") |
|
|
||
| **Behavior:** `WatchDirs()` includes all watch `from:` directories (or glob roots) in the watch list. `ClassifyChange()` maps files under watch directories to `ContentChange`, `LayoutChange`, or `DataChange` based on their declared `type`. All three types map to `RebuildPipeline` via `RebuildScopeForChangeType()` — this is the key difference from passthrough (which triggers `RebuildRecopy`). Watch directories are checked before passthrough directories in `ClassifyChange` — a directory in both `watch:` and `passthrough:` uses the watch classification. | ||
|
|
||
| **Validation:** `from` must not be empty. `type` must be one of: `content`, `layout`, `data`. Invalid entries produce a validation error with the array index (e.g., `watch[1].type must be content, layout, or data`). Duplicate `from` paths are rejected — ambiguous classification (which type wins?). `from` paths matching base structure directories (`content`, `layouts`, `data`, `assets`, `static`) are rejected — those already have fixed `ChangeType` classification. Trailing slashes in `from` are normalized (stripped), not rejected. |
| - HTTP server with mode-aware behavior (dev/preview) | ||
| - File watcher with debouncing and change classification | ||
| - **Passthrough watching (issue #275)**: `WatchDirs(cfg)` must iterate `cfg.Passthrough` and append each `from:` path. `ClassifyChange(path, cfg)` must add a `PassthroughChange` type for files matching passthrough source directories. The serve rebuild handler should recopy only the changed file to `_site/<to>/<relative-path>` on `PassthroughChange` instead of triggering a full pipeline rebuild. In `alloy dev`, passthrough changes only trigger a browser reload (files are served from source). `addRecursiveWatch` must be called on each passthrough `from:` directory. | ||
| - **Watch directory config (issue #530)**: `WatchMapping` struct in `internal/config/config.go` — `From string`, `Type string` with yaml/toml/json tags. `Watch []WatchMapping` field on `Config` after `Passthrough`. `Validate()`: reject empty `from`, reject `type` not in {content, layout, data}, include array index in error. Reject duplicate `from` paths (build a seen-set, error on collision). Reject `from` matching base structure dirs (content, layouts, data, assets, static). Normalize trailing slashes in `from` (strip before storing/comparing). `WatchDirs()`: loop after passthrough — append `w.From` (or `static.GlobRoot(w.From)` for globs). `ClassifyChange()`: in `default:` branch, check watch dirs before passthrough loop — switch on `w.Type` to return `ContentChange`/`LayoutChange`/`DataChange`. No new `ChangeType` constant needed — reuses existing types. No changes to `cmd/serve.go`, `cmd/watcher.go`, or `RebuildScopeForChangeType` — existing dispatch handles all three types correctly. |
| // WatchMapping registers an external directory for pipeline-triggering | ||
| // file watching during serve mode. | ||
| type WatchMapping struct { | ||
| From string `yaml:"from" toml:"from" json:"from"` | ||
| Type string `yaml:"type" toml:"type" json:"type"` | ||
| } |
zeroedin
left a comment
zeroedin
left a comment
There was a problem hiding this comment.
Review #3 — Ready to merge
Verdict: Ready to merge — 5 reviewers dispatched (correctness, testing, maintainability, project-standards, previous-comments). 0 actionable findings.
New in commit 4f600dc
4 new tests (22 total, up from 18):
- "rejects duplicate watch entries with same from" — addresses review #2 gap #2
- "rejects watch from: that overlaps a base structure directory" — addresses review #2 gap #3
- "normalizes trailing slash in watch from: path" + "ClassifyChange matches watch dir with trailing slash in from" — addresses review #2 gap #4
Plan docs (PLAN.md, IMPLEMENTATION.md) updated with duplicate/overlap/trailing slash rules.
Prior Review Findings
| Review | Finding | Status |
|---|---|---|
| #1 | Empty type test missing ContainSubstring("type") |
Addressed — 61e9289 |
| #1 | Watch/passthrough precedence untested | Addressed — 61e9289 |
| #2 | Scope assertions lack "(issue #530)" | False positive — verified all 3 assertions always included the reference (lines 239, 245, 251) |
Red/Green Verification (22 tests)
- Config: 6 FAIL red (empty from, invalid type, empty type, array index, duplicate from, base dir overlap), 4 PASS green (parsing, valid types, no entries, trailing slash)
- Watcher: 6 FAIL red (WatchDirs ×3, ClassifyChange layout/data/precedence), 6 PASS green (content via fallback, standard dirs, trailing slash via fallback, scope ×3)
Remaining Testing Gaps
- Nonexistent watch directory behavior (runtime vs validation error)
- File extension mismatch under watch directory (e.g., .css under type: content)
- ClassifyChange with glob-pattern watch from: (WatchDirs tests glob root but ClassifyChange doesn't)
- Trailing slash ClassifyChange test passes green via fallback, not via watch-aware normalization — cannot distinguish correct handling from coincidental pass
All are edge cases. None block this spec PR.
Verdict: Ready to merge. All prior findings addressed. Test count 17 → 22 across 3 commits. 0 actionable findings. Clean.
Run artifact: /tmp/compound-engineering/ce-code-review/20260512-58746cbc/
Validate that watch from: pointing to a nonexistent directory fails at config validation time — fail fast with a clear error instead of silently watching nothing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tal pass (#530) The trailing slash ClassifyChange test passed via the default ContentChange fallback, not via watch-aware normalization. Switch to type: layout so the test can only pass if ClassifyChange actually recognizes the watch directory after normalizing the trailing slash. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WatchDirs tests glob root extraction but ClassifyChange had no corresponding test. Add one with type: data to avoid the default ContentChange fallback coincidental pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Final test counts after review feedback: Config (11 tests): 7 FAIL red (empty from, invalid type, empty type, array index, duplicate from, base dir overlap, nonexistent dir) / 4 PASS green (parsing, valid types, no entries, trailing slash normalization) Watcher (13 tests): 8 FAIL red (WatchDirs x3, ClassifyChange layout/data/glob/precedence/trailing-slash) / 5 PASS green (ClassifyChange content [default fallthrough], standard dirs, RebuildPipeline x3) All coincidental green-via-fallback tests have been eliminated or use non-content types to distinguish real handling from default |
Summary
WatchMappingstruct stub andWatchfield toConfigfor compilationfrom, invalid/emptytype, array index), acceptance (all types, no entries)WatchDirsinclusion (basic, mixed, glob),ClassifyChangemapping (content/layout/data + standard dirs),RebuildPipelinescope (all three types)PLAN.mdwith Watch Directories section andIMPLEMENTATION.mdwith implementation guidanceDesign
Watch dirs reuse existing
ContentChange/LayoutChange/DataChange— no newChangeTypeconstants needed. All three triggerRebuildPipeline, unlike passthrough which triggersRebuildRecopy. Watch dirs are checked before passthrough inClassifyChange.Test plan
go test ./internal/config/... --ginkgo.focus="issue #530"— 4 FAIL red (validation not implemented), 3 PASS greengo test ./internal/server/... --ginkgo.focus="issue #530"— 5 FAIL red (WatchDirs/ClassifyChange not implemented), 5 PASS greengo test ./internal/config/... --ginkgo.skip="issue #530"— all existing tests passgo test ./internal/server/... --ginkgo.skip="issue #530"— all existing tests passgo vet ./internal/config/... ./internal/server/...— clean🤖 Generated with Claude Code