test: spec tests for inline tag hardening (#931, #932)

[zeroedin]

Summary

• 1 red test for fail-closed sandbox when _contentRoot is missing (inline tag: fail closed when _contentRoot is empty #932)
• 2 green regression tests for case-insensitive extension matching (inline tag: add mixed-case extension test #931)
• 3 existing tests updated to include _contentRoot in render context to match production shape
• PLAN.md and IMPLEMENTATION.md updated with fail-closed requirement and case-insensitive extension note

Test summary

#	Test	Status	Discriminator
1	returns error when _contentRoot is not set (#932)	🔴 RED	Render() succeeds without error — sandbox silently skipped
2	matches file extensions case-insensitively (#931)	✅ GREEN	strings.ToLower already normalizes .SVG → .svg
3	rejects binary file type with mixed-case extension (#931)	✅ GREEN	strings.ToLower normalizes .PNG → .png before blocklist check

Existing test updates

Tests 1 ("inlines an SVG file"), 2 ("inserts content raw"), and 5 ("returns error when file not found") now set _contentRoot alongside _contentDir. These changes are green — the current code ignores _contentRoot when set (it just runs the sandbox check, which passes for same-directory files). After the developer makes _contentRoot required, these tests will continue to pass because they already provide it.

Implementation guidance

In internal/template/inline.go, the resolve method at the contentRoot check (currently if contentRoot != ""):

// Current (skip sandbox when empty):
contentRoot, _ := context.FindVariable("_contentRoot", false).(string)
if contentRoot != "" {
    // sandbox check
}

// Required (fail closed):
contentRoot, _ := context.FindVariable("_contentRoot", false).(string)
if contentRoot == "" {
    return "", fmt.Errorf("inline tag requires _contentRoot in render context")
}
contentRoot = filepath.Clean(contentRoot)
rel, err := filepath.Rel(contentRoot, resolved)
if err != nil || strings.HasPrefix(rel, "..") {
    return "", fmt.Errorf("inline path escapes content root: %s", relPath)
}

The change is minimal: invert the guard from if contentRoot != "" (skip) to if contentRoot == "" (error), then unconditionally run the sandbox check.

Refs #931, #932

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds/updates specification-level tests and planning docs around {% inline %} tag hardening in the Liquid template engine—specifically (a) requiring _contentRoot for path sandboxing (fail-closed) and (b) documenting/test-covering case-insensitive extension handling.

Changes:

• Update PLAN/IMPLEMENTATION docs to explicitly require fail-closed behavior when _contentRoot is missing and to note case-insensitive extension normalization.
• Adjust existing inline-tag spec tests to pass _contentRoot (to match production render context shape).
• Add new spec tests for missing _contentRoot (expected to be red until implementation changes) and mixed-case extension behavior.

Reviewed changes

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

File	Description
plans/PLAN.md	Documents fail-closed _contentRoot requirement and notes case-insensitive extension handling.
plans/IMPLEMENTATION.md	Updates inline-tag implementation notes with fail-closed and case-insensitive extension guidance.
internal/template/liquid_test.go	Updates inline-tag tests to include _contentRoot and adds new regression/spec tests for _contentRoot and mixed-case extensions.

---

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

[zeroedin]

Code Review Results

Scope: internal/template/liquid_test.go, plans/PLAN.md, plans/IMPLEMENTATION.md
Intent: Harden the {% inline %} tag with a fail-closed sandbox test (#932) and regression tests for case-insensitive extension matching (#931). Update specs to match.
PR type: Architect spec PR — reviewing test quality, coverage, spec alignment, and PLAN.md/IMPLEMENTATION.md consistency.

Verification

All claims in the PR body confirmed by running the test suite:

Test	Claimed	Actual
returns error when _contentRoot is not set (#932)	🔴 RED	🔴 RED — Render() returns nil error; sandbox silently skipped
matches file extensions case-insensitively (#931)	✅ GREEN	✅ GREEN — strings.ToLower normalizes .SVG
rejects binary file type with mixed-case extension (#931)	✅ GREEN	✅ GREEN — strings.ToLower normalizes .PNG
Updated: inlines an SVG file	✅ GREEN	✅ GREEN
Updated: inserts content raw	✅ GREEN	✅ GREEN
Updated: returns error when file not found	✅ GREEN	✅ GREEN

Analysis

Test quality. All three new tests are self-contained, deterministic, use DeferCleanup for temp directory cleanup, and have specific assertions with descriptive failure messages. The red test (#932) asserts both that an error occurs AND that the error message mentions _contentRoot — this constrains the developer's implementation to produce a useful error, not just any error.

Existing test updates. The three updated tests add _contentRoot alongside _contentDir to match production render context shape. The tests that were NOT updated (returns error for binary file types, returns error for absolute paths) correctly omit _contentRoot — both error before reaching the sandbox check (inline.go:72-73 and inline.go:67-68 respectively), so adding _contentRoot would test nothing new and obscure the test's actual focus.

Spec consistency. PLAN.md and IMPLEMENTATION.md changes are internally consistent:

• Both describe the fail-closed requirement the same way: error when _contentRoot is missing/empty rather than skipping the sandbox
• Both document case-insensitive extension matching via strings.ToLower
• IMPLEMENTATION.md includes concrete implementation guidance (invert the if contentRoot != "" guard to if contentRoot == "" → error) and specifies the exact error message string

No Skip() used. Red test fails with a real assertion failure as required.

Coverage

• Testing gaps: none for the scope of this PR
• Residual risks: none

---

Verdict: Ready to merge

Reasoning: Clean spec PR. The red test correctly captures the fail-closed security requirement at inline.go:85 where the if contentRoot != "" guard silently skips the sandbox. The green regression tests pin existing strings.ToLower behavior so future removals would be caught. Spec docs are consistent with the tests. Implementation guidance is concrete and minimal (one guard inversion). No issues found.

[zeroedin]

Code Review Results (Re-review)

Scope: internal/template/liquid_test.go, plans/PLAN.md, plans/IMPLEMENTATION.md
Intent: Harden the {% inline %} tag with a fail-closed sandbox test (#932) and regression tests for case-insensitive extension matching (#931). Update specs to match.
PR type: Architect spec PR — reviewing test quality, coverage, spec alignment, and PLAN.md/IMPLEMENTATION.md consistency.

Prior Feedback Status

All three Copilot comments (allowlist → blocklist terminology in PLAN.md, IMPLEMENTATION.md, and liquid_test.go) were addressed in 395c1cd. Each fix is correct and minimal.

Re-verification

Test	Expected	Actual
returns error when _contentRoot is not set (#932)	🔴 RED	🔴 RED — Render() returns nil; sandbox silently skipped
matches file extensions case-insensitively (#931)	✅ GREEN	✅ GREEN
rejects binary file type with mixed-case extension (#931)	✅ GREEN	✅ GREEN
inlines an SVG file (updated)	✅ GREEN	✅ GREEN
inserts content raw (updated)	✅ GREEN	✅ GREEN
returns error when file not found (updated)	✅ GREEN	✅ GREEN
4 pre-existing inline tests	✅ GREEN	✅ GREEN

Analysis

No new issues found. The fix commit is clean — three one-line terminology corrections, no behavioral changes. The blocklist terminology now accurately describes the code at inline.go:13-20 and inline.go:71-73. PLAN.md, IMPLEMENTATION.md, and test comments are consistent with each other and with the implementation.

Coverage

• Testing gaps: none
• Residual risks: none

---

Verdict: Ready to merge

Reasoning: Prior feedback fully addressed. All test status claims verified. No new issues introduced by the fix commit.