fix(pipeline): preserve enriched SiteData across incremental rebuilds

[zeroedin]

Summary

• Adds SiteData field to BuildResult so dev.go can capture enriched data (external sources + onDataFetched hooks) from Build()
• Injects enriched SiteData into PipelineState after InitPipelineState() at dev server startup, and after full-rebuild fallback
• Re-runs onDataFetched hook after data file reload in BuildIncremental, mirroring the enrichment path in Build()

Fixes #721

Test plan

• All 3 spec tests from PR spec(pipeline): pagination data persistence across incremental rebuilds (#721) #722 pass (183/184, 1 pre-existing Node-not-in-PATH failure)
• Manual: run alloy dev, edit a content file unrelated to paginated pages, verify pagination data persists
• Manual: edit a data file, verify onDataFetched re-enriches the reloaded data

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR aims to fix dev-server incremental rebuild correctness by keeping PipelineState.SiteData aligned with the “enriched” site data produced by Build() (data files + external sources + onDataFetched hook output), so pagination and other site-data consumers don’t lose data across rebuilds.

Changes:

• Add SiteData to pipeline.BuildResult so callers (notably cmd/dev.go) can reuse the enriched data.
• Update cmd/dev.go to copy Build().SiteData into the cached PipelineState at startup and after full-rebuild fallbacks.
• Update BuildIncremental to re-run onDataFetched after data-directory reloads and merge returned keys back into SiteData.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
internal/pipeline/incremental.go	Re-runs onDataFetched and merges returned keys after reloading data files during incremental rebuilds.
internal/pipeline/build.go	Extends BuildResult to include the enriched SiteData produced during Build().
cmd/dev.go	Copies enriched SiteData from full builds into the long-lived PipelineState used for incremental rebuilds.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[zeroedin]

Code Review — PR #730

fix(pipeline): preserve enriched SiteData across incremental rebuilds

8 reviewers dispatched. 14 raw findings deduplicated to 6.

---

Verdict: Ready with fixes

---

#1 [P1] Nil dereference of ps at cmd/dev.go:185

ps.SiteData = fullResult.SiteData dereferences ps without a nil check. If InitPipelineState at line 135 fails, ps is nil (the error is logged as a warning and execution continues). When the watcher later triggers a full rebuild, line 185 panics.

The initial SiteData injection at line 139 correctly guards ps != nil. This new line does not.

Fix: Add the nil guard to match line 139:

if ps != nil && fullResult != nil && fullResult.SiteData != nil {
    ps.SiteData = fullResult.SiteData
}

#2 [P2] BuildIncremental does not populate BuildResult.SiteData

Build() populates result.SiteData at both return paths (lines 557, 942). BuildIncremental() never sets it (line 512). Today this works only because BuildIncremental mutates ps.SiteData in-place via the shared PipelineState pointer, so dev.go already sees the updated data. But the contract implied by BuildResult is that SiteData is always available. Any future consumer that reads incrResult.SiteData will silently get nil.

Fix: Add SiteData: ps.SiteData, to the BuildResult literal at incremental.go:512.

#3 [P2] Duplicated onDataFetched hook merge logic — advisory

The type-switch pattern merging hook results (map[string]interface{} vs *ordered.Map) into siteData is copy-pasted between build.go:283-292 and incremental.go:203-214. A future change to the merge logic (e.g., supporting a third return type) must be applied in both places. Consider extracting a helper like mergeSiteDataHookResult(target map[string]interface{}, result interface{}). Each call site retains its own error handling for RunWithTimeout.

Not blocking — the two copies are small and the error semantics intentionally differ (see #4).

#4 [P2] onDataFetched error handling inconsistency — advisory

Build() treats hook failure as fatal (return nil, fmt.Errorf(...)). BuildIncremental() logs a warning and continues. This is a reasonable design choice for dev server resilience, but worth a brief comment documenting the intent. No action required for merge.

#5 [P2] Testing gaps — downstream

• BuildResult.SiteData has zero test assertions. The field assignment could be removed from both Build() return paths with no test failure.
• The onDataFetched hook enrichment path in BuildIncremental (the core fix) has zero direct test coverage — all spec tests from PR spec(pipeline): pagination data persistence across incremental rebuilds (#721) #722 use empty plugin registries and never exercise the hook.

Per the project's spec-first workflow, these are for the architect to address in a spec PR.

#6 [P3] Pre-existing: duplicate pages in pagesToRender — advisory

When hasDataChange is true, paginated pages are unconditionally appended to pagesToRender even if already present from content-hash invalidation. The post-pagination renderRelPaths set deduplicates by RelPath, so this is cosmetic (affects only the skipped counter). Pre-existing, not introduced by this PR.

[zeroedin]

Review response

All actionable items fixed in 6342220.

#1 [P1] — Fixed. Added ps != nil guard on the full-rebuild SiteData update at dev.go:191.

#2 [P2] — Fixed. Added SiteData: ps.SiteData to BuildIncremental's BuildResult at incremental.go:523.

#3 [P2] — Acknowledged. Two small copies, intentionally different error semantics (see #4). Will extract a helper if a third call site appears.

#4 [P2] — Acknowledged. Intentional: Build() is fatal because a broken hook during initial build means the site is wrong. BuildIncremental() logs and continues for dev server resilience — a transient hook failure shouldn't kill the watcher loop.

#5 [P2] — Downstream. BuildResult.SiteData assertions and onDataFetched hook coverage in BuildIncremental need spec tests from the architect.

#6 [P3] — Acknowledged. Pre-existing cosmetic issue, not introduced by this PR.

Additionally addressed Copilot's feedback: both SiteData injection points in dev.go (startup and full-rebuild fallback) now call rt.SetSiteData on all plugin runtimes so QuickJS alloy.data stays consistent.