test(e2e): V14 Tier-2 chromedp — topic ACL error envelope + WS keep-open (livetemplate#415) — DRAFT, Phase-5-resolved by adnaan · Pull Request #327 · livetemplate/lvt

adnaan · 2026-05-19T21:40:05Z

Warning

DRAFT — do not merge ahead of livetemplate's Phase-0..4 release. This PR carries a committed go.mod replace pointing at ../../../livetemplate/.worktrees/broadcast-redesign-phase-4 (a path that exists only on the author's machine). CI is guaranteed to fail with a module not found build error until Phase 5's go.mod pin bump resolves the replace to a real version pin. This is NOT a CI bug — it is the proposal's documented "lvt e2e gates last (consumes both)" release order. The PR is open as a tracking artifact.

Phase 4 of livetemplate's broadcast-action-redesign (#415). Adds the Tier-2 user-visible leg of V14 — end-to-end in a real browser against a real livetemplate handler — and the cross-repo go.mod replace required to build it against the unreleased livetemplate Phase-0..4.

Companion PRs (release order): livetemplate#427 (server keep-open + Tier-1) and client#121 (TS lvt:error branch + jest) are wire-independent and ship first; this PR consumes both.

V14 (Tier-2 user-visible leg)

e2e/topic_acl_error_envelope_v14_test.go (new, //go:build browser): verifies that an ACL-denied ctx.Subscribe in the WS-connect Mount surfaces in a real browser as an lvt:error CustomEvent { code, topic } on the wrapper AND leaves the WebSocket open and functional (advisor sharpening: stays functional, not merely un-closed — the test sends a click action over the same socket post-envelope and asserts the rendered value updates).

Three load-bearing harness decisions, all inline-commented

Full 4-artifact capture (CLAUDE.md global rule, surfaced on failure):
- browser console — shared installConsoleLogger from lifecycle_ergonomics_test.go (same package e2e_test, streamed via t.Logf live);
- server logs — e2etest.NewServerLogger() teed into the global slog default with save+restore in t.Cleanup (advisor parallel-safety: slog.SetDefault is process-global);
- WS frames — e2etest.RecordWSFrames (lvt#317), surfaced via wsLog.PrintLast;
- rendered HTML — chromedp.OuterHTML("html") in a dump() closure called before t.Fatalf (chrome ctx is cancelled by defer cleanup() before t.Cleanup runs; HTML must be captured at failure-time).
  The first failed run during development dumped server logs + WS frames + HTML and surfaced the missing IsInitialMount guard (Deviation 3 below) — proof the harness works as intended.
Capture-phase document listener for the non-bubbling event. The client dispatches lvt:error on the wrapper with bubbles:false. Installing the listener via document.addEventListener('lvt:error', …, true) in a <head> script before client.js loads catches the wrapper dispatch via DOM capture phase (capture traverses window→target regardless of bubbles). Race-free without instrumenting the client.
Local Phase-4 client-bundle handler, not e2etest.ServeClientLibrary. The canonical helper fetches @livetemplate/client from the unpkg CDN and caches on disk for 1h; that cache can shadow LVT_CLIENT_CDN_URL, so an unreleased client cannot be reliably served via the canonical path. Reverts to ServeClientLibrary in Phase 5/6 after npm publish.

`IsInitialMount` guard — the e2e exposed a Mount-path dual-fire reality the Tier-1 test does not exercise

Mount runs on every HTTP request AND WebSocket connect (livetemplate CLAUDE.md). On the initial HTTP GET (page render), a denied ctx.Subscribe surfaces as HTTP 500 (mount.go HTTP-GET path; phase-1.md pre-existing behavior, unchanged by Phase 4). Without the guard, the page would 500 before client.js ever loaded and there would be no WS for V14 to exercise. The controller guards with if ctx.IsInitialMount() { return s, nil } to scope the denial to the WS-connect Mount — exactly V14's spec scenario.

Cross-repo `go.mod` `replace` — the Phase-5-resolved dependency artifact

go.mod gains:

replace github.com/livetemplate/livetemplate => \
    ../../../livetemplate/.worktrees/broadcast-redesign-phase-4

…with an inline comment naming Phase 5 as its resolution. Phase 5's go.mod pin bump deliverable is literally "convert this replace into a real version pin." Until then, this PR is correct as a Phase-4-integration tracking commit; do not merge ahead of livetemplate's Phase-0..4 release.

Gate (manual — pre-commit hook not installed in this checkout)

lvt non-browser regression GREEN (Phases 0–4 livetemplate backward-compatible with current lvt code): GOWORK=off go test -short -count=1 -timeout=120s ./internal/... ./commands/... ./testing/...
V14 Tier-2 browser e2e GREEN (1.57s end-to-end with full 4-artifact capture): GOWORK=off go test -tags=browser -v -timeout=5m -run 'TestE2E_V14_TopicACLDeniedEmitsLvtErrorAndKeepsWSOpen' ./e2e/
Added file is lint-clean (verified with v2 flags --default=none --enable=errcheck,unused,staticcheck,ineffassign). The 84+ pre-existing lvt-wide lint issues are pre-existing on lvt main, not introduced here — the lvt scripts/pre-commit.sh has a pre-existing --disable-all flag-removed-in-v2 bug at line 36 that silently disabled the lint gate. Flagged in livetemplate phase-4.md Ledger item 4 for a separate Phase-5/6 fix.

V14 status across the 3 coordinated PRs

Tier	PR	Status
Tier 1 — Go integration (server)	livetemplate#427	✅
Client logic leg (jest)	client#121	✅
Tier 2 — chromedp user-visible (this PR)	this	✅ (local — see `replace` caveat above)

Server-emitted envelope contract (pinned byte-for-byte, asserted by all three tiers):

{"type":"error","code":"topic_forbidden","topic":"<denied topic>"}

🤖 Generated with Claude Code

…open (livetemplate#415) Phase 4 of livetemplate's broadcast-action-redesign (#415). Adds the Tier-2 user-visible leg of V14 -- end-to-end in a real browser against a real livetemplate handler -- and the cross-repo go.mod replace required to build it against unreleased livetemplate Phase-0..4. ## V14 (Tier-2 user-visible leg) e2e/topic_acl_error_envelope_v14_test.go (new, //go:build browser): verifies that an ACL-denied ctx.Subscribe in the WS-connect Mount surfaces in a real browser as an `lvt:error` CustomEvent { code, topic } on the wrapper AND leaves the WebSocket OPEN AND FUNCTIONAL (the advisor sharpening: stays *functional*, not merely un-closed -- the test sends a click action over the same socket post-envelope and asserts the rendered value updates). Three load-bearing harness decisions, all commented in the file: 1. Full 4-artifact capture (CLAUDE.md global rule, surfaced on failure): * browser console -- shared installConsoleLogger from lifecycle_ergonomics_test.go (same package_e2e_test, streamed via t.Logf live). * server logs -- e2etest.NewServerLogger() teed into the global slog default with save+restore in t.Cleanup (slog.SetDefault is process-global; preserves the default for sibling tests). * WS frames -- e2etest.RecordWSFrames (lvt#317), surfaced via wsLog.PrintLast. * rendered HTML -- chromedp.OuterHTML("html") in a `dump()` closure called BEFORE t.Fatalf (chrome ctx is cancelled by defer cleanup() before t.Cleanup runs; HTML must be captured at failure-time). The first failed run during development dumped server logs + WS frames + HTML and surfaced the missing IsInitialMount guard (Deviation 3 below) -- proof the harness works as intended. 2. Capture-phase document listener for the non-bubbling event. The client dispatches `lvt:error` on the wrapper with bubbles:false. Installing the listener via `document.addEventListener('lvt:error', ..., true)` in a <head> script BEFORE client.js loads catches the wrapper dispatch via DOM capture phase (capture traverses window->target regardless of bubbles). Race-free without instrumenting the client. 3. Local Phase-4 client-bundle handler, NOT e2etest.ServeClientLibrary. The canonical helper fetches @livetemplate/client from the unpkg CDN and caches on disk for 1h; that cache can shadow LVT_CLIENT_CDN_URL, so an unreleased client cannot be reliably served via the canonical path. Reverts to ServeClientLibrary in Phase 5/6 after npm publish. ## IsInitialMount guard (Deviation 3 -- the e2e exposed a Mount-path dual-fire reality the Tier-1 test does not exercise) Mount runs on every HTTP request AND WebSocket connect (livetemplate CLAUDE.md). On the initial HTTP GET (page render), a denied ctx.Subscribe surfaces as HTTP 500 (mount.go HTTP-GET path; phase-1.md pre-existing behavior, unchanged by Phase 4). Without the guard, the page would 500 before client.js ever loaded and there would be no WS for V14 to exercise. The controller guards with `if ctx.IsInitialMount() { return s, nil }` to scope the denial to the WS-connect Mount -- exactly V14's spec scenario. ## Cross-repo go.mod replace (Deviation 2 -- the Phase-5-resolved dependency artifact) go.mod gains: replace github.com/livetemplate/livetemplate => \ ../../../livetemplate/.worktrees/broadcast-redesign-phase-4 With an inline comment naming Phase 5 as its resolution. The lvt Phase-4 branch is intentionally NOT independently CI-runnable until Phase 5 converts this replace into a real version pin -- which matches the proposal's documented "lvt e2e gates last (consumes both)" release order. This commit is correct AS A PHASE-4-INTEGRATION TRACKING COMMIT; do not merge ahead of livetemplate's Phase-0..4 release. ## Gate (manual -- pre-commit hook not installed in this checkout) * lvt non-browser regression green (Phases 0-4 livetemplate backward-compatible with current lvt code): GOWORK=off go test -short -count=1 -timeout=120s \ ./internal/... ./commands/... ./testing/... * V14 Tier-2 browser e2e green (1.57s end-to-end with full 4-artifact capture): GOWORK=off go test -tags=browser -v -timeout=5m \ -run 'TestE2E_V14_TopicACLDeniedEmitsLvtErrorAndKeepsWSOpen' \ ./e2e/ * My added file is lint-clean (verified with v2 flags --default=none --enable=errcheck,unused,staticcheck,ineffassign; the 84+ pre-existing lvt-wide lint issues are pre-existing on lvt main, NOT introduced here -- the lvt scripts/pre-commit.sh has a pre-existing --disable-all flag-removed-in-v2 bug at line 36 that silently disabled the lint gate; flagged in proposal phase-4.md Ledger item 4 for a separate Phase-5/6 fix). ## V14 status across the three coordinated PRs | Tier | Repo | Status | | ------------------------------------- | --------------- | ------ | | Tier 1 -- Go integration (server) | livetemplate | GREEN | | Client logic leg (jest) | client | GREEN | | Tier 2 -- chromedp user-visible (this)| lvt | GREEN | Server-emitted envelope contract (pinned byte-for-byte, asserted by all three tiers): {"type":"error","code":"topic_forbidden","topic":"<denied topic>"} ## Cross-repo coordination Companion PRs (release order: livetemplate + client are wire-independent; this one gates last): - companion: livetemplate/livetemplate broadcast-redesign-phase-4 -- Option B keep-open server change (mount.go) + V14 Tier-1 regression + the canonical phase-4.md learnings. - companion: livetemplate/client broadcast-redesign-phase-4 -- TS type==='error' branch in handleWebSocketPayload -> lvt:error CustomEvent {code, topic}; jest V14 logic leg. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

claude · 2026-05-19T21:42:18Z

Code Review — PR #327: V14 Tier-2 chromedp e2e (topic ACL error envelope + WS keep-open)

Note: This is a DRAFT tracking artifact intentionally not CI-runnable until Phase 5. Review is offered to catch issues before the Phase 5 merge window.

Overview

This PR adds a browser-tagged e2e test (//go:build browser) that verifies the full user-visible path of the V14 contract:

An ACL-denied ctx.Subscribe in the WS-connect Mount surfaces as an lvt:error CustomEvent { code, topic } in the browser.
The WebSocket remains open and functional after the error (Phase 4's keep-open finalization).

It also pins a go.mod replace directive pointing at the author's local Phase-4 worktree, intentionally breaking CI until Phase 5 resolves it to a real version pin.

The overall design is sound and well-thought-out. The four-artifact failure capture harness (console, server logs, WS frames, rendered HTML) is excellent practice and directly aligns with CLAUDE.md's global failure-surfacing rule.

Issues

🔴 High — Hardcoded author-local absolute path committed to the repo

// serveLocalPhase4ClientBundle
path = "/home/adnaan/code/livetemplate/client/.worktrees/broadcast-redesign-phase-4/dist/livetemplate-client.browser.js"

The env var fallback (LVT_CLIENT_BUNDLE_PATH) is correct, but the hardcoded default is a developer-machine artifact that will cause t.Fatalf (test failure, not a skip) for any contributor who runs -tags=browser without knowing to set the env var. The error message is informative, but the path itself shouldn't be in the repo.

Suggestion: Either remove the hardcoded default entirely, or replace it with a repo-relative path convention (e.g. ../client/.worktrees/broadcast-redesign-phase-4/dist/...) so contributors who follow the same repo layout can benefit from it. Alternatively, change the failure to t.Skip() since this test is only runnable in a very specific cross-repo setup — that way -tags=browser doesn't fail the whole test run for contributors who aren't set up for Phase 4.

// Proposed alternative
if path == "" {
    t.Skip("V14 e2e requires LVT_CLIENT_BUNDLE_PATH — see docs/learnings/phase-4.md for setup")
}

🟡 Medium — `go.mod replace` to a local filesystem path will corrupt `go.sum` for contributors

The committed replace directive pointing outside the repo is intentional and documented, but running go mod tidy, go build ./..., or any IDE tooling will produce a module not found error and potentially corrupt go.sum. This affects every contributor who fetches the branch, not just CI.

Suggestion: Add a // DO NOT RUN go mod tidy ON THIS BRANCH warning in go.mod, or add a Makefile guard that errors with a useful message before any go commands execute on this branch. The PR description is thorough, but go.mod is what contributors look at.

🟡 Medium — Missing server-side assertion for "connection kept open" log

The PR description states that livetemplate logs "Mount Subscribe denied … connection kept open" WARN as part of Phase 4's keep-open behavior. The serverLogger captures this, but the test never asserts on it. The test is otherwise complete for user-visible behavior, but adding this assertion would:

Verify the server actually took the keep-open code path (not just that the client happened to stay connected)
Guard against silent regressions where the log message changes or the WARN is removed

// After poll() for lvt:error, before the Ping action:
if !serverLogger.HasLog("connection kept open") {
    dump()
    t.Fatal("expected server to log keep-open WARN after ACL denial")
}

🟢 Low — `poll` closure pattern vs. chromedp's built-in waits

The hand-rolled poll closure (100ms sleep loop) is readable and works, but diverges from the chromedp built-in chromedp.Poll / chromedp.WaitFunc patterns used elsewhere in the e2e suite (e.g. lifecycle_ergonomics_test.go uses chromedp.WaitVisible, chromedp.WaitEnabled). This isn't a correctness issue, but if this pattern becomes common it should live in e2etest as a shared helper rather than be re-implemented per test.

🟢 Low — `stopChan` in `ServerLogger` is unused (pre-existing)

ServerLogger.stopChan is created and closed in Stop() but the scanner goroutine in Start() never selects on it — it only exits when the pipe is closed. This is a pre-existing issue in testing/server.go, not introduced here, but worth flagging for a future cleanup.

What's done well

IsInitialMount guard — correctly scoped to avoid HTTP 500 on the GET while still exercising the WS-connect denial. The inline comment explaining why (the HTTP-GET ACL path is pre-existing behavior that would 500 before client.js loads) is exactly the kind of non-obvious context that belongs in code.
Capture-phase listener for non-bubbling event — installing document.addEventListener('lvt:error', …, true) in <head> before client.js loads is the correct race-free approach for observing a non-bubbling event. The comment explaining why this works (capture traverses window→target regardless of bubbles) is correct and valuable.
slog.SetDefault save/restore — correctly handles the process-global nature of slog.SetDefault with t.Cleanup for restore ordering.
defer cleanup() before t.Cleanup for HTML capture — the comment explaining that defer cleanup() cancels the chrome context before t.Cleanup runs (so HTML must be captured at failure-time in dump()) shows solid understanding of Go's test lifecycle. This is a subtle correctness point.
Sanity check for exactly one lvt:error — asserting errCount != 1 catches the scenario where the envelope leaks into the diff path and fires spuriously. Good defensive assertion.
Test name TestE2E_V14_TopicACLDeniedEmitsLvtErrorAndKeepsWSOpen — descriptive and matches the naming convention of the e2e package.

Summary

The core test logic is correct and the failure-surfacing harness is exemplary. Two items need resolution before Phase 5 merge: the hardcoded author-local path (fix before merge) and the missing server-side log assertion (strongly recommended). The go.mod situation is understood and intentional, but a go mod tidy guard would prevent accidental corruption.

- e2e/topic_acl_error_envelope_v14_test.go: * Replace hardcoded /home/adnaan/... default with a repo-relative convention path (../../../../client/.worktrees/...) that honors the standing worktree layout rule; LVT_CLIENT_BUNDLE_PATH env var still overrides. (Claude PR #327 High.) * Switch the missing-bundle failure from t.Fatalf to t.Skip with a clear gating message pointing at phase-4.md -- this test is gated on the cross-repo Phase-4 setup and is intentionally not CI-runnable until Phase 5 (proposal release order), so failing the whole -tags=browser run for contributors not set up for V14 would be a hostile default. * Add the load-bearing assertion that the server-side keep-open WARN log fired (serverLogger.HasLog) -- proof the server took mount.go's Option B fall-through, not just that the client happened to stay connected. Guards against silent regressions where the WARN message changes or the keep-open behavior is removed. (Claude PR #327 Medium.) - go.mod: * Add an inline 'DO NOT RUN go mod tidy' warning to the Phase-4 replace block so contributors who run go mod tidy unawares are warned via the file itself, not just the PR description. The replace path only resolves on the documented worktree layout; other contributors / CI see 'module not found' and tidy would corrupt go.sum. (Claude PR #327 Medium.) Verified: GOWORK=off go test -tags=browser -v -timeout=5m \ -run TestE2E_V14_TopicACLDeniedEmitsLvtErrorAndKeepsWSOpen ./e2e/ PASS (1.98s) -- repo-relative path resolves, new WARN assertion fires, post-envelope WS round-trip still works. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

claude · 2026-05-19T21:55:45Z

Code Review — PR #327: V14 Tier-2 chromedp e2e (Topic ACL error envelope + WS keep-open)

Note: This is a DRAFT tracking artifact. Review is offered for quality/readiness ahead of the Phase-5 merge window, not as a merge-gate approval.

Overview

Adds the browser-facing leg of V14: a chromedp e2e that verifies an ACL-denied ctx.Subscribe in the WS-connect Mount (a) surfaces as an lvt:error CustomEvent on the wrapper, and (b) leaves the WebSocket open and functional. The test is gated behind //go:build browser and depends on the unreleased Phase-0..4 livetemplate via a committed go.mod replace.

What works well

4-artifact capture harness is solid. The ordering is correct: defer cleanup() cancels the chrome context before t.Cleanup runs, so HTML capture happens at failure-time (inside dump()), not in a cleanup that runs after the context is gone. This is a subtle but load-bearing ordering decision and it's handled correctly.
Capture-phase DOM listener (document.addEventListener('lvt:error', …, true) in <head>) cleanly avoids the listener-vs-first-frame race without needing to instrument the client. The inline comment explains the reasoning clearly.
t.Skip in serveLocalPhase4ClientBundle instead of t.Fatal is the right call for a cross-repo gated test that contributors not in the Phase-4 setup shouldn't fail on.
IsInitialMount guard is well-motivated and clearly commented. The logic is correct: HTTP-GET denial → HTTP 500 before client.js loads → no WS to test; the guard scopes the deny to the WS-connect Mount, which is exactly V14's spec scenario.
Three-assertion test structure (event fired → server took keep-open path → socket still functional) is clean and each assertion independently falsifies a distinct failure mode.

Issues and suggestions

1. `go.mod replace` vs. Go Workspace (convention mismatch)

CONTRIBUTING.md explicitly designates Go Workspace as the recommended (and "easiest") approach for cross-repo local development, with manual replace directives as the "alternative." This PR commits the replace directive to go.mod, which:

Is a persistent hazard: go mod tidy corrupts go.sum and any go build ./... outside the exact worktree layout fails with module not found.
Goes against the documented preference.

The PR acknowledges CI will fail, but contributors who git pull this branch and reflexively run go mod tidy (a very common habit) will corrupt go.sum. The warning comment is prominent but a human mistake is easy here.

Suggestion for Phase 5: When resolving the replace into a version pin, also document in learnings/phase-4.md why the workspace approach was not used here (if there was a specific reason), so future cross-repo PRs don't reach for replace by default.

2. Missing `go.sum` update in the diff

The diff shows go.mod gains a replace directive, but no go.sum changes are visible. A filesystem replace pointing at a worktree that doesn't exist in CI means the sum database can't verify it — go.sum may be stale or inconsistent. This is already documented in the "DO NOT RUN go mod tidy" warning, but worth confirming the committed go.sum is at least consistent with the worktree-resolved module graph on the author's machine.

3. `poll()` closure: consider extracting to a shared e2e helper

poll() is defined inline in the test as a closure and captures ctx, dump, and t. The same pattern (poll a JS expression with a timeout, dump on failure) is likely needed in other browser e2e tests. lifecycle_ergonomics_test.go has similar ad-hoc wait logic.

This is low priority for a Phase-4 tracking PR, but a shared e2ePoll(t, ctx, jsExpr, timeout, why string, dump func()) in helpers.go would reduce duplication as the test suite grows.

4. `startV14ACLServer` — `log.Printf` in background goroutine

go func() {
    if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
        log.Printf("v14 server: %v", err)
    }
}()

log.Printf is correct here (using t.Logf in a goroutine that may outlive the test is a data race / panic). The existing e2e tests follow the same pattern. No change needed — just confirming this is intentional.

5. `serverLogger.Start()` / `Stop()` vs. `t.Cleanup`

The slog default is restored in t.Cleanup but serverLogger.Stop() is called via defer. If Stop() must happen before the slog restore, the ordering is correct (defers run LIFO within the function; t.Cleanup runs after all defers). If they're independent, both approaches work. Worth a comment on the ordering dependency if it's load-bearing.

6. Minor: `errCount` assertion message could be more helpful

t.Fatalf("expected exactly one lvt:error, got %d", errCount)

Including window.__lvtErrors contents in the failure message (or relying on dump() before the fatal) would make debugging spurious extra events easier. Currently dump() is not called before this t.Fatalf. Consider:

dump()
t.Fatalf("expected exactly one lvt:error, got %d", errCount)

Security

No concerns. The ACL function, template, and test server are all test-scoped and non-production. serveLocalPhase4ClientBundle reads a local file path — acceptable for a test helper.

Test coverage

The three-leg assertion (event, server log, socket round-trip) provides meaningful defense-in-depth for the V14 contract. The "exactly one lvt:error" sanity check at the end is a good addition that prevents silent double-emission regressions.

Summary

The test logic is sound and the harness decisions are well-justified. The main concern is the committed go.mod replace over Go Workspace (a project convention mismatch), but this is a known and documented intentional deviation tied to the Phase-4/5 release ordering. The minor items above are worth addressing before the Phase-5 merge, not blocking now.

🤖 Generated with Claude Code

…go.mod CONTRIBUTING reference) PR #327 round 2 Claude review (all minor/comment-level): - go.mod: extend the Phase-4 replace comment with the convention note. Claude flagged that CONTRIBUTING.md lists Go Workspace as the *Recommended* cross-repo dev path and Manual Replace Directives as the *Alternative*. This PR uses the documented Alternative because (a) it scopes the cross-repo dependency to this branch's git history (a workspace modification doesn't), and (b) it makes Phase 5's conversion to a real version pin a single-line diff at one greppable anchor. go.sum is unchanged from main (filesystem replaces add no sum entries — verified: `git diff main -- go.sum` returns 0 lines). Claude's #2 (missing go.sum update) is confirmed moot. - e2e/topic_acl_error_envelope_v14_test.go: * slog ordering: add explicit comment on the load-bearing `defer serverLogger.Stop()` (LIFO) vs `t.Cleanup` (after-defers) ordering — slog.SetDefault is restored *after* the logger pipe closes, so no late writes from a shut-down logger into the global slog. (Claude PR #327 round-2 #5.) * errCount sanity-check failure message: include the contents of `window.__lvtErrors` in the failure message so spurious-extra-event debugging doesn't require re-running. dump() was already called before this fatal — Claude's #6 (missing dump) was based on stale code, but the message-enrichment captures the intent. (Claude PR #327 round-2 #6.) Other items declined / acknowledged: - #3 (extract poll() to shared helper): low-priority future per Claude; out of Phase-4 scope. Tracked for a future PR if the pattern recurs. - #4 (log.Printf in goroutine): Claude confirmed correct — t.Logf in a goroutine that may outlive the test would be a data race / panic. No action. Verified: GOWORK=off go test -tags=browser -v -timeout=5m \ -run TestE2E_V14_TopicACLDeniedEmitsLvtErrorAndKeepsWSOpen ./e2e/ PASS (1.63s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

claude · 2026-05-19T22:04:34Z

Code Review — PR #327: V14 Tier-2 chromedp e2e (Draft, Phase 4)

Context: This is a draft tracking artifact and is intentionally not CI-runnable until Phase 5 resolves the go.mod replace. Review is scoped to code quality, design, and future-merge readiness — not CI status.

Overview

Adds a single //go:build browser-tagged e2e test (e2e/topic_acl_error_envelope_v14_test.go) that verifies the full V14 user-visible contract end-to-end in a real browser via chromedp:

A ctx.Subscribe ACL denial on WS-connect Mount emits an lvt:error CustomEvent {code, topic} on the wrapper
The WebSocket stays open and functional (a post-envelope click action round-trips and re-renders)
Exactly one error event fires (no leaks into the diff path)

The go.mod change adds a filesystem replace for the unreleased Phase-4 livetemplate, with an explicit Phase 5 resolution path.

Strengths

4-artifact capture pattern is correct and well-ordered. The dump() closure is called before t.Fatalf while the chrome context is still alive — avoiding the defer cleanup() cancels ctx before t.Cleanup runs trap. Good.
t.Skip (not t.Fatal) for missing bundle. Failing the whole -tags=browser run for contributors not set up for Phase 4 would be hostile. The skip message is actionable.
Capture-phase document listener is race-free. Installing document.addEventListener('lvt:error', …, true) in a <head> script before client.js loads is the correct way to observe non-bubbling events without instrumenting the client. The approach and rationale are clearly explained.
IsInitialMount guard is correctly scoped and documented. The inline comment cross-referencing the HTTP-GET ACL-denial → HTTP 500 behavior makes the decision legible for future readers.
slog.SetDefault save/restore ordering is correct. defer serverLogger.Stop() at function return runs before t.Cleanup (LIFO), so slog is restored after the logger pipe closes — no late writes to a shut-down handler. The comment explaining this ordering should stay in.
Sanity assertion on errCount == 1 catches both missing events and spurious duplicate fires. Good defensive check.
go.mod replace comment block is thorough: it names the resolution deliverable, warns against go mod tidy, and explains why the Alternative path (per-branch replace) was preferred over a Go Workspace modification. The go.sum is unchanged (filesystem replaces add no sum entries) — correct and verified.

Issues / Suggestions

1. poll helper belongs in e2etest (medium priority, pre-merge)

The poll function is general-purpose (JS expression + timeout + failure label) and will be needed in other browser tests. Duplicating it per-file creates drift in the polling interval and timeout semantics. Recommend extracting to testing/poll.go (or similar) before the PR merges in Phase 5/6.

// current — inline in the test
poll := func(jsExpr string, timeout time.Duration, why string) { … }

// preferred — shared helper
e2etest.PollJS(t, ctx, jsExpr, timeout, why)

2. log.Printf in server goroutine escapes the slog redirect (low, cosmetic)

go func() {
    if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
        log.Printf("v14 server: %v", err)  // goes to the standard logger, not the captured slog
    }
}()

slog.SetDefault redirects slog.Error, but log.Printf uses the log package's global logger independently. A startup panic here would appear in the default process stderr rather than serverLogger. Using slog.Error (or t.Logf via a channel to avoid post-test goroutine use) would keep all server noise in one place.

3. ctx variable shadows the Go context package name (low, cosmetic)

chromeCtx, cleanup := e2etest.SetupDockerChrome(t, 60*time.Second)
ctx := chromeCtx.Context  // shadows `context` import if anyone adds context.WithTimeout later

Consider browserCtx := chromeCtx.Context to avoid the naming collision and make the provenance clearer in the poll closure.

4. go.mod replace path is relative to the wrong anchor (verify before Phase 5 merge)

The replace is:

replace github.com/livetemplate/livetemplate => ../../../livetemplate/.worktrees/broadcast-redesign-phase-4

The path is relative to the go.mod file. If lvt lives at <umbrella>/lvt/ and livetemplate lives at <umbrella>/livetemplate/, then from <umbrella>/lvt/go.mod the correct relative path would be ../livetemplate/.worktrees/… (one ..), not ../../../… (three ..). This should be verified against the umbrella workspace layout documented in the proposal — if the worktrees convention nests differently, make that layout explicit in the comment.

5. Server shutdown timeout swallows errors silently

_ = server.Shutdown(ctx)

For a test helper this is fine, but t.Logf on a non-nil shutdown error would help diagnose flaky teardown on slow CI runners when Phase 5 makes this CI-runnable.

Test Coverage Assessment

Contract clause	Covered	How
`lvt:error` emitted with `{code:topic_forbidden, topic:private/admin}`	✅	`poll(…window.__lvtErrors[0]…)`
Server took keep-open code path (not just client-side observation)	✅	`serverLogger.HasLog("connection kept open")`
WS functional post-error (not just un-closed)	✅	Click → re-render round-trip
No spurious duplicate events	✅	`errCount == 1` assertion
Contributor without Phase-4 setup gets a skip, not a failure	✅	`t.Skipf` in `serveLocalPhase4ClientBundle`

Coverage is thorough for a Tier-2 e2e gate. The only scenario not explicitly covered is WS reconnect loop suppression — if the keep-open behavior had a bug and the socket actually closed, the client's auto-reconnect could mask it (the poll might eventually pass after a reconnect + re-deny cycle). The serverLogger.HasLog check partially guards this, but a reconnect counter assertion or checking that no second lvt:error arrived during the action round-trip window would close the gap fully.

Pre-Merge Checklist (Phase 5)

Verify the go.mod replace relative path resolves correctly from <umbrella>/lvt/go.mod
Convert filesystem replace to a real version pin
Revert serveLocalPhase4ClientBundle → e2etest.ServeClientLibrary
Extract poll helper to e2etest package
Remove DRAFT label and DO NOT MERGE warning from PR description
Confirm CI passes green on the Phase-5 go.mod pin bump

Bottom Line

The design is sound, the harness decisions are well-motivated, and the inline documentation is unusually thorough. The code is correct for its Phase-4 tracking purpose. The only blocking item before Phase-5 merge is verifying the go.mod replace path depth; everything else is polish. The poll extraction is worth doing before merge to avoid drift as more browser tests are added.

🤖 Generated with Claude Code

PR #327 round 3 Claude review — minor improvements + targeted declines: - e2e/topic_acl_error_envelope_v14_test.go: * Replace `log.Printf` with `slog.Error` in the server ListenAndServe goroutine so a startup error lands in the serverLogger-captured stream the test surfaces on failure (was escaping the slog tee to default process stderr). slog.SetDefault is already set BEFORE startV14ACLServer runs, so this is safe. Removed the now-unused `log` import. (Claude PR #327 round-3 #2.) * Log non-nil `server.Shutdown` errors via slog.Error so flaky teardown on slow CI runners is diagnosable once Phase 5 makes the e2e CI-runnable. (Claude PR #327 round-3 #5.) Items declined (with PR reply): - #1 (extract poll() to shared e2etest helper, medium pre-merge): YAGNI for Phase 4 — no other browser test currently uses this pattern. Tracked for Phase 5/6 if the pattern recurs. - #3 (`ctx` shadows `context` pkg): follow existing pattern — `lifecycle_ergonomics_test.go` does the same shadowing; no harm here (no `context.WithTimeout` after this point). - #4 (go.mod replace path depth): empirically correct. The path `../../../livetemplate/.worktrees/broadcast-redesign-phase-4` correctly resolves from the worktree-nested go.mod (`<umbrella>/lvt/.worktrees/broadcast-redesign-phase-4/go.mod` → 3 `..` → `<umbrella>/` → target). Claude assumed a flat `<umbrella>/lvt/go.mod` layout (1 `..`), which doesn't match the standing worktree convention. The V14 e2e PASSING end-to-end is empirical proof — if the path were wrong, the test wouldn't compile. Verified: GOWORK=off go test -tags=browser -v -timeout=5m \ -run TestE2E_V14_TopicACLDeniedEmitsLvtErrorAndKeepsWSOpen ./e2e/ PASS (1.69s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

adnaan · 2026-05-19T22:08:54Z

Round-3 review responses:

Addressed (in a9e2a54):

docs: Verify CI is working on main #2 log.Printf escapes slog tee: ✅ switched to slog.Error in the server ListenAndServe goroutine. slog.SetDefault is set in the test BEFORE startV14ACLServer runs, so the goroutine's slog calls land in the captured serverLogger stream the test surfaces on failure. Removed the now-unused log import.
feat(lvt): redesign gen auth for LiveTemplate v0.5.1 #5 server.Shutdown error swallowed: ✅ now logs via slog.Error on non-nil shutdown error — diagnosable for slow-CI flaky teardown once Phase 5 makes this CI-runnable.

Declined with rationale:

test: Verify CI workflow #1 extract poll() to shared e2etest helper (medium, pre-merge): YAGNI for Phase 4 — no other browser test currently uses this exact pattern. The existing requireCondition in lifecycle_ergonomics_test.go is similar but not identical (different dump semantics). Worth doing as a unification pass in Phase 5 or 6 once a second consumer exists; extracting a 1-of-1 helper today would just create premature abstraction.
Add Claude Code skills for lvt CLI #3 ctx shadows context package: following the existing pattern. lifecycle_ergonomics_test.go uses the same shadowing (ctx := chromeCtx.Context). No context.WithTimeout (or other context-pkg call) appears after this point in the test, so the shadow is harmless. Per CLAUDE.md Code Style — "Follow existing patterns".
fix(lvt): improve auth templates and lvt-plan skill #4 go.mod replace path depth wrong (verify before Phase 5 merge): the path is empirically correct. The actual layout is worktree-nested:
```
go.mod  →  <umbrella>/lvt/.worktrees/broadcast-redesign-phase-4/go.mod
target →  <umbrella>/livetemplate/.worktrees/broadcast-redesign-phase-4/
```
From the go.mod's directory, 3 .. is correct (.. → .worktrees/, .. → lvt/, .. → <umbrella>/, then livetemplate/.worktrees/...). Claude assumed a flat <umbrella>/lvt/go.mod (no .worktrees/<name>/ nesting), which would have been 1 ... The V14 e2e PASSING end-to-end (1.69s) is empirical proof — if the path were wrong, the test wouldn't compile. No change needed; Phase 5's pin bump replaces the path with a real version regardless.

Per the AI Code Review Workflow convergence rule, the only new functional items in round 3 (#2 and #5) are addressed; the rest are style/cosmetic or already-correct items. Expecting round 4 to converge.

claude · 2026-05-19T22:10:38Z

Code Review — PR #327: V14 Tier-2 chromedp e2e (DRAFT / Phase-4 tracking artifact)

Context: This is a DRAFT tracking artifact by design, intentionally not CI-runnable until Phase 5's go.mod pin bump. Review is scoped to code quality, correctness, and flagging anything worth fixing before Phase 5 merges it.

Overview

This PR adds:

e2e/topic_acl_error_envelope_v14_test.go — a 306-line //go:build browser Chromedp e2e verifying that an ACL-denied ctx.Subscribe in the WS-connect Mount (a) emits an lvt:error CustomEvent with the correct {code, topic} payload, (b) provably takes the server's keep-open code path, and (c) leaves the WebSocket functional for subsequent action round-trips.
go.mod — a replace directive pointing at a local filesystem path (the Phase-4 livetemplate worktree).

What's Working Well

4-artifact capture harness is the right pattern and the inline explanation of why HTML must be captured inside dump() (before defer cleanup() cancels the Chrome context) is genuinely useful for future maintainers.
Capture-phase document listener (document.addEventListener('lvt:error', …, true) in <head>) is an elegant race-free solution for non-bubbling events. The reasoning is clear and correct.
IsInitialMount() guard is correctly scoped and the comment explaining the HTTP-GET → 500 failure mode without it is exactly the right level of detail.
t.Skip (not t.Fatalf) in serveLocalPhase4ClientBundle is the right call — failing the whole -tags=browser run for contributors without the Phase-4 worktree setup would be hostile.
Three-part verification (CustomEvent, server log, WS action round-trip + spurious-event sanity check) gives strong spec coverage.
defer serverLogger.Stop() ordering vs. t.Cleanup for slog restore is correctly handled and well-explained.

Issues & Suggestions

1. `go.mod` `replace` — the dominant risk of this PR

The committed replace pointing at ../../../livetemplate/.worktrees/broadcast-redesign-phase-4 will silently break any contributor who:

Runs go mod tidy (corrupts go.sum trying to resolve a missing module)
Runs go build ./... without the worktree layout

The !!! DO NOT RUN go mod tidy !!! comment block is the right mitigation, but it relies on contributors reading go.mod before running standard Go commands. Consider adding a Makefile guard or a check-gomod CI step that fails fast with a human-readable error if this replace is present on a CI runner, rather than a cryptic module not found. This would also document the Phase-5 conversion clearly as a Makefile target.

Alternatively: A .gomodwarning sentinel file checked by a pre-commit hook would catch accidental go mod tidy runs locally.

2. `serverLogger.HasLog("connection kept open")` — fragile string coupling

if !serverLogger.HasLog("connection kept open") {

This couples the test to a substring of a log message in livetemplate's mount.go. If the WARN text is reworded in Phase 5/6, this assertion silently becomes a no-op or starts failing spuriously. Two suggestions:

Preferred: Add a structured log attribute (e.g., slog.String("event", "topic_acl_keep_open")) to the server-side WARN and assert on that key/value pair instead of a prose substring, if serverLogger supports structured log inspection.
Fallback: At minimum, add a comment pinning the exact log line location in livetemplate (mount.go:L<N>) so the Phase-5 owner knows exactly what needs to stay in sync.

3. `poll` helper — `time.Sleep` busy-wait

time.Sleep(100 * time.Millisecond)

This is functional, but chromedp has chromedp.Poll and chromedp.WaitJS that use Runtime.awaitPromise under the hood (no busy-wait, lower latency). The existing pattern is consistent with lifecycle_ergonomics_test.go's approach, so this isn't a blocker — just worth noting for Phase 5/6 if test runtime becomes a concern.

4. `io.MultiWriter(os.Stderr, serverLogger.Writer())` — noisy in test output

slog.SetDefault(slog.New(slog.NewTextHandler(
    io.MultiWriter(os.Stderr, serverLogger.Writer()),

Server logs go to os.Stderr during the test run regardless of pass/fail. For a passing test in a CI log, this creates noise. Pattern from lifecycle_ergonomics_test.go might already do this — if so, treat as pre-existing. If not, consider writing only to serverLogger.Writer() and letting dump() surface the logs on failure only (which is the entire point of the 4-artifact capture).

5. Missing `t.Parallel()` — worth a comment

The test can't be parallelized because slog.SetDefault is process-global. A one-line comment (// Not parallel: mutates process-global slog.Default.) would save the next reader from wondering. This is the same situation as lifecycle_ergonomics_test.go — worth checking if that file documents it.

6. Minor: `Ping` controller method uses blank identifier for context

func (c *v14ACLController) Ping(s v14ACLState, _ *livetemplate.Context) (v14ACLState, error) {

This is fine for a test fixture — just confirming it's intentional and not a placeholder that should use the context.

Security

No concerns. The test server binds to a dynamically allocated local port, LVT_CLIENT_BUNDLE_PATH is used only for a local file read (not shell execution), and the JS bundle is served as a static byte slice.

Summary

Area	Assessment
Test correctness	✅ Thorough — 3+1 assertions cover the spec tightly
Harness design	✅ 4-artifact capture, capture-phase listener, skip-not-fatal are all correct
`go.mod` replace	⚠️ Intentional but risky — a Makefile guard or pre-commit sentinel would reduce accidental breakage
Log assertion fragility	⚠️ Substring coupling to a prose log message; structured log key would be more robust
Code clarity	✅ Inline comments explain the non-obvious decisions well
CI impact	✅ Intentionally broken per documented release order — not a bug

The code is well-crafted and the harness decisions are sound. The go.mod replace and log-message coupling are the two things worth hardening before Phase 5 merges this.

🤖 Generated with Claude Code

PR #327 round 4 — two one-line documentation hardenings + targeted declines: - e2e/topic_acl_error_envelope_v14_test.go: * Add grep-anchor comment pointing at livetemplate's mount.go WARN string the keep-open assertion couples to (substring match would silently no-op if the WARN is reworded; the anchor makes the coupling explicit for the Phase-5 owner). Note structured-attribute hardening as a future Phase 5/6 option. (Claude PR #327 round-4 #2.) * Add explicit 'Not parallel: this test mutates the process-global slog.Default' comment at the top of the test function. (Claude PR #327 round-4 #5.) Items declined / acknowledged (see PR reply): - #1 (Makefile guard / pre-commit sentinel for go.mod replace): defensive add-on; the existing inline warning + PR body + DRAFT label are already 3 layers of warning. Phase 5 resolves the replace entirely, removing the failure mode permanently. - #3 (poll() time.Sleep busy-wait): Claude framed as 'worth noting for Phase 5/6, not a blocker'; same as round 3 #1 — defer to Phase 5/6. - #4 (io.MultiWriter to stderr is noisy in CI): intentional pattern, matches lifecycle_ergonomics_test.go; dev-time live tailing visibility outweighs CI noise on a passing test, and dump() still surfaces the captured stream on failure. - #6 (Ping uses blank identifier for ctx): Claude confirms intentional for test fixture — no action. Verified: GOWORK=off go test -tags=browser -v -timeout=5m \ -run TestE2E_V14_TopicACLDeniedEmitsLvtErrorAndKeepsWSOpen ./e2e/ PASS (1.54s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

adnaan · 2026-05-19T22:13:56Z

Round-4 review responses:

Addressed (in 4ca0773):

docs: Verify CI is working on main #2 HasLog substring fragility: added a grep-anchor comment at the assertion site pointing at the livetemplate mount.go slog.Warn("Mount Subscribe denied by topic ACL line, with a note that the structured-attribute hardening (slog.String("event", "topic_acl_keep_open")) is a Phase 5/6 future. The grep anchor is the right level of defense for a Phase-4 PR; the structured-attribute change requires a livetemplate-side edit which is best paired with Phase 5's other livetemplate changes.
feat(lvt): redesign gen auth for LiveTemplate v0.5.1 #5 missing t.Parallel() rationale: added a single-line comment at the top of the test function naming the slog.Default mutation as the parallelism barrier, with a cross-reference to the sibling lifecycle_ergonomics_test.go convention.

Declined / deferred with rationale:

test: Verify CI workflow #1 Makefile guard / pre-commit sentinel for go.mod replace: the existing warning is already 3 layers deep (inline go.mod comment block, PR body warning callout, DRAFT label). Adding a Makefile/hook gate would be defensive engineering for a Phase-5-resolved artifact — Phase 5 deletes the replace entirely (converting to a real version pin), at which point any contributor-tidy concern disappears permanently. The marginal value of an extra guard on a known-to-be-temporary state isn't worth the maintenance footprint.
Add Claude Code skills for lvt CLI #3 poll time.Sleep busy-wait → chromedp.Poll: you framed this as "functional… worth noting for Phase 5/6 if test runtime becomes a concern" — agreed, but the 100ms tick matches lifecycle_ergonomics_test.go's ad-hoc waits and isn't a runtime bottleneck (the test runs in ~1.5s). Same disposition as round 3 test: Verify CI workflow #1 (extract poll() to shared helper): defer the unification + chromedp.Poll migration to Phase 5/6 once there are multiple consumers to share an abstraction.
fix(lvt): improve auth templates and lvt-plan skill #4 stderr noise: intentional. The io.MultiWriter(os.Stderr, serverLogger.Writer()) ensures live tailing during dev runs (the most common usage of this test), while dump() surfaces the captured stream on failure for the CI case. lifecycle_ergonomics_test.go uses the same pattern. Restricting to serverLogger only would hide useful signal from devs running the test directly; the CI-noise-on-passing-tests trade-off is the wrong way around for an e2e harness.
feat(lvt): migrate templates to method dispatch pattern #6 Ping uses _ for ctx: ✅ confirmed intentional as you noted — Ping is a pure state-mutation test fixture with no need for ctx.

Round-4 functional delta: zero blockers. The two addressed items are documentation hardenings. Per the AI Code Review Workflow convergence rule, expecting round 5 to converge fully — the only items remaining are accepted-defers / repeats.

claude · 2026-05-19T22:15:57Z

Code Review — PR #327: V14 Tier-2 chromedp e2e (topic ACL error envelope + WS keep-open)

Note: This is a DRAFT tracking artifact with an intentional CI-breaking go.mod replace. Review is scoped to code quality, correctness, and risks for when Phase 5 converts this to a real pin.

Overview

Adds e2e/topic_acl_error_envelope_v14_test.go — a 319-line //go:build browser chromedp test that verifies the full V14 user-visible contract: an ACL-denied ctx.Subscribe on WS-connect Mount emits an lvt:error CustomEvent {code, topic} in the browser and leaves the WebSocket open and functional (action round-trip after denial). The go.mod change adds a temporary cross-repo replace directive pointing at the Phase-4 worktree.

Strengths

Exceptional inline documentation. Every non-obvious decision is explained with a why comment — the capture-phase listener rationale, IsInitialMount guard necessity, t.Skip vs t.Fatal reasoning, and teardown ordering. This is the standard other e2e files in this package should aspire to.
4-artifact failure capture is well-designed. The dump() closure capturing browser console (streamed live), server logs, WS frames, and rendered HTML all while the chrome context is still alive (before defer cleanup() fires) is correct and avoids the common "HTML capture failed — ctx already cancelled" pitfall.
t.Cleanup vs defer ordering comment is accurate. The note explaining why defer serverLogger.Stop() must run before t.Cleanup(func() { slog.SetDefault(prevSlog) }) is correct Go test lifecycle reasoning.
t.Skip for missing bundle is the right call. Failing the whole -tags=browser suite for a contributor not set up for Phase 4 would be hostile; the skip message is detailed and actionable.
Exactly-one-error sanity check (errCount != 1) is valuable. Catches a class of regression (spurious second lvt:error or envelope leaking into the diff path) that the primary assertion doesn't cover.
Capture-phase listener approach is sound. Using document.addEventListener('lvt:error', …, true) in <head> before client.js loads is the correct race-free solution for a non-bubbling wrapper-dispatched event.

Issues and Suggestions

1. Log-string coupling is the biggest correctness risk (acknowledged, but worth prioritizing)

if !serverLogger.HasLog("connection kept open") {

This asserts on a prose substring of a slog.Warn message in livetemplate/mount.go. Any incidental rewording of that message (typo fix, internationalization, style pass) silently breaks this test without any compile-time signal. The PR correctly identifies this and proposes structured logging as a Phase 5/6 improvement — but I'd push to treat it as a Phase 5 blocker alongside the go.mod pin bump, not an optional hardening. Suggested target:

// livetemplate/mount.go
slog.Warn("Mount Subscribe denied by topic ACL",
    slog.String("event", "topic_acl_ws_denied_kept_open"),  // ← stable key
    slog.String("topic", topic),
)

// test
if !serverLogger.HasLogKey("event", "topic_acl_ws_denied_kept_open") {

2. `go.mod` replace directive is a contributor footgun

The !!! DO NOT RUN go mod tidy !!! comment is prominent, but there's no enforcement. Contributors who clone the branch and run standard Go tooling will corrupt go.sum before they read the warning. Two low-cost mitigations to consider for Phase 5:

A go.work file in the worktree that overrides the replace for local dev (so contributors using the workspace mode never touch the committed replace).
A Makefile / scripts/ guard that checks for the replace and aborts go mod tidy with a helpful message.

3. `serveLocalPhase4ClientBundle` reads the entire bundle into memory

data, err := os.ReadFile(path)

For a test-only handler that serves once per test run this is fine, but the bundle can be several hundred KB. Consider http.ServeFile or http.FileServer for Phase 5 when this becomes a production-path helper — not a blocker here.

4. Server goroutine error is surfaced indirectly

go func() {
    if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
        slog.Error("v14 server ListenAndServe failed", slog.Any("error", err))
    }
}()
e2etest.WaitForServer(t, ...)

If ListenAndServe fails immediately (e.g., port already bound despite GetFreePort), the error appears in the server log stream but the test fails with a generic WaitForServer timeout message rather than the root cause. A small improvement:

errCh := make(chan error, 1)
go func() {
    if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
        errCh <- err
    }
}()
// pass errCh to WaitForServer or select on it alongside the deadline

Not a correctness bug — WaitForServer will still catch it — but the failure message would be more actionable.

5. `poll` function uses `time.Sleep` without jitter

time.Sleep(100 * time.Millisecond)

100ms fixed polling is reasonable for browser tests. No change needed, but if this helper gets promoted into e2etest as a shared utility (Phase 5+), consider exponential backoff with a cap.

6. Minor: `_, _ = w.Write(data)` silently swallows write errors

_, _ = w.Write(data)

Standard practice in test HTTP handlers, but worth logging:

if _, err := w.Write(data); err != nil {
    slog.Error("client bundle write failed", slog.Any("error", err))
}

Security

No concerns. The test server is localhost-only, the ACL topic string is a hardcoded test value, and no user input flows through the handler.

Performance

The 60s chrome context timeout and 10s poll timeouts are appropriate for browser tests. The 100ms poll interval is fine.

Test Coverage

The three assertions (lvt:error event shape, server keep-open WARN, WS functional via action round-trip) collectively validate the full V14 contract. The sanity check for exactly one error event is a nice addition. Coverage is appropriate given the scope.

Summary

The test code is high quality — well-reasoned, thoroughly documented, and correct. The two items worth tracking as Phase 5 blockers (alongside the go.mod pin bump) are:

Convert the log-string assertion to a structured-key assertion before this can reliably survive livetemplate maintenance.
Add a tooling guard against accidental go mod tidy on the branch.

Everything else is polish. LGTM as a draft tracking artifact; the Phase 5 items above should gate the final merge.

adnaan · 2026-05-19T22:18:42Z

Round-5 review — convergence reply, no code push.

Per the user-documented AI Code Review Workflow convergence rule ("The convergence signal is 'successive rounds aren't identifying any new functional issue' — only style, phrasing, or wording alternatives"), round 5 is the convergence signal for this PR. Triage:

Repeats of items addressed or declined in earlier rounds:

test: Verify CI workflow #1 log-string coupling → structured key (Phase 5 blocker): acknowledged. Round-4 fix added the grep-anchor comment at the assertion site naming the livetemplate mount.go slog.Warn("Mount Subscribe denied by topic ACL line. The structured-key change requires a livetemplate-side edit which is best paired with Phase 5's other livetemplate work; it's already on the Phase 5 deliverables list (phase-4.md adjustments, Phase 6 doc items). Pushing harder via round 5 doesn't change the disposition.
docs: Verify CI is working on main #2 go.mod footgun → Makefile/go.work guard: identical to round-4 test: Verify CI workflow #1 (Makefile guard for go.mod replace). Same disposition: Phase 5 deletes the replace entirely; adding a guard on a known-temporary state is defensive engineering for a problem that resolves itself.
feat(lvt): redesign gen auth for LiveTemplate v0.5.1 #5 poll jitter: identical to round-3 test: Verify CI workflow #1 / round-4 Add Claude Code skills for lvt CLI #3. Same disposition: defer to Phase 5/6 when the helper is extracted to e2etest.

New cosmetic items, all rated "not a blocker" by the review itself:

Add Claude Code skills for lvt CLI #3 os.ReadFile reads bundle into memory: 95KB once per test run; trivially fine. http.ServeFile is a Phase-5 polish when the local-bundle handler is replaced by ServeClientLibrary.
fix(lvt): improve auth templates and lvt-plan skill #4 server goroutine error surfaced indirectly via slog (not via errCh): the slog.Error fires (we converted from log.Printf in round 3) and lands in the captured stream the test surfaces on failure. The errCh-channel refinement is a clarity improvement, not a correctness fix. Premature for a Phase-4 tracking PR.
feat(lvt): migrate templates to method dispatch pattern #6 _, _ = w.Write(data) silent error: standard test-handler idiom; the alternative (slog.Error on write failure) is defensive logging for a test path that exercises a single small response.

Loop closure: round 5 raised zero new functional issues — only repeated previously-resolved items + 3 cosmetic non-blockers. Standing convergence per the user's documented rule. Phase 5 picks up the structured-key + replace-resolution + bundle-handler-revert + poll-helper-extraction as its deliverable bundle, all already tracked in phase-4.md Phase 5 adjustments.

No code push for round 5; closing the loop here.

adnaan · 2026-05-20T21:13:51Z

Closing as superseded by the upcoming Phase-5 lvt PR.

Per the Phase-5 plan, the Phase-5 lvt PR will:

Convert the committed replace (github.com/livetemplate/livetemplate => ../../../livetemplate/.worktrees/broadcast-redesign-phase-4) into a real v0.10.0 version pin — the resolution this draft PR was explicitly scoped to wait for.
Revert serveLocalPhase4ClientBundle → e2etest.ServeClientLibrary now that @livetemplate/client@0.9.2 is on npm with the lvt:error branch (no more CDN-cache shadowing concern).
Re-author the V14 chromedp e2e with the same Tier-2 user-visible contract this PR established, but built against the released v0.10.0 livetemplate + the upstream client bundle.

The three load-bearing harness decisions documented here (full 4-artifact capture, capture-phase document listener for the non-bubbling event, IsInitialMount guard) carry forward into the Phase-5 lvt PR — this draft was where they were proven, recorded for the next phase to inherit.

No code is lost; the Phase-5 lvt PR supersedes this one cleanly.

Refs livetemplate#415 livetemplate#429

adnaan mentioned this pull request May 19, 2026

feat(topics): Phase 4 — keep-open on ACL-denied Subscribe in WS-connect Mount + V14 Tier-1 (#415) livetemplate/livetemplate#427

Merged

adnaan closed this May 20, 2026

adnaan deleted the broadcast-redesign-phase-4 branch May 20, 2026 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(e2e): V14 Tier-2 chromedp — topic ACL error envelope + WS keep-open (livetemplate#415) — DRAFT, Phase-5-resolved#327

test(e2e): V14 Tier-2 chromedp — topic ACL error envelope + WS keep-open (livetemplate#415) — DRAFT, Phase-5-resolved#327
adnaan wants to merge 5 commits into
mainfrom
broadcast-redesign-phase-4

adnaan commented May 19, 2026

Uh oh!

claude Bot commented May 19, 2026

Uh oh!

claude Bot commented May 19, 2026

Uh oh!

claude Bot commented May 19, 2026

Uh oh!

adnaan commented May 19, 2026

Uh oh!

claude Bot commented May 19, 2026

Uh oh!

adnaan commented May 19, 2026

Uh oh!

claude Bot commented May 19, 2026

Uh oh!

adnaan commented May 19, 2026

Uh oh!

adnaan commented May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

adnaan commented May 19, 2026

V14 (Tier-2 user-visible leg)

Three load-bearing harness decisions, all inline-commented

IsInitialMount guard — the e2e exposed a Mount-path dual-fire reality the Tier-1 test does not exercise

Cross-repo go.mod replace — the Phase-5-resolved dependency artifact

Gate (manual — pre-commit hook not installed in this checkout)

V14 status across the 3 coordinated PRs

Uh oh!

claude Bot commented May 19, 2026

Code Review — PR #327: V14 Tier-2 chromedp e2e (topic ACL error envelope + WS keep-open)

Overview

Issues

🔴 High — Hardcoded author-local absolute path committed to the repo

🟡 Medium — go.mod replace to a local filesystem path will corrupt go.sum for contributors

🟡 Medium — Missing server-side assertion for "connection kept open" log

🟢 Low — poll closure pattern vs. chromedp's built-in waits

🟢 Low — stopChan in ServerLogger is unused (pre-existing)

What's done well

Summary

Uh oh!

claude Bot commented May 19, 2026

Code Review — PR #327: V14 Tier-2 chromedp e2e (Topic ACL error envelope + WS keep-open)

Overview

What works well

Issues and suggestions

1. go.mod replace vs. Go Workspace (convention mismatch)

2. Missing go.sum update in the diff

3. poll() closure: consider extracting to a shared e2e helper

4. startV14ACLServer — log.Printf in background goroutine

5. serverLogger.Start() / Stop() vs. t.Cleanup

6. Minor: errCount assertion message could be more helpful

Security

Test coverage

Summary

Uh oh!

claude Bot commented May 19, 2026

Code Review — PR #327: V14 Tier-2 chromedp e2e (Draft, Phase 4)

Overview

Strengths

Issues / Suggestions

Test Coverage Assessment

Pre-Merge Checklist (Phase 5)

Bottom Line

Uh oh!

adnaan commented May 19, 2026

Uh oh!

claude Bot commented May 19, 2026

Code Review — PR #327: V14 Tier-2 chromedp e2e (DRAFT / Phase-4 tracking artifact)

Overview

What's Working Well

Issues & Suggestions

1. go.mod replace — the dominant risk of this PR

2. serverLogger.HasLog("connection kept open") — fragile string coupling

3. poll helper — time.Sleep busy-wait

4. io.MultiWriter(os.Stderr, serverLogger.Writer()) — noisy in test output

5. Missing t.Parallel() — worth a comment

6. Minor: Ping controller method uses blank identifier for context

Security

Summary

Uh oh!

adnaan commented May 19, 2026

Uh oh!

claude Bot commented May 19, 2026

Code Review — PR #327: V14 Tier-2 chromedp e2e (topic ACL error envelope + WS keep-open)

Overview

Strengths

Issues and Suggestions

1. Log-string coupling is the biggest correctness risk (acknowledged, but worth prioritizing)

2. go.mod replace directive is a contributor footgun

3. serveLocalPhase4ClientBundle reads the entire bundle into memory

4. Server goroutine error is surfaced indirectly

5. poll function uses time.Sleep without jitter

6. Minor: _, _ = w.Write(data) silently swallows write errors

Security

Performance

Test Coverage

Summary

Uh oh!

adnaan commented May 19, 2026

`IsInitialMount` guard — the e2e exposed a Mount-path dual-fire reality the Tier-1 test does not exercise

Cross-repo `go.mod` `replace` — the Phase-5-resolved dependency artifact

🟡 Medium — `go.mod replace` to a local filesystem path will corrupt `go.sum` for contributors

🟢 Low — `poll` closure pattern vs. chromedp's built-in waits

🟢 Low — `stopChan` in `ServerLogger` is unused (pre-existing)

1. `go.mod replace` vs. Go Workspace (convention mismatch)

2. Missing `go.sum` update in the diff

3. `poll()` closure: consider extracting to a shared e2e helper

4. `startV14ACLServer` — `log.Printf` in background goroutine

5. `serverLogger.Start()` / `Stop()` vs. `t.Cleanup`

6. Minor: `errCount` assertion message could be more helpful

1. `go.mod` `replace` — the dominant risk of this PR

2. `serverLogger.HasLog("connection kept open")` — fragile string coupling

3. `poll` helper — `time.Sleep` busy-wait

4. `io.MultiWriter(os.Stderr, serverLogger.Writer())` — noisy in test output

5. Missing `t.Parallel()` — worth a comment

6. Minor: `Ping` controller method uses blank identifier for context

2. `go.mod` replace directive is a contributor footgun

3. `serveLocalPhase4ClientBundle` reads the entire bundle into memory

5. `poll` function uses `time.Sleep` without jitter

6. Minor: `_, _ = w.Write(data)` silently swallows write errors