Skip to content

feat!: extract sequence contract - #1753

Draft
Benoît Cortier (CBenoit) wants to merge 3 commits into
cbenoit-introduce-sequenceerror-sspi-free-sequenfrom
cbenoit-extract-ironrdp-sequence-crate
Draft

feat!: extract sequence contract#1753
Benoît Cortier (CBenoit) wants to merge 3 commits into
cbenoit-introduce-sequenceerror-sspi-free-sequenfrom
cbenoit-extract-ironrdp-sequence-crate

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Aug 21, 2026

Copy link
Copy Markdown
Member

Stacked on #1751 (base branch: cbenoit-introduce-sequenceerror-sspi-free-sequen). This targets that branch, not master; it should merge after #1751.

Implements #1426. GitHub only auto-closes an issue when the closing PR lands on the repository's default branch, so Closes #1426 below will only take effect once this stack (starting with #1751) reaches master — not on merge of this PR alone.

What

Creates ironrdp-sequence, a new sans-I/O core-tier crate that owns the Sequence trait and its supporting contract types previously defined in ironrdp-connector: State, Written, SequenceError/SequenceErrorKind/SequenceResult, the general_err!/reason_err!/custom_err! macros, ServerName, and DesktopSize.

Also moves MonotonicInstant out of ironrdp-core into ironrdp-sequence, alongside Sequence. Per the maintainer audit on #1530, the clock read by a sequence's step is part of the sans-I/O contract, not a foundational encoding primitive, so it no longer belongs in ironrdp-core.

Design decisions

  • Feature gating: MonotonicInstant and DesktopSize need no allocation and stay available unconditionally (no features required). Everything else needs ironrdp-pdu (for PduHint and negotiation failure codes), so it is gated behind a default-disabled state-machine feature. This keeps ironrdp-rdpeudp — which depends on this crate only for MonotonicInstant — from acquiring ironrdp-pdu's much larger dependency tree (der-parser, x509-cert, pkcs1, ...) just for the clock type. A new xtask check dependencies forbidden-edge entry (ironrdp-rdpeudpironrdp-pdu) enforces this permanently.
  • alloc vs state-machine: ServerName only needs alloc::string::String, so it's gated on plain alloc rather than the heavier state-machine.
  • Facades preserved: ironrdp-connector and ironrdp-rdpeudp both keep permanent re-exports of everything they previously defined directly, so existing ironrdp_connector::{Sequence, MonotonicInstant, State, Written, SequenceError, ServerName, DesktopSize, ...} and ironrdp_rdpeudp::MonotonicInstant import paths are unaffected.
  • Publishable from the start: unlike a typical new internal crate, ironrdp-sequence is version = "0.1.0" with no publish = false, because published crates (ironrdp-connector) depend on it — cargo publish requires all path dependencies to be resolvable from the registry.
  • Scope discipline: ironrdp-sequence has no dependency on ironrdp-connector, sspi, or any CredSSP/RDP connection-flow logic — verified via cargo tree and the new xtask check dependencies guard.

Breaking change

ironrdp_core::MonotonicInstant is removed. This breaks the path as it exists on master/unreleased ironrdp-core; the type was never part of a published ironrdp-core release (it does not exist in ironrdp-core v0.2.1 on crates.io), so no published consumer is affected. Direct consumers on master must switch to ironrdp_sequence::MonotonicInstant, or use the stable facade re-exports at ironrdp_connector::MonotonicInstant / ironrdp_rdpeudp::MonotonicInstant (unaffected by this change).

Explicitly out of scope

This PR is strictly the extraction layer. It does not touch the FFI timestamp/API surface (e.g. how the C#/native FFI layer supplies MonotonicInstant readings) — that is a separate follow-up.

Checks

  • cargo check --workspace (default features): clean.
  • cargo xtask check fmt -v, check lints -v, check tests -v, check locks -v: all pass.
  • cargo xtask check features --case workspace/powerset-foundation (now includes ironrdp-sequence): 29/29 sub-checks pass.
  • cargo xtask check dependencies: all forbidden edges (including the new ironrdp-rdpeudpironrdp-pdu one) confirmed absent.
  • cargo tree -p ironrdp-rdpeudp -e no-dev -i ironrdp-pdu: confirms ironrdp-pdu is absent from ironrdp-rdpeudp's dependency tree.
  • cargo doc -p ironrdp-sequence --no-deps --all-features: clean, no broken intra-doc links.
  • cargo test -p ironrdp-sequence --no-default-features --features state-machine and --features state-machine,std: reason_err!'s macro-hygiene regression test passes in both configurations.
  • Pre-existing, unrelated --all-features issues in ironrdp-tls/ironrdp-rdpeusb (mutually-exclusive TLS backend features) reproduce identically on the base branch and are out of scope here.

Create ironrdp-sequence, a new sans-I/O core-tier crate that owns the
`Sequence` trait and its supporting contract types previously defined
in ironrdp-connector: `State`, `Written`, `SequenceError`/
`SequenceErrorKind`/`SequenceResult`, the `general_err!`/`reason_err!`/
`custom_err!` macros, `ServerName`, and `DesktopSize`.

Also move `MonotonicInstant` out of ironrdp-core into ironrdp-sequence,
alongside `Sequence`. Per the PR #1530 audit, the clock read by a
sequence's `step` is part of the sans-I/O contract, not a foundational
encoding primitive, so it no longer belongs in ironrdp-core.

`MonotonicInstant` and `DesktopSize` need no allocation and stay
available unconditionally. Everything else needs `ironrdp-pdu`, so it
is gated behind a default-disabled `state-machine` feature: this keeps
ironrdp-rdpeudp, which depends on this crate only for
`MonotonicInstant`, from acquiring `ironrdp-pdu`'s much larger
dependency tree. A new xtask dependency-edge check enforces this.

ironrdp-connector and ironrdp-rdpeudp both keep permanent facade
re-exports of everything they used to define directly, so existing
`ironrdp_connector::{Sequence, MonotonicInstant, ...}` and
`ironrdp_rdpeudp::MonotonicInstant` paths are unaffected.

Stacked on #1751 (branch cbenoit-introduce-sequenceerror-sspi-free-sequen).
Closes #1426 once this stack lands on master.

BREAKING CHANGE: `ironrdp_core::MonotonicInstant` is removed. Use
`ironrdp_sequence::MonotonicInstant`, or the stable facade re-exports
at `ironrdp_connector::MonotonicInstant` / `ironrdp_rdpeudp::MonotonicInstant`.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny scope/core Touches the core architectural tier scope/cross-cutting Spans multiple architectural boundaries scope/tooling Build, CI, release, or developer tooling size/XL Size: up to 1299 counted lines and 49 files; exceeds L in either measure labels Aug 21, 2026
@CBenoit Benoît Cortier (CBenoit) changed the title feat(sequence)!: extract ironrdp-sequence crate feat!: extract sequence contract Aug 21, 2026
@CBenoit Benoît Cortier (CBenoit) changed the title feat!: extract sequence contract feat(sequence)!: extract ironrdp-sequence crate Aug 21, 2026
Enable `all-features` under `[package.metadata.docs.rs]` so docs.rs
renders `ironrdp-sequence` with `state-machine`/`alloc` on instead of
the default-disabled feature set, which left 12 of 14 public items
undocumented and produced 17 broken intra-doc links.

Restore the `$crate::__private::format!` indirection in `reason_err!`
that a prior self-review mistakenly reverted to a bare `format!` call.
Macro invocations inside a `macro_rules!` body resolve through the
caller's scope, not the definition site, so a no_std consumer without
`format!` imported could not compile this macro with the bare call.
Add `tests/reason_err.rs`, a `#![no_implicit_prelude]` regression test
that reproduces that caller shape and fails to compile without the
fix.

Register `sequence` as a canonical PR title scope in `xtask/src/pr.rs`
(and the `commit-scope` skill), following the same pattern used for
`rdpei` in bbef8d9, and restore the PR title to use it instead of the
scope-less stopgap title.

Condense the `ironrdp-sequence` ARCHITECTURE.md entry to sentence-per-
line with the `**Architectural Invariant**:` prefix used elsewhere in
the file, trim the redundant feature-split rationale restated in
`time.rs`'s doc comment, and correct the README's claim that this
extraction already stops `ironrdp-acceptor` from depending on
`ironrdp-connector`: it still does, and that rewiring is a follow-up.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit Benoît Cortier (CBenoit) changed the title feat(sequence)!: extract ironrdp-sequence crate feat!: extract sequence contract Aug 21, 2026
@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries and removed kind/protocol Affects RDP or related protocol behavior labels Aug 21, 2026
fuzz/Cargo.lock never picked up the new ironrdp-sequence crate
introduced by 5cfbfe7, so cargo xtask check locks -v only caught the
drift once CI ran a fuzz-workspace command against it: the check
itself only diffs the lock files after some other invocation touches
them, it does not proactively validate each workspace.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added kind/protocol Affects RDP or related protocol behavior ai-reviewed/1 One automated review completed and removed maintainer-required Maintainer review or intervention is required labels Aug 21, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mechanical extraction of the Sequence contract (State, Written, SequenceError, ServerName, DesktopSize, NegotiationFailure, MonotonicInstant, macros) from ironrdp-connector/ironrdp-core into a new ironrdp-sequence crate, with permanent facade re-exports preserving all existing import paths. Types and logic are moved verbatim; feature gating (state-machine/alloc/std) is coherent and matches the stated dependency-avoidance goal, verified by a new forbidden-edge check and cargo-tree evidence in the PR body. The move also surfaces and fixes a real macro-hygiene bug (reason_err!'s bare format! call would break for no_std callers of the relocated macro), backed by a regression test. No wire-format, encode/decode, or state-transition behavior changes.

Protocol analysis: accepted — Independent diff review confirms the handoff's assessment: every moved item (SequenceError/Kind, ServerName's sanitize_server_name, NegotiationFailure's FailureCode match, State/Written, MonotonicInstant's arithmetic, the three macros) is byte-for-byte identical to its pre-move definition; only module paths, feature-gate cfg attributes, and doc comments changed. No PDU field, encode/decode path, or state machine transition is touched.

Comment thread ARCHITECTURE.md

`ironrdp-connector` and `ironrdp-rdpeudp` both re-export everything they previously defined directly from this crate, so existing `ironrdp_connector::{Sequence, MonotonicInstant, ...}` and `ironrdp_rdpeudp::MonotonicInstant` import paths remain stable.

**Architectural Invariant**: this crate must never depend on `ironrdp-connector`, `sspi`, or any RDP connection-flow logic; it is the generic contract that `ironrdp-connector` (and any future accept-side or session-side sequence) is built on top of, not the other way around.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

non_blocking / low: ironrdp-sequence is placed in the Core Tier, which carries a 'must be fuzzed' architectural invariant, but ServerName::sanitize_server_name (which parses untrusted-shaped socket-address/hostname strings) has no fuzz target, and none is added by this PR. This is not a regression introduced here (the code was equally unfuzzed in ironrdp-connector), so it doesn't block this pure-extraction PR, but it's now a Core Tier crate carrying an unmet invariant worth a follow-up.

@CBenoit
Benoît Cortier (CBenoit) marked this pull request as draft August 21, 2026 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/1 One automated review completed breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny scope/core Touches the core architectural tier scope/cross-cutting Spans multiple architectural boundaries scope/tooling Build, CI, release, or developer tooling size/XL Size: up to 1299 counted lines and 49 files; exceeds L in either measure

Development

Successfully merging this pull request may close these issues.

1 participant