build(release): license texts, contributor docs, and release-plz automation - #11
Merged
Merged
Conversation
`Cargo.toml` has declared `license = "MIT OR Apache-2.0"` for a while, but neither license text was ever committed. A manifest field is not what a customer's legal review looks for -- it looks for the file -- and GitHub's own API reported `license: null` for this repo because it keys off the file too, understating the project's posture. Both texts are copied verbatim from the sibling `uds_protocol` repo so the protocol libraries carry identical wording. They are also picked up by `cargo package`, so a published `.crate` now ships its own license terms. Neither file names a copyright holder -- that matches `uds_protocol` today, and is worth settling across all three protocol repos at once rather than diverging here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…h workflow
The crate has complete publish metadata and CI already proves publishability
on every run (`cargo publish --dry-run` in the `package` job), but there was
no way to actually cut a release. This adds the two missing pieces.
`release.toml` configures cargo-release for a local version bump: a
`v{version}` tag, a `chore(release):` commit, and `cargo test --locked
--all-features` as the pre-release hook. `--all-features` rather than a bare
`cargo test`, because `default = []` here, so the default-feature run
exercises almost none of the crate. `publish = false` keeps a local
`cargo release <bump>` from ever reaching crates.io; the CI job uses the
`cargo release publish` subcommand, which overrides that field.
`release.yml` publishes on a `v*` tag push and is inert until deliberately
enabled -- it needs both the canonical repository (so a fork pushing a tag
can never publish under this crate's name) and the
`SIMPLE_DOIP_PUBLISH_ENABLED` repo variable. Merging this publishes nothing.
The job verifies the tag names the version the manifest actually carries
before it spends it, since a crates.io publish can only be yanked, never
undone. `--allow-branch '*'` is required because a tag-push run is in
detached-HEAD state, which cargo-release's branch check rejects.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o tooling `default = []`, so docs.rs would have built the `no_std` core alone and published a reference containing no `client`, `server`, `codec`, `alloc` or `std` API -- most of what the README points a reader at. Every feature is additive, so `all-features = true` is safe and gives the published docs the whole surface. Verified with `cargo doc --no-deps --all-features`: the `client`, `server` and `message_codec` modules now render. `release.toml` and `rust-toolchain.toml` were also landing inside the published `.crate`. Neither is part of the library a consumer receives -- `rust-toolchain.toml` has no effect on a dependency at all -- so both move to `exclude`. The package drops from 77 files to 75. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All six configurations passed `--package=doip`, the crate's name before it was renamed to `simple_doip`, so every one of them failed for anyone who clicked it. The launch config is excluded from the published crate but is visible to anyone who opens the repository. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`futures-util 0.3.28` is yanked on crates.io, so every `cargo publish` run warned about it. `cargo update -p futures-util` takes it (and `futures-task`) to 0.3.34. Lockfile only -- no manifest requirement changes, and consumers of the published crate resolve their own graph regardless. Verified `cargo check --locked` with `--all-features` and with `--no-default-features`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The crate is at 0.5.2 with three breaking changes behind it -- the 0.2.0 zero-copy `no_std` core, 0.4.0's `ServerConnectionHandler::diagnostic_message` signature, and 0.5.0's acknowledgement timeout -- and no record of any of them. Anyone arriving at 0.5.2 had no way to see what changed or what broke. Reconstructed from the version-bump commits and the pull requests that carried them, in Keep a Changelog form to match `uds_protocol`. Only v0.1.0 and v0.5.1 were ever tagged, so the intermediate versions have no comparison range to link; the file says so rather than carrying links that 404. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A public automotive diagnostics library with no stated way to report a vulnerability leaves a finder with only the public issue tracker. `SECURITY.md` routes reports through GitHub's private vulnerability reporting, which is now enabled on the repository. It also records the two protocol properties a report has to be assessed against, because both are ISO 13400-2's design rather than defects here: DoIP carries no transport security, and routing activation is addressing, not authentication. `CONTRIBUTING.md` says pull requests are welcome and gives the feature-aware test commands -- `default = []`, so a bare `cargo test` exercises only the `no_std` core, which is not obvious -- the bare-metal target check, what CI adds, the golden-vector rule for wire-format changes, and the dual-license terms contributions are accepted under. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The README opened with ~45 lines of known-gap inventory written in a defect-report register, down to internal state names and private method paths. Accurate, but it is the crate's front page on crates.io, and the detail served a maintainer rather than someone deciding whether the crate fits. What survives is the five bounds that change how the crate is used -- no TLS, one connection at a time, no unsolicited announcement, the acknowledge-before- respond requirement, and the untracked per-connection logical address -- each in a line, pointing at ARCHITECTURE.md for the mechanics. Nothing is lost: ARCHITECTURE.md section 7.6 already carried all of it, and described itself as a repeat of the README. That relationship is now the other way round, so the section reads as the authority it has become. Also links the license files from the README's license section, now that they exist, and points at the new contributing and security policies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the `CARGO_REGISTRY_TOKEN` secret with `rust-lang/crates-io-auth-action`, which trades the workflow's GitHub OIDC identity for a publish token scoped to this crate and revoked when the job ends. Nothing long-lived is stored in the repository, so there is no token to provision, rotate, or leak, and no personal account's credential standing behind the crate. The trade-off is a prerequisite rather than a cost: a trusted publisher can only be configured on a crate that already exists, so the first version is published by hand. That is the intent regardless -- the first publish claims the name permanently. The job's gates are unchanged, and it gains only `id-token: write`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 9, 2026
JustinKovacich
added this pull request to stack #14
September 9, 2026 18:24
This was referenced Sep 9, 2026
JustinKovacich
marked this pull request as ready for review
September 10, 2026 13:21
JustinKovacich
added a commit
that referenced
this pull request
Sep 10, 2026
The stack from #11 breaks no signature -- `MessageError` is `#[non_exhaustive]`, so the added `PayloadTooLarge` variant is additive, and `ClientConnectionInfo::logical_address` keeps its type and only starts carrying a real value. What earns the minor bump is `Message::encode` deriving the header's declared length: a caller that set a mismatched `payload_length` on purpose stops being able to emit that frame, and nothing in the type system says so. A silent change in emitted bytes is the case the 0.x minor bump exists for, so the CHANGELOG entry is marked breaking to match the version. v0.5.2 is now tagged at the #10 merge on main, so the section that had no comparison range gets one, and the release links run 0.5.2...0.6.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zheylmun
requested changes
Sep 10, 2026
`uds_protocol` and `automotive_wire_codec` both release with release-plz, off a `release-plz.toml` that is byte-identical between them and jobs that live in the org-wide reusable workflow. This repo was the odd one out with cargo-release plus a hand-rolled tag-driven `release.yml`, so adopt the same config verbatim and delete both. What that 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 it publishes, tags, and cuts the GitHub release. The `Release & Publish` job in the reusable workflow turns itself off when `use-release-plz` is on, so the two paths cannot both fire. One thing is genuinely lost. The deleted `release.yml` published through crates.io trusted publishing, which stores no registry credential at all; the reusable workflow's release-plz job takes a `CARGO_REGISTRY_TOKEN` secret. That is the org's shape today and matching it is the point of this change, but it is a step back on that specific axis and worth revisiting in `rust_workflow` rather than here -- one `id-token: write` and a `crates-io-auth-action` step would give every repo on the shared workflow trusted publishing at once. `CONTRIBUTING.md` gains the release section this makes necessary: the PR title is what lands under squash-merge, so the title is what the version is computed from, and a `fix:` needs an explicit `!` to become the minor bump that a pre-1.0 breaking change calls for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JustinKovacich
added a commit
that referenced
this pull request
Sep 10, 2026
The stack from #11 breaks no signature -- `MessageError` is `#[non_exhaustive]`, so the added `PayloadTooLarge` variant is additive, and `ClientConnectionInfo::logical_address` keeps its type and only starts carrying a real value. What earns the minor bump is `Message::encode` deriving the header's declared length: a caller that set a mismatched `payload_length` on purpose stops being able to emit that frame, and nothing in the type system says so. A silent change in emitted bytes is the case the 0.x minor bump exists for, so the CHANGELOG entry is marked breaking to match the version. v0.5.2 is now tagged at the #10 merge on main, so the section that had no comparison range gets one, and the release links run 0.5.2...0.6.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zheylmun
approved these changes
Sep 10, 2026
JustinKovacich
added a commit
that referenced
this pull request
Sep 10, 2026
…Info (#12) **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. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
JustinKovacich
added a commit
that referenced
this pull request
Sep 10, 2026
**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>
JustinKovacich
added a commit
that referenced
this pull request
Sep 10, 2026
**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>
JustinKovacich
added a commit
that referenced
this pull request
Sep 10, 2026
…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>
This was referenced Sep 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The two things standing between
simple_doipand 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.tomlonmainalready carries the full publish metadata —description,license,repository,readme,keywords,categories,exclude. Nopublish = false.cargo publish --dry-runin thepackagejob, sopublishability is verified on every run.
automotive-wire-codec 0.3.0, is alreadyon crates.io (published 2026-07-17). That was the hard prerequisite —
cargo publishrefuses a crate whose dependencies aren't on a registry.simple_doipis unclaimed on crates.io (both spellings 404).main(0 ahead / 0 behind, bothat 0.5.2), so there's no fork to reconcile first.
What was actually missing was the license files and the release plumbing.
Commits
docs(license)— addsLICENSE-MIT+LICENSE-APACHE, copied verbatimfrom
uds_protocolso the protocol libraries carry identical wording. Themanifest has declared
MIT OR Apache-2.0for a while with no text in therepo; GitHub's API reported
license: nullfor exactly that reason. Bothfiles land inside the published
.crate.build(release)— originally cargo-release plus a hand-rolledtag-driven
release.yml. Superseded: see "Release tooling" below.Both files are deleted;
release-plz.tomlreplaces them.Release tooling: release-plz, matching the sibling repos
uds_protocolandautomotive_wire_codecboth release with release-plz, offa
release-plz.tomlthat is byte-identical between them, and neither hasa 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.ymlare gone.The switch itself (
use-release-plz: true, permissions, secrets) lands onelayer 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
mainmaintains an open release PR; merging thatPR publishes, tags, and cuts the GitHub release.
The one real regression
The deleted
release.ymlpublished through crates.io trusted publishing —OIDC, no stored registry credential anywhere. The reusable workflow's
release-plz job takes a
CARGO_REGISTRY_TOKENsecret instead. Matching theorg 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: oneid-token: writeplus acrates-io-auth-actionstep would give trusted publishing to every repo onthe shared workflow at once.
Merging this still publishes nothing — the release-plz jobs need secrets
and a
crates-ioenvironment this repo does not have yet (listed in #13).CONTRIBUTING.mdgains a Releases sectionBecause the repo squash-merges with
squash_merge_commit_title: PR_TITLE, thePR title is the commit subject on
main— so PR titles, not branchcommits, 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 thingthat produces one from a
fix:, andcargo-semver-checksreads the APIsurface 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.0fromcrates.io. This is the proof that 0.5.2 is publishable as-is.
cargo package --list— confirmsLICENSE-APACHEandLICENSE-MITarein the
.crate.cargo fmt -- --checkandcargo clippy --all-features -- -D warnings -Dclippy::pedantic— both clean.release-plz.tomlparses, and its valuesdiffclean againstuds_protocol's andautomotive_wire_codec's copies.cargo package --listconfirmsrelease-plz.tomlandrust-toolchain.tomlstay out of the packaged crate (the
excludeentry was updated with therename).
empirically during Phase A: the
publishsubcommand overridespublish = false;--allow-branch '*'is required on a tag-push(detached-HEAD) run; and
--version '^0.25'is needed because cargo rejectsa bare
--version 0.25.Still needs a decision (not in this PR)
simple_doiphas no repo secrets at all.The precedent next door is a personal account — both
uds_protocolandautomotive-wire-codecare owned on crates.io byzheylmun. A crates.ioteam owner (
github:luminartech:<team>) would be the durable answer.CARGO_REGISTRY_TOKENthen goes in this repo's secrets.(Worth noting:
uds_protocolis at 0.1.0 onmainbut still 0.0.2 oncrates.io, and has no
CARGO_REGISTRY_TOKENin its secrets — its mergedrelease tooling has never actually published either.)
because the version number is spent permanently once published.
luminartech/rust_workflow. The org-wide reusable workflow(
rust-ci.yml@v1) already does tag-gated publish, apublish-repositoryfork guard, a
publish-environmentapproval gate andcargo semver-checks—uds_protocolandautomotive_wire_codecare both thincallers of it. Adopting it here would replace this repo's hand-rolled
ci.ymland thisrelease.yml, and it needs scaffolding this repodoesn't have yet (pre-commit config,
deny.toml,.typos.toml,.config/nextest.toml, fuzz targets) — much of which is what feat(ci): Implement robust rust CI. #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.maingrew the publishmetadata it was adding. feat(ci): Implement robust rust CI. #1 (@gavin-dunlap-luminar) is a different question:
its hand-rolled 297-line
main.ymlis superseded byrust_workflow, butits scaffolding is a prerequisite for adopting it. Both are currently
CONFLICTINGagainstmain.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:
build(cargo)— docs.rs would have published a nearly empty APIreference.
default = [], and docs.rs builds default features only, soclient,server,codec,allocandstd— most of what the READMEpoints a reader at — would have been absent from the docs page. Fixed with
[package.metadata.docs.rs] all-features = true; verified that theclient,serverandmessage_codecmodules now render. The same commitstops shipping
release.tomlandrust-toolchain.tomlinside the.crate(77 files → 75).
chore(vscode)— all six debug configurations passed--package=doip,the crate's pre-rename name, so every one of them failed.
chore(deps)—futures-util 0.3.28is yanked on crates.io, so everycargo publishrun 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:
docs— addsCHANGELOG.md(110 lines), reconstructed from the releasehistory, 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.
docs— addsSECURITY.md(a stated way to report a vulnerability inan automotive diagnostics library) and
CONTRIBUTING.md(a stated positionon outside contributions, plus the Releases section described above).
docs(readme)— replaces the 45-line "Status" known-gaps inventory witha "Scope and limitations" section. The README is the crate's front page on
crates.io; it is now 144 lines.
ci(release)— moved the hand-rolledrelease.ymlonto crates.iotrusted publishing. Superseded by commit 10, which deletes that file.
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(...)]insrc/;#![warn(missing_docs, missing_debug_implementations)]is on; all 16symbols the README names exist; examples are clean.
Still open, needs a decision
Four bullets that stood here are now closed:
CHANGELOG.md,SECURITY.mdandCONTRIBUTING.mdare added by commits 6-7, the README's known-gaps inventory isreplaced by commit 8, and
v0.5.2is now tagged and pushed at304d014, sothe tag history no longer has a hole in front of a first publish. What is left:
src/(connection.rs,routing_activation_request.rs,and four in
server.rs), notablyserver.rs:631(
LogicalAddress(0x0000), // TODO fix this constant) — the same defect theREADME documents as
ClientConnectionInfo::logical_addressalways being0x0000. Fixed one layer up in fix(server): carry the tester's logical address into ClientConnectionInfo #12; the rest are fix-or-drop.strumis a major behind (0.27 vs 0.28.0); bumping likely forces amatching bump in dft's workspace.
semver-checks is the one that matters once published — it's what stops an
accidental breaking release. All four come free with
rust_workflow, whichci: adopt the org-wide reusable Rust workflow #13 adopts.
One audit finding was withdrawn:
ARCHITECTURE.md§7.1 is marked "RESOLVEDin 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.2references insrc/client_inner.rsandtests/integration_test.rs.Review status
Marked ready for review; not reviewed by anyone yet. CI is green — 7 pass.