Skip to content

Commit 4ca0773

Browse files
adnaanclaude
andcommitted
fix: address PR #327 round-4 review (doc comments only)
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>
1 parent a9e2a54 commit 4ca0773

1 file changed

Lines changed: 15 additions & 2 deletions

File tree

e2e/topic_acl_error_envelope_v14_test.go

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,12 @@ func startV14ACLServer(t *testing.T) (port int, shutdown func()) {
178178
}
179179

180180
func TestE2E_V14_TopicACLDeniedEmitsLvtErrorAndKeepsWSOpen(t *testing.T) {
181+
// Not parallel: this test mutates the process-global slog.Default. Other
182+
// tests in this package that also touch slog.Default (or that observe
183+
// livetemplate's internal logging via the same global) would race if
184+
// this ran via t.Parallel(). Sibling browser e2es (e.g.
185+
// lifecycle_ergonomics_test.go) follow the same non-parallel convention.
186+
181187
// Artifact 2/4 — server logs. livetemplate logs via the global slog
182188
// default (incl. the new "Mount Subscribe denied … connection kept open"
183189
// WARN). Tee into a captured buffer; restore the default in Cleanup
@@ -265,8 +271,15 @@ func TestE2E_V14_TopicACLDeniedEmitsLvtErrorAndKeepsWSOpen(t *testing.T) {
265271
// not just that the client happened to stay connected. The server-side
266272
// WARN log is the load-bearing signal that mount.go's *TopicForbiddenError
267273
// branch (Option B fall-through) ran instead of the pre-Phase-4 return.
268-
// Guards against silent regressions where the log message changes or the
269-
// WARN is removed without the corresponding behavior change.
274+
// Guards against silent regressions where the WARN is removed without the
275+
// corresponding behavior change.
276+
//
277+
// Coupling: substring of the WARN message in livetemplate's
278+
// mount.go (grep anchor: `slog.Warn("Mount Subscribe denied by topic ACL`).
279+
// If that prose is reworded, update both here and the
280+
// livetemplate-side log call together. A future hardening would be to
281+
// switch the server WARN to a structured `slog.String("event", "<key>")`
282+
// and assert on the structured key — tracked for Phase 5/6.
270283
if !serverLogger.HasLog("connection kept open") {
271284
dump()
272285
t.Fatal("expected server to log the Phase-4 keep-open WARN " +

0 commit comments

Comments
 (0)