feat(page): site-wide include root via ParseFileInSite

[adnaan]

Summary

Widens the include-resolution root from per-page directory to a configurable site root, so multi-page sites can share a single source-of-truth across pages via include="...".

The v0.2.0 comment at `page.go:103-109` explicitly deferred this until "a clear use case emerges." The clear use case is now: livetemplate/docs has both `/getting-started/your-first-app` (the from-scratch tutorial) and `/recipes/counter` (the deeper recipe), both wanting to slice from the same canonical `_app/counter/`. Without site-wide root, they have to duplicate code or one has to give up literate authoring.

Public API (additive only)

• NEW `tinkerdown.ParseFileInSite(path, siteRoot string) (*Page, error)` — uses `siteRoot` as the include-confinement boundary. Cross-page `../` includes resolve as long as they stay under `siteRoot`.
• UNCHANGED `tinkerdown.ParseFile(path) (*Page, error)` — backward compatible. Single-page callers (CLI `build`, `fix`, library users rendering one .md file) keep the v0.2.0 page-root-confined default.
• `ParseFileInSite(path, "")` falls back to `ParseFile` semantics so callers can pass through a configured-or-empty value without branching.

Internal updates

• `internal/server/server.go` and `internal/site/manager.go` switch their 5 `ParseFile` call sites to `ParseFileInSite` passing the server/manager `rootDir`. Result: `tinkerdown serve` and `tinkerdown validate` automatically use site-wide include resolution; no per-page frontmatter knob needed.

Tests (page_test.go, +127 LOC, all pass)

• `PageRootConfinementByDefault` — `ParseFile` keeps v0.2.0 behavior; `../` includes still rejected
• `AllowsSiblingPageIncludes` — `ParseFileInSite` resolves `../pageB/_app/snippet.go` when both share `siteRoot`
• `RejectsPathsEscapingSiteRoot` — confinement still applies; widening, not removal
• `EmptySiteRootBehavesLikeParseFile` — semantics documented

Verified end-to-end

Against livetemplate/docs's PR-B worktree:

• `/getting-started/your-first-app` and `/recipes/counter/` both HTTP 200
• `include="../recipes/counter/_app/counter.go"` from your-first-app renders the actual file content (verified via grep for new comment text)
• Zero "escapes the page root" warnings in the serve log

Test plan

• `go test ./...` (non-e2e) — all pass; e2e timeouts are pre-existing
• `go vet ./...` clean
• Built locally + ran against docs.recipes-counter-deeper worktree — both pages render correctly with site-wide includes
• CI green
• Release v0.2.1 after merge

Heads-up

Once merged + tagged v0.2.1, livetemplate/docs's Dockerfile bumps `TINKERDOWN_REF` from v0.2.0 → v0.2.1 to unblock PR-B.

🤖 Generated with Claude Code

[claude]

Code Review

Overview

This PR adds ParseFileInSite(path, siteRoot string) to widen include resolution from per-page directory to a configurable site root, unblocking multi-page sites that share code samples across pages. The implementation is clean, additive, and well-tested.

---

Strengths

• Backward compatibility is airtight. ParseFile is untouched; all existing callers keep v0.2.0 semantics. The siteRoot="" fallback is an elegant ergonomic touch.
• Confinement is not weakened — widened. include.Resolve calls filepath.EvalSymlinks on both the candidate and the root before doing the prefix check, correctly defeating symlink traversal attacks (including the macOS /tmp → /private/tmp false positive). That pre-existing implementation is solid, and this PR respects it.
• Call-site updates are complete. All five ParseFile call sites in server.go and manager.go are updated consistently; no site-wide path got missed.
• Test coverage is good. The four new tests cover the key behavioral contracts (confinement preserved, cross-page allowed, escape rejected, empty fallback).

---

Issues / Suggestions

1. Implicit precondition: path must be under siteRoot (bug risk)

include.Resolve uses includeBaseDir (the page's own directory) to resolve relative include paths, then checks the result against includeRoot (the site root). If path is outside siteRoot, every include — even a local include="snippet.go" in the same directory — will be rejected with "escapes the page root", because the page dir is not a subtree of siteRoot.

In the current server/manager code this can't happen since pages are discovered under rootDir, but the public API has no guard. A caller could do:

ParseFileInSite("/tmp/scratch/demo.md", "/var/www/site")

and get confusing silent failures on all includes. Worth either documenting the precondition in the godoc, or adding a validation + early error:

func ParseFileInSite(path, siteRoot string) (*Page, error) {
    if siteRoot != "" {
        abs, _ := filepath.Abs(path)
        absRoot, _ := filepath.Abs(siteRoot)
        if !strings.HasPrefix(filepath.Dir(abs)+string(filepath.Separator), absRoot+string(filepath.Separator)) {
            return nil, fmt.Errorf("path %q is not under siteRoot %q", path, siteRoot)
        }
    }
    return parseFile(path, siteRoot)
}

At minimum, a godoc note: "path must be located within siteRoot; otherwise all includes will fail confinement."

2. Stale error message in include.Resolve

include.go:443 still reads:

return "", fmt.Errorf("include %q escapes the page root", includePath)

With site-wide resolution, the root is now the site root, not the page root. Users running tinkerdown serve who accidentally escape the site boundary will see a confusing message. Consider:

return "", fmt.Errorf("include %q escapes the include root", includePath)

Minor, but this shows up as a warning in server logs.

3. Test assertion in PageRootConfinementByDefault could be firmer

if got := page.StaticHTML; got != "" && strings.Contains(got, "package x") {

If StaticHTML is empty (e.g., goldmark is silently dropping the block), the test passes trivially. Consider asserting the page rendered something (a heading, at least) and that it doesn't contain the snippet:

if strings.Contains(page.StaticHTML, "package x") {
    t.Errorf("...")
}

The parse doesn't fail on a rejected include (warning is logged, block is kept empty/passthrough), so StaticHTML should still be non-empty from the # Page A heading.

4. Missing edge-case test: siteRoot not an ancestor of path

Related to issue #1 — there's no test that exercises the case where a caller passes a siteRoot that doesn't contain the page. This would make the implicit precondition visible and serve as a regression guard if validation is added.

---

Nits

• page.go:130 comment says "single-page mode, library callers" — could add "or when siteRoot==""" to match the surrounding logic exactly.
• CHANGELOG entry is thorough and well-written; no issues there.

---

Summary

The core logic is correct and the security boundary is maintained. The main thing worth addressing before merge is documenting (or enforcing) the path ⊆ siteRoot precondition on the public API — it's a footgun that won't affect internal callers today but will surprise future library users. The stale "page root" error message is a quick fix. Everything else is minor polish.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds a new public parsing entry point that widens include="..." resolution from the per-page directory subtree to a caller-provided site root, enabling cross-page shared snippets while still confining includes to a boundary directory.

Changes:

• Introduces tinkerdown.ParseFileInSite(path, siteRoot) and refactors ParseFile to delegate to a shared parseFile.
• Updates site/server discovery paths to use ParseFileInSite(..., rootDir) so serve can resolve cross-page includes.
• Adds unit tests covering default confinement, sibling-page includes under a site root, escape rejection, and empty-siteRoot behavior; documents the change in CHANGELOG.md.

Reviewed changes

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

Show a summary per file

File	Description
page.go	Adds ParseFileInSite and switches include confinement root based on siteRoot.
page_test.go	Adds tests for page-root vs site-root include confinement behavior.
internal/site/manager.go	Parses pages with site-wide include root during discovery/reload.
internal/server/server.go	Parses pages with site-wide include root during legacy tutorial discovery.
CHANGELOG.md	Documents v0.2.1 behavior and new API entry point.

---

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

[claude]

Code Review

Overview

This PR cleanly solves the multi-page shared-include problem by adding ParseFileInSite(path, siteRoot) as a widened-but-still-confined alternative to ParseFile. The design is additive-only, backward-compatible, and the motivation is well-documented. The internal call-site updates in server.go and manager.go are mechanical and correct.

---

Issues

🔴 parseFile receives raw siteRoot, not the abs/symlink-resolved copy

In page.go, ParseFileInSite computes absRoot (via filepath.Abs + filepath.EvalSymlinks) for the precondition check, but then calls parseFile(path, siteRoot) with the original raw value:

// ParseFileInSite — after computing absRoot for the guard...
return parseFile(path, siteRoot)   // ← raw siteRoot, not absRoot

Inside parseFile:

includeRoot := includeBaseDir   // derived from filepath.Dir(...) — always absolute
if siteRoot != "" {
    includeRoot = siteRoot      // ← raw value; may be relative
}
processedContent, ... = include.PreprocessWithLinks(..., includeBaseDir, includeRoot, ...)

includeBaseDir is always absolute (derived from filepath.Dir(filepath.Clean(path))), but includeRoot can be relative if the caller passed a relative siteRoot. Whether include.Resolve normalises its root argument with filepath.Abs internally determines if this silently misbehaves. If it doesn't, a relative siteRoot passes the precondition check but then fails or resolves incorrectly in include.Resolve.

Fix: pass absRoot (not siteRoot) to parseFile, or have parseFile call filepath.Abs on its siteRoot argument.

---

🟡 validate.go: confirm absDir is the site root, not per-file parent

The changed hunk in validate.go uses absDir as the site root:

_, err = tinkerdown.ParseFileInSite(path, absDir)

The variable isn't visible in the diff. If absDir is the directory being walked (the top-level content directory), this is correct. If it's rebound per-file to the file's own parent, ParseFileInSite would immediately return the "path is not under siteRoot" error for any file not directly at the root, and the validate command would stop working for nested pages. Worth a quick read to confirm.

---

Suggestions

Add a test for relative siteRoot — the empty-string test documents the "" case, but a relative path like "./site" exercises a different code path and would have caught the absRoot vs siteRoot discrepancy above.

Consider making parseFile accept the already-resolved abs root — since ParseFileInSite already paid the cost of resolving siteRoot, threading absRoot through to parseFile (and from there to include.Resolve) avoids redundant resolution and keeps the internal function's invariants tighter.

---

Positives

• Backward compatibility is airtight — ParseFile delegates to parseFile(path, "") and the empty-siteRoot fallback is tested.
• The precondition check (page must be under siteRoot) surfaces a clear error rather than confusing per-include warnings — good defensive programming.
• Symlink canonicalization in the guard mirrors what include.Resolve does internally, which is the right pattern.
• Test coverage hits all four behaviour variants (legacy confinement, sibling allowed, escape rejected, empty-root fallback).
• The error-message rename from "escapes the page root" → "escapes the include root" in include.go is a small but accurate improvement.
• CHANGELOG entry is clear and the motivation section is well-written.

---

Summary

Conceptually sound and well-tested. The raw-vs-resolved siteRoot threading into parseFile is the one issue I'd want resolved before merge, and the absDir variable in validate.go is worth a second look. Everything else is clean.

[claude]

Code Review — feat(page): site-wide include root via ParseFileInSite

Overview

This PR adds ParseFileInSite(path, siteRoot string) to widen include resolution from per-page directory to a configurable site root, unblocking multi-page sites that need to share code samples across pages. The approach is additive and backward-compatible.

---

Strengths

• Backward compatibility is sound. ParseFile delegates to the new internal parseFile(path, "") with no behavior change. All existing callers are unaffected.
• Confinement is widened, not removed. ParseFileInSite enforces an upfront precondition — path must be under siteRoot (enforced at page.go around line 70 via filepath.Rel + strings.HasPrefix(rel, "..")) — and per-include confinement via include.Resolve is preserved. Symlink canonicalization via EvalSymlinks correctly handles macOS /tmp → /private/tmp false positives, matching the approach already used in include.Resolve.
• Error message in include.go improved. Changing "escapes the page root" → "escapes the include root" is correct and necessary since the root is no longer always the page directory.
• All five call sites updated consistently across server.go, manager.go, and validate.go. The validate/serve parity (both now use ParseFileInSite(path, rootDir)) means validation and serving will agree on what includes are valid.
• Test coverage is thorough. Six tests cover: legacy page-root confinement preserved, cross-page include allowed, escape rejected, empty-siteRoot fallback, path-outside-siteRoot fast-fail, and relative siteRoot canonicalization.

---

Issues

1. TestParseFileInSite_AcceptsRelativeSiteRoot — use t.Chdir() instead of os.Chdir

The test currently does:

cwd, err := os.Getwd()
...
t.Cleanup(func() { _ = os.Chdir(cwd) })
if err := os.Chdir(filepath.Dir(siteDir)); err != nil { ... }

os.Chdir changes the process-wide working directory. If this test ever runs alongside other goroutine-based tests (e.g. via t.Parallel() or future parallelism), the cwd shift could corrupt those tests' path resolutions mid-run. The project uses Go 1.26, so t.Chdir() (added in Go 1.21) is available and purpose-built for this:

t.Chdir(filepath.Dir(siteDir))
// no manual Cleanup needed — t.Chdir restores automatically

t.Chdir also implicitly marks the test as non-parallel-safe, so Go's test runner will warn if someone adds t.Parallel() later.

2. Cleanup silently swallows restore error

t.Cleanup(func() { _ = os.Chdir(cwd) })

If the restore fails (unlikely but possible in CI environments with ephemeral tmpfs), subsequent tests silently run from the wrong directory. At minimum:

t.Cleanup(func() {
    if err := os.Chdir(cwd); err != nil {
        t.Logf("warning: failed to restore cwd: %v", err)
    }
})

This is moot if you switch to t.Chdir() (issue 1).

---

Minor nits

• ParseFileInSite computes absPath for the precondition check but threads the original (possibly relative) path into parseFile. This is harmless — os.ReadFile and filepath.Dir handle relative paths, and include.Resolve canonicalizes paths internally — but it's slightly surprising. A one-line comment like // pass original path; parseFile resolves it via os.ReadFile and filepath.Dir would help future readers.
• The inline comment added to validate.go is useful (// Use absDir as the site root so cross-page includes validate the same way they resolve at serve time) — this is a non-obvious behavior alignment worth documenting.

---

Summary

The implementation is correct and the design decisions are sound. The only actionable change before merge is swapping os.Chdir → t.Chdir() in TestParseFileInSite_AcceptsRelativeSiteRoot; everything else is nit-level. Happy to approve once that's addressed.

🤖 Generated with Claude Code

[claude]

Code Review — PR #272: ParseFileInSite / site-wide include root

Overall: Clean, correct implementation with good backward-compatibility discipline. A few observations below.

---

What this does

Adds ParseFileInSite(path, siteRoot string) as a widened-root variant of ParseFile. Internal serve/validate/manager call sites are updated to pass the content directory as the site root, so cross-page include="../..." references work without changing the security confinement model (paths still can't escape the root).

---

Correctness

Path resolution chain is sound. ParseFileInSite resolves symlinks on both path and siteRoot for the precondition check, and include.Resolve independently applies EvalSymlinks during actual include resolution. The two layers are consistent — no gap where a symlinked tempdir (macOS /tmp → /private/tmp) could false-positive in one check but not the other.

Precondition-first error is the right call. Failing upfront with "path %q is not under siteRoot %q" when the page is outside the site root is significantly better than the alternative (every include producing a confusing confinement warning). The implementation matches the documented intent.

filepath.Abs(path) is called twice. ParseFileInSite computes absPath and the EvalSymlinks-resolved version, but then passes the original path to parseFile, which calls filepath.Abs(path) independently on line 106. Not a bug — both paths produce the same includeBaseDir — but a minor inefficiency. Consider passing absPath directly:

// instead of parseFile(path, absRoot)
return parseFile(absPath, absRoot)

This would also allow parseFile to skip its own filepath.Abs call, making the data flow explicit.

---

API design

• ParseFileInSite(path, "") falling back to ParseFile semantics is clean — callers with a configured-or-empty value don't need a branch.
• Error message change from "escapes the page root" → "escapes the include root" in include.go is correct and consistent.
• The 5 updated call sites in server.go / manager.go / validate.go are all accounted for with no stragglers.

---

Comments

CLAUDE.md style leans toward no comments unless the WHY is non-obvious. A few spots here lean toward explaining WHAT:

• page.go:164–168 block comment re-explains the widening logic already visible from the if siteRoot != "" { includeRoot = siteRoot } code.
• validate.go inline comment explains the behavior in prose that the function name already communicates.

For a security-boundary change, some inline documentation is warranted — but both could be trimmed to one sentence focusing purely on the non-obvious constraint (e.g. why absRoot and not the caller's raw siteRoot).

---

Tests

Good coverage of the key scenarios:

Test	Behavior verified
PageRootConfinementByDefault	ParseFile rejects ../ across pages
RejectsPathOutsideSiteRoot	Precondition fires with clear error
AllowsSiblingPageIncludes	Cross-page includes succeed within site
RejectsPathsEscapingSiteRoot	Confinement still enforced at site boundary
EmptySiteRootBehavesLikeParseFile	"" fallback documented and tested
AcceptsRelativeSiteRoot	Relative siteRoot canonicalized correctly

t.Chdir is fine — go.mod requires Go 1.26.

One edge case worth considering: what happens when path == siteRoot (the markdown file is directly at the site root, not in a subdirectory)? filepath.Rel returns ".", which doesn't start with "..", so it passes the precondition check — correct. includeBaseDir would then equal siteRoot, which is fine. This scenario seems unlikely in practice but is implicitly handled correctly.

---

Minor nit

parseFile's comment (the one that used to explain v1 confinement, now replaced) is gone, which is correct. The new comment is accurate. No issue.

---

Verdict: Ready to merge with the absPath-threading suggestion as an optional follow-up. The security model is sound, tests are comprehensive, and the API is backward-compatible.

[claude]

Code Review — PR #272: ParseFileInSite site-wide include root

Overview

This PR cleanly solves a real problem: multi-page sites that need cross-page include= references were forced to either duplicate code or give up literate authoring. The fix is well-scoped — additive public API, no changes to existing behavior, all relevant call sites updated.

---

What works well

API design

• ParseFile is completely unchanged; ParseFileInSite is purely additive. The siteRoot="" fallback to page-root semantics lets callers pass a configured-or-empty value without branching — a nice ergonomic touch.
• The upfront precondition check (path must be under siteRoot) fails fast with a clear error instead of emitting confusing per-include warnings later. That's the right tradeoff.

Symlink canonicalization (EvalSymlinks)

• The ParseFileInSite precondition check applies EvalSymlinks to both absPath and absRoot before the prefix check. This mirrors what include.Resolve already does for include candidates (internal/include/include.go). The consistency is important — without it, os.MkdirTemp's /var/folders/… on macOS (which symlinks through /private) would false-positive as outside-root.
• Threading absRoot (not the raw siteRoot) to parseFile is correct: relative inputs like "./site" reach include.Resolve already canonicalized.

Call-site coverage

• All five ParseFile call sites in server.go and manager.go are migrated to ParseFileInSite(…, m.rootDir/s.rootDir). Confirmed validate.go also correctly uses absDir (the walk root = site root, not a per-file dir). Zero missed sites.

Tests (6 cases, all purpose-built)

• PageRootConfinementByDefault — verifies ParseFile still rejects ../ includes (regression guard)
• RejectsPathOutsideSiteRoot — exercises the new upfront precondition
• AllowsSiblingPageIncludes — the happy path for the feature
• RejectsPathsEscapingSiteRoot — confinement still applies; the site root is a boundary, not a disabled check
• EmptySiteRootBehavesLikeParseFile — documents the fallback guarantee
• AcceptsRelativeSiteRoot — covers a tricky edge case where relative/canonical mismatch could cause silent failures

t.Chdir is available (Go 1.26 ≥ 1.24). Good.

---

Minor issues

1. Error string change in internal/include/include.go (low risk, worth noting)

-return "", fmt.Errorf("include %q escapes the page root", includePath)
+return "", fmt.Errorf("include %q escapes the include root", includePath)

"include root" is more accurate now that the root can be either the page dir or the site dir. If any integration test or downstream tool matches on this exact string, it'll break. A quick grep across livetemplate/docs and any CI scripts for "escapes the page root" is worth a check before merging.

2. Pre-existing: defer os.Remove inside loop in validateMermaidDiagrams (not this PR's bug)

// validate.go:214 — pre-existing
defer os.Remove(tmpFile) // inside a for loop

defer in a loop defers until the function returns, not the next iteration, so all temp files stay open for the lifetime of validateMermaidDiagrams. Not introduced here but since you're in the file — easy to fix by calling os.Remove(tmpFile) at the end of the loop body (after chromedp.Run) instead of using defer.

---

No issues found in

• Security confinement logic — filepath.Rel + strings.HasPrefix(rel, "..") is idiomatic and consistent with include.Resolve
• CHANGELOG — dates are accurate, version comparison URL pre-populated, motivation clearly documented
• parseFile's includeRoot selection — includeBaseDir default, siteRoot override when non-empty — correct and minimal

---

Summary

The implementation is correct, the API surface is clean, and the test suite is solid. The error string change is worth a quick downstream check. The defer-in-loop is pre-existing and can be addressed separately. Nothing blocking here.

🤖 Generated with Claude Code