spec(ssr): simplify Phase 2 from per-instance to per-page CLI model

[zeroedin]

Summary

Closes #149

• Replace complex multi-step SSR pipeline (scan → deduplicate → per-instance render via stdin/stdout → stamp-back with marker comments) with a simple per-page CLI invocation: pass full page HTML to ssr.command, external tool returns transformed HTML
• Consolidate ssr.build + ssr.serve config into single ssr.command field
• Remove SSREngine interface, multiple protocol support (HTTP/stdio/IPC), and mock tests
• Preserve component tracking (tag scanning, ComponentMap persistence) for cache invalidation

Test plan

• Review spec changes in plans/PLAN.md §2, §6, §8, §12 for consistency
• Review plans/IMPLEMENTATION.md Phase 4D and 6B updates
• Verify test fixtures (YAML/TOML/JSON) all use new ssr.command format
• Verify all test files reference SSRConfig{Command: "..."} not SSRConfig{Build: "..."}
• Confirm mock_test.go deletion is correct (SSREngine interface removed)
• Confirm depgraph_test.go is preserved unchanged
• Grep for stale references: ssr.build, ssr.serve, SSRServeConfig, RenderViaHTTP, RenderViaStdio, InsertMarkers, StampBack, DeduplicateInstances, ComponentCacheKey

Note: Tests will not compile until the developer implements the corresponding code changes — that is expected and tracked in a separate for development issue.

🤖 Generated with Claude Code

[zeroedin]

PR #150 Review — SSR Per-Page CLI Model (Spec Simplification)

Scope: Rewrites SSR Phase 2 from per-instance stdin/stdout model to per-page CLI model across spec (PLAN.md, IMPLEMENTATION.md) and tests (12 files touched).

What This Changes

The SSR architecture simplifies from a 4-step per-instance pipeline (scan → deduplicate → insert markers → render per unique instance via stdin → stamp back) to a 2-step per-page model (pass full page HTML to ssr.command → engine returns SSR'd HTML). The SSR engine is now treated as a black box — Alloy gives it an entire page and gets back the fully rendered result.

Config simplifies from two fields (ssr.build + ssr.serve with separate SSRServeConfig) to a single ssr.command string.

Verdict: Correct directional simplification

This removes ~300 lines of spec complexity (dedup, markers, stamp-back, SSREngine interface, multiple protocol support, per-instance rendering) that existed to solve problems the SSR engine already handles internally. The insight from the nested component discussion (PR #137 review) — that golit/lit-ssr handle nested components internally and Alloy doesn't need to orchestrate sub-component rendering — naturally leads to this simplification.

Specific Observations

1. Config & Type Changes — Clean

• SSRConfig{Build string; Serve SSRServeConfig} → SSRConfig{Command string} — all config fixtures (valid.yaml, valid.toml, valid.json, alloy.config.yaml) updated consistently.
• SSRServeConfig struct eliminated entirely.

2. Scanner API Simplification — Correct

• ScanComponents return: []ComponentInstance → []string (unique tag names). This is the right abstraction — Alloy only needs to know which custom elements appear on a page for cache invalidation, not their full instance details.
• Removed: DeduplicateInstances, InsertMarkers, StampBack, ComponentInstance struct, ParseSSRConfig, RenderViaHTTP, RenderViaStdio, ComponentCacheKey.
• Added: RenderPage(command string, html string) (string, error) — clean single-responsibility function.

3. Component Tracking Preserved — Good

• ComponentMap.PageToInstances → PageToComponents (stores tag names not hashes). Cache invalidation via DepGraph remains intact. This is the right call — component tracking is still needed for incremental rebuilds even though component orchestration is delegated to the engine.

4. Test Changes — Appropriate

• 128 lines removed (dedup, markers, stamp-back, config parsing, protocol integration, cache keys).
• 59 lines added (RenderPage with cat pass-through, command not found, non-zero exit; unique tag name extraction; fresh build empty component map).
• Pipeline tests: cat (stdin/stdout) → echo (CLI args), SSRConfig{Build:} → SSRConfig{Command:}.
• mock_test.go deleted — SSREngine interface eliminated. Correct, the abstraction boundary is now the CLI command itself.
• Test count: 25 → 15. The removed tests cover concepts that no longer exist in the spec (dedup, markers, stamp-back, multiple protocols).

5. IMPLEMENTATION.md — Consistent

• Phase 4D rewritten for per-page model. Phase 6B test count updated. Lists all removed functions/types explicitly — helpful for implementation PR.

Items to Address

A. SSR_ENGINES.md Cross-Reference (Medium)
PR #143 added SSR_ENGINES.md which documents the stdin/stdout contract and references ssr.render. With this PR's change to ssr.command and per-page CLI args model, that document's contract description may be stale on merge. Verify whether SSR_ENGINES.md needs updating to reflect:

• Config field rename (ssr.render → ssr.command)
• Contract change (stdin/stdout per-instance → CLI arg per-page)

B. RenderPage CLI Arg Length Limits (Low — Note for Implementation)
Passing full page HTML as a CLI argument (ssr.command <html>) is subject to OS argument length limits (ARG_MAX — typically 2MB on Linux, 1MB on macOS). For pages with large amounts of content or many components, this could silently truncate or fail. The spec might want to note this constraint or specify that the implementation should fall back to stdin for pages exceeding a threshold. This is more of an implementation concern but worth a spec note.

C. Three Intentionally Failing Tests — Expected
Tests reference SSRConfig{Command:} which doesn't exist yet in the implementation. This is the established TDD pattern — tests define the target API, implementation follows.

Summary

Clean, well-motivated simplification. The per-page model correctly delegates component rendering complexity to the SSR engine where it belongs. Component tracking for incremental builds is preserved. The only actionable item is ensuring SSR_ENGINES.md stays consistent with the new contract.

[zeroedin]

PR #150 Re-review — Two New Commits

Two commits since the initial review:

Commit 2: 1db9616 — Remove stale SSR references from TESTSUITE_CRITIQUE.md

Replaces 4 lines of stale "Missing" items (SSR config parsing, HTTP protocol, stdio protocol, cache key) with a single accurate line reflecting the new per-page model. Clean housekeeping.

Commit 3: f7cdd7f — Use stdin/stdout transport instead of CLI argument

This directly addresses item B from my initial review (CLI arg length limits / ARG_MAX). The transport model changes from passing full page HTML as a CLI argument to piping it via stdin — which has no practical size limit.

Changes are consistent across all 6 files:

Spec (PLAN.md, IMPLEMENTATION.md) — Every mention of "CLI argument" updated to "stdin/stdout". The example command changes from golit render --defs ./bundles '<html>...' to echo '<html>...' | golit render --defs ./bundles. RenderPage function docs updated to describe stdin pipe + stdout read.

Tests (scanner_test.go, build_test.go) — Two important improvements:

1. RenderPage pass-through test strengthened: The previous version had a weak conditional assertion (if err == nil { Expect(...) }) that silently passed when cat failed. The new version uses unconditional Expect(err).NotTo(HaveOccurred()) + Expect(result).To(ContainSubstring("ds-card")). This works correctly now because cat reads stdin natively — it's the canonical stdin→stdout passthrough. With the old CLI arg model, cat would try to open a file named after the HTML string, which is why the test had to be conditional.

2. Pipeline tests: echo → cat throughout, matching the stdin model. Comments updated consistently.

TESTSUITE_CRITIQUE.md — Description updated from "CLI arg" to "stdin/stdout".

Previous Review Items — Status

Item	Status
A. SSR_ENGINES.md cross-reference	Resolved — PR #143 was closed without merging, so the file doesn't exist on main. No stale reference.
B. CLI arg length limits (ARG_MAX)	Resolved — stdin/stdout transport eliminates the concern entirely.
C. Intentionally failing tests	Expected — TDD pattern, unchanged.

Verdict

No issues. The stdin/stdout transport is the right call — it's the standard Unix convention for text transformation pipelines, eliminates the ARG_MAX concern, and cat is a proper pass-through test command for this model (unlike the CLI arg model where cat semantics were wrong). All spec documents, tests, and critique doc are internally consistent.

[zeroedin]

PR #150 Re-review #2 — Stream Mode (Commit 90db358)

What This Adds

A second SSR communication mode: stream — a persistent process that stays alive across all pages, using NUL-byte (\0) delimited messages over stdin/stdout. Complements the existing exec mode (one process per page).

ssr:
  command: "golit serve --stdio"
  mode: "stream"    # persistent process, NUL-delimited

New API surface:

• SSRConfig.Mode field ("exec" default, "stream")
• NewStreamRenderer(command string) (*StreamRenderer, error) — starts persistent process
• (*StreamRenderer) RenderPage(html string) (string, error) — NUL-delimited request/response
• (*StreamRenderer) Close() error — shuts down process

Verdict: Well-motivated, mostly clean

Process-per-page has non-trivial startup overhead (Node.js cold start, engine init, component definition loading). For a 500-page site, that's 500 process spawns. Stream mode amortizes this to one startup. The dual-mode design keeps exec as the simple default while offering stream for performance-sensitive builds.

Specific Observations

1. NUL Delimiter — Correct Choice
Per HTML spec §12.2.2, NUL characters in HTML input are parse errors replaced with U+FFFD. Valid HTML output cannot contain \0, making it a safe, unambiguous message boundary. Worth adding a one-line note in PLAN.md explaining why NUL was chosen (so future implementors don't second-guess it or try switching to newlines, which HTML does contain).

2. cat as Stream Test Command — Clever
cat works as a stream-mode pass-through because it faithfully echoes bytes including NUL. Write html\0 → cat outputs html\0 → reader stops at \0, gets html. For the multi-page test, cat stays alive (reads until stdin EOF), so successive writes each produce output. One implementation caveat the developer should be aware of: cat may use block buffering when stdout is a pipe (typically 4-8KB). The test HTML is small (~80 bytes), so it could sit in cat's output buffer unflushed, causing a deadlock. The StreamRenderer implementation may need to account for this (e.g., reading in a goroutine, or documenting that SSR engines must flush after each response).

3. API Design — Clean Parallel

• Exec: ssr.RenderPage(command, html) → (string, error) — command per call
• Stream: sr.RenderPage(html) → (string, error) — command at construction

Same method name, different lifecycle semantics. BuildPhase2 switches on Mode to choose which path. This is the right abstraction.

4. Test Coverage — Good (6 new tests)

Scanner tests (+4):

• NewStreamRenderer starts persistent process (cat)
• Multi-page render on single process (two sequential pages)
• Command not found error
• Close shuts down cleanly

Pipeline tests (+2):

• BuildPhase2 attempts stream startup when mode: "stream"
• BuildPhase2 defaults to exec when mode is empty

Test count in IMPLEMENTATION.md updated 15 → 19 for internal/ssr. Correct (scanner tests only; pipeline tests are counted separately).

5. Config Fixtures — One Gap

All config fixtures (valid.yaml, valid.toml, valid.json, alloy.config.yaml) changed from exec mode to stream mode:

# Before:  command: "golit render --defs bundles/"
# After:   command: "golit serve --stdio"
#          mode: "stream"

This means config-file parsing is only validated for stream mode. Exec mode (the default) is tested programmatically in pipeline tests via SSRConfig{Command: "cat"} but has no config-file fixture. Consider either:

• Keeping one fixture (e.g., valid.yaml) in exec mode to test that omitting mode works from a config file, or
• Adding a minimal exec-mode fixture

This is low-severity since the config parser doesn't do anything special with mode (it's just a string field), but it would be more complete.

6. Spec Consistency — Clean

PLAN.md shows both modes with examples, explains the amortized startup benefit, and notes the persistent process lifecycle ("stays alive until build completes or dev server stops"). Phase 3 checklist correctly split into 3 items: config parser, exec mode, stream mode.

IMPLEMENTATION.md BuildPhase2 description updated with both mode paths. StreamRenderer API fully documented.

Not Flagged (Intentionally Out of Scope)

• Error recovery: What happens if the stream process crashes mid-build? Implementation concern, not spec.
• Stream mode for dev server: Process could persist across incremental rebuilds. Good optimization opportunity but doesn't need spec coverage yet.
• Timeout: No per-message timeout specified for stream mode. Implementation can add this without spec change.

Summary

Stream mode is a sound optimization with a clean protocol choice (NUL delimiter). The dual-mode design preserves simplicity for small sites while enabling performance for large ones. One minor gap: config fixtures all moved to stream mode, leaving exec mode without file-based parsing coverage.

[Copilot]

Pull request overview

This PR updates Alloy’s SSR specification and test suite to move from the prior per-instance, multi-protocol SSR pipeline to a simpler per-page external CLI model driven by ssr.command (with optional ssr.mode: stream for a persistent process).

Changes:

• Replace ssr.build + ssr.serve.* config references with ssr.command + ssr.mode across docs, fixtures, and tests.
• Update SSR spec sections in plans/PLAN.md and implementation guidance in plans/IMPLEMENTATION.md to describe per-page stdin→stdout rendering (plus stream/NUL-delimited mode).
• Remove mock-based SSR engine tests (internal/ssr/mock_test.go) and refocus SSR tests around page-level rendering, component tracking, and component-map persistence.

Reviewed changes

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

Show a summary per file

File	Description
test/fixtures/components/alloy.config.yaml	Updates fixture config to ssr.command + ssr.mode.
plans/TESTSUITE_CRITIQUE.md	Updates SSR test gaps to reflect per-page ssr.command rendering.
plans/PLAN.md	Rewrites Phase 2 SSR spec to per-page command model and updates cache invalidation narrative.
plans/IMPLEMENTATION.md	Updates implementation checklist/expected APIs for per-page exec/stream rendering and persistence.
internal/ssr/scanner_test.go	Refactors SSR tests away from per-instance pipeline toward per-page RenderPage + stream renderer concepts.
internal/ssr/mock_test.go	Deletes SSREngine mock tests now that the interface is being removed by the spec.
internal/server/watcher_test.go	Updates SSR config usage in watcher tests to SSRConfig.Command.
internal/server/server_test.go	Updates SSR config usage in server tests to SSRConfig.Command.
internal/pipeline/build_test.go	Updates pipeline tests/spec comments to per-page SSR, adds stream-mode expectations.
internal/config/testdata/valid.yaml	Updates config testdata to ssr.command + ssr.mode.
internal/config/testdata/valid.toml	Updates config testdata to ssr.command + ssr.mode.
internal/config/testdata/valid.json	Updates config testdata to ssr.command + ssr.mode.
internal/config/config_test.go	Updates config parsing expectations to SSR.Command and SSR.Mode.

---

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

[Copilot]

The PR description/issue text says Alloy will pass full page HTML to ssr.command as a CLI argument, but the spec here defines stdin/stdout piping. Please align the PR description (or this section) so the contract is unambiguous—stdin is typically preferable anyway due to shell escaping and command-line size limits.

[zeroedin]

Accepted — this was valid when filed. The contract was aligned to stdin/stdout in commit f7cdd7f, and issue #151 was updated to match. No remaining CLI arg references in spec or tests.

[Copilot]

These tests rely on external shell utilities (cat) being available in PATH. That makes the suite platform-dependent (e.g., Windows CI) and can also behave differently across environments. Consider replacing this with a Go helper process (common TestHelperProcess pattern) or a small test binary invoked via os.Executable() so stdin→stdout behavior is deterministic.

[zeroedin]

Rejected — Alloy's SSR contract is inherently Unix (stdin/stdout piping to an external CLI). The SSR engines (golit, lit-ssr-wasm) are Unix CLI tools. Windows is not a target platform for SSR functionality. Using cat in tests is the simplest, most readable way to prove the stdio contract. A TestHelperProcess pattern would add significant complexity for a platform concern that doesn't apply to this project.

[zeroedin]

Reconsidered — golit supports Windows, so SSR tests should be cross-platform. Updating tests to use the Go TestHelperProcess pattern instead of cat/false/sleep. Fixed in next commit.

[zeroedin]

Final decision: keeping cat in tests. The runtime SSR contract (stdin/stdout via os/exec) is fully cross-platform — Go abstracts process spawning and pipe I/O on all platforms. The only Windows gap is the test utilities (cat, false), not the contract itself. Windows contributors can use WSL to run tests. The TestHelperProcess pattern would add complexity for a scenario that doesn't justify it.

[Copilot]

Using the false command to force a non-zero exit is not portable (not present on Windows) and couples tests to the host shell environment. Prefer a Go helper process that exits with a chosen status code so the test remains cross-platform and deterministic.

[zeroedin]

Rejected — same reasoning as above. false is the standard Unix idiom for "exit non-zero" and Windows is not a target platform for SSR. The readability benefit of false over a TestHelperProcess outweighs the portability concern.

[zeroedin]

Reconsidered — accepting. Replacing false with a TestHelperProcess that exits non-zero. Fixed in next commit.

[zeroedin]

Final decision: keeping false. Same reasoning — test utilities don't need to be Windows-native. The runtime contract works cross-platform via Go's os/exec. Windows contributors use WSL for go test.

[Copilot]

This test currently swallows the Phase 2 error (_ = err), so it will pass even if Phase 2 never runs or returns an unexpected failure. Since these tests are the spec, it would be better to assert either success (and that output equals input for cat-style echo) or assert specific error semantics for the configured command/mode.

[zeroedin]

Accepted — the swallowed error was a genuine weakness. The test now asserts success (cat must complete) and verifies that every Phase 1 page appears in Phase 2 output with valid HTML. Fixed in 4879feb.

[Copilot]

The assertion here checks for "echo" in the error, but the configured SSR command in this test is cat. This makes the test both looser than intended and potentially misleading. Consider tightening the expectation to reference the actual configured ssr.command (or a stable Phase 2 error prefix) so failures are attributed correctly.

[zeroedin]

Accepted — leftover from the echo→cat rename. The assertion now checks for "cat" instead of "echo". Fixed in 4879feb.

[Copilot]

This test uses the external cat command to validate stdin→stdout piping, which is not guaranteed to exist on all supported platforms (notably Windows). If the project intends cross-platform go test, consider using a Go helper process/test binary instead of depending on host utilities.

[zeroedin]

Rejected — same as the scanner_test.go feedback. The SSR contract requires Unix stdin/stdout piping. cat is the correct test tool for proving this contract. Cross-platform go test is not a goal for SSR-related tests.

[zeroedin]

Reconsidered — accepting. Replacing cat with a TestHelperProcess. Fixed in next commit.

[zeroedin]

Final decision: keeping cat. The SSR runtime works on Windows natively (Go's os/exec handles stdin/stdout piping cross-platform). Only the test commands require a Unix-like env, and WSL covers that for Windows contributors.

[Copilot]

Using the false command to ensure Phase 2 would fail if invoked is not portable across platforms. To keep the spec tests deterministic and cross-platform, consider a Go helper process that exits non-zero (or a small test script committed as a fixture) instead of relying on a Unix command.

[zeroedin]

Rejected — same reasoning. false is the standard Unix idiom and Windows portability is not a concern for SSR tests. The test is clear and deterministic on all target platforms.

[zeroedin]

Reconsidered — accepting. Replacing false with a TestHelperProcess. Fixed in next commit.

[zeroedin]

Final decision: keeping false. Same as above — runtime is cross-platform, test utilities can assume Unix. WSL for Windows contributors.

[Copilot]

In the Phase 2 step list, component tracking is listed after per-page SSR (step 17 after 16), but the text says tracking scans the pre-SSR HTML. Later in this doc (§Phase 2: SSR Transform) you also describe tracking as happening before SSR. Please make the ordering consistent (either swap the step numbers or clarify that tracking happens before invoking ssr.command).

Suggested change

	16. **Per-page SSR** — For each page, pipe the full intermediate HTML to the configured `ssr.command` via stdin. The command reads stdin, transforms the HTML, and writes the result to stdout with Declarative Shadow DOM.
	17. **Component tracking** — Scan each page's pre-SSR HTML for custom element tags (anything with a hyphen). Record which pages use which components for cache invalidation.
	16. **Component tracking** — Scan each page's pre-SSR HTML for custom element tags (anything with a hyphen). Record which pages use which components for cache invalidation.
	17. **Per-page SSR** — For each page, pipe the full intermediate HTML to the configured `ssr.command` via stdin. The command reads stdin, transforms the HTML, and writes the result to stdout with Declarative Shadow DOM.

[zeroedin]

Accepted — good catch on the ordering inconsistency. Steps swapped: component tracking is now step 16 (scans pre-SSR HTML), per-page SSR is step 17. Fixed in 4879feb.