fix(server): carry the tester's logical address into ClientConnectionInfo by JustinKovacich · Pull Request #12 · luminartech/simple_doip

JustinKovacich · 2026-09-09T17:52:39Z

Stacked on #11 — merge order: #11 → this. --base is
build/license-and-release-tooling, so the diff shown here is only this PR's
two commits.

What

ClientConnectionInfo::logical_address was the literal LogicalAddress(0x0000)
for every connection. A ServerConnectionHandler could not tell which tester
was asking, and the default alive_check answered every alive check with source
address 0x0000 no matter who sent it.

It now carries the address the tester activated routing with.

Why this scope

The address is held per connection, which is where routing activation
applies — it is a property of the socket it arrived on, not of the server.

It is recorded only when the handler accepts the activation. A denied tester
is not activated, so attributing its claimed address to the connection would
report an identity the entity refused.

0x0000 stays the value before activation, and it is an unambiguous sentinel
rather than a placeholder: ISO 13400-2 reserves everything below
MIN_CLIENT_ADDRESS (0x0E00), so it can never be a tester's own address. The
UDP identification path has no connection and no activation to learn from, so it
still reports 0x0000 — now for a documented reason rather than by accident.

How it was tested

Two integration tests drive it over a real socket and assert on the alive check
response the default handler builds from the field, so they cover the path a
consumer actually gets:

alive_check_reports_the_activated_tester_logical_address
a_denied_activation_does_not_record_the_testers_address

The first is a real regression test — reverting the one line to the old literal
makes it fail with left: 0x0000, right: 0x0E01, and pass again when restored.

cargo test --features client,server → 20 integration tests pass (was 18).
Both of CI's clippy invocations are clean, as are cargo fmt -- --check and
cargo doc with RUSTDOCFLAGS=-D warnings.

Naked TODOs cleared

Three sat on or beside this code and none pointed at work anyone was going to
do:

Server::new — "validate the provided handler". There is nothing to validate
about a trait object.
run_server — an unsolicited-vehicle-announcement note that repeated what
the method's own doc comment says two lines above it.
BUFFER_SIZE — "move this to a config file". Connector is already the seam
for a caller that needs different socket options, so the constant is
deliberately fixed; that is now what the doc comment says.

The open question on RoutingActivationRequest::encode (whether the optional
vehicle-manufacturer field should ever be written when absent) moved from a bare
TODO to ARCHITECTURE.md §7.6, where the rest of the deferred work lives, with
the concrete reason no golden vector settles it.

That leaves src/ with no naked TODOs before the crate is published.

Docs

The README's scope list, ARCHITECTURE.md §7.6 and the changelog all described
the field as untracked, so all three are updated in the second commit.

Review status

Not reviewed by anyone yet. Draft.

…Info `ClientConnectionInfo::logical_address` was the literal `LogicalAddress(0x0000)` for every connection, so a handler could not tell which tester was asking, and the default `alive_check` answered every alive check with source address `0x0000` regardless of who sent it. The address is now learned from the routing activation the connection carries and held per-connection, which is the right scope: routing activation applies to the socket it arrived on. It is recorded only when the handler accepts the activation -- attributing a denied tester's claimed address to the connection would report an identity the entity refused. `0x0000` remains the value before activation, and it is an unambiguous sentinel rather than a placeholder: ISO 13400-2 reserves everything below `MIN_CLIENT_ADDRESS` (`0x0E00`), so it can never be a tester's own address. The UDP identification path has no connection and no activation to learn from, so it still reports `0x0000` -- now for a documented reason. Two integration tests cover it through the wire, asserting on the alive check response the default handler builds from the field. The first fails with `0x0000` against `0x0E01` on the previous code. Also clears the three naked TODOs this touched or sat beside: - `Server::new`'s "validate the provided handler" -- there is nothing to validate about a trait object, and no validation was ever going to be added. - `run_server`'s unsolicited-vehicle-announcement note, which repeated what the method's own doc comment says two lines above it. - `BUFFER_SIZE`'s "move this to a config file" -- `Connector` is already the seam for a caller that needs different socket options, so the constant is deliberately fixed. Documented as such. The open question on `RoutingActivationRequest::encode` moves from a bare TODO to ARCHITECTURE.md, where the rest of the deferred work lives, with the concrete reason no golden vector settles it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The README's scope list, ARCHITECTURE.md section 7.6 and the changelog all described `ClientConnectionInfo::logical_address` as untracked. It is tracked as of the previous commit, so they described the crate as it no longer is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@gavin-dunlap-luminar

…mation (#11) ## What The two things standing between `simple_doip` and a first crates.io release: the license texts, and a way to cut a release. ## Why The crate turned out to be much closer to publishable than the Phase A notes suggested. Checked against the live repo: - `Cargo.toml` on `main` already carries the **full publish metadata** — `description`, `license`, `repository`, `readme`, `keywords`, `categories`, `exclude`. No `publish = false`. - CI already runs **`cargo publish --dry-run`** in the `package` job, so publishability is verified on every run. - The only first-party dependency, **`automotive-wire-codec 0.3.0`, is already on crates.io** (published 2026-07-17). That was the hard prerequisite — `cargo publish` refuses a crate whose dependencies aren't on a registry. - The name **`simple_doip` is unclaimed** on crates.io (both spellings 404). - DFT's submodule pointer is **identical** to `main` (0 ahead / 0 behind, both at 0.5.2), so there's no fork to reconcile first. What was actually missing was the license files and the release plumbing. ## Commits 1. **`docs(license)`** — adds `LICENSE-MIT` + `LICENSE-APACHE`, copied verbatim from `uds_protocol` so the protocol libraries carry identical wording. The manifest has declared `MIT OR Apache-2.0` for a while with no text in the repo; GitHub's API reported `license: null` for exactly that reason. Both files land inside the published `.crate`. 2. **`build(release)`** — originally cargo-release plus a hand-rolled tag-driven `release.yml`. **Superseded:** see "Release tooling" below. Both files are deleted; `release-plz.toml` replaces them. ## Release tooling: release-plz, matching the sibling repos `uds_protocol` and `automotive_wire_codec` both release with release-plz, off a `release-plz.toml` that is **byte-identical between them**, and neither has a release workflow of its own — the jobs live in `luminartech/rust_workflow`. This repo was the odd one out. It now carries the same config verbatim, and `release.toml` + `.github/workflows/release.yml` are gone. The switch itself (`use-release-plz: true`, permissions, secrets) lands one layer up in **#13**, which owns `main.yml`. What this changes in practice: **no version is bumped by hand and no tag is pushed by hand.** A push to `main` maintains an open release PR; merging that PR publishes, tags, and cuts the GitHub release. ### The one real regression The deleted `release.yml` published through **crates.io trusted publishing** — OIDC, no stored registry credential anywhere. The reusable workflow's release-plz job takes a `CARGO_REGISTRY_TOKEN` secret instead. Matching the org is the point of this change, so that's the trade accepted here, but it is a step backwards on that one axis and the right place to fix it is `rust_workflow`, not this repo: one `id-token: write` plus a `crates-io-auth-action` step would give trusted publishing to every repo on the shared workflow at once. **Merging this still publishes nothing** — the release-plz jobs need secrets and a `crates-io` environment this repo does not have yet (listed in #13). ## `CONTRIBUTING.md` gains a Releases section Because the repo squash-merges with `squash_merge_commit_title: PR_TITLE`, the **PR title** is the commit subject on `main` — so PR titles, not branch commits, are what release-plz computes the version from. The new section spells that out with the type-to-bump table, and calls out the trap that bit this stack: pre-1.0 a breaking change is a *minor* bump, `!` is the only thing that produces one from a `fix:`, and `cargo-semver-checks` reads the API surface so it will not catch a behavioral break for you. ## How it was tested - `cargo publish --dry-run` — packages 77 files / 106.6 KiB compressed, verifies the packaged crate, resolves `automotive-wire-codec 0.3.0` from crates.io. **This is the proof that 0.5.2 is publishable as-is.** - `cargo package --list` — confirms `LICENSE-APACHE` and `LICENSE-MIT` are in the `.crate`. - `cargo fmt -- --check` and `cargo clippy --all-features -- -D warnings -Dclippy::pedantic` — both clean. - `release-plz.toml` parses, and its values `diff` clean against `uds_protocol`'s and `automotive_wire_codec`'s copies. - `cargo package --list` confirms `release-plz.toml` and `rust-toolchain.toml` stay out of the packaged crate (the `exclude` entry was updated with the rename). - The three cargo-release 0.25 behaviors this depends on were verified empirically during Phase A: the `publish` subcommand overrides `publish = false`; `--allow-branch '*'` is required on a tag-push (detached-HEAD) run; and `--version '^0.25'` is needed because cargo rejects a bare `--version 0.25`. ## Still needs a decision (not in this PR) 1. **crates.io owner + token.** `simple_doip` has no repo secrets at all. The precedent next door is a personal account — both `uds_protocol` and `automotive-wire-codec` are owned on crates.io by `zheylmun`. A crates.io **team owner** (`github:luminartech:<team>`) would be the durable answer. `CARGO_REGISTRY_TOKEN` then goes in this repo's secrets. (Worth noting: `uds_protocol` is at 0.1.0 on `main` but still 0.0.2 on crates.io, and has no `CARGO_REGISTRY_TOKEN` in its secrets — its merged release tooling has never actually published either.) 2. **First-publish version.** 0.5.2 as-is works. Worth being deliberate, because the version number is spent permanently once published. 3. **`luminartech/rust_workflow`.** The org-wide reusable workflow (`rust-ci.yml@v1`) already does tag-gated publish, a `publish-repository` fork guard, a `publish-environment` approval gate and `cargo semver-checks` — `uds_protocol` and `automotive_wire_codec` are both thin callers of it. Adopting it here would replace this repo's hand-rolled `ci.yml` *and* this `release.yml`, and it needs scaffolding this repo doesn't have yet (pre-commit config, `deny.toml`, `.typos.toml`, `.config/nextest.toml`, fuzz targets) — much of which is what #1 adds. That migration is worth doing, but it's a bigger change than unblocking a first publish, so this PR deliberately doesn't touch `ci.yml`. 4. **#1 and #2.** #2 (mine) is superseded by this PR — `main` grew the publish metadata it was adding. #1 (@gavin-dunlap-luminar) is a different question: its hand-rolled 297-line `main.yml` is superseded by `rust_workflow`, but its scaffolding is a prerequisite for adopting it. Both are currently `CONFLICTING` against `main`. ## Pre-publication audit (commits 3-10) A pass over the whole repo for things that would ship visibly wrong. Commits 3-5 needed no decisions: 3. **`build(cargo)`** — **docs.rs would have published a nearly empty API reference.** `default = []`, and docs.rs builds default features only, so `client`, `server`, `codec`, `alloc` and `std` — most of what the README points a reader at — would have been absent from the docs page. Fixed with `[package.metadata.docs.rs] all-features = true`; verified that the `client`, `server` and `message_codec` modules now render. The same commit stops shipping `release.toml` and `rust-toolchain.toml` inside the `.crate` (77 files → 75). 4. **`chore(vscode)`** — all six debug configurations passed `--package=doip`, the crate's pre-rename name, so every one of them failed. 5. **`chore(deps)`** — `futures-util 0.3.28` is yanked on crates.io, so every `cargo publish` run warned about it; moved to 0.3.34. Lockfile only. Commits 6-8 then closed the documentation gaps the audit turned up, and 9-10 settled how the crate actually gets published: 6. **`docs`** — adds `CHANGELOG.md` (110 lines), reconstructed from the release history, so the crate does not arrive on crates.io with the 0.2.0 zero-copy core, the 0.4.0 server API break and the 0.5.x client fixes behind it and no record of any of them. 7. **`docs`** — adds `SECURITY.md` (a stated way to report a vulnerability in an automotive diagnostics library) and `CONTRIBUTING.md` (a stated position on outside contributions, plus the Releases section described above). 8. **`docs(readme)`** — replaces the 45-line "Status" known-gaps inventory with a "Scope and limitations" section. The README is the crate's front page on crates.io; it is now 144 lines. 9. **`ci(release)`** — moved the hand-rolled `release.yml` onto crates.io trusted publishing. **Superseded** by commit 10, which deletes that file. 10. **`build(release)`** — hands versioning and publishing to release-plz. See "Release tooling" above, including the trusted-publishing regression this trade accepts. Also updated the repo's GitHub **description** (was "Crate for Rust DoIP") to match the manifest, and added **topics** (`doip`, `iso13400`, `automotive`, `diagnostics`, `no-std`, `rust`). ### Clean Worth recording, since the audit went looking: no Luminar branding anywhere except the org name in the repository URL; no internal hostnames, IP addresses, ticket numbers or names in any file; no `dbg!`/`println!`/`#[allow(...)]` in `src/`; `#![warn(missing_docs, missing_debug_implementations)]` is on; all 16 symbols the README names exist; examples are clean. ### Still open, needs a decision Four bullets that stood here are now closed: `CHANGELOG.md`, `SECURITY.md` and `CONTRIBUTING.md` are added by commits 6-7, the README's known-gaps inventory is replaced by commit 8, and **`v0.5.2` is now tagged and pushed at `304d014`**, so the tag history no longer has a hole in front of a first publish. What is left: - **Six naked TODOs in `src/`** (`connection.rs`, `routing_activation_request.rs`, and four in `server.rs`), notably `server.rs:631` (`LogicalAddress(0x0000), // TODO fix this constant`) — the same defect the README documents as `ClientConnectionInfo::logical_address` always being `0x0000`. Fixed one layer up in #12; the rest are fix-or-drop. - **`strum` is a major behind** (0.27 vs 0.28.0); bumping likely forces a matching bump in dft's workspace. - **CI has no cargo-audit, cargo-deny, cargo-semver-checks or typos check.** semver-checks is the one that matters once published — it's what stops an accidental breaking release. All four come free with `rust_workflow`, which #13 adopts. One audit finding was **withdrawn**: `ARCHITECTURE.md` §7.1 is marked "RESOLVED in 0.4.0" while sitting under "Known issues and deferred work", which looked stale — but §7's own preamble states the policy deliberately ("Resolved entries are kept because the analysis that led to the fix is still the fastest way to understand the shape the API ended up with"). Left alone. Renumbering would also have broken the `§7.2` references in `src/client_inner.rs` and `tests/integration_test.rs`. ## Review status Marked ready for review; not reviewed by anyone yet. CI is green — 7 pass. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

@gavin-dunlap-luminar

**Stacked on #12 — merge order: #11 → #12 → this.** `--base` is `fix/server-tracks-tester-logical-address`, so the diff here is only this PR's two commits. ## What Replaces this repo's seven hand-rolled CI jobs with a thin caller for `luminartech/rust_workflow/.github/workflows/rust-ci.yml@v1` — the same shape `uds_protocol` and `automotive_wire_codec` use. What the crate gains over the old `ci.yml`: **pre-commit**, a **security audit** (`cargo audit` + `cargo deny`), **coverage**, **miri**, and **`cargo-semver-checks`** — the last being the gate that stops an accidental breaking release once the crate is public. ## Publishing stays out of it `release.yml` (from #11) owns publishing, through **trusted publishing** rather than the registry token this workflow expects, so `publish-crate: false`. The publish *dry run* stays on — it needs no credentials and checks the crate still packages. ## The four inputs that are off, and when each expires | Input | Why | Flips when | |---|---|---| | `run-semver-checks: false` | Diffs against the published baseline; there is none yet | The change that enables publishing | | `run-fuzz-tests: false` | `cargo fuzz build` needs a `fuzz/` directory | #1 lands (it adds four targets) | | `run-property-tests: false` | No `prop_` tests, so the filter selects nothing — and `cargo nextest` **exits 4 on an empty selection**, so the job would fail rather than skip | A property test exists | | `publish-crate: false` | See above | Not intended to flip | Two overrides that are **not** temporary: - **`no-std-target: thumbv7em-none-eabihf`** rather than the workflow's `thumbv6m` default — it's the target this crate's bare-metal support is written against and `examples/bare_metal_codec` is built for. (`thumbv6m` does build, for what it's worth; I checked.) - **`miri-args: '--lib'`** — `tests/golden_vectors.rs` reads its `.hex` fixtures off disk, and miri's isolation refuses `open`. The library tests are where the zero-copy decode paths worth checking for UB live. MSRV is left unset so the job reads `rust-version` from `Cargo.toml` and cannot drift from what the manifest promises. ## Commit 1 is the prerequisite: 12 lints and 2 typos The shared workflow lints with `--all-targets` **and** with `--no-default-features`; this repo's CI ran clippy over the library with all features on. Twelve findings sat in that gap, all in test code or behind a feature combination nothing ever linted — six `assert!(a == b)` → `assert_eq!`, two truncating `as` casts in test fixtures, a missing `#[must_use]`, an unchecked `Duration` subtraction, two doc comments needing backticks on `DoIP`. `typos` found **two real ones**, and one matters: **`diagnotics` was in a user-visible warning string** in `logical_address.rs`, so it would have shipped to anyone using the crate. `recieve` was in a doc comment. It also caught something my earlier audit missed: a test doc comment named an **internal MicroVision application** as its example of a tester holding the `TCP_DATA` slot. This repository is public, so it now says "a diagnostic tool already polling the same ECU" — which is what the test actually exercises. (My branding sweep grepped for the company and sensor names but not the application's, which is how it slipped through.) ## How it was tested Every gate the caller enables, run locally: | | | |---|---| | `cargo fmt --all -- --check` | ok | | clippy `--all-targets --all-features` (pedantic) | ok | | clippy `--no-default-features` (pedantic) | ok | | clippy `--no-default-features --features alloc` | ok | | `cargo build --release --all-features` | ok | | no-std build, `thumbv7em-none-eabihf` (± `alloc`) | ok | | `cargo doc` with `RUSTDOCFLAGS=-D warnings` + doctests | ok | | `cargo +1.88 build --all-features` (MSRV) | ok | | `cargo test --all-features` | ok | | `cargo publish --dry-run` | ok | | **miri**, exactly as the job runs it (`--lib`) | ok — 21 tests, no UB | | `pre-commit run --all-files` | ok | | `typos` | ok | `cargo deny` and `cargo audit` aren't installed on this machine, so **this PR's own run is their first check** — the `deny.toml` is `uds_protocol`'s, and its allow-list may need widening for a dependency it doesn't share. ## Scaffolding `.pre-commit-config.yaml`, `.typos.toml` and `deny.toml`, copied from `uds_protocol`. Two hooks the siblings run are deliberately absent, with the reasons in the config: **mdformat** would mangle this crate's markdown tables and reference-style links, and **check-json** has no strict JSON to check here (the only JSON is `.vscode/launch.json`, which is JSONC — and excluding it instead makes `check-hooks-apply` fail a hook that matches nothing). ## First run failed three jobs — and finding out was the point The three that failed are exactly the three that could not be checked locally, and two shared one root cause I had missed. **`rust-toolchain.toml` pinned `channel = "stable"`, and a directory-local toolchain file overrides whatever toolchain CI installs** — `rustup default` does not beat it. So every job whose purpose is to run a *different* toolchain was either failing or lying: - **Miri** failed: the job installs nightly with the `miri` component, then the bare `cargo miri test` resolved to stable, which has no miri. - **The MSRV check never checked the MSRV.** It installs 1.88 and runs `cargo build`, which the file redirected to stable. That is not a regression from this PR — the previous hand-rolled `ci.yml` used the same pattern, so **the MSRV gate has been decorative for as long as the file existed.** 1.88 does hold; `cargo +1.88` verifies it explicitly. - **Pre-commit** failed on the file's `components = ["clippy", "rustfmt"]`: rustup tried to add clippy to a runner whose stable toolchain already ships `bin/cargo-clippy` and refused with a file conflict. Neither `uds_protocol` nor `automotive_wire_codec` carries a toolchain file, so removing it also stops this repo being the odd one out. `CONTRIBUTING.md` now says the absence is deliberate and why. **Worth being explicit about my own verification miss:** every local check passed because `cargo +nightly miri` and `cargo +1.88 build` name a toolchain explicitly, which *does* beat the file. The workflow runs bare `cargo`. I was testing a different thing than CI was. **`Security Audit` was a genuine finding, not a config problem.** `cargo audit` reported three vulnerabilities in the committed lockfile, all from versions years behind what the manifest already permits: | Crate | Locked | Advisory | Patched | |---|---|---|---| | `bytes` | 1.4.0 | RUSTSEC-2026-0007 | >= 1.11.1 | | `mio` | 0.8.8 | RUSTSEC-2024-0019 | >= 0.8.11 | | `tracing-subscriber` | 0.3.19 | RUSTSEC-2025-0055 | >= 0.3.20 | `cargo update` takes them to 1.12.1, 1.2.3 and 0.3.23 and moves 53 other packages — `tokio 1.30.0` → 1.53.1, `anyhow 1.0.75` → 1.0.104 among them, clearing three `unsound` advisories that were only warnings. The changelog gets a `Security` entry, since a consumer deciding whether to upgrade should see it. The real risk in a 56-package update is the MSRV, which is now actually enforced. `cargo +1.88` builds the refreshed graph with `--all-features` and with `--no-default-features`. Full battery re-run green after the fixes, including miri through `rustup run nightly` (what the job now does), `pre-commit run --all-files`, and `cargo publish --dry-run`. **One commit is mislabeled.** `chore(deps)` also carries the `rust-toolchain.toml` deletion — the `git rm` was already staged when I wrote its message, which is why that message points at a "next commit" that does not contain it. Left as-is rather than rewritten; the reason it was removed is in the following commit and in `CONTRIBUTING.md`. ## Relationship to #1 This overlaps @gavin-dunlap-luminar's #1, which proposed its own 297-line `main.yml` — written before `rust_workflow` was tagged `v1`. The scaffolding that PR adds is still wanted, **the fuzz targets especially**, since they're what unblocks `run-fuzz-tests` here. Its workflow file is superseded by this caller. Worth a conversation rather than a close. ## Review status Not reviewed by anyone yet. Draft. --- ## Release automation (added after the original review pass) This PR now also flips `use-release-plz: true`, so the reusable workflow's `Release-plz PR` and `Release-plz Release` jobs take over versioning, the changelog, tags, GitHub releases and the crates.io publish. That input also disables the workflow's tag-gated `Release & Publish` job, so the two paths cannot both fire. #11 carries the matching `release-plz.toml` and deletes the old cargo-release tooling. Wiring copied from `automotive_wire_codec`: - `contents: write` + `pull-requests: write` on the called workflow — the test jobs downscope themselves back to `contents: read`; this grant exists only so release-plz can push the release-PR branch and the release tag. - `cargo-registry-token`, `release-plz-app-id`, `release-plz-app-private-key` passed through as secrets. - `publish-repository: luminartech/simple_doip`, so a fork cannot publish under this crate's name. ### Repo configuration this needs, none of it in this diff | | | |---|---| | `CARGO_REGISTRY_TOKEN` | secret; does not exist yet | | `RELEASE_PLZ_APP_ID` / `RELEASE_PLZ_APP_PRIVATE_KEY` | secrets; do not exist yet | | `crates-io` environment | **this repo has no environments at all**; both siblings have this one, and the reusable workflow gates each release job on it | Until those exist the release-plz jobs run and no-op rather than publishing, which is why merging this is safe. `run-semver-checks` stays off — there is still no published baseline to diff against. The comment now also records what it will *not* do once enabled: it reads the public API surface, so a changed signature is caught and a changed behavior is not. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

@gavin-dunlap-luminar

**Stacked on #13 — merge order: #11 → #12 → #13 → this.** ## Issue URL Closes #1 (supersedes it — see below). Files #15. ## What Brings simple_doip#1's test suites forward instead of rebasing that branch, and turns on the two CI gates that were off for want of them. #1's merge base is 2026-04-03, before the `no_std` migration, the error-taxonomy refactor and the 0.4.0 server break. Its 26 property tests and 3 of its 4 fuzz targets drive `Message::read`, `Message::write` and `Payload::read` — **none of which exist on `main`**, all removed in 0.2.0. A rebase resolves textually (12 conflicts on the first of 7 commits) and then fails to compile. The properties were sound; only the calls were stale. | | | |---|---| | **`tests/property.rs`** | 24 properties ported to `Encode`/`Decode`, all passing on the first run | | **`fuzz/`** | 4 cargo-fuzz targets, 3 rewritten and 1 unchanged | | **`.github/` templates** | PR + bug/epic/task, verbatim from #1 — no API coupling, no porting needed | | **`main.yml`** | `run-property-tests` and `run-fuzz-tests` both on | **24 of 26 properties land.** The two that don't are byte round trips through serde, and this crate has no serde dependency. They live in `tests/property.rs` rather than `#[cfg(test)]` modules inside `src/` as the original did: proptest needs `std`, the library is `no_std`, and an integration target gets `std` with no conditional-compilation gymnastics. The cost is only reaching the public API, which is all these properties touch. ## What they add over `golden_vectors.rs` The golden fixtures pin the exact bytes the crate emits, so they catch the wire format changing. These check `encode` and `decode` agree with **each other** across the whole input space, which catches a field written in one order and read in another. Neither finds a misreading of the standard that both directions share symmetrically — that's what the fixtures are for. ## `fuzz_roundtrip` found a real bug in under a second **#15**: `Message::encode` can emit a frame that `Message::decode` rejects. `decode` takes exactly `header.payload_length` bytes and lets `Payload::decode` consume fewer without complaint; the decoded `Message` keeps the declared length; `encode` then writes that stale length beside a payload of its real size. A NACK frame declaring 5 body bytes and carrying 1 decodes fine, re-encodes to 9 bytes with the header still claiming 5, and fails to re-decode with `Incomplete { needed: 5, available: 1 }`. The target **skips that specific shape**, with #15 referenced at the check, so it keeps hunting field-order asymmetries without asserting a property the crate violates today. The skip comes out with the fix, which is stacked on this PR. ## Testing | | | |---|---| | `cargo test --all-features` / `--no-default-features` | pass | | the 24 properties | pass | | `cargo nextest run -E 'test(~prop_)'` | **selects exactly 24**, not zero — the `exit 4` failure mode | | `cargo fuzz build` (real cargo-fuzz, nightly) | pass | | 8s per fuzz target | 5.4M / 6.1M / 1.6M / 4.1M execs, all clean | | `clippy --all-targets --all-features -Dclippy::pedantic` | clean | | `clippy --no-default-features -Dclippy::pedantic` | clean | | `cargo fmt --all --check`, `pre-commit run --all-files` | clean | | `cargo publish --dry-run` | pass — `fuzz/` does not enter the packaged crate | `unit-test-filter` stays `all()` rather than excluding `prop_`: the unit job measures coverage, and coverage should describe the whole suite. The property job re-runs the same 24 under their own name for a readable signal. ## Not included The **PR description lint** the sibling repos pair with the templates. It requires `## Issue URL` and `## Testing` sections and would fail every currently open PR, including this one's stack-mates. A `No Issue` label now exists as its escape hatch; the job belongs in a follow-up once bodies conform. ## On #1 Its `main.yml` predates `rust_workflow@v1` by six weeks, so it wasn't a misjudgment — the better option didn't exist. Its scaffolding was independently written in #13 before I'd read his closely enough, which is on me. What was uniquely valuable was the tests, and they're here. Recommend closing #1 with a pointer to this PR rather than leaving it to rot; @gavin-dunlap-luminar is credited in the commit and in `tests/property.rs`. ## Note on the commit This is one commit, not the four its message describes — everything was already staged when I wrote the first one, so it swept the lot. Since the repo squash-merges with the body taken from the PR description, the rationale that would have been in those messages is above instead. ## Review status Not reviewed by anyone yet. Draft. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

…sage (#17) **Stacked on #16 — merge order: #11 → #12 → #13 → #16 → this.** ## Issue URL Closes #15. ## What `Message::encode` could emit a frame that `Message::decode` rejects. `decode` takes exactly `header.payload_length` bytes and hands them to `Payload::decode`, which is not required to consume all of them. The decoded `Message` keeps the header verbatim, declared length included, and `encode` wrote that stale field beside a payload of its real size. Note that `encoded_size()` already disagreed with the header being written — it returns `Header::SIZE + payload.encoded_size()`, not the declared length. ```rust // A NACK body is one byte. This header claims five. let framed = [0x02, 0xFD, 0, 0, 0, 0, 0, 0x05, 0x03, 0, 0, 0, 0]; let (msg, _) = Message::decode(&framed).unwrap(); // accepted assert_eq!(msg.header.payload_length, 5); // preserved // re-encode -> 9 bytes, header still claims 5 Message::decode(&encode(&msg)); // Err(Incomplete { needed: 5, available: 1 }) ``` Anything that decodes a frame and re-emits it — a proxy, a replay tool, a logging fake, a test harness echoing what it received — was turning a malformed-but-accepted frame into a corrupt one on the wire. This crate's own `MessageCodec` encoder is on that path. ## The fix `encode` builds its header from `payload.encoded_size()` rather than trusting `self.header.payload_length`, so an encoded frame is always self-consistent regardless of how lenient `decode` is. **No input that is accepted today starts being rejected.** The consequence, documented on the method: for a frame that arrived with a mismatched declared length, `decode(encode(m)).header.payload_length` is the payload's real size rather than the length it arrived with. That is the point — but it is a visible behavior change, so it's in the changelog. A **well-formed frame is byte-identical**, which is why all 11 golden vectors still pass unchanged. That's also pinned as a test. `MessageError::PayloadTooLarge` covers the one fallible step — a payload too large for the `u32` length field, unreachable for a frame off the wire whose length was itself a `u32`. `MessageError` is `#[non_exhaustive]`, so adding it breaks nothing. ## Why not make `decode` strict instead That is the standards-correct complement — ISO 13400-2 has an entity answer an invalid payload length with NACK `0x04`, and `MessageError::PayloadLengthTooShort` sits unused for exactly this. But it's a redesign, not a fix: `Payload::decode` would have to report unconsumed bytes, and the identification requests **deliberately** discard their EID/VIN body (`ARCHITECTURE.md` §7.6), so a `0x0002` request carrying its six EID bytes would start being rejected outright rather than declined — breaking the UDP responder path. Recorded in `ARCHITECTURE.md` §7.6 as deferred rather than dropped. ## Testing | | | |---|---| | `tests/encode_consistency.rs` | 3 regression cases: overlong length on a fixed payload, nonzero length on a unit payload, and a well-formed frame encoding to the bytes it came from | | golden vectors | 11/11 pass **unchanged** — the fix cannot touch a frame whose declared length was already right | | `fuzz_roundtrip`, skip removed, idempotence asserted | **21,033,263 executions clean** | | other three fuzz targets | 6.4M / 8.1M / 1.7M clean | | full test suite, all features and none | pass | | clippy `--all-targets --all-features -Dclippy::pedantic`, and `--no-default-features` | clean | | `cargo doc` with `-D warnings`, `cargo fmt`, `pre-commit`, `cargo publish --dry-run`, MSRV 1.88 | pass | The fuzz target now asserts payload and payload-type equality plus **idempotence** (encode, decode, encode again → identical bytes) rather than whole-`Message` equality. Asserting full equality would assert the bug back into existence, since the declared length legitimately normalizes; idempotence buys back the field-order asymmetry detection that equality was providing. ## Also Corrects `PayloadLengthTooShort`'s message, which read "does match" where it meant "does not match" — a user-visible error string. ## Release bump: 0.6.0 This PR sits at the top of the stack, so it also carries `chore(release): v0.6.0` — the version the whole stack (#11 → #17) publishes as. It follows the same pattern as 0.5.2, whose bump was made inside #10's branch rather than by `cargo release` on main. Nothing in the stack breaks a signature: `MessageError` is `#[non_exhaustive]`, so `PayloadTooLarge` is additive, and `ClientConnectionInfo::logical_address` keeps its type and only starts carrying a real value. The bump is for the encode change in this PR — a caller that set a mismatched `payload_length` deliberately (a negative-test fake, a corpus generator, a proxy replaying what it saw) stops being able to emit that frame, with no compiler diagnostic anywhere. The CHANGELOG entry is marked **Breaking:** so the version and the section header tell the same story. `v0.5.2` is now tagged at the #10 merge on `main` (`304d014`), so that section has a comparison range and the release links run `v0.5.2...v0.6.0`. ## Review status Not reviewed by anyone yet. Draft. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

JustinKovacich self-assigned this Sep 9, 2026

JustinKovacich added the bug Something isn't working label Sep 9, 2026

JustinKovacich mentioned this pull request Sep 9, 2026

ci: adopt the org-wide reusable Rust workflow #13

Merged

JustinKovacich added this pull request to stack #14 September 9, 2026 18:24

JustinKovacich marked this pull request as ready for review September 10, 2026 13:22

JustinKovacich requested a review from zheylmun September 10, 2026 13:22

zheylmun approved these changes Sep 10, 2026

View reviewed changes

JustinKovacich and others added 2 commits September 10, 2026 09:51

JustinKovacich force-pushed the fix/server-tracks-tester-logical-address branch from 43d60f4 to 837e218 Compare September 10, 2026 13:51

JustinKovacich mentioned this pull request Sep 10, 2026

build(release): license texts, contributor docs, and release-plz automation #11

Merged

JustinKovacich merged commit abe8acf into main Sep 10, 2026
7 checks passed

JustinKovacich deleted the fix/server-tracks-tester-logical-address branch September 10, 2026 16:50

Navigation Menu

Uh oh!

fix(server): carry the tester's logical address into ClientConnectionInfo - #12

fix(server): carry the tester's logical address into ClientConnectionInfo#12
JustinKovacich merged 2 commits into
build/license-and-release-toolingfrom
fix/server-tracks-tester-logical-address

JustinKovacich commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

JustinKovacich commented Sep 9, 2026

What

Why this scope

How it was tested

Naked TODOs cleared

Docs

Review status

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants