test/unit/sse.test.ts (1)

120-137: Accessing private internals is acceptable here for regression testing.

This test verifies the core mechanism added by this PR: setting _writerIsClosed = true on write failure. While accessing private fields via (stream as any) is fragile and will break if internals are renamed, it's justified here since:

1. The PR objective specifically requires testing that _writerIsClosed is set on write error
2. There's no public API to observe this internal state
3. This is a regression test that "fails without change and fixes after" as requested by the reviewer

Consider adding a brief comment explaining why internal access is necessary:

📝 Suggested documentation

     it("marks writer as closed on write failure to prevent silent retries", async () => {
       const event = mockEvent("/");
       const stream = new EventStream(event);
 
-      // Access internals to verify state
+      // Access internals to verify state — required because _writerIsClosed is private
+      // and this regression test needs to confirm the fix for issue `#1322`
       const writeSpy = vi.fn().mockRejectedValue(new Error("write failed"));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/sse.test.ts` around lines 120 - 137, Add a short explanatory
comment above the lines that access private internals in the test to justify
using (stream as any) to inspect _writerIsClosed and _writer.write: explain this
is a regression test for EventStream.push that must verify the private flag
_writerIsClosed is set on write failure and there is no public API to observe
this state. Keep the test logic unchanged (references: EventStream, push,
_writer.write, _writerIsClosed, writeSpy) and ensure the comment is concise and
placed immediately before the internal access.

test/unit/sse.test.ts (1)

77-95: ⚠️ Potential issue | 🟠 Major

Make this test matrix-safe; raw process.on/off is runtime-fragile.

Line 83 and Line 93 rely on Node-only process APIs. This can fail in non-Node targets and bypasses the standard ctx.errors flow. Please move this case to the describeMatrix pattern and use runtime-aware skipping plus matrix error tracking. Also, ensure listener cleanup runs in finally.

#!/bin/bash
set -euo pipefail

echo "Check for runtime-fragile Node process hooks in this test:"
rg -n 'process\.(on|off)\("unhandledRejection"' test/unit/sse.test.ts

echo
echo "Check whether matrix test utilities are used in this file:"
rg -n 'describeMatrix|ctx\.errors|it\.skipIf\(' test/unit/sse.test.ts

As per coding guidelines: test/**/*.test.ts should use describeMatrix, track unhandled errors with ctx.errors, and use runtime-specific skips with it.skipIf(...).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/sse.test.ts` around lines 77 - 95, The test currently attaches
Node-only process handlers directly which is fragile; refactor the "EventStream
onClosed" test to use describeMatrix and the test matrix utilities: wrap the
case in describeMatrix, use it.skipIf(runtime !== "node") or equivalent to skip
non-Node runtimes, replace raw process.on/off with the matrix error tracker
(ctx.errors) to capture unhandled rejections, register the listener via
ctx.errors (or equivalent helper) when testing EventStream.onClosed and ensure
listener removal occurs in a finally block so cleanup always runs; locate
references to EventStream, stream.onClosed, and the test case in this file to
apply the changes.

test/unit/sse.test.ts (2)

8-8: Avoid barrel import from src/index.ts in this .ts test.

Please import mockEvent directly from its defining module instead of ../../src/index.ts.

As per coding guidelines: **/*.ts: “Do not use barrel files — import directly from specific modules”.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/sse.test.ts` at line 8, The test currently uses a barrel import of
mockEvent from ../../src/index.ts; change the import in test/unit/sse.test.ts to
import mockEvent directly from the module that defines it (the file where
mockEvent is declared) instead of the barrel file — update the import statement
to reference that specific module (the module that exports mockEvent) so the
test imports mockEvent directly from its defining source.

---

97-118: “Stops retrying” needs an observable assertion.

These tests currently validate “does not throw” but don’t directly prove no retry/write attempt occurred. Consider asserting write-call behavior (similar to Lines 120-166) to make the test intent explicit.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/unit/sse.test.ts` around lines 97 - 118, The tests should assert that no
write attempts are made after the stream is closed rather than only asserting no
throw; update both test cases to spy/mock the underlying writer write method and
verify it was not called after close. Specifically, locate the EventStream
instance and its internal writer (refer to EventStream, close, push,
pushComment) and replace or spyOn the writer.write (or the
WritableStreamDefaultWriter.write) with a jest.fn(), call await stream.close(),
then call stream.push/stream.pushComment and assert the mocked write was not
invoked (call count remains 0) for both messages to prove no retry/write
occurred.