Skip to content

refactor(sso): serve inter-host requests through typed handlers - #628

Draft
pgherveou wants to merge 8 commits into
sso-rpc-macrosfrom
sso-rpc-service
Draft

refactor(sso): serve inter-host requests through typed handlers#628
pgherveou wants to merge 8 commits into
sso-rpc-macrosfrom
sso-rpc-service

Conversation

@pgherveou

@pgherveou pgherveou commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Use typed SSO handlers with generated dispatch and shared canonical payloads, removing duplicate request/response structs. Resource consent stays bound to the signing session.

SSO encoding remains compatible. Product–host signing now uses OptionBool; clients and hosts must update together.

Stack 2/2; depends on #651. Further encoding simplification: #654.

Validation: Rust and TypeScript tests, workspace Clippy, WASM build, and Swift binding generation passed.

@pgherveou

Copy link
Copy Markdown
Collaborator Author

@lore-bot-app review

@lore-bot-app

lore-bot-app Bot commented Sep 8, 2026

Copy link
Copy Markdown

Reading the diff and checking what the record says. Back in a few minutes.

@lore-bot-app lore-bot-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

TL;DR
Replaces the hand-matched SSO request/response plumbing with SsoWire/SsoResponse derives and an #[sso_service]-generated dispatcher, and binds resource consent to the request's signing session. 0 blocking, 2 minor concerns, 3 open questions.

Summary
v1::RemoteMessage becomes the single wire spec: the derive pairs each *Request variant with its response, emits classify()/name()/responding_to(), and the trait macro generates dispatch() so a missing handler is a compile error. The pairing host sends through one generic call(request) whose reply matcher fails fast on a mistyped reply instead of waiting for the timeout. Message types are renamed to TrUAPI method stems (wire indices unchanged, host-papp 0.8.11 fixtures still pinned). On the signing host, serve_resource_allocation re-checks require_current_session around every allocation step so consent given before an account switch or reactivation cannot release keys, with three tests covering swap, same-account reactivation, and disconnect.

What the record says

  • This is PR #628. Lore's summary notes the battery run passed but "some legacy account failures persist due to design constraints rather than the refactor". The diff does not say which.
  • PR #618 (merged 2026-09-07) introduced binding signing and paired SSO work to the wallet generation via require_current_session. This PR extends the same mechanism to resource consent, consistent with that decision.
  • No recorded discussion covers transcript request-name stability, the placement of server-only derives in truapi-macros, or the item-failure detail in allocation transcripts. Searches on all three returned nothing relevant.

Concerns

  • Allocation transcripts lose the failure cause. rust/crates/truapi-server/src/runtime/signing_host/sso_service.rs:238 now only warn!s the per-item error and maps it to NotAvailable. The old allocation_result_with_failures appended those reasons to the transcript reason; resource_allocation_outcome at rust/crates/truapi-server/src/host_logic/sso/messages.rs:547 has no input for them. The CLI renders that reason as the activity detail (rust/crates/truapi-host-cli/src/terminal_ui.rs:2695) and its test at terminal_ui.rs:4253 expects text like "rejected: timed out waiting for Bulletin authorization". After this PR a user sees only "Requested resource is not available". The CLI test still passes because it is fed a synthetic event, so nothing catches the drop.
  • README overstates the naming. README.md:112 says transcripts use "the service method names shared with the typed client". sign, resource_allocation, and product_subtree have no same-named TrUAPI method (rust/crates/truapi/src/api/signing.rs:230 and :267 are sign_raw/sign_payload). Either tighten the sentence or rename those three stems.

Questions for the author

  1. Cargo.lock changed (272 chars, not shown to me). Is that only truapi-macros added to truapi-server's dependency list, with no version bumps?
  2. truapi-macros is a dependency of the canonical truapi crate (rust/crates/truapi/Cargo.toml:14), and the new derives hardcode crate::host_logic::sso::wire paths (rust/crates/truapi-macros/src/sso.rs:17). Was a truapi-server-local proc-macro crate considered, so the protocol crate does not carry runtime-only macro code?
  3. Which legacy-account battery failures remain after this change, and are they tracked anywhere?

🤖 Reviewed by Lore (Parity knowledge base) · 34 agent turns · 356.0s · knowledge as of 2026-09-08

Comment thread rust/crates/truapi-server/src/runtime/signing_host/sso_service.rs Outdated
Comment thread README.md Outdated
@pgherveou

Copy link
Copy Markdown
Collaborator Author

@lore-bot-app review

@lore-bot-app

lore-bot-app Bot commented Sep 8, 2026

Copy link
Copy Markdown

Reading the diff and checking what the record says. Back in a few minutes.

@lore-bot-app lore-bot-app Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since last review

  • Resolved: allocation transcripts keep the per-item failure cause again. serve_resource_allocation collects them and SsoReply::finish appends them to the outcome reason (rust/crates/truapi-server/src/runtime/sso_service.rs:95), covered by allocation_failure_details_reach_the_response_transcript.
  • Resolved: the README sentence now says "typed SSO client", which is accurate.
  • Answered: Cargo.lock only adds truapi-macros to truapi-server's dependency list. No version bumps.
  • Still open: macro placement in truapi-macros with hardcoded crate::runtime::... paths, and which legacy-account battery failures remain. The record has nothing on either.
  • New in this revision: #[sso_service] is also applied to the signing-host impl and wraps bodies into SsoReply; SsoRequestContext carries the call context and the session resolved before the handler runs; the five *AuthorityRequest structs are gone and ProductAuthority now takes the SSO message request types directly; three new waiter tests pin batch-order semantics.

TL;DR
Typed SSO RPC layer: derives replace hand-matched pairing, dispatch, correlation and transcript classification, and consent is bound to the request's session. 0 blocking, 5 minor.

Summary
v1::RemoteMessage stays the hand-written wire spec (indices unchanged, host-papp 0.8.11 fixtures still pinned). SsoWire generates classification and correlation helpers from it, SsoResponse reads each response's Result payload, and #[sso_service] on the trait derives the request-to-response pairing and an exhaustive dispatch() from method signatures. On the impl it wraps ordinary Result bodies into SsoReply, so handlers keep ?. The pairing host sends everything through one generic call, whose matcher fails immediately on a reply of the wrong kind instead of waiting for the timeout. Resource consent re-validates the captured session around every allocation step, with tests for account swap, same-account reactivation, and disconnect.

What the record says

  • PR #618 (merged) bound signing, SSO and allowance work to the wallet generation via require_current_session. This PR extends that to SSO resource consent. The same-account reactivation test only works because local_session_validation_id mixes in activation_generation (signing_host.rs:1144).
  • PR #584 (merged) made peer-initiated disconnect cleanup local and sent exactly one Disconnected. The new batch semantics fit: a waiter that matches a reply earlier in the same batch does not see a later Disconnected, but the separate disconnect monitor still does.
  • Lore's summary of PR #628 records 1,285 passing tests and host-papp/Android fixture coverage. It no longer mentions the legacy-account battery failures the earlier revision reported.
  • Nothing in the record covers transcript request-name stability, the action span field, or where server-only derives should live. Owner by expertise: pgherveou.

Concerns

  1. Pairing-host tracing loses signing granularity. rust/crates/truapi-server/src/runtime/pairing_host/sso_channel.rs:148 sets action = R::NAME, so sign_payload, sign_raw, and legacy-account sign_payload all log as sign, and the six AuthorityRequestKind labels are gone. No in-repo consumer reads the field, so this is a debuggability regression, not a break.
  2. The pure layer now depends on the runtime layer. The generated v1::RemoteMessage::name() calls <Payload as SsoRequest>::NAME, and those impls are emitted by #[sso_service] in runtime/sso_service.rs. rust/crates/truapi-server/README.md:259 still says host_logic stays pure, and line 261 says pairing is "derived from the wire enum" when it is derived from the trait signatures. Fix the text, or emit NAME from the wire derive.
  3. Top-level README carries macro internals. README.md:111 to :122 add eight sentences about SsoReply, dispatch and correlation to the wallet-host paragraph, duplicating the crate README and CLAUDE.md. The consent-binding sentence belongs there; the rest does not.
  4. Misattached doc comment. rust/crates/truapi-server/src/test_support.rs:265 now describes sso_host_and_responder_sessions with the sentence that belonged to sso_session_info (:316), which is left undocumented.
  5. Stale design doc. docs/rfcs/0024-personhood-as-product.md:332 still names RingVrfProofRequest, RingVrfAliasRequest and RingVrfSignRequest. CLAUDE.md asks for design docs to follow renames.

Questions for the author

  1. truapi-macros is a dependency of the canonical truapi crate, yet sso.rs hardcodes crate::host_logic::sso and crate::runtime::sso_service paths. Was a truapi-server-local proc-macro crate considered?
  2. host_logic is pub at truapi-server/src/lib.rs:30, so the removed builders, SsoRemoteResponse, and the renamed message types are public API changes. Does anything outside this workspace import them?
  3. Which legacy-account battery failures remain, and are they tracked?

🤖 Reviewed by Lore (Parity knowledge base) · 29 agent turns · 435.8s · knowledge as of 2026-09-08 · re-review

Comment thread rust/crates/truapi-server/src/runtime/pairing_host/sso_channel.rs
Comment thread README.md Outdated
Comment thread rust/crates/truapi-server/src/test_support.rs Outdated
@pgherveou pgherveou changed the title refactor(sso): dispatch inter-host messages through a typed SsoService refactor(sso): serve inter-host requests through typed handlers Sep 8, 2026
@pgherveou
pgherveou changed the base branch from main to sso-rpc-macros September 8, 2026 20:58
@pgherveou
pgherveou force-pushed the sso-rpc-service branch 2 times, most recently from 069bea2 to 2f515d1 Compare September 9, 2026 04:16
@pgherveou
pgherveou force-pushed the sso-rpc-service branch 2 times, most recently from 13a2742 to 261a771 Compare September 9, 2026 09:16
@pgherveou

Copy link
Copy Markdown
Collaborator Author

@lore-bot-app review

@lore-bot-app

lore-bot-app Bot commented Sep 9, 2026

Copy link
Copy Markdown

Reading the diff and checking what the record says. Back in a few minutes.

@lore-bot-app

lore-bot-app Bot commented Sep 9, 2026

Copy link
Copy Markdown

Lore couldn't complete this review: success (after 45 turns)

Nothing was changed. Try @lore-bot review again, or ping Dhruv.

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