[kixelated]

Plans A peer's closed session reads as Closed in the bindings in the m2 api-shape epic. This is the follow-up from moq-dev/moq#8373, which guarantees Closed only for two endpoints sharing one in-process origin. A peer across a real session instead sees Protocol { kind: SessionClosed } on moq-lite, or Internal on moq-transport.

Decisions

Settled in /quest-complete on 2026-11-09 by the maintainer:

• ✅ Plan it in m2, because it changes the bindings' public errors (rejected: m1 as additive; dropping it and only documenting the difference).

Impact

Quests only. Wire: none.

Models

Size: S
Implementer: Claude Code, claude-opus-5-5, high effort

🤖 Generated with Claude Code

(Written by Claude Opus 5.5 with high effort)

Review

Rewritten by quest review after each round.

• Reviewer: Codex, gpt-6.1-sol, medium effort
• Session: 01a1c202-c3c6-7072-b6e3-46f89d347e5f, round 2
• Head: 32ea7dd
• Base: origin/main at 985e889
• Score: 10/10, The updated follow-up plan accurately describes the current error mappings and additional shutdown paths to investigate, with no actionable defects or runtime changes.
• Findings: none

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 32ea7dd

No actionable findings in this documentation-only change. The m2 binding-error follow-up is appropriately scoped: both wire decoders already preserve SESSION_CLOSED as Stream(Session(Internal)), and the plan explicitly includes the separate Error::Session teardown path rather than assuming a stream-conversion change covers every reader. It also accounts for cross-package parity and migration impact.

Verification: GitHub-only static review of both changed quest files and the referenced FFI conversion, wire decoders, session shutdown paths, C error surface, and Python socket tests. No local commands or runtime tests were run; actual peer-close behavior remains for the implementation to verify.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The API-shape README adds a requirement to map remote SESSION_CLOSED to MoqError::Closed. A new planning document lists investigations and updates related to session-closed errors. The changes are documentation only; they do not include implementation changes.

Merge Risk: Minimal · up to 32ea7

The PR records a proposed session-closure change without changing behavior. It is ready to merge after normal checks.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title clearly describes the planned change: map a peer's closed session to Closed in the bindings.
Description check		The description explains the planned binding error change, its context, and its scope.
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (2 skipped: 2 unsupported.)

Finishing Touches

Simplify code

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5bfc6d5f-6206-487f-a8ab-b356d8c2f6d1

Commits

Reviewing files that changed from the base of the PR and between 985e889 and 32ea7dd.

Files selected for processing: 2

• quest/m2/api-shape/README.md
• quest/m2/api-shape/session-closed.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

[pullfrog]

ℹ️ No critical issues — minor suggestions inline.

Reviewed changes

• New quest plan — quest/m2/api-shape/session-closed.md plans mapping a peer's SESSION_CLOSED to MoqError::Closed in moq-ffi, plus the comment, test, js/net, and moq-c follow-ups.
• Epic index — links the new child from quest/m2/api-shape/README.md.
• Decode correction — the second commit fixes the quest's moq-transport decode claim to match ietf/error.rs.

ℹ️ The PR body still says moq-transport yields Internal

Both wires decode SESSION_CLOSED to StreamError::Session(SessionError::Internal), which is what the quest now says. The body's opening paragraph still describes moq-transport as Internal, so the merged description contradicts the plan it introduces.

Technical details

# stale body claim

## Affected sites
- PR body, opening paragraph — "or `Internal` on moq-transport"
- `rs/moq-net/src/ietf/error.rs:90` — `SESSION_CLOSED => StreamError::Session(SessionError::Internal)`
- `rs/moq-ffi/src/error.rs:159` — maps any `Session(_)` to `MoqProtocolKind::SessionClosed`, carrying code `0x3`

## Required outcome
- The body describes the same mapping as `session-closed.md`.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

On current main, #8373 already removed this test's skip and changed its endpoint-close branch to pytest.raises(moq.Error.Closed), and added endpoint_close_resets_an_open_tcp_session_as_closed in rs/moq-ffi/src/socket.rs. Neither crosses a real session, so the gap stands, but this sentence describes a main that no longer exists. Please also name the test that will cover the peer path, since updating local_session_close_is_closed only checks the conversion.

Suggested change

	moq-transport does not turn it into `Internal`. No test on main checks this
	path: the one related test, `test_tcp_rst_and_endpoint_close_are_errors`
	(`py/moq-rs/tests/test_socket.py`), is skipped as flaky and covers endpoint
	close, not a peer.
	moq-transport does not turn it into `Internal`. No test on main crosses a real
	session: both endpoint-close tests (`test_tcp_rst_and_endpoint_close_are_errors`
	in `py/moq-rs/tests/test_socket.py`, and
	`endpoint_close_resets_an_open_tcp_session_as_closed` in
	`rs/moq-ffi/src/socket.rs`) cover only the shared in-process origin.

[pullfrog]

Broadening this arm to StreamError::Session(_) leaves MoqProtocolKind::SessionClosed with no producer: stream_kind is its only constructor, and every non-test from_stream caller then sees either a non-Session stream or is filtered to Internal first. The epic's rule is to delete a public item with no caller, so the plan should say whether that kind is removed and record it in the Impact.