test(pipeline): fix Phase 2 tests to require ssr.build execution

[zeroedin]

Summary

• Fixes existing Phase 2 tests that incorrectly validated local DSD transform (shadowrootmode insertion). Per spec §6, BuildPhase2 must shell out to the configured ssr.build command — Alloy does not perform SSR itself.
• Adds 2 new tests proving BuildPhase2 must error when the command fails (not silently fall back to local transform).
• Updates IMPLEMENTATION.md Phase 4D to specify os/exec command execution with no fallback.

Test changes (16 → 18 specs)

Test	Change
"Phase 2 transforms intermediate HTML"	Fixed: expects command execution error, not shadowrootmode output
"Phase 2 receives Phase 1 output"	Fixed: uses echo ok as ssr.build command
"with SSR config, Phase 2 runs"	Fixed: tolerates command execution error
"BuildPhase2 returns error when ssr.build command fails"	New: nonexistent command must error
"BuildPhase2 does not fall back to local transform"	New: must not produce shadowrootmode via local fallback

Current state: 15 pass, 3 fail

The 3 failing tests prove the bug — BuildPhase2 currently does a local transformCustomElements instead of executing ssr.build. These will pass once the implementation shells out via os/exec.

Reopened from #124 (closed with merge conflicts, now resolved).

Test plan

• go vet ./internal/pipeline/... compiles clean
• 15 previously passing tests remain green
• 3 new/modified tests fail, proving the spec violation

Closes #117

🤖 Generated with Claude Code

[zeroedin]

Review: PR #126 — test(pipeline): fix Phase 2 tests to require ssr.build command execution

Scope: internal/pipeline/build_test.go (65 additions, 24 deletions), plans/IMPLEMENTATION.md (2 lines)

Background

The spec (PLAN.md §6, lines 1949-1962) is explicit: "Alloy does not import any SSR engine as a Go dependency. External engines are CLI binaries... Alloy runs the ssr.build command after Phase 1 writes _site/." But the current BuildPhase2 implementation at build.go:797-811 calls transformCustomElements() — a local regex-based DSD transform. The existing tests validated this local behavior, which contradicts the spec.

What changed

Modified tests (3):

1. "Phase 2 executes ssr.build command against intermediate HTML" (was "Phase 2 transforms intermediate HTML") — No longer asserts shadowrootmode in output. Instead asserts that BuildPhase2 returns an error referencing the command (golit, exec, or not found) since the tool won't exist in CI. Correct: the test proves the function attempts external execution rather than locally transforming.

2. "Phase 2 receives Phase 1 output as its input" — Changed ssr.build from "golit render _site/**/*.html" to "echo ok". Since the test only needs to verify Phase 1 output reaches Phase 2, using a command that exists (echo) but doesn't transform files is sufficient. The _ = err pattern is acceptable here — either success or failure proves Phase 2 accepted the input and attempted execution.

3. "with SSR config, Phase 2 runs" — Changed to "echo ok" and wrapped the SSRSkipped assertion in if err == nil. This is pragmatic: the test's purpose is to verify SSRSkipped is false when SSR is configured, but it can't guarantee echo ok will succeed on all platforms.

New tests (2):

4. "BuildPhase2 returns error when ssr.build command fails" — Uses "nonexistent-ssr-tool --transform _site/", asserts error. Clean and direct.

5. "BuildPhase2 does not fall back to local transform when command is unavailable" — The key TDD test. If no error, asserts output does NOT contain shadowrootmode. This directly catches the current bug: transformCustomElements produces shadowrootmode output even when the external command doesn't exist.

IMPLEMENTATION.md:

• Phase 4D test count 16→18.
• BuildPhase2 description updated: "executes the external ssr.build command via os/exec... no silent fallback."

Analysis

1. Spec alignment is correct: The spec says Alloy shells out. The old tests validated local transform. The new tests require external execution.

2. TDD is well-structured: 3 tests will fail against the current implementation, proving the spec violation. Once BuildPhase2 is changed to use os/exec, all 18 tests should pass.

3. The SatisfyAny matcher in test 1 is appropriate: Different OS/shell environments report missing commands differently ("executable file not found" vs "not found" vs referencing the command name). Matching any of golit/exec/not found covers the common cases.

4. No orphaned code concern: transformCustomElements still exists in build.go but will become dead code once the implementation PR lands. The implementation PR should remove it.

One observation

Test 3 ("with SSR config") using if err == nil makes the SSRSkipped assertion conditional. If echo ok fails on some platform, the test silently passes without validating the SSRSkipped field. This is a minor coverage gap, but acceptable given the test's comment explains the intent.

Verdict

LGTM — correct spec alignment. The tests now validate the right contract (external command execution, no local fallback). Clean TDD: 15 pass, 3 fail, proving the bug exists.

[zeroedin]

Re-review: PR #126 — commit cf19d07 addresses SSR skip test gap

New commit: cf19d07 — "fix(test): address review — SSR skip test always validates something"

What changed

The "with SSR config, Phase 2 runs" test previously had a silent pass when echo ok errored — the if err == nil guard meant the SSRSkipped assertion was conditional, and the error path had only a comment.

Now both branches assert:

• Error path: Verifies the error came from Phase 2 command execution (matches echo/exec/ssr), not from Phase 1 or config validation. This proves Phase 2 was attempted.
• Success path: Unchanged — asserts SSRSkipped == false.

Verdict

LGTM — the review concern is resolved. Every code path in the test now validates something meaningful. No other changes in the PR.