feat: source-include for literate docs (A2/A3)

[adnaan]

Builds on top of #251. Targets feat/literate-and-embed-lvt so the diff stays scoped to the new include feature; once #251 lands the base will rebase to main.

Summary

Adds an include="..." fence attribute that pulls a file slice into the rendered code block. Paired with embed-lvt from #251, this delivers the literate "read the code, see it run" experience without any new runtime — snippets are sliced from the same .go / .tmpl files the deployed app uses, so there's no drift between docs and production.

Authoring

The state — pure data, cloned per session:

```go include="./_app/counter.go" lines="5-8"
```

The handler — note the broadcast call on line 20:

```go include="./_app/counter.go" lines="13-35" highlight="20"
```

The template:

```html include="./_app/counter.tmpl" lines="10-13"
```

Click below — your changes round-trip to the deployed counter:

```embed-lvt path="/apps/counter/" upstream="http://127.0.0.1:9090"
```

Five include features

Feature	Syntax	Purpose
Single line range	lines="5-8"	Slice a contiguous range, gracefully clamps if file shrinks
Multi-range	lines="5-8,15-22"	Multiple ranges joined with a language-appropriate ellipsis (// ..., # ..., <!-- ... -->)
Named region	region="state" + // >>> region:state / // <<< region:state markers	Survives line-number drift; works with any single-line comment style
Line highlight	highlight="20" (file-absolute)	Prism Line Highlight overlay on the cited line(s); aligns with the gutter
Source-link footer	auto-generated from source_repo + source_path frontmatter	Each snippet shows a counter.go:13-35 link to the cited file at the cited range

The default git ref for footer links is set from tinkerdown.DefaultSourceRef, which cmd/tinkerdown/main.go populates with the binary's release version (via existing ldflags). Released docs always link at the matching tag; dev builds fall back to main.

Examples

• examples/literate-counter-include/ — A2: real deployable counter app (_app/) with state struct + Increment / Decrement / Reset handlers, paired with a docs page that includes ranges from each.
• examples/literate-linked-include/ — A3: two embed-lvt blocks against the same upstream with session="tour". The counter uses ctx.BroadcastAction("Increment", nil) and a constant-groupID authenticator so all connections share state. Clicking in one region updates both, and either matches a tab visiting :9090 directly.

Implementation

• internal/include/include.go — Resolve, Slice, SliceRanges, Dedent, FindRegion, LanguageEllipsis, Preprocess / PreprocessWithLinks
• parser.go + page.go — preprocess includes after auto-tasks/auto-tables; track IncludedFiles per page
• internal/server/watcher.go — SetExtraFiles so the file watcher fires reload on included-file changes
• internal/server/server.go — wires Prism Line Highlight + Line Numbers plugins (vendored from prismjs@1.29.0)
• Footer renders inline raw HTML so each include= block carries class="line-numbers", data-start=, optional data-line= / data-line-offset=

Bug fixes found during testing

• client/src/blocks/embed-lvt-block.ts: rename pending wrapper IDs to be unique-per-block. Without this, two embeds pointing at the same upstream collide on LiveTemplate's per-id event-delegator key (livetemplate generates one wrapperID per template, not per session) — clicks in one region silently route to the other's delegator. Surfaced building the linked example. Tightly relevant to feat: literate show-source and embed-lvt for rich interactive docs #251 but landing here since that PR isn't merged yet.
• Nested fence handling in Preprocess — markdown examples that contain LANG include="..." text inside a wrapping fence (e.g. inside the literate-docs.md guide itself) now pass through verbatim instead of accidentally triggering substitution.

Test plan

• Unit tests pass workspace-wide: GOWORK=off go test -tags=ci -count=1 ./... — 16 packages green
• internal/include unit tests: 28 cases (line-range parsing, path confinement, dedent, slice, multi-range, named regions, highlight, link-footer, nested-fence pass-through, full Preprocess loop)
• chromedp e2e: TestInclude_RendersSlice, TestInclude_HotReload, TestInclude_PathConfinement
• Manual iPhone-over-Tailscale on both examples — handlers, state struct, template all render with line numbers; broadcast call highlighted via Prism overlay; cross-region (R1↔R2) and cross-tab (:9090 direct ↔ embed) sync verified

Out of scope (deliberately deferred)

• Multi-range gutter numbering with non-contiguous file lines (current behavior shows sequential gutter numbers from the first range's start; lines="5-8,15-22" displays gutter 5,6,7,8,9,10... rather than skipping)
• Remote includes (include="https://...")
• WASM / child-process Go execution for inline go server blocks — the include= primitive plus embed-lvt covers the literate use case without a new runtime

🤖 Generated with Claude Code

[claude]

Code Review — PR #256: source-include for literate docs (A2/A3)

Great feature overall. The design is clean, the security posture (path confinement, HTML escaping) is solid, and the test coverage is thorough. A few issues worth addressing before merge:

---

Bugs / Correctness

replace directives break out-of-box example builds

Both examples/literate-counter-include/_app/go.mod and examples/literate-linked-include/_app/go.mod use:

replace (
    github.com/livetemplate/livetemplate => ../../../../../../livetemplate
    github.com/livetemplate/lvt         => ../../../../../../lvt
)

This assumes a very specific sibling-repo layout that won't exist for contributors who clone only tinkerdown. go mod tidy will fail with "directory not found". Either pin published module versions, or add a note in the READMEs about the required directory layout.

isIncludedFile is called redundantly inside the watcher callback

In server.go, isIncludedFile rescans all pages on every watcher-callback invocation, even though the watcher's Start() loop has already filtered events using SetExtraFiles. The double-scan isn't incorrect, but it means a full page scan happens on every .md file save event too (watcher calls onReload for .md files and the callback checks isIncludedFile before the isPageFile/isSourceFile path). For large sites this is O(pages × includes-per-page) on every hot-reload.

---

Performance

Prism line-highlight and line-numbers are loaded unconditionally on every page

// server.go ~line 2902
<link href="/assets/prism-line-highlight.css" rel="stylesheet" />
<link href="/assets/prism-line-numbers.css" rel="stylesheet" />
// and the corresponding <script> tags

These four assets (~30 KB combined) are injected into every rendered page, even pages with no include= blocks. Consider gating this on len(page.IncludedFiles) > 0 so pages that don't use the feature pay no cost.

---

Code Quality

escapeAttr/escapeText re-implement html.EscapeString by hand

func escapeAttr(s string) string {
    r := strings.NewReplacer(`&`, "&amp;", `"`, "&quot;", `<`, "&lt;", `>`, "&gt;")
    return r.Replace(s)
}

This is correct but creates a new Replacer on each call. The idiomatic approach is html.EscapeString (already in stdlib) and a pre-built strings.Replacer for the " variant. Alternatively, html.EscapeString + strings.ReplaceAll(s, """, """).

DefaultSourceRef global var can cause test interference

tinkerdown.go exports a package-level var DefaultSourceRef string that main.go mutates at startup. Any parallel test that exercises PreprocessWithLinks with an empty Branch and a non-zero DefaultSourceRef (e.g., set by a previously-run test binary) will get unexpected footers. Consider making this explicit test-level state (e.g., pass the ref as a parameter, or reset it in test helpers).

source_path directory vs. file semantics

// page.go
linkOpts.PagePathInRepo = filepath.ToSlash(filepath.Dir(fmPre.SourcePath))

The frontmatter field is documented as source_path: "examples/literate-counter-include/index.md" (a file path), but the code strips the filename. The field name and its documented value imply a file, but only the directory is used. Either rename the field to source_dir or accept a directory directly to avoid the implicit Dir() — the current approach will silently misbehave if someone sets source_path: "examples/literate-counter-include" (no filename).

---

Security

URL is HTML-escaped but not URL-encoded

url := strings.TrimRight(link.RepoURL, "/") + "/blob/" + branch + "/" + repoPath
// ...
`<a href="` + escapeAttr(url) + `"`

escapeAttr encodes HTML special chars but not URL-unsafe characters (spaces, non-ASCII) that could appear in a repo path. The content is author-trusted so it's low risk, but using url.PathEscape on each path segment before joining would make the output more robust.

---

Minor

• go 1.26.0 in example go.mod files — Go 1.26 is unreleased as of this review. Intentional?
• Example go.sum files include testcontainers-go, chromedp, and Docker-related packages as transitive dependencies (bleeding in from the livetemplate library's test deps). These add ~40 entries to the sum and may confuse contributors. Worth running go mod tidy -e against a clean module to see if they can be pruned.
• The "out of scope" note about multi-range gutter numbering (lines="5-8,15-22" shows sequential gutter instead of per-segment line numbers) is a good callout. A // TODO comment in the data-start emit block pointing to a follow-up issue would help future contributors find it.

---

What's working well

• Path confinement in Resolve (symlink-following + canonical comparison) is correct and well-tested.
• Nested fence pass-through (the "literate docs show include= syntax without triggering substitution" case) is a subtle correctness requirement and it's handled and tested.
• region= marker approach for drift-resistant snippets is a clean authoring UX.
• SetExtraFiles + sync.RWMutex in the watcher is the right threading model.
• The data-line-offset = start - 1 calculation for Prism Line Highlight alignment is correct.
• Graceful degradation (bad include passes through as an empty block with a warning) matches the existing embed-lvt posture.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR adds a literate-documentation “source include” feature that lets markdown code fences pull and render slices of real source files (with optional line highlights and source-link footers), and wires the server/file-watcher so included-file edits trigger hot reload—complementing embed-lvt for “read the code, see it run” docs.

Changes:

• Add include="..." (with lines=, region=, highlight=) preprocessing that substitutes fenced blocks with sliced file contents and optional GitHub source links.
• Track per-page IncludedFiles and update the watcher/server reload path to re-discover + reload when an included file changes.
• Vendor Prism line-numbers/line-highlight plugins and fix embed-lvt wrapper ID collisions for multiple embeds of the same upstream.

Reviewed changes

Copilot reviewed 24 out of 31 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tinkerdown.go	Adds DefaultSourceRef and Page.IncludedFiles to support include-footers + watcher tracking.
parser.go	Adds source_ref frontmatter field for source-link pinning.
page.go	Runs include preprocessing and stores IncludedFiles on the page.
internal/server/watcher.go	Adds an “extra files” set so non-.md included files can trigger reloads.
internal/server/server.go	Pushes included-file paths into the watcher; reloads on included-file changes; serves Prism plugins; adds footer CSS.
internal/include/include.go	New include resolver/slicer/preprocessor implementation (path confinement, ranges/regions, dedent, footer).
internal/include/include_test.go	Unit tests for include parsing/slicing/regions/footers.
internal/assets/assets.go	Exposes getters for Prism line-highlight/line-numbers plugin assets.
internal/assets/vendor/prism/prism-line-numbers.min.js	Vendored Prism line-numbers plugin JS.
internal/assets/vendor/prism/prism-line-numbers.min.css	Vendored Prism line-numbers plugin CSS.
internal/assets/vendor/prism/prism-line-highlight.min.js	Vendored Prism line-highlight plugin JS.
internal/assets/vendor/prism/prism-line-highlight.min.css	Vendored Prism line-highlight plugin CSS.
internal/assets/client/tinkerdown-client.browser.js	Updates embed-lvt wrapper ID behavior in the built client bundle.
client/src/blocks/embed-lvt-block.ts	Makes data-lvt-id unique per embed block to avoid event-delegator collisions.
cmd/tinkerdown/main.go	Propagates release version into tinkerdown.DefaultSourceRef for source-link pinning.
include_e2e_test.go	Adds e2e coverage for include rendering, hot reload, and path confinement.
docs/guides/literate-docs.md	Documents include= authoring, ranges/regions/highlight, hot reload, and source-link footers.
examples/literate-counter-include/index.md	Adds an example page using include fences + embed-lvt.
examples/literate-counter-include/README.md	Adds run instructions for the example.
examples/literate-counter-include/_app/*	Adds a demo LiveTemplate counter app used as “real source” for includes.
examples/literate-linked-include/index.md	Adds a linked-embeds example demonstrating shared state across multiple embeds.
examples/literate-linked-include/_app/*	Adds the linked-embeds counter app.

---

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

[claude]

Code Review -- PR 256: source-include for literate docs (A2/A3)

Overview

This PR delivers a clean, well-scoped feature: the include=... fence attribute lets docs pages cite line ranges from real source files. The implementation -- preprocess markdown before goldmark, emit raw HTML pre blocks so Prism plugins can read their config attributes -- is pragmatic and fits the existing codebase. The bug fix for colliding data-lvt-id values on same-upstream embeds is well-motivated and correctly handled.

Security

renderSourceFooter -- source_repo is not validated as an http(s) URL. link.RepoURL comes directly from the source_repo frontmatter field. If an author sets source_repo to a javascript: URI, the generated footer link is a working XSS vector. The escapeAttr call prevents quotes from breaking the attribute, but a javascript: scheme passes through untouched. Add a scheme allowlist check (reject anything not starting with https:// or http://) before constructing the href. Since frontmatter is author-controlled the blast radius is small, but the fix is a one-liner.

Path-traversal confinement -- solid. The EvalSymlinks-before-filepath.Rel approach correctly handles macOS /tmp -> /private/tmp aliasing. The e2e confinement test using an absolute path for the secret file is a good edge case to cover.

Correctness

embed-lvt-block.ts: removed null-guard changes behavior on missing attribute. The old code guarded setAttribute with a null-check; the new code using the empty-string fallback always writes data-lvt-id with a leading dash when the original attribute is absent. In practice the attribute should always be present, but an early-return guard would make the intent explicit.

FindRegion -- duplicate region names use first occurrence silently. If the start marker appears twice in a file (copy-paste mistake), the first pair is used with no diagnostic. A warn-on-duplicate would help authors catch stale markers.

Test Coverage

TestInclude_HotReload does not test the WebSocket push path. The test manually calls srv.Discover() then navigates again, verifying re-rendering picks up new content -- but not that the watcher SetExtraFiles / event-loop path actually fires a WebSocket reload. The watcher integration is the new piece; a follow-up test that mutates the file, waits for the WebSocket broadcast, and asserts the DOM updated without a full navigation would be more faithful.

Missing unit tests worth adding:

• region= and lines= mutual exclusion (that region= wins and emits the warning)
• renderSourceFooter rejects a javascript: scheme (once the fix above lands)
• Multi-range edge cases: overlapping ranges, reversed ranges

Minor Issues

Repetitive Prism-asset handlers in server.go. The four new handlers (prism-line-highlight.js/css, prism-line-numbers.js/css) are structurally identical. A small helper would reduce the duplication. Style only.

Both example _app directories share the same go.mod module path. literate-counter-include/_app and literate-linked-include/_app both declare module example.com/literate-counter. This causes go work confusion if someone adds them to a workspace. The linked example should use example.com/literate-linked-counter.

highlight= plus multi-range lines= misalignment is not surfaced to authors. The PR notes that only the first range start feeds data-start, so highlight= in a multi-range block is off for ranges after the first. Consider emitting a warning log when highlight= is combined with comma-separated lines=.

DefaultSourceRef is a mutable package-level variable. Acceptable for set-once-at-startup, but parallel tests that mutate it would race. A server config field or functional option would scale better.

Strengths

• Nested-fence passthrough logic (non-include openers copy their body verbatim) prevents docs pages from accidentally expanding their own authoring examples -- a tricky edge case that is explicitly tested.
• Dedent handles mixed-indent snippets correctly by operating on the literal whitespace prefix.
• LanguageEllipsis covers the practical language set without over-engineering.
• Loading Prism plugins only on pages with include= blocks is a good performance touch.
• Unit test coverage of the include package (28 cases) is thorough for happy paths and confinement.

Summary

The core approach is sound and the implementation is clean. Three things worth resolving before merge:

1. Block javascript:-scheme values in renderSourceFooter (security, one-liner).
2. Restore the null-guard in the TypeScript attribute rename (correctness).
3. Fix the duplicate go.mod module name in literate-linked-include/_app.

The hot-reload test gap and multi-range highlight limitation are good follow-up tickets rather than blockers.

Generated with Claude Code (https://claude.com/claude-code)

[claude]

Code Review: PR #256 — include= fence attribute

Overall this is a well-designed feature with solid security fundamentals and good documentation. A few issues worth addressing before merge.

---

Strengths

• Clean architecture: Layered separation of path resolution → slicing → dedenting → HTML rendering in internal/include/include.go is easy to follow and extend.
• Security-first path confinement: Uses filepath.EvalSymlinks on both the candidate and the doc-root before comparison, which correctly handles macOS /tmp → /private/tmp and symlink traversal attempts. The TestInclude_PathConfinement E2E test validates real escape attempts.
• HTML safety: All user-controlled content goes through html.EscapeString; source link RepoURL is validated to allow only http/https (no javascript: injection); URL path segments use url.PathEscape.
• Graceful degradation: Missing files, bad ranges, and path-confined paths all emit warnings and fall back to an empty block rather than crashing the build.
• Thorough docs + examples: Both examples/literate-counter-include/ and examples/literate-linked-include/ are runnable and cover the key authoring patterns.

---

Issues to Address

1. No file size limit on os.ReadFile (include.go ~line 1924)

A large included file (logs, generated code, vendored JSON) will be read entirely into memory. Consider a configurable cap (e.g., 10 MB) with a clear error message rather than silent OOM on a docs build.

// consider something like:
if fi, err := os.Stat(absPath); err == nil && fi.Size() > maxIncludeBytes {
    return "", fmt.Errorf("include %q: file too large (%d bytes)", path, fi.Size())
}

2. Multi-range + highlight= produces misleading output (include.go ~lines 1707–1722)

When lines="5-8,15-22" and highlight="20" are both set, data-start is pinned to the first range's start (line 5) and the gutter runs sequentially from there — so the Prism Line Highlight overlay for line 20 will land on the wrong visual row. This is a known limitation for non-contiguous gutter numbering, but the combination silently produces confusing output. Suggest logging a warning when both multi-range and highlight= are present together, e.g.:

warn: include "./counter.go": highlight= with multi-range lines= may produce incorrect overlay positions

3. Redundant ".." guard (include.go ~line 1895)

strings.HasPrefix(rel, "..") || rel == ".."

rel == ".." is already matched by strings.HasPrefix. Minor cleanup but avoids the implication that the two conditions are meaningfully distinct.

4. Missing tests for source link rendering

The link-footer logic (lines ~1755–1797) — including relative path calculation, RepoURL validation, url.PathEscape — has no dedicated unit test. It's easy to regress the URL construction silently. A table-driven test covering: happy path, path outside page dir (no footer), missing source_repo frontmatter, and an ftp:// URL that should be rejected, would give good coverage.

5. Region marker deduplication is silent

If a file accidentally has two >>> region:state markers, FindRegion uses the first start + first end-after-start, silently producing a truncated slice. A warning log when a second open-marker is found for an already-found region would help authors catch the mistake.

---

Minor Observations

• No caching of repeated includes within a page: two include="./same.go" blocks cause two os.ReadFile calls. Acceptable now, but a single-page read-cache in Preprocess would be a small optimization if large pages become common.
• Warning message format inconsistency: some warn messages use %q for paths (which adds outer quotes), others don't — a consistent style would help users parse build output.
• embed-lvt wrapper ID fix (embed-lvt-block.ts): The per-block unique ID disambiguation is the right fix for the LiveTemplate delegator collision. The comment explaining the root cause (wrapperID is per-template, not per-session) is good — keep it.

---

Test Coverage Summary

Area	Unit	E2E
Line range parsing	✅	✅
Path confinement	✅	✅
Dedent	✅	—
Slice / multi-range	✅	—
Named regions	✅	— (no E2E)
Hot reload	—	✅
Source link rendering	❌	❌
Multi-range + highlight interaction	❌	❌

---

CLAUDE.md Note

The CLAUDE.md instructs checking livetemplate/client before implementing lvt-* attributes. This PR does not introduce a lvt-* attribute — the include= feature is a Tinkerdown-level Markdown preprocessor, not a LiveTemplate fence. No conflict.

---

Overall: Ready to merge with the file-size guard and source-link unit tests addressed. The multi-range + highlight warning is a nice-to-have but not a blocker given the limitation is already documented.

🤖 Generated with Claude Code

[claude]

Code Review — PR #256: source-include for literate docs (A2/A3)

This is a well-scoped feature that delivers the literate "read the code, see it run" experience cleanly. The implementation is security-conscious, the tests cover the important paths, and the bug fix for the embed-lvt delegator collision is a correct and clearly explained improvement. A few things worth discussing before merge:

---

Overview

• Adds include="..." lines="N-M" fence attribute that slices real source files into rendered code blocks at render time — no drift between docs and production code.
• Adds region="name", highlight="N", multi-range lines=, source-link footers, and hot-reload for included files.
• Fixes a real bug in embed-lvt-block.ts where two embeds of the same upstream collide on LiveTemplate's per-id event delegator, silently making one region inert.
• Prism Line Numbers + Line Highlight plugins are vendored and conditionally injected only on pages with include= blocks.

---

Potential Issues

1. Locking concern in isIncludedFile vs collectPagesForAutoRoutes

isIncludedFile acquires s.mu.RLock() then calls s.collectPagesForAutoRoutes():

// server.go
func (s *Server) isIncludedFile(filePath string) bool {
    ...
    s.mu.RLock()
    defer s.mu.RUnlock()
    pages := s.collectPagesForAutoRoutes()  // does this also acquire the mutex?
    ...
}

Meanwhile, refreshWatcherIncludes (called from Discover, which holds the write lock) also calls collectPagesForAutoRoutes() without taking the lock itself, with the comment "Caller must hold s.mu". If collectPagesForAutoRoutes internally acquires the same mutex, isIncludedFile will deadlock under RLock. Worth a quick verification that collectPagesForAutoRoutes is lock-free.

2. DefaultSourceRef is a mutable package-level global

// tinkerdown.go
var DefaultSourceRef string

Set once at startup by cmd/tinkerdown/main.go — fine in production. But it's a shared global that parallel tests can silently race on if any test sets it. Worth adding a note in the doc comment, or exposing it as a config option on the Server/Page rather than a package global. At minimum, consider making it an atomic.Value or documenting the "set before any goroutines start" contract explicitly.

3. TestInclude_PathConfinement uses a hardcoded absolute path in markdown

md := "---\ntitle: \"escape\"\n---\n\n" +
    "```text include=\"" + secret + "\"\n```\n"

Resolve correctly rejects absolute paths outside the root (the strings.HasPrefix(rel, "..") check covers this). The test passes, but it's worth noting this is the only case testing absolute-path input — the more common attack vector (include="../../etc/passwd") is tested implicitly by the relative-path traversal in the unit tests. Both are handled correctly, just flagging for completeness.

---

Code Quality

internal/include/include.go — strong overall

• Resolve correctly follows symlinks on both candidate and root before the containment check (covers the macOS /tmp → /private/tmp edge case). Good.
• readFileBounded with a 10 MB cap prevents accidentally pulling large binaries into the docs build.
• renderSourceFooter allowlists https:// and http:// schemes before constructing the <a href> — correct defense against javascript: or data: URIs in author-controlled frontmatter.
• escapeAttr / escapeText both delegate to html.EscapeString — fine and correct, though they're semantically identical, so the distinction is cosmetic.

One edge case in parseLineRange:

parts := strings.SplitN(s, "-", 2)

Input like "5-" yields parts = ["5", ""], and strconv.Atoi("") returns an error — that's handled. But "-5-8" (leading dash) yields parts = ["", "5-8"] where Atoi("") errors first. All edge cases produce errors, not wrong results. Fine.

PreprocessWithLinks root confinement:

// page.go
processedContent, includedFiles, includeWarnings = include.PreprocessWithLinks(
    processedContent, includeBaseDir, includeBaseDir, linkOpts)

Both baseDir and root are filepath.Dir(absPath) — the page's own directory. This deliberately confines includes to the page's subtree (documented as v1 scope). Worth a one-line comment at the call site noting that root == baseDir is intentional and what changing root to the site root would unlock.

Nested fence pass-through:

The logic that copies non-include fence openers and their bodies verbatim (to prevent include= inside docs examples from triggering substitution) is correct and tested. The closer-search loop scans forward for an exact fence match — safe by CommonMark rules.

---

Minor

• The four if path == "prism-line-*.js/.css" blocks in serveAsset are structurally repetitive. A small serveVendored(w, path, contentType, getter) helper would make future plugins a one-liner to add, but it's not blocking.
• Both example _app/ modules import github.com/livetemplate/lvt/testing in non-test main.go files — which pulls test infrastructure into the example binary. Acceptable for demos, but worth a comment explaining why (it provides ServeClientLibrary/ServeCSS as a convenience for local dev without a full LiveTemplate install).
• counter.go and counter.tmpl are byte-for-byte identical across both examples. A shared _app/ or a symlink would reduce duplication, but standalone examples with their own go.mod need their own copies — understood.

---

Test Coverage

• 28 unit tests covering line-range parsing, path confinement, dedent, multi-range, named regions, highlight, link-footer, and nested-fence pass-through — solid.
• Three chromedp e2e tests: RendersSlice, HotReload, PathConfinement — covers the golden path and the critical security case.
• The HotReload test calls srv.Discover() synchronously rather than waiting for the async watcher, with a clear comment explaining why. Acceptable but means the watcher's async path in the hot-reload callback isn't exercised by tests.

---

Summary

The implementation is sound and the approach is the right one — render-time substitution keeps docs and code in sync with no new runtime. The security posture (path confinement, URL scheme allowlist, file size limit) is appropriately careful for author-controlled content. The locking question in point 1 is the only thing I'd want to verify before merging; everything else is minor or stylistic.

🤖 Generated with Claude Code

[claude]

Code Review — feat: source-include for literate docs (A2/A3)

Well-considered feature with solid security properties and good test coverage. A few things worth discussing before merge.

---

Overview

Adds include="..." fence attribute that pulls file slices into rendered code blocks at parse time. The implementation is thorough: path confinement, auto-dedent, named regions, multi-range joins with language-appropriate ellipses, Prism line-number + highlight overlays, and source-link footers. The embed-lvt wrapperID collision fix is a welcome bonus.

---

Security — Positive

The security-sensitive parts are done right:

• Path confinement (Resolve in internal/include/include.go): symlinks are evaluated on both candidate and root before the containment check, so ln -s /etc foo inside the docs tree doesn't escape. The e2e confinement test (TestInclude_PathConfinement) exercises absolute-path attempts too.
• XSS: all user-derived content goes through html.EscapeString before being emitted into raw HTML — escapeAttr and escapeText are consistent wrappers.
• source_repo scheme allowlist (renderSourceFooter): javascript: / data: URLs are blocked by only accepting http:// or https://. Allowlist beats escape-only.
• 10 MB cap on included files (maxIncludeBytes): prevents accidental log-file DoS.

---

Issues

1. DefaultSourceRef is a mutable package-level global (medium)

tinkerdown.go:

var DefaultSourceRef string

This is exported, set once at startup by main(), and the docstring tells test authors not to mutate it. That relies on convention, not enforcement. A library consumer who imports the package and calls ParseFile in parallel test goroutines with different versions would hit a data race.

A cleaner alternative: accept the source ref as a ParseOption / functional option on ParseFile, falling back to a package-level default only when not provided. The frontmatter source_ref override already exists for per-page overrides, so the global is mostly a convenience for the binary.

2. Preprocess root and baseDir are always the same value (minor, documented)

page.go:

processedContent, includedFiles, includeWarnings = include.PreprocessWithLinks(
    processedContent, includeBaseDir, includeBaseDir, linkOpts)

Both baseDir and root are filepath.Dir(absPath), so includes are confined to the markdown file's own directory, not the tinkerdown page-discovery root. The PR comment acknowledges this ("v1 confines includes to the markdown file's parent directory tree"). If the function signature implies they can differ, callers that want the broader root will need to find the right value to pass. Worth a comment at the call site explaining why they're the same (not just that they are).

3. Fence closer matching is stricter than CommonMark (minor)

internal/include/include.go:

if strings.TrimSpace(lines[j]) == fence

CommonMark §4.5 says a closing fence must have at least as many backticks as the opener. This code requires exactly the same string. An edge case like:

```go include="./foo.go"
````   ← four backticks is a valid CommonMark closer

would not be recognized, leaving the include unsubstituted. Low probability in practice, but worth a comment noting the intentional simplification.

4. TestInclude_HotReload bypasses the watcher (low)

include_e2e_test.go:

// Re-discover synchronously (the watcher would also do this; we
// don't want to depend on its async timing for the assertion).
if err := srv.Discover(); err != nil {

The test manually calls Discover() rather than letting the fsnotify goroutine fire. This means the actual code path — watcher.Start → onReload → isIncludedFile → Discover — is never exercised end-to-end. The unit test for SetExtraFiles covers the watcher extension, but the callback chain itself isn't integration-tested. A follow-up to add at least one test with a time.Sleep + assertion (or polling) against the watcher goroutine would close this gap.

5. Duplicate example _app/ modules (minor)

examples/literate-counter-include/_app/ and examples/literate-linked-include/_app/ contain byte-for-byte identical counter.go, counter.tmpl, and go.sum (181 lines each). main.go is also nearly identical. These will diverge independently when the livetemplate dependency is bumped. Consider:

• Extracting to a shared examples/_shared/counter-app/ and symlinking
• Or at minimum noting in READMEs that they're intentionally duplicated and should be kept in sync

6. Redundant isIncludedFile scan (nit)

In server.go's EnableWatch callback, isIncludedFile is called only after the watcher's extra map has already confirmed the file is interesting. The O(pages × included_files) linear scan in isIncludedFile will always return true for those events. The watcher already has the answer; the server-side re-check is redundant and will slow down hot-reload on large sites. For v1 site sizes this is fine, but worth noting for future optimization.

---

Minor / Style

• Asset serving (server.go): four near-identical if path == "prism-..." blocks. A small lookup table would be cleaner but it's harmless.
• renderSourceFooter allows http://: fine for local dev tunnels (as the examples use 127.0.0.1:9090), but http:// source links in public docs are unusual. No security issue since it's author-controlled content.
• TOCTOU in readFileBounded: stat then read race is benign for trusted author content.

---

What's Well Done

• The nested-fence pass-through fix is subtle and correct — without it, a docs page demonstrating include= syntax would accidentally trigger substitution on its own examples.
• FindRegion with marker-based extraction is a good future-proof complement to line ranges.
• The data-line-offset logic for Prism Line Highlight is exactly right — without it the overlay would misalign when data-start > 1.
• Prism plugins loaded conditionally (if len(page.IncludedFiles) > 0) — no cost on pages that don't use includes.
• The embed-lvt wrapperID fix (${original}-${this.id}) is correct and well-explained.

---

Summary

The core implementation is sound and the security properties are right. Main asks before merge:

1. Address or explicitly defer the DefaultSourceRef global — at minimum add a note in the exported var doc that it's not safe for concurrent mutation by library callers.
2. Add a comment at the PreprocessWithLinks call in page.go explaining why baseDir == root.
3. Consider whether the duplicate example _app/ modules can share a single go.mod.

The test coverage at 28 unit + 3 e2e cases is solid for this surface area.

🤖 Generated with Claude Code

[claude]

Code Review — PR #256: source-include for Literate Docs

Overall: ✅ High quality — production-ready with a few minor observations noted below.

---

Overview

This PR adds a well-scoped include=\"...\" fence attribute that pulls file slices into rendered code blocks. The implementation covers line ranges, multi-ranges, named regions, line highlighting, auto-dedent, and source-link footers. The companion bug fix for embed-lvt-block.ts (unique wrapper IDs per block) is tightly relevant and correct.

---

Security

Path traversal protection: strong. Resolve calls filepath.EvalSymlinks on both the candidate and the root before the filepath.Rel + ".." prefix check. This correctly handles symlink-escape vectors (including macOS /tmp → /private/tmp). The E2E TestInclude_PathConfinement test confirms this end-to-end.

XSS protection: strong. Code content is escaped via html.EscapeString; attributes (data-line, class) likewise. The source-link footer uses an explicit https:// / http:// allowlist — rejecting javascript:, data:, file:, etc. — and URL-encodes path segments via url.PathEscape.

File size limit: strong. The 10 MB cap checked before os.ReadFile prevents allocation bombs. Tested.

One note: the v1 decision to confine includes to the markdown file's own directory tree (passing includeBaseDir as both baseDir and root) is conservative and well-documented with a code comment. No issues.

---

Code Quality

• Package design is clean: Resolve, Slice, SliceRanges, Dedent, FindRegion are single-responsibility and well-named.
• Regexes are package-level vars — compiled once, not per call. Good.
• strings.Builder used correctly for HTML generation; no string concatenation in loops.
• The DefaultSourceRef global has a clear concurrency contract comment: set-once before goroutines, tests use frontmatter instead of mutating it. Excellent discipline.
• Lock contracts in refreshWatcherIncludes and collectPagesForAutoRoutes are documented and correct.

One nit — fence scanning is O(n²) in fence count (PreprocessWithLinks scans forward for the closing fence on each opening). This is entirely fine for realistic doc sizes (<500 fences/page) but worth noting if docs grow very large.

---

Potential Issues

Clamping asymmetry in SliceRanges (intentional, but worth confirming intent):

• Start out-of-range → hard error
• End out-of-range → silent clamp

This is tested and documented, so it's a deliberate design choice. Just confirming the team is happy with it — a stale lines="5-80" on a file that shrinks to 60 lines will silently truncate rather than warn.

data-lvt-id collision fix in embed-lvt-block.ts: The fix (${original}-${this.id}) is minimal and correct. One question: does LiveTemplate's connect() API tolerate arbitrary suffixes on wrapper IDs? It would be worth a quick comment confirming that livetemplate matches by exact data-lvt-id value (which it does, per the block's own connect call), not by prefix.

---

Test Coverage

28 unit tests + 3 chromedp E2E tests is solid for this surface area. A few gaps worth considering (none are blockers):

1. Symlink edge cases: TestResolve_PathConfinement covers ../ traversal but not a symlink-based escape (e.g., included dir is itself a symlink to a directory outside root). Low risk given EvalSymlinks handles it, but an explicit test would be good hygiene.
2. Concurrent SetExtraFiles during file change: The locking is correct, but there's no concurrent stress test. Low risk.
3. data-line-offset arithmetic: No dedicated unit test for the start-1 offset math when highlight spans multi-line ranges. Verified via E2E, but a unit test would be tighter.

---

Minor Observations

• include attribute appearing twice in the same fence — first match wins silently. Acceptable for v1, but a warning log would be friendly.
• The multi-range gutter (showing sequential numbers rather than skipping) is correctly called out as a known v1 limitation in the PR description. Good transparency.
• docs/guides/literate-docs.md is thorough and accurate — the v1 confinement caveat, file-absolute highlight semantics, and region marker format are all clearly explained.

---

Summary

Area	Assessment
Security (path confinement, XSS, size limit)	✅ Strong
Code quality / Go idioms	✅ Strong
Test coverage	✅ Good (minor gaps noted)
TypeScript bug fix	✅ Correct and minimal
Documentation	✅ Thorough
Concurrency handling	✅ Correct with documented contracts
Performance	✅ Acceptable (O(n²) fence scan noted)

This is ready to merge once #251 lands and the base rebases to main. The implementation is solid, the security posture is careful, and the test coverage is comprehensive.

🤖 Generated with Claude Code

[claude]

Code Review — feat: source-include for literate docs (A2/A3)

Good feature overall. The path-confinement design is solid, the graceful-degradation posture (bad includes pass through instead of failing the page) is the right call, and the nested-fence fix for example authoring is a subtle but important correctness detail. A few things worth addressing before merging:

---

Potential deadlock in isIncludedFile / refreshWatcherIncludes

internal/server/server.go — isIncludedFile acquires s.mu.RLock() then calls collectPagesForAutoRoutes(). If that helper also calls s.mu.RLock() (or any code inside it does), you get a nested RLock on the same goroutine. Go's sync.RWMutex is not reentrant — when a writer is blocked waiting for Lock(), the second RLock() from the same goroutine will block too, creating a deadlock. The same concern applies to refreshWatcherIncludes, whose comment says "Caller must hold s.mu" — if it also calls collectPagesForAutoRoutes() while Discover() holds the write lock, that's similarly fragile. Either:

• Pass the pre-collected pages slice into both helpers to avoid the re-acquisition, or
• Document explicitly that collectPagesForAutoRoutes must never acquire any variant of s.mu, with a test that catches violations.

---

fenceOpenerRe closer-matching may miss CommonMark-spec closers

In PreprocessWithLinks, closing fence detection uses:

strings.TrimSpace(lines[j]) == fence

The CommonMark spec allows closing fences to carry leading spaces (up to 3) and requires ≥ the opener's backtick count. TrimSpace would strip trailing whitespace too, potentially matching lines it shouldn't (e.g., ``` trailing garbage would strip to ``` and match). In practice this is low-risk for the non-include pass-through path, but the include-fence closer uses the same logic — an unclosed fence caused by a spec-non-conformant closer would leave the loop searching until closer == -1 and fall through silently. Worth a comment acknowledging the intentional approximation, and ideally a test case for it.

---

escapeAttr and escapeText are identical

func escapeAttr(s string) string { return html.EscapeString(s) }
func escapeText(s string) string { return html.EscapeString(s) }

The names document intent clearly (good), but html.EscapeString is correct for both contexts only because the href is already assembled from allowlisted scheme + url.PathEscape segments — the final HTML-escaping of the href is for & in query strings (not present here) and " in attribute context. This is fine but fragile: if the href construction ever changes to include untrusted input, escapeAttr != full href sanitization. A brief comment would help future authors know why both are safe here.

---

Hot-reload test relies on chromedp.Sleep

TestInclude_HotReload uses chromedp.Sleep(2*time.Second) twice. Sleep-based assertions are a flakiness source under load. The test already manually calls srv.Discover() synchronously (bypassing the watcher), so the sleep is purely for the browser to reflect the re-navigated page. Consider using chromedp.WaitVisible or polling for the expected text to appear instead of a fixed sleep.

---

Duplicate _app/ modules — sync reminder

The README notes that literate-counter-include/_app/ and literate-linked-include/_app/ are intentionally near-identical with separate go.mod files. The counter.go files are byte-for-byte identical. Consider a comment in both READMEs (already there in literate-counter-include/README.md) referencing where to bump both together. A small CI check (e.g., a script that diffs the two go.mod files and warns on divergence) would prevent accidental drift after a livetemplate version bump.

---

Minor nit: non-empty body warning fires before the closer is confirmed reachable

In PreprocessWithLinks, the "non-empty fence body is ignored" warning fires as long as closer != -1 but before Resolve or SliceRanges succeed. If Resolve fails (bad path), the block passes through with the original body including that non-empty content — but the warning was already emitted. The author would see "body is ignored" even though the body was in fact preserved. Low-stakes but potentially confusing.

---

What's well done

• Path confinement correctly resolves symlinks on both the candidate and root before comparing (EvalSymlinks both sides prevents macOS /tmp → /private/tmp false positives and symlink-escape attacks).
• renderSourceFooter XSS prevention: scheme allowlist (http:// / https:// only) + url.PathEscape per segment + html.EscapeString on the attribute. Belt-and-suspenders, exactly right.
• Graceful degradation: bad includes pass through the original (empty) block with a stderr warning — the page keeps rendering.
• maxIncludeBytes = 10MB: prevents a large file from silently blowing out the render.
• Conditional Prism plugin loading: pages without include= blocks pay no extra JS/CSS weight.
• watcher.extra protected by sync.RWMutex: correct concurrency for the mutable extra-files set.
• Nested-fence pass-through: the fix that skips the body of a non-include fence prevents include= inside an authoring example from triggering substitution — a subtle but real correctness win.
• 28 unit tests + 3 chromedp e2e tests including a path-confinement test that actually asserts the secret file doesn't appear in rendered HTML.
• DefaultSourceRef concurrency contract is explicitly documented in the var comment with a call-out that tests must not mutate it from parallel test bodies.

[claude]

Code Review — PR #256: source-include for literate docs (A2/A3)

Overview

This PR adds an include="..." fence attribute that pulls real source files into rendered code blocks, enabling a literate "read the code, see it run" pattern when paired with embed-lvt. The implementation is well-structured and the security posture (path confinement, HTML escaping, URL scheme allowlisting, file size cap) is solid. A few items worth discussing before merge:

---

Security

Path confinement ✅ — Resolve calls filepath.EvalSymlinks on both the candidate and the root before the containment check, preventing symlink-based escapes (e.g. macOS /tmp → /private/tmp). The unit test covers in-tree, escape attempt, empty path, and missing file. Good.

XSS prevention in renderSourceFooter ✅ — source_repo is scheme-allowlisted (http:///https:// only before any use), path segments are encoded with url.PathEscape, and both attribute and text contexts use html.EscapeString. The dual escapeAttr/escapeText aliases are intentional and the comment explains why — no issue.

File size cap ✅ — maxIncludeBytes = 10 MB via readFileBounded prevents accidentally pulling a large binary into the docs build.

DefaultSourceRef global concurrency caveat — The doc comment warns that parallel test bodies must not mutate it. This is convention-enforced, not compiler-enforced. If this var is ever needed in tests across packages, a t.Setenv-style helper or passing it through LinkOptions directly would be safer. For v1 the current posture is acceptable — just worth flagging.

---

Correctness

data-line-offset math ✅ — For lines="13-22" and highlight="20", the code emits data-start="13" and data-line-offset="12" (= start − 1). Prism Line Highlight interprets data-line as snippet-relative by subtracting the offset: 20 − 12 = 8, which is the 8th line of the snippet = file line 20. The math lines up correctly with Prism's implementation.

Multi-range ellipsis join ✅ — LanguageEllipsis covers the major comment styles (C-family, Python/shell, HTML, SQL/Lua/Haskell) and falls back to // .... One edge case: Makefile is mapped to # ... but Makefile uses tab-indented recipes — # ... is still a valid comment, so no functional issue.

Fence closer detection (known approximation) — The closer search uses strings.TrimSpace(lines[j]) == fence (exact backtick count match). CommonMark §4.5 allows a closing fence to have more backticks than the opener, so a document like:

```go include="./x.go"

would not be recognized as closed. This is acknowledged in the comment as a known approximation. Since the current failure mode is that the block is passed through unmodified (no substitution, no panic), it's safe for v1, but worth tracking if users start using non-matching fence lengths.

**`FindRegion` empty-region error** — An empty region (start and end markers adjacent with nothing between them) returns an error rather than an empty slice. This seems intentional but might surprise authors; the error message ("end marker not after start marker (or empty region)") is clear enough.

---

### Testing

**`TestInclude_HotReload` doesn't exercise the watcher branch** — The test manually calls `srv.Discover()` after mutating the file rather than waiting for the watcher event. The comment acknowledges this ("we don't want to depend on its async timing"). However, this means the `isIncludedFile → Discover → BroadcastReload` code path in `server.go` has no automated test. If a future change breaks the watcher integration (e.g., `SetExtraFiles` not being called after re-discovery), no test will catch it. Consider adding even a short-lived integration assertion that the watcher fires — or at minimum a unit test for `isIncludedFile` itself.

**`TestInclude_PathConfinement` uses an absolute path ✅** — The test injects the secret file's absolute path directly into the fence attribute to verify that `Resolve` rejects it as escaping the root. This works correctly since `Resolve` handles absolute paths without joining them to `baseDir`, then checks confinement. Good coverage.

---

### Performance

**`isIncludedFile` is O(pages × includes-per-page) per watcher event** — For a dev server this is fine. If this ever becomes a bottleneck for large sites, an inverted index `map[string][]*Page` built during `refreshWatcherIncludes` would be a straightforward optimization.

**Conditional Prism plugin loading ✅** — The `~30 KB` Line Numbers + Line Highlight plugins are only injected into pages that have `IncludedFiles`. Pages without includes pay no cost. Good.

---

### Minor items

**`sharedAuth` in example `main.go`** — `WithPermissiveOriginCheck()` is fine for a tutorial counter on trusted dev infra, but it's listed without a comment in the handler setup. Since the `sharedAuth` type already has a thorough comment, adding one sentence near `WithPermissiveOriginCheck()` ("safe only because this app is deployed on the same trusted host as the docs; production apps should use strict origin checking") would help prevent copy-paste misuse.

**`refreshWatcherIncludes` calls `collectPagesForAutoRoutes()` and so does `isIncludedFile`** — Two separate linear scans of the same page list. No correctness issue, just a note that if the page count grows, consolidating them (or caching the inverted index) would help.

**Duplicate example `_app/` directories** — The README notes this is intentional (each example must be clone-and-run independently). The note about keeping both `go.mod` files in sync when bumping `livetemplate`/`lvt` is clear. One alternative would be a `go.work` workspace, but the current approach is simpler for users.

---

### Summary

The implementation is clean, the security model is sound, and the test coverage is good. The main asks before merge:

1. **Should address**: Add at least a unit-level test for `isIncludedFile` (or a narrow integration assertion that `SetExtraFiles` is populated after `Discover()`). The hot-reload watcher branch has zero automated coverage today.
2. **Optional**: Add a one-line origin-check warning in the example `main.go` near `WithPermissiveOriginCheck()`.
3. **Acknowledged / deferred**: Fence closer approximation, multi-range gutter numbering, `DefaultSourceRef` global concurrency — all appropriately documented and scoped.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

[claude]

Code Review — feat: source-include for literate docs (A2/A3)

Overall this is a well-scoped and thoughtfully implemented feature. The package architecture is clean, security posture is solid, and the test coverage is good for a first version. A few items worth addressing before merge.

---

Overview

Adds include="..." / lines="..." / region="..." / highlight="..." fence attributes that pull real source files into rendered code blocks, paired with source-link footers and hot-reload watcher support. The implementation is cleanly separated into internal/include/include.go with the rest of the stack (parser, page, server, watcher) wired around it.

---

Potential Bugs

TestInclude_HotReload does not actually exercise the watcher path.
The test manually calls srv.Discover() after writing the new file, then re-navigates — it proves that re-discovery picks up new content, but never lets the watcher fire a reload. The chromedp "wait for websocket reload" flow is absent. The test name is misleading; either rename it TestInclude_ReDiscoverUpdatesContent or add a second test that mutates the file and waits for the WS reload event without the manual Discover() call.

Multi-range data-line-offset is always set from the first range's start.

if start, err := strconv.Atoi(dataStart); err == nil && start > 1 {
    sb.WriteString(fmt.Sprintf(` data-line-offset="%d"`, start-1))
}

For lines="5-8,15-22" the gutter is sequential from 5 but the snippet content jumps from 8 to 15. A highlight= pointing at line 15 will land on the wrong visual row. The PR description calls this out-of-scope, which is fine — but a comment at this code site noting the known limitation would prevent a future contributor from thinking this is correct for multi-range cases.

highlight= input is not validated before being written into data-line.
The value is HTML-escaped but not structurally validated. A malformed value like highlight="3-" silently produces a non-functional Prism overlay with no diagnostic. A quick format check (same regex you use in Prism's existing data-line rendering, or a minimal parse) would catch authoring mistakes early. Prism itself silently ignores bad data-line values, so the page doesn't break — but the author gets no feedback.

---

Security

Good: renderSourceFooter allowlists https:// / http:// before emitting an <a href>, blocking javascript: / data: injection from frontmatter. Solid.

Good: Resolve calls filepath.EvalSymlinks on both the candidate and the root before the containment check. macOS /tmp → /private/tmp is correctly handled.

Good: readFileBounded caps included files at 10 MB before allocating. Good DoS guard for the docs-server use case.

Minor — Prism assets served without cache-busting keys.
Cache-Control: public, max-age=31536000 with no content-hash or versioned path means a tinkerdown upgrade that ships updated Prism plugins will serve stale JS/CSS to users with warm caches. The existing prism.js has the same pattern so this is consistent, but worth tracking as a follow-up if upgrades cause visual regressions.

---

Code Quality

DefaultSourceRef is a package-level mutable global.
The concurrency contract is documented clearly, and "set-once before goroutines start" is a well-established Go pattern (think http.DefaultServeMux). The concern is library callers who might call ParseFile from multiple goroutines with different desired refs without knowing about this global. A per-call or per-page option (e.g., adding it to ParseFile's signature or a config struct) would be safer long-term. The frontmatter source_ref escape hatch partially addresses this for tests, but library consumers can't easily override it. Worth at least a note in the package-level godoc.

renderPage format string now has 12 positional %s arguments.

</html>`, wsURL, showSidebar, page.Title, seoTags, stylingOverrideCSS, prismIncludeCSS, sidebar, contentWithNav, defaultTheme, prismIncludeJS, mermaidScript, chartScript)

This is pre-existing technical debt, but adding two more args makes it harder to maintain. Worth a follow-up to switch to a struct-based template (even text/template) rather than a positional fmt.Sprintf. Not a blocker for this PR.

escapeAttr and escapeText are identical wrappers.

func escapeAttr(s string) string { return html.EscapeString(s) }
func escapeText(s string) string { return html.EscapeString(s) }

The comment explains the intent (document call-site context), which is reasonable. Minor nit: in an HTML attribute context, html.EscapeString doesn't escape single quotes ('), which is fine only if all attributes use double quotes — which they do here. Still worth a comment acknowledging that escapeAttr is only safe in double-quoted contexts.

isIncludedFile has O(pages × files) complexity per watcher event.
For docs sites this is negligible, but the full page graph is traversed on every non-.md FS event under the root. If a future use case involves large site maps, the extra map already maintained in Watcher could be consulted directly (it's the pre-built set). isIncludedFile in server.go currently re-derives it from pages instead of reading the watcher's set. Consider delegating: s.watcher.IsExtra(absPath) would be O(1).

w.Write(js) / w.Write(css) return values are ignored in the new Prism asset handlers. This matches the style elsewhere in the codebase, but a //nolint:errcheck or a helper that logs write errors on disconnect would reduce reviewer surprise.

---

Test Coverage

Good coverage overall: 28 unit tests across internal/include/include_test.go, 1 server integration test (TestIsIncludedFile_Tracking), and 3 chromedp e2e tests. The security-critical path (path confinement) is tested at both unit and e2e levels.

Missing unit test: no case for the region= + lines= mutual-exclusion warning path. The behavior (region wins, warning logged) is described in comments and docs but not asserted.

Missing unit test: FindRegion with mismatched or out-of-order markers (start found, end found, but endIdx <= startIdx+1) is partially covered by the empty-region error path, but a test where end comes before start in the file would add confidence.

---

CLAUDE.md Check

The project instructions say to verify that lvt-* attributes aren't being reimplemented when they're already available in livetemplate/client. The embed-lvt-block.ts fix touches data-lvt-id-pending → data-lvt-id renaming, which is an existing protocol attribute, not a new one — the fix changes how tinkerdown disambiguates multiple embeds, not what livetemplate/client expects. This looks correct, but it's worth confirming the suffixed id (${original}-${this.id}) is still recognized correctly through the client's event-delegator chain, especially if livetemplate/client ever validates the format of data-lvt-id values.

---

Summary

Area	Status
Security	✅ Solid path confinement, scheme allowlist, size limit
Core logic (include.go)	✅ Clean, well-tested
Hot-reload watcher	⚠️ Test doesn't exercise actual watcher path
Multi-range highlight alignment	⚠️ Known limitation, needs a code comment
highlight= validation	⚠️ Silent failure on bad input
DefaultSourceRef global	⚠️ Acceptable for v1, worth a follow-up
Test coverage gaps	⚠️ region+lines warning, out-of-order markers
Example duplication (_app/)	✅ Intentional and documented

The hot-reload test gap and the multi-range highlight comment are the two items I'd want addressed before merge. The rest are nits or v2 considerations.

🤖 Generated with Claude Code