Skip to content

quest: plan ffi-shape request accept and upgrade-page renames - #4701

Merged
kixelated merged 3 commits into
quest/m1/ffi-shape/READMEfrom
quest/plan-ffi-request-accept
Oct 2, 2026
Merged

kixelated merged 3 commits into
quest/m1/ffi-shape/READMEfrom
quest/plan-ffi-request-accept

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Follow-ups from #4697 (ffi-shape/net).

Decisions

Follow-ups from #4697 to plan

  • ✅ MoqRequest origins as accept() arguments
  • ✅ The upgrade page lists these renames
  • Start ffi-shape/media next

Where they go

  • ✅ A new XS quest, plus a plan line in the README
  • Both into the README's own work

Note: quest check fails on this line branch with errors that were already there before this PR (outside-condition Required entries elsewhere in the tree); this PR adds none.

Public API: none (planning). Wire: none.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 2 commits October 1, 2026 20:07
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…accept

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

MERGE — quest/docs planning only; claims check out against the tree and #4697.

Reviewed head: 7d2404f9aeed1feece1b26a49c485427a373cbb4

Findings

No blocking issues.

Non-blocking

  1. request-accept.md “Mirror … moq-net” — Rust’s shape is builder + complete, not accept-with-args: moq_tokio::server::Request / moq_net::server::Handshake use with_publisher / with_subscriber then ok(). FFI’s proposed accept(publish?, consume?) (or a small record) is the right UniFFI translation; worth saying that explicitly so the implementer does not hunt for an accept-with-args API in Rust.

  2. Server config defaults stay — After refactor(ffi)!: root namespace matches moq-net (config records, consume, demand) #4697, MoqServerConfig still carries publish / consume; only MoqRequest::set_publish / set_consume remain as root setters (confirmed on 4697 head 172d929ea6cc). One sentence that those config defaults remain (and are what Server.accept seeds into the request) would stop a reader from thinking origins move only to Request.accept.

Claims check

  • #4697 did remove client/server root setters in favor of config records and left the two MoqRequest setters — accurate on 4697’s head.
  • Upgrade-page rename list matches refactor(ffi)!: root namespace matches moq-net (config records, consume, demand) #4697 / net.md (subscribe→consume, Go All(ctx) + ConnectionStatus*, Kotlin/Dart announced().updates(), config records).
  • Links to net.md / media.md / codec.md resolve on the line base; new request-accept.md is wired into Required after Net.
  • Public API / wire notes are correct for a planning quest.

CI: Test passed; Check still pending (docs-only, not a concern).

Verdict

MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: ca44fe4

No actionable new bugs found in this planning-only change. Passing origins when accepting a request is a simpler FFI shape than separately mutating the request, and the revised explanation now accurately distinguishes Rust's consuming builders from the shared FFI handle (quest/m1/ffi-shape/request-accept.md:11–17). The upgrade-page additions match the changes in #4697.

One non-blocking clarification, independently confirming the existing review: state how optional accept arguments interact with the server's configured origins. #4697 still seeds each request from MoqServerConfig.publish/consume; the current setters can replace either with None. Preserve or explicitly decide omission/null/default behavior when implementing this quest, with coverage for inherited, overridden, and cleared origins. This is a planning clarification, not a demonstrated regression.

Verification: inspected both changed documents, the relevant Rust request/handshake code, and #4697 at 172d929. No local builds, tests, or quest checks run; the required tooling is unavailable. Check CI for the reviewed head was still running.

(Written by review (OpenAI))

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

MERGE — quest/docs planning only; ca44fe44 correction matches moq-net, and claims check out against #4697.

Reviewed head: ca44fe44b6c9cbed7eda4641e7401c5d5bb6a3ca

Findings

No blocking issues.

Non-blocking

  1. MoqServerConfig defaults stay — After refactor(ffi)!: root namespace matches moq-net (config records, consume, demand) #4697, MoqServerConfig still carries publish / consume; Server.accept seeds those into MoqRequest, and only MoqRequest::set_publish / set_consume remain as root setters (confirmed on 4697 head 172d929ea6cc). One sentence that those config defaults remain (and are what get overridden at Request.accept) would stop a reader from thinking origins move only onto accept().

What changed since 7d2404f9

  • Fixed: earlier NB that request-accept.md said “mirror moq-net accept” — now correctly describes with_publisher / with_subscriber then ok(), and why a shared FFI Arc handle cannot use that shape.
  • Still open: soft NB that MoqServerConfig publish/consume defaults remain and seed the request.

Claims check

  • Second commit correctly replaces “mirror moq-net accept” with with_publisher / with_subscriber (consuming builders) then ok(), and explains why a shared FFI Arc handle cannot use that shape — matches rs/moq-net / rs/moq-tokio and current FFI (set_* then arg-less accept() which itself calls the builders).
  • #4697 removed client/server root setters in favor of config records and left the two MoqRequest setters — accurate; refactor(ffi)!: root namespace matches moq-net (config records, consume, demand) #4697 follow-ups already name this accept-args idea.
  • Upgrade-page rename list matches refactor(ffi)!: root namespace matches moq-net (config records, consume, demand) #4697 / net.md (subscribe→consume, Go All(ctx) + ConnectionStatus*, Kotlin/Dart announced().updates(), config records).
  • Links to net.md / media.md / codec.md resolve on the line base; new request-accept.md is wired into Required after Net.
  • Public API / wire notes are correct for a planning quest.

CI: Test passed; Check still pending (docs-only, not a concern).

Verdict

MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated

Copy link
Copy Markdown
Collaborator Author

MERGE — quest/docs follow-up; 22b24c75 closes the open NB from ca44fe44.

Reviewed head: 22b24c7584fc5e760179765f072f2eb85d54b8a4

Findings

No blocking issues. No remaining non-blocking issues.

What changed since ca44fe44

  • Fixed: prior NB that request-accept.md should say MoqServerConfig.publish/consume stay as server defaults seeding each request (omitted accept args inherit them). Also notes today's set_publish(None) clear-inherited case and calls out testing inherited / overridden / (if kept) cleared origins — good for the eventual implementation quest.
  • Still open: none from earlier Grok reviews on this PR.

Claims check

CI: Test passed; Check still pending (docs-only, not a concern).

Verdict

MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed delta: ca44fe4 → 22b24c7.

The previous clarification is addressed in quest/m1/ffi-shape/request-accept.md:18–22: server defaults remain, omission inherits them, and explicit clearing must be deliberately retained or removed with matching coverage. The remaining clearing choice is correctly left to implementation on the breaking-change branch.

No new actionable bugs found. The direction remains sound and keeps the API decision explicit without prescribing unnecessary machinery.

Verification: static review of the sole changed document and its surrounding plan. No builds, tests, or quest checks run.

(Written by review (OpenAI))

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merging. Adds ffi-shape/request-accept [XS] (MoqRequest origins become accept() arguments, removing the last root setters) and lists #4697's renames in the README's upgrade-page work. Review fixes: the plan now says moq-net uses consuming with_publisher/with_subscriber builders before ok() (so there is no accept-with-args shape to mirror), and that MoqServerConfig origins stay as defaults an omitted argument inherits. quest check failures on this line base predate this PR and are in unrelated quests.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit 058d854 into quest/m1/ffi-shape/README Oct 2, 2026
3 checks passed
@kixelated
kixelated deleted the quest/plan-ffi-request-accept branch October 2, 2026 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant