From 23cf4048673c39ff92cec3c6923ce607f236742e Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 27 Oct 2026 21:10:36 +0100 Subject: [PATCH 1/8] Separate UI validation from runtime coverage Run compiler-driven UI checks in a dedicated CI job and require it before the existing build-test status can pass. Exclude those harnesses from instrumented coverage while retaining their normal test paths. Scope example and test dependencies to development builds and declare the runtime features that were previously enabled transitively. --- .github/workflows/ci.yml | 29 +++- .github/workflows/coverage-main.yml | 9 +- Cargo.lock | 1 - Cargo.toml | 14 +- crates/wireframe-verification/Cargo.toml | 1 - docs/developers-guide.md | 42 +++-- tests/compile_error.rs | 2 + .../ci_runner_placement_test.py | 51 +++---- .../dependency_scoping_test.py | 64 ++++++++ .../nextest_timeout_test.py | 143 ++++++++++++++++++ .../runner_placement_policy.py | 12 +- wireframe_testing/tests/loom_api_surface.rs | 1 + .../tests/ui/loom_non_loom_exports.stderr | 24 ++- 13 files changed, 316 insertions(+), 77 deletions(-) create mode 100644 tests/workflow_contracts/dependency_scoping_test.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e3d9a4d84..06fdc387e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,6 +19,8 @@ concurrency: jobs: build-test: + # The existing required status must wait for the UI job to succeed. + needs: ui-test # The one developer-blocking lane, so the one that moves. It waited 461 s # for a GitHub-hosted runner on 2026-09-16 to do 556 s of work. # @@ -33,12 +35,8 @@ jobs: # https://github.com/leynos/wireframe/actions/runs/39038420830). That is # about 29% less per run for 3m40s more wall time. # - # The coverage action supplies a 180-second per-test deadline. The - # aggregate tests/compile_error.rs test builds and executes twelve - # trybuild fixtures under coverage; its cold nested build exceeded that - # deadline even after the ordinary tests completed. .config/nextest.toml - # extends only this test to eight minutes, preserves the ordinary - # deadline, and retains the ten-minute whole-run ceiling. + # Compiler-driving UI fixtures run in the dedicated `ui-test` job. The + # coverage lane measures runtime tests without a nested trybuild build. # # Disk is the margin to watch: the minimum free space on two vCPU was # 3,998 MiB, against 78,392 MiB on four. @@ -158,3 +156,22 @@ jobs: # The ratchet reads this report; nothing else does, so it is not # published as an artefact. publish-artefact: 'false' + + # Compile-time API checks require their own cold compiler build and are not + # meaningful runtime coverage. Keep them mandatory on both PRs and pushes. + ui-test: + runs-on: >- + ${{ (github.event.pull_request.head.repo.fork + || github.event.pull_request.user.login == 'dependabot[bot]') + && 'ubuntu-latest' || 'ubicloud-standard-2' }} + timeout-minutes: 30 + permissions: + contents: read + steps: + - uses: actions/checkout@v7 + - name: Setup Rust and linker + uses: leynos/shared-actions/.github/actions/setup-rust@8e6a325765ba382691d1b1c65fcccf1fc1d1b00c + with: + install-mold: 'true' + - name: Validate compile-time APIs + run: make test-ui diff --git a/.github/workflows/coverage-main.yml b/.github/workflows/coverage-main.yml index 7c92a11d8..21a4407df 100644 --- a/.github/workflows/coverage-main.yml +++ b/.github/workflows/coverage-main.yml @@ -32,11 +32,10 @@ jobs: # arm: `push` carries no pull request, so the guard would be constant # here and a constant guard reads as a decision nobody made. # - # Four vCPU, and not measured at two: this lane runs the same - # `generate-coverage` over the same workspace as `ci.yml`, whose - # `build-test` now runs on two vCPU after a warm run passed the trybuild - # case that nextest had killed at 180 s there. This lane's own 240 s - # sample was taken on Namespace's shared profile, which is four vCPU and + # Four vCPU, and not measured at two: this lane runs the same runtime + # coverage as `ci.yml`; compile-time UI checks run in its separate job. + # This lane's own 240 s sample was taken on Namespace's shared profile, + # which is four vCPU and # 16 GB, and it has not been sampled on two. It runs on merge pushes # with nobody watching, so moving it needs its own measurement. runs-on: ubicloud-standard-4 diff --git a/Cargo.lock b/Cargo.lock index 0886c4bcc..c879c531e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3621,7 +3621,6 @@ version = "0.1.0" dependencies = [ "rstest", "stateright", - "wireframe", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 658eadcc2..d67e23bd3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,7 +38,7 @@ tokio = { version = "1.47.1", default-features = false, features = [ "time", "io-util", ] } -tokio-util = { version = "0.7.16", features = ["rt", "codec"] } +tokio-util = { version = "0.7.16", features = ["rt", "codec", "io"] } tokio-stream = "0.1.17" futures = "0.3.31" async-trait = "0.1.89" @@ -47,17 +47,17 @@ log = "0.4.28" dashmap = "6.1.0" leaky-bucket = "1.1.2" tracing = { version = "0.1.41", features = ["log", "log-always"] } -tracing-subscriber = "0.3.18" metrics = { version = "0.24.2", optional = true } monotony = "1.0.0" thiserror = "2.0.16" -itoa = "1.0" static_assertions = "1.1.0" derive_more = { version = "2.0.1", features = ["display", "from"] } socket2 = "0.6.0" bb8 = { version = "0.9.1", optional = true } [dev-dependencies] +itoa = "1.0" +tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } rstest = "0.27.0" rstest-bdd = "0.5.0" rstest-bdd-macros = { version = "0.5.0", features = ["strict-compile-time-validation"] } @@ -95,12 +95,12 @@ tokio = { version = "1.47.1", default-features = false, features = [ [target.'cfg(loom)'.dependencies] loom = "0.7.2" -[target.'cfg(not(loom))'.dependencies] -metrics-exporter-prometheus = { version = "0.18.3", optional = true, features = ["http-listener"] } +[target.'cfg(not(loom))'.dev-dependencies] +metrics-exporter-prometheus = { version = "0.18.3", features = ["http-listener"] } [features] default = ["metrics", "serializer-bincode"] -metrics = ["dep:metrics", "dep:metrics-exporter-prometheus"] +metrics = ["dep:metrics"] pool = ["dep:bb8"] serializer-bincode = [] serializer-serde = [] @@ -145,7 +145,7 @@ big_endian_bytes = "deny" unknown_lints = "deny" renamed_and_removed_lints = "deny" missing_docs = "deny" -unexpected_cfgs = { level = "warn", check-cfg = ['cfg(loom)'] } +unexpected_cfgs = { level = "warn", check-cfg = ['cfg(loom)', 'cfg(coverage)'] } [workspace.lints.rustdoc] missing_crate_level_docs = "deny" diff --git a/crates/wireframe-verification/Cargo.toml b/crates/wireframe-verification/Cargo.toml index 1b02d1c06..9ec995183 100644 --- a/crates/wireframe-verification/Cargo.toml +++ b/crates/wireframe-verification/Cargo.toml @@ -9,7 +9,6 @@ workspace = true [dependencies] stateright = "0.31.0" -wireframe = { path = "../..", features = ["test-support"] } [dev-dependencies] rstest = "0.27.0" diff --git a/docs/developers-guide.md b/docs/developers-guide.md index f07ff5f47..444493bfc 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -12,12 +12,16 @@ than all four repository-owned lanes sharing one profile. | Workflow | Job | Trigger | Runner | Ceiling | | ------------------------ | ------------------- | ------------------ | --------------------- | ------- | | `ci.yml` | `build-test` | pull request, push | `ubicloud-standard-2` | 30 min | +| `ci.yml` | `ui-test` | pull request, push | `ubicloud-standard-2` | 30 min | | `coverage-main.yml` | `coverage-upload` | push | `ubicloud-standard-4` | 20 min | | `advanced-tests.yml` | `advanced` | schedule | `ubuntu-latest` | 60 min | | `delayed-pr-comment.yml` | `delay_and_comment` | dispatch | `ubuntu-latest` | none | *Table 1: Where each repository-owned lane runs, and its ceiling.* +`build-test` depends on `ui-test`, so UI failures block the existing required +`build-test` status. + Pull-request, push and tag lanes move to Ubicloud, because those are the ones a developer waits on: `build-test` waited 461 seconds for a GitHub-hosted runner on 2026-09-16 to do 556 seconds of work. Scheduled and delayed-comment lanes @@ -87,15 +91,20 @@ Two vCPU cost about 29% less per run for 3m40s more wall time, and memory is far inside the 7.9 GiB that shape has. The coverage action supplies a 180-second per-test deadline and a ten-minute -whole-run ceiling. `tests/compile_error.rs` runs fourteen trybuild fixtures -inside one test, including nested Cargo builds. A cold coverage run exceeded -that deadline on two vCPU after the other tests had completed -([PR #777's coverage run](https://github.com/leynos/wireframe/actions/runs/39203098537)). -The checked-in `.config/nextest.toml` preserves the action's deadlines for -ordinary tests and extends only `wireframe::compile_error::compile_tests` to -eight minutes. The ten-minute ceiling still bounds the coverage test run. The -four-vCPU `coverage-upload` lane remains on its measured shape; the smaller -runner has not been sampled with this per-test budget. +whole-run ceiling. `tests/compile_error.rs` runs trybuild fixtures inside one +test, including nested Cargo builds, so it is not useful runtime coverage and +can exceed that per-test deadline on a cold two-vCPU runner. The `ui-test` job +runs `make test-ui` separately for pull requests and pushes to `main`, with a +30-minute job ceiling. This keeps the compile-time API fixtures covered without +making the instrumented runtime-coverage run compile them. + +The compile-time harnesses are excluded only when `cfg(coverage)` is active; +the fixtures remain in `tests/ui/`, and `make test-ui` runs both the root and +Loom API suites. The checked-in `.config/nextest.toml` retains its eight-minute +override for `wireframe::compile_error::compile_tests` during ordinary, +noncoverage nextest runs. The ten-minute ceiling still bounds the coverage test +run. The four-vCPU `coverage-upload` lane remains on its measured shape; the +smaller runner has not been sampled for coverage generation. Free disk is the margin to watch: 3,998 MiB at its lowest on two vCPU. @@ -1023,10 +1032,10 @@ The Loom, verification, and Whitaker commands keep their own `RUSTFLAGS`, as the development and release recipes to the standard, and `build_standard_ci_test.py` holds the CI install order. The coverage lane (`coverage-main.yml:coverage-upload`) installs `mold` through `setup-rust`'s -`install-mold` input before `generate-coverage`, because the trybuild cases -compile scratch crates under `target/` that link with `mold` through the Cargo -configuration. The same file requires every Linux job that runs -`generate-coverage` to install `mold` first, by that input or by `apt`. +`install-mold` input before `generate-coverage`, because `.cargo/config.toml` +selects `mold` for Linux builds, including the instrumented runtime tests. The +same file requires every Linux job that runs `generate-coverage` to install +`mold` first, by that input or by `apt`. The fragment configures the backend only and declares no `[target.<...>]` table, so a direct `--config tools/dev-fast/config.toml` invocation behaves the @@ -1199,6 +1208,9 @@ tracing and the current-thread Tokio runtime explicitly, then use `WireframeServer::from_app` for server preparation, listener binding, connection spawning, and shutdown-aware accept loops. Keep example-specific address parsing, app construction, handlers, and middleware in the example file. +`tracing-subscriber` and the Prometheus exporter are development dependencies +for examples, tests, and doctests; applications supply their own tracing +subscriber and exporter. Codec benchmark helpers live in `wireframe_testing::codec_benchmarks`. Bench targets, direct unit tests, and BDD fixtures should import the workload matrix, @@ -1459,8 +1471,8 @@ Place new snippets in `tests/ui/`, register them in `tests/compile_error.rs`, and commit the generated `.stderr` file for compile-fail cases after verifying that the diagnostics describe the intended contract. These files are exact `rustc` diagnostic snapshots: generate and refresh them with the repository's -pinned `rust-toolchain.toml`. Normal validation is -`cargo test --test compile_error`; a deliberate developer-only refresh uses +pinned `rust-toolchain.toml`. Normal validation is `make test-ui`; a deliberate +developer-only refresh uses `TRYBUILD=overwrite cargo test --test compile_error`. Review refreshed fixtures as maintenance changes, and never run the overwrite command in CI. diff --git a/tests/compile_error.rs b/tests/compile_error.rs index 2063856c8..b941f4aab 100644 --- a/tests/compile_error.rs +++ b/tests/compile_error.rs @@ -1,5 +1,7 @@ //! Compile-time trait-bound and public-default tests. +#![cfg(not(coverage))] + /// Check public type contracts and reject a zero fixture budget at compile time. #[test] fn compile_tests() { diff --git a/tests/workflow_contracts/ci_runner_placement_test.py b/tests/workflow_contracts/ci_runner_placement_test.py index 302ea8f3f..2419c8412 100644 --- a/tests/workflow_contracts/ci_runner_placement_test.py +++ b/tests/workflow_contracts/ci_runner_placement_test.py @@ -58,9 +58,9 @@ of them. Without it, a new GitHub-hosted job added to the placement table with no ceiling entry passes every ceiling test, because the per-coordinate test iterates the ceiling table and the tree walk reads only ``ubicloud-`` -lanes; and a second ``None`` placement value escapes both the label test, -which skips ``None``, and the expression tests, which look only at the one -fork-fallback coordinate. +lanes; and an unreviewed ``None`` placement value escapes both the label test, +which skips ``None``, and the expression tests, which cover only the reviewed +fork-fallback coordinates. Mutation proof; each applied alone and reverted: @@ -85,7 +85,7 @@ - removing the ``advanced`` entry from ``EXPECTED_CEILING_MINUTES`` fails ``test_the_reviewed_tables_are_total`` alone, and nothing else: that is the gap the totality test was added to close; -- giving a second coordinate the ``None`` placement value fails +- giving an unreviewed coordinate the ``None`` placement value fails ``test_the_reviewed_tables_are_total`` alone, for the same reason; - deleting ``timeout-minutes`` from ``coverage-upload`` fails ``test_the_job_declares_its_reviewed_ceiling`` and @@ -108,19 +108,6 @@ lane that has not been sampled on two vCPU cannot follow ``build-test`` unnoticed. -A ninth is not a mutation but a real event, recorded because it is better -evidence than a contrived one. Moving both Ubicloud lanes from -``ubicloud-standard-2`` to ``ubicloud-standard-4`` after the smaller shape -killed a test failed ``test_the_job_carries_its_reviewed_label``, -``test_the_runner_expression_is_exactly_the_reviewed_one`` and -``test_the_actionlint_registry_matches_the_labels_in_use`` the moment the -label changed and before the registry was updated, unprompted. -``test_every_ubicloud_job_has_a_ceiling`` stayed green throughout, because it -tests the ``ubicloud-`` prefix rather than a literal label; written against a -literal it would have silently stopped noticing an unbounded lane at exactly -that moment, which is a dead-guard defect repovec-appliance found in its own -placement contract. - Run via ``make test-workflow-contracts``. """ @@ -134,7 +121,7 @@ EXPECTED_CEILING_MINUTES, EXPECTED_PLACEMENT, FOREIGN_RUNNER_FRAGMENTS, - FORK_FALLBACK_JOB, + FORK_FALLBACK_JOBS, GITHUB_HOSTED_LABEL, GITHUB_HOSTED_LABELS, HOSTED_GUARD, @@ -187,19 +174,17 @@ def test_the_reviewed_tables_are_total() -> None: ``ubicloud-`` lanes, so a new GitHub-hosted lane would inherit the six-hour default unremarked. - A second coordinate carrying the ``None`` placement value escapes the - label test, which skips ``None``, and the expression tests, which read - only ``FORK_FALLBACK_JOB``. Its runner would then be checked by nothing - at all. + A coordinate carrying ``None`` must have an expression assertion, since + the label test skips it. """ expression_jobs = { coordinate for coordinate, label in EXPECTED_PLACEMENT.items() if label is None } - assert expression_jobs == {FORK_FALLBACK_JOB}, ( - "only the fork-fallback lane may be checked by expression rather than " - f"by label; {sorted(expression_jobs - {FORK_FALLBACK_JOB})} would have " + assert expression_jobs == FORK_FALLBACK_JOBS, ( + "only reviewed fallback lanes may be checked by expression rather than " + f"by label; {sorted(expression_jobs - FORK_FALLBACK_JOBS)} would have " "no runner assertion at all" ) needing_ceilings = set(EXPECTED_PLACEMENT) - {CEILING_FREE_JOB} @@ -236,7 +221,8 @@ def test_the_job_carries_its_reviewed_label( ) -def test_the_runner_expression_parses_to_one_line() -> None: +@pytest.mark.parametrize("coordinate", sorted(FORK_FALLBACK_JOBS), ids=case_id) +def test_the_runner_expression_parses_to_one_line(coordinate: tuple[str, str]) -> None: """Scenario: the folded scalar's continuation is indented one level deeper. Invariant: the parsed value is a single line. A more-indented @@ -244,19 +230,20 @@ def test_the_runner_expression_parses_to_one_line() -> None: expression. GitHub evaluates the broken value and the job runs, so a green run is not evidence; only the parsed value shows it. """ - value = runner_value(jobs()[FORK_FALLBACK_JOB]) + value = runner_value(jobs()[coordinate]) assert isinstance(value, str), ( - f"{FORK_FALLBACK_JOB[0]}:{FORK_FALLBACK_JOB[1]} should declare runs-on " + f"{coordinate[0]}:{coordinate[1]} should declare runs-on " f"as one scalar, got {value!r}" ) assert "\n" not in value, ( - f"{FORK_FALLBACK_JOB[0]}:{FORK_FALLBACK_JOB[1]} has a line break inside " + f"{coordinate[0]}:{coordinate[1]} has a line break inside " f"its runs-on expression ({value!r}); the folded scalar's continuation " "line must sit at the same indent as the line above it" ) -def test_the_runner_expression_is_exactly_the_reviewed_one() -> None: +@pytest.mark.parametrize("coordinate", sorted(FORK_FALLBACK_JOBS), ids=case_id) +def test_the_runner_expression_is_exactly_the_reviewed_one(coordinate: tuple[str, str]) -> None: """Scenario: the fork guard is swapped for a plausible sibling field. Invariant: the guard and both arms equal the reviewed strings. Matching @@ -265,10 +252,10 @@ def test_the_runner_expression_is_exactly_the_reviewed_one() -> None: exactly the same form that sends every fork pull request to a runner it cannot obtain, and every other pull request to the wrong place. """ - value = str(runner_value(jobs()[FORK_FALLBACK_JOB])) + value = str(runner_value(jobs()[coordinate])) match = RUNNER_EXPRESSION.match(value) assert match is not None, ( - f"{FORK_FALLBACK_JOB[0]}:{FORK_FALLBACK_JOB[1]} should declare the " + f"{coordinate[0]}:{coordinate[1]} should declare the " f"reviewed fork fallback expression; got {value!r}" ) assert match["guard"] == HOSTED_GUARD, ( diff --git a/tests/workflow_contracts/dependency_scoping_test.py b/tests/workflow_contracts/dependency_scoping_test.py new file mode 100644 index 000000000..12d544a90 --- /dev/null +++ b/tests/workflow_contracts/dependency_scoping_test.py @@ -0,0 +1,64 @@ +"""Protect the development-only scope of test and doctest dependencies.""" + +from __future__ import annotations + +import tomllib +import typing as typ +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[2] + + +@pytest.fixture +def manifest() -> dict[str, typ.Any]: + """Read Cargo's dependency tables as structured data.""" + with (ROOT / "Cargo.toml").open("rb") as cargo_file: + return tomllib.load(cargo_file) + + +@pytest.mark.parametrize("name", ["tracing-subscriber", "metrics-exporter-prometheus", "itoa"]) +def test_test_dependencies_are_not_runtime_dependencies(manifest: dict[str, typ.Any], name: str) -> None: + """Test and doctest helpers must not enter the published runtime graph.""" + assert name not in manifest["dependencies"] + for target in manifest.get("target", {}).values(): + assert name not in target.get("dependencies", {}) + + +def test_tracing_subscriber_remains_available_to_tests(manifest: dict[str, typ.Any]) -> None: + """Examples, benchmarks and tests retain their tracing subscriber.""" + subscriber = manifest["dev-dependencies"]["tracing-subscriber"] + assert "env-filter" in subscriber["features"] + + +def test_prometheus_exporter_remains_available_to_non_loom_doctests(manifest: dict[str, typ.Any]) -> None: + """The documented integration uses a development-only HTTP exporter.""" + exporter = manifest["target"]["cfg(not(loom))"]["dev-dependencies"][ + "metrics-exporter-prometheus" + ] + assert "http-listener" in exporter["features"] + assert not exporter.get("optional", False) + assert "use metrics_exporter_prometheus::PrometheusBuilder;" in ( + ROOT / "src" / "metrics.rs" + ).read_text() + + +def test_metrics_feature_does_not_enable_exporter(manifest: dict[str, typ.Any]) -> None: + """Runtime metrics emit through the facade without installing an exporter.""" + assert manifest["features"]["metrics"] == ["dep:metrics"] + + +def test_request_stream_reader_enables_tokio_util_io_directly(manifest: dict[str, typ.Any]) -> None: + """Request streaming must not rely on exporter feature unification.""" + assert "io" in manifest["dependencies"]["tokio-util"]["features"] + assert "use tokio_util::io::StreamReader;" in ( + ROOT / "src" / "request" / "mod.rs" + ).read_text() + + +def test_itoa_remains_available_to_resp_example(manifest: dict[str, typ.Any]) -> None: + """The RESP example can format integers without a runtime dependency.""" + assert "itoa" in manifest["dev-dependencies"] + example = ROOT / "examples" / "resp_codec_impl" / "encode.rs" + assert "itoa::Buffer::new()" in example.read_text() diff --git a/tests/workflow_contracts/nextest_timeout_test.py b/tests/workflow_contracts/nextest_timeout_test.py index 0d2367089..5103e3df5 100644 --- a/tests/workflow_contracts/nextest_timeout_test.py +++ b/tests/workflow_contracts/nextest_timeout_test.py @@ -138,3 +138,146 @@ def test_only_the_aggregate_compile_test_gets_the_extended_budget() -> None: "grace-period": "5s", } assert 60 * override["slow-timeout"]["terminate-after"] < 10 * 60 + + +def assert_ui_job_is_mandatory(job: dict[str, typ.Any]) -> None: + """Require the UI job to run the target after Rust and linker setup.""" + assert "if" not in job, "UI job must run for every CI event" + assert not job.get("continue-on-error"), "UI job must fail CI on errors" + steps = job["steps"] + ui_steps = [step for step in steps if step.get("run") == "make test-ui"] + assert len(ui_steps) == 1, "UI job must run make test-ui exactly once" + ui = ui_steps[0] + assert "if" not in ui and not ui.get("continue-on-error"), ( + "UI step must run and propagate failures" + ) + setup = [ + step for step in steps + if str(step.get("uses", "")).startswith( + "leynos/shared-actions/.github/actions/setup-rust@" + ) + ] + assert len(setup) == 1, "UI job must set up Rust" + assert "if" not in setup[0] and not setup[0].get("continue-on-error") + assert str(setup[0].get("with", {}).get("install-mold")).lower() == "true" + assert steps.index(setup[0]) < steps.index(ui), "setup must precede UI tests" + for scope in (job, ui, setup[0]): + env = scope.get("env") or {} + flags = " ".join(str(env.get(key, "")) for key in ( + "RUSTFLAGS", "CARGO_ENCODED_RUSTFLAGS" + )) + assert "coverage" not in flags, "UI job must not enable coverage" + + +def test_ui_job_runs_without_suppression_for_all_ci_events() -> None: + """CI retains a separate mandatory, non-instrumented UI gate.""" + workflow = typ.cast(dict[str, typ.Any], workflows()["ci.yml"]) + events = workflow.get(True, workflow.get("on")) + assert "pull_request" in events and "push" in events + workflow_flags = workflow.get("env") or {} + assert "coverage" not in str(workflow_flags.get("RUSTFLAGS", "")) + assert "coverage" not in str(workflow_flags.get("CARGO_ENCODED_RUSTFLAGS", "")) + assert "main" in events["push"]["branches"] + assert_ui_job_is_mandatory(workflow["jobs"]["ui-test"]) + + +def assert_ui_blocks_required_status(job: dict[str, typ.Any]) -> None: + """The required build status depends on successful UI validation.""" + needs = job.get("needs", []) + if isinstance(needs, str): + needs = [needs] + assert "ui-test" in needs, "build-test must depend on ui-test" + assert "if" not in job, "build-test must not bypass failed dependencies" + + +def test_ui_failure_blocks_required_build_status() -> None: + """A failed UI job prevents the required build status succeeding.""" + workflow = typ.cast(dict[str, typ.Any], workflows()["ci.yml"]) + assert_ui_blocks_required_status(workflow["jobs"]["build-test"]) + + +@pytest.mark.parametrize("mutation", ["missing_dependency", "always_run"]) +def test_required_build_status_cannot_bypass_ui_failure(mutation: str) -> None: + """Missing needs or an unconditional job expression must be rejected.""" + from copy import deepcopy + + workflow = typ.cast(dict[str, typ.Any], workflows()["ci.yml"]) + job = deepcopy(workflow["jobs"]["build-test"]) + if mutation == "missing_dependency": + job.pop("needs") + else: + job["if"] = "always()" + with pytest.raises(AssertionError): + assert_ui_blocks_required_status(job) + + +@pytest.mark.parametrize("mutation", ["conditional", "suppressed", "missing", "late_setup"]) +def test_ui_job_contract_rejects_missing_or_ineffective_gate(mutation: str) -> None: + """The gate contract detects realistic ways UI validation could be skipped.""" + from copy import deepcopy + + workflow = typ.cast(dict[str, typ.Any], workflows()["ci.yml"]) + job = deepcopy(workflow["jobs"]["ui-test"]) + ui = next(step for step in job["steps"] if step.get("run") == "make test-ui") + if mutation == "conditional": + job["if"] = "false" + elif mutation == "suppressed": + ui["continue-on-error"] = True + elif mutation == "missing": + job["steps"].remove(ui) + else: + job["steps"].remove(ui) + job["steps"].insert(0, ui) + with pytest.raises(AssertionError): + assert_ui_job_is_mandatory(job) + + +UI_HARNESSES = frozenset({ + REPO_ROOT / "tests" / "compile_error.rs", + REPO_ROOT / "wireframe_testing" / "tests" / "loom_api_surface.rs", +}) + + +def assert_coverage_guard_scope(guarded: set[Path]) -> None: + """Keep coverage exclusion restricted to both compiler-driving tests.""" + assert guarded == UI_HARNESSES, "only the two UI harnesses need coverage guards" + + +def test_coverage_guards_only_compiler_driving_harnesses() -> None: + """Coverage excludes nested compiler runs while ordinary UI tests remain.""" + guarded = { + path + for root in (REPO_ROOT / "tests", REPO_ROOT / "wireframe_testing" / "tests") + for path in root.rglob("*.rs") + if "#![cfg(not(coverage))]" in path.read_text() + } + assert_coverage_guard_scope(guarded) + loom_harness = REPO_ROOT / "wireframe_testing" / "tests" / "loom_api_surface.rs" + assert "#![cfg(loom)]" in loom_harness.read_text() + manifest = tomllib.loads((REPO_ROOT / "Cargo.toml").read_text()) + checks = manifest["workspace"]["lints"]["rust"]["unexpected_cfgs"]["check-cfg"] + assert "cfg(loom)" in checks and "cfg(coverage)" in checks + + +def test_ui_target_keeps_both_harnesses() -> None: + """The non-instrumented gate runs root and Loom compile-time suites.""" + makefile = (REPO_ROOT / "Makefile").read_text() + target = makefile.split("\ntest-ui:", 1)[1].split("\n\n", 1)[0] + assert "test --test compile_error" in target + assert 'CARGO_ENCODED_RUSTFLAGS="--cfg=loom"' in target + assert ( + "test -p wireframe_testing --test loom_api_surface --features runtime-benchmarks" + in target + ) + + +@pytest.mark.parametrize("mutation", ["missing_guard", "unrelated_test"]) +def test_coverage_guard_scope_rejects_missing_or_extra_guards(mutation: str) -> None: + """A missing UI guard or an excluded runtime test violates the scope.""" + guarded = set(UI_HARNESSES) + if mutation == "missing_guard": + guarded.remove(REPO_ROOT / "tests" / "compile_error.rs") + else: + guarded.add(REPO_ROOT / "tests" / "request_handler.rs") + with pytest.raises(AssertionError): + assert_coverage_guard_scope(guarded) diff --git a/tests/workflow_contracts/runner_placement_policy.py b/tests/workflow_contracts/runner_placement_policy.py index af91098ee..3ab7f0efb 100644 --- a/tests/workflow_contracts/runner_placement_policy.py +++ b/tests/workflow_contracts/runner_placement_policy.py @@ -52,16 +52,19 @@ #: The exact guard the fallback keys on: a fork or a Dependabot pull request. HOSTED_GUARD: typ.Final = f"({FORK_GUARD} || {DEPENDABOT_GUARD})" -#: The lane that carries the fork fallback: the only one serving pull -#: requests, and so the only one that can meet a fork. -FORK_FALLBACK_JOB: typ.Final = ("ci.yml", "build-test") +#: Both pull-request jobs require the same fallback for forks and Dependabot. +FORK_FALLBACK_JOBS: typ.Final = frozenset({ + ("ci.yml", "build-test"), + ("ci.yml", "ui-test"), +}) #: Every job this repository places, with its reviewed label. A literal label #: is written as itself; the fork-fallback lane is checked by its expression #: instead and appears here as ``None`` so the coordinate set stays total. -#: ``test_the_reviewed_tables_are_total`` holds it to exactly one ``None``. +#: ``test_the_reviewed_tables_are_total`` holds it to the reviewed fallback jobs. EXPECTED_PLACEMENT: typ.Final = { ("ci.yml", "build-test"): None, + ("ci.yml", "ui-test"): None, ("coverage-main.yml", "coverage-upload"): UBICLOUD_LABEL, ("advanced-tests.yml", "advanced"): GITHUB_HOSTED_LABEL, ("delayed-pr-comment.yml", "delay_and_comment"): GITHUB_HOSTED_LABEL, @@ -80,6 +83,7 @@ #: all. EXPECTED_CEILING_MINUTES: typ.Final = { ("ci.yml", "build-test"): 30, + ("ci.yml", "ui-test"): 30, ("coverage-main.yml", "coverage-upload"): 20, ("advanced-tests.yml", "advanced"): 60, } diff --git a/wireframe_testing/tests/loom_api_surface.rs b/wireframe_testing/tests/loom_api_surface.rs index 757738400..9a3420eb3 100644 --- a/wireframe_testing/tests/loom_api_surface.rs +++ b/wireframe_testing/tests/loom_api_surface.rs @@ -1,6 +1,7 @@ //! Check that network-backed test helpers stay outside the Loom API surface. #![cfg(loom)] +#![cfg(not(coverage))] #[test] fn loom_build_omits_non_loom_helpers() { diff --git a/wireframe_testing/tests/ui/loom_non_loom_exports.stderr b/wireframe_testing/tests/ui/loom_non_loom_exports.stderr index 48e035edb..41803716d 100644 --- a/wireframe_testing/tests/ui/loom_non_loom_exports.stderr +++ b/wireframe_testing/tests/ui/loom_non_loom_exports.stderr @@ -2,7 +2,9 @@ error[E0432]: unresolved import `wireframe_testing::client_pair` --> tests/ui/loom_non_loom_exports.rs:4:5 | 4 | use wireframe_testing::client_pair as _; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `client_pair` in the root + | ^^^^^^^^^^^^^^^^^^^-----------^^^^^ + | | + | no `client_pair` in the root | note: found an item that was configured out --> src/lib.rs @@ -16,7 +18,9 @@ error[E0432]: unresolved import `wireframe_testing::runtime_ownership_benchmarks --> tests/ui/loom_non_loom_exports.rs:5:5 | 5 | use wireframe_testing::runtime_ownership_benchmarks as _; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `runtime_ownership_benchmarks` in the root + | ^^^^^^^^^^^^^^^^^^^----------------------------^^^^^ + | | + | no `runtime_ownership_benchmarks` in the root | note: found an item that was configured out --> src/lib.rs @@ -30,7 +34,9 @@ error[E0432]: unresolved import `wireframe_testing::wait_for_listener_release` --> tests/ui/loom_non_loom_exports.rs:6:5 | 6 | use wireframe_testing::wait_for_listener_release as _; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `wait_for_listener_release` in the root + | ^^^^^^^^^^^^^^^^^^^-------------------------^^^^^ + | | + | no `wait_for_listener_release` in the root | note: found an item that was configured out --> src/lib.rs @@ -44,11 +50,17 @@ error[E0432]: unresolved imports `wireframe_testing::WireframePair`, `wireframe_ --> tests/ui/loom_non_loom_exports.rs:8:5 | 8 | WireframePair as _, - | ^^^^^^^^^^^^^^^^^^ no `WireframePair` in the root + | -------------^^^^^ + | | + | no `WireframePair` in the root 9 | spawn_wireframe_pair as _, - | ^^^^^^^^^^^^^^^^^^^^^^^^^ no `spawn_wireframe_pair` in the root + | --------------------^^^^^ + | | + | no `spawn_wireframe_pair` in the root 10 | spawn_wireframe_pair_default as _, - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `spawn_wireframe_pair_default` in the root + | ----------------------------^^^^^ + | | + | no `spawn_wireframe_pair_default` in the root | note: found an item that was configured out --> src/lib.rs From 4a7d4e6ed3943777f37e756ff266042e93f03645 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 27 Oct 2026 22:12:45 +0100 Subject: [PATCH 2/8] Make UI validation block the required check Run the bounded UI step inside the required CI job so a failure cannot turn that status into a passing skip. Pin checkout and prevent it from retaining credentials during compiler-driven tests. Strengthen workflow and dependency contracts and document application ownership of the metrics exporter. --- .github/workflows/ci.yml | 35 ++---- .github/workflows/coverage-main.yml | 2 +- docs/developers-guide.md | 36 +++--- docs/users-guide.md | 24 ++++ tests/workflow_contracts/ci_workflow_test.py | 10 +- .../dependency_scoping_test.py | 63 ++++++++-- .../nextest_timeout_test.py | 113 ++++++++++-------- .../runner_placement_policy.py | 13 +- 8 files changed, 184 insertions(+), 112 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 06fdc387e..58aa99a42 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,8 +19,6 @@ concurrency: jobs: build-test: - # The existing required status must wait for the UI job to succeed. - needs: ui-test # The one developer-blocking lane, so the one that moves. It waited 461 s # for a GitHub-hosted runner on 2026-09-16 to do 556 s of work. # @@ -35,8 +33,8 @@ jobs: # https://github.com/leynos/wireframe/actions/runs/39038420830). That is # about 29% less per run for 3m40s more wall time. # - # Compiler-driving UI fixtures run in the dedicated `ui-test` job. The - # coverage lane measures runtime tests without a nested trybuild build. + # Compiler-driving UI fixtures have a separate bounded step. Runtime + # coverage does not start their nested trybuild compilation. # # Disk is the margin to watch: the minimum free space on two vCPU was # 3,998 MiB, against 78,392 MiB on four. @@ -60,14 +58,16 @@ jobs: ${{ (github.event.pull_request.head.repo.fork || github.event.pull_request.user.login == 'dependabot[bot]') && 'ubuntu-latest' || 'ubicloud-standard-2' }} - timeout-minutes: 30 + timeout-minutes: 60 permissions: contents: read env: CARGO_TERM_COLOR: always BUILD_PROFILE: debug steps: - - uses: actions/checkout@v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 + with: + persist-credentials: false - name: Setup Rust uses: leynos/shared-actions/.github/actions/setup-rust@8e6a325765ba382691d1b1c65fcccf1fc1d1b00c - name: Install development backend and linker @@ -78,6 +78,10 @@ jobs: sudo apt-get update sudo apt-get install -y mold mold --version + # This compiler-driven suite is bounded separately from runtime coverage. + - name: Validate compile-time APIs + timeout-minutes: 30 + run: make test-ui - name: Install mdtablefix # `make check-fmt` runs `mdtablefix --check --git`; 0.6.0 is the # first release with `--check` and `--git`. @@ -156,22 +160,3 @@ jobs: # The ratchet reads this report; nothing else does, so it is not # published as an artefact. publish-artefact: 'false' - - # Compile-time API checks require their own cold compiler build and are not - # meaningful runtime coverage. Keep them mandatory on both PRs and pushes. - ui-test: - runs-on: >- - ${{ (github.event.pull_request.head.repo.fork - || github.event.pull_request.user.login == 'dependabot[bot]') - && 'ubuntu-latest' || 'ubicloud-standard-2' }} - timeout-minutes: 30 - permissions: - contents: read - steps: - - uses: actions/checkout@v7 - - name: Setup Rust and linker - uses: leynos/shared-actions/.github/actions/setup-rust@8e6a325765ba382691d1b1c65fcccf1fc1d1b00c - with: - install-mold: 'true' - - name: Validate compile-time APIs - run: make test-ui diff --git a/.github/workflows/coverage-main.yml b/.github/workflows/coverage-main.yml index 21a4407df..0c61d80f8 100644 --- a/.github/workflows/coverage-main.yml +++ b/.github/workflows/coverage-main.yml @@ -33,7 +33,7 @@ jobs: # here and a constant guard reads as a decision nobody made. # # Four vCPU, and not measured at two: this lane runs the same runtime - # coverage as `ci.yml`; compile-time UI checks run in its separate job. + # coverage as `ci.yml`; compile-time UI checks run in a separate CI step. # This lane's own 240 s sample was taken on Namespace's shared profile, # which is four vCPU and # 16 GB, and it has not been sampled on two. It runs on merge pushes diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 444493bfc..65143abaa 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -11,16 +11,18 @@ than all four repository-owned lanes sharing one profile. | Workflow | Job | Trigger | Runner | Ceiling | | ------------------------ | ------------------- | ------------------ | --------------------- | ------- | -| `ci.yml` | `build-test` | pull request, push | `ubicloud-standard-2` | 30 min | -| `ci.yml` | `ui-test` | pull request, push | `ubicloud-standard-2` | 30 min | +| `ci.yml` | `build-test` | pull request, push | `ubicloud-standard-2` | 60 min | | `coverage-main.yml` | `coverage-upload` | push | `ubicloud-standard-4` | 20 min | | `advanced-tests.yml` | `advanced` | schedule | `ubuntu-latest` | 60 min | | `delayed-pr-comment.yml` | `delay_and_comment` | dispatch | `ubuntu-latest` | none | *Table 1: Where each repository-owned lane runs, and its ceiling.* -`build-test` depends on `ui-test`, so UI failures block the existing required -`build-test` status. +The required `build-test` job includes an unconditional, bounded 30-minute +`make test-ui` step after Rust and linker setup and before runtime coverage. +This keeps compile-time API validation within the existing required status, +without a job-level condition that could leave branch protection without a +failure result. Pull-request, push and tag lanes move to Ubicloud, because those are the ones a developer waits on: `build-test` waited 461 seconds for a GitHub-hosted runner @@ -92,11 +94,12 @@ far inside the 7.9 GiB that shape has. The coverage action supplies a 180-second per-test deadline and a ten-minute whole-run ceiling. `tests/compile_error.rs` runs trybuild fixtures inside one -test, including nested Cargo builds, so it is not useful runtime coverage and -can exceed that per-test deadline on a cold two-vCPU runner. The `ui-test` job -runs `make test-ui` separately for pull requests and pushes to `main`, with a -30-minute job ceiling. This keeps the compile-time API fixtures covered without -making the instrumented runtime-coverage run compile them. +test, including nested Cargo builds, which do not measure runtime behaviour and +can exceed that per-test deadline on a cold two-vCPU runner. An unconditional +`make test-ui` step runs in `build-test` for pull requests and pushes to +`main`, before runtime coverage, with a separate 30-minute step ceiling. This +keeps the compile-time API fixtures covered without making the instrumented +runtime- coverage run compile them. The compile-time harnesses are excluded only when `cfg(coverage)` is active; the fixtures remain in `tests/ui/`, and `make test-ui` runs both the root and @@ -121,12 +124,15 @@ default is the expensive failure mode. A ceiling close to the measured work is the other one, because it cancels the run at the moment an overrun becomes interesting and discards the log that would explain it. -Two ceilings are measured: `build-test` at 556 seconds on four vCPU (750 -seconds on two, still well inside 30 minutes) and `coverage-upload` at 240 -seconds on four vCPU. Two are judgements, and the guide says so rather than -implying otherwise. `advanced` has failed every night since at least 2026-09-09 -and its last green run was 2025-10-04 at 32 seconds, so 60 minutes is generous -enough not to mask the repair when it lands. +The historical `build-test` sample was 556 seconds on four vCPU and 750 seconds +on two; the `coverage-upload` sample was 240 seconds on four vCPU. The 60-minute +`build-test` job ceiling is a judgement, not a measured limit: it allows the +existing lane and its new bounded UI step to finish, including cold compiler +work. The UI step's 30-minute ceiling is also a judgement. The four-vCPU +`coverage-upload` lane remains capped at 20 minutes. `advanced` has failed +every night since at least 2026-09-09 and its last green run was 2025-10-04 at +32 seconds, so 60 minutes is generous enough not to mask the repair when it +lands. `delay_and_comment` declares no ceiling, deliberately. Its entire duration is a `sleep` of the caller's `delay_minutes` input, so any fixed ceiling cancels a diff --git a/docs/users-guide.md b/docs/users-guide.md index d6ab4d955..7e6c04f31 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1568,6 +1568,30 @@ worker tasks.[^20][^37][^38] `ServerError` surfaces bind, factory-build, and application-preparation failures as typed errors so callers can react appropriately.[^21] +## Metrics + +Wireframe's default `metrics` feature emits instrumentation through the +`metrics` facade but does not install a global recorder. The Prometheus +exporter is a development dependency of Wireframe, so applications that want +Prometheus metrics must add `metrics-exporter-prometheus` as a direct +dependency and install its recorder once during startup, before Wireframe +begins emitting metrics: + +```rust,no_run +use metrics_exporter_prometheus::PrometheusBuilder; + +fn main() -> Result<(), Box> { + let prometheus = PrometheusBuilder::new().install_recorder()?; + + // Start the application after installing the recorder. + let _scrape_output = prometheus.render(); + Ok(()) +} +``` + +Keep the returned handle where the application can expose or render the +Prometheus output. Applications choose how to serve or otherwise collect it. + ## Client runtime `WireframeClient` provides a first-class client runtime that mirrors the diff --git a/tests/workflow_contracts/ci_workflow_test.py b/tests/workflow_contracts/ci_workflow_test.py index b21dbe1e0..c1384a46d 100644 --- a/tests/workflow_contracts/ci_workflow_test.py +++ b/tests/workflow_contracts/ci_workflow_test.py @@ -208,8 +208,16 @@ def test_build_test_checks_out_at_the_default_depth() -> None: assert len(checkouts) == 1, ( f"build-test should check out exactly once; found {len(checkouts)}" ) - options = checkouts[0].get("with") or {} + checkout = checkouts[0] + uses = str(checkout.get("uses", "")) + assert re.fullmatch(r"actions/checkout@[0-9a-f]{40}", uses), ( + f"build-test must pin checkout to a full commit SHA, got {uses!r}" + ) + options = checkout.get("with") or {} assert isinstance(options, dict), "the checkout's with: must be a mapping" + assert options.get("persist-credentials") is False, ( + "build-test checkout must not persist credentials for test builds" + ) assert "fetch-depth" not in options, ( "build-test must check out at the default depth; it sets fetch-depth " f"{options.get('fetch-depth')!r}, and nothing in the lane reads history" diff --git a/tests/workflow_contracts/dependency_scoping_test.py b/tests/workflow_contracts/dependency_scoping_test.py index 12d544a90..1fd183679 100644 --- a/tests/workflow_contracts/dependency_scoping_test.py +++ b/tests/workflow_contracts/dependency_scoping_test.py @@ -1,4 +1,9 @@ -"""Protect the development-only scope of test and doctest dependencies.""" +"""Protect dependency scopes in the root and verification manifests. + +Check the root manifest against example, request-streaming and doctest source +uses, and ensure the verification crate retains its model checker without a +runtime dependency on the main crate. +""" from __future__ import annotations @@ -21,15 +26,21 @@ def manifest() -> dict[str, typ.Any]: @pytest.mark.parametrize("name", ["tracing-subscriber", "metrics-exporter-prometheus", "itoa"]) def test_test_dependencies_are_not_runtime_dependencies(manifest: dict[str, typ.Any], name: str) -> None: """Test and doctest helpers must not enter the published runtime graph.""" - assert name not in manifest["dependencies"] - for target in manifest.get("target", {}).values(): - assert name not in target.get("dependencies", {}) + assert name not in manifest["dependencies"], ( + f"{name} must not be a runtime dependency in Cargo.toml" + ) + for target_name, target in manifest.get("target", {}).items(): + assert name not in target.get("dependencies", {}), ( + f"{name} must not be a runtime dependency for {target_name}" + ) def test_tracing_subscriber_remains_available_to_tests(manifest: dict[str, typ.Any]) -> None: """Examples, benchmarks and tests retain their tracing subscriber.""" subscriber = manifest["dev-dependencies"]["tracing-subscriber"] - assert "env-filter" in subscriber["features"] + assert "env-filter" in subscriber["features"], ( + "tracing-subscriber must enable env-filter for benchmarks" + ) def test_prometheus_exporter_remains_available_to_non_loom_doctests(manifest: dict[str, typ.Any]) -> None: @@ -37,28 +48,54 @@ def test_prometheus_exporter_remains_available_to_non_loom_doctests(manifest: di exporter = manifest["target"]["cfg(not(loom))"]["dev-dependencies"][ "metrics-exporter-prometheus" ] - assert "http-listener" in exporter["features"] - assert not exporter.get("optional", False) + assert "http-listener" in exporter["features"], ( + "metrics-exporter-prometheus must enable http-listener for doctests" + ) + assert not exporter.get("optional", False), ( + "metrics-exporter-prometheus must be available to non-Loom doctests" + ) assert "use metrics_exporter_prometheus::PrometheusBuilder;" in ( ROOT / "src" / "metrics.rs" - ).read_text() + ).read_text(), "src/metrics.rs must retain the documented Prometheus example" def test_metrics_feature_does_not_enable_exporter(manifest: dict[str, typ.Any]) -> None: """Runtime metrics emit through the facade without installing an exporter.""" - assert manifest["features"]["metrics"] == ["dep:metrics"] + assert manifest["features"]["metrics"] == ["dep:metrics"], ( + "the metrics feature must enable only the runtime metrics facade" + ) def test_request_stream_reader_enables_tokio_util_io_directly(manifest: dict[str, typ.Any]) -> None: """Request streaming must not rely on exporter feature unification.""" - assert "io" in manifest["dependencies"]["tokio-util"]["features"] + assert "io" in manifest["dependencies"]["tokio-util"]["features"], ( + "tokio-util must enable io directly for request streaming" + ) assert "use tokio_util::io::StreamReader;" in ( ROOT / "src" / "request" / "mod.rs" - ).read_text() + ).read_text(), "src/request/mod.rs must retain its StreamReader import" def test_itoa_remains_available_to_resp_example(manifest: dict[str, typ.Any]) -> None: """The RESP example can format integers without a runtime dependency.""" - assert "itoa" in manifest["dev-dependencies"] + assert "itoa" in manifest["dev-dependencies"], ( + "itoa must remain a dev dependency for the RESP example" + ) example = ROOT / "examples" / "resp_codec_impl" / "encode.rs" - assert "itoa::Buffer::new()" in example.read_text() + assert "itoa::Buffer::new()" in example.read_text(), ( + "examples/resp_codec_impl/encode.rs must retain its itoa use" + ) + + +def test_verification_crate_retains_model_checker_without_wireframe() -> None: + """The model checker needs stateright but does not depend on wireframe.""" + path = ROOT / "crates" / "wireframe-verification" / "Cargo.toml" + with path.open("rb") as cargo_file: + verification = tomllib.load(cargo_file) + dependencies = verification["dependencies"] + assert "stateright" in dependencies, ( + "wireframe-verification must retain its stateright runtime dependency" + ) + assert "wireframe" not in dependencies, ( + "wireframe-verification must not depend on the unused wireframe crate" + ) diff --git a/tests/workflow_contracts/nextest_timeout_test.py b/tests/workflow_contracts/nextest_timeout_test.py index 5103e3df5..2554d43dd 100644 --- a/tests/workflow_contracts/nextest_timeout_test.py +++ b/tests/workflow_contracts/nextest_timeout_test.py @@ -140,37 +140,70 @@ def test_only_the_aggregate_compile_test_gets_the_extended_budget() -> None: assert 60 * override["slow-timeout"]["terminate-after"] < 10 * 60 -def assert_ui_job_is_mandatory(job: dict[str, typ.Any]) -> None: - """Require the UI job to run the target after Rust and linker setup.""" - assert "if" not in job, "UI job must run for every CI event" - assert not job.get("continue-on-error"), "UI job must fail CI on errors" +def assert_ui_step_is_mandatory(job: dict[str, typ.Any]) -> None: + """Require bounded UI tests after Rust and linker setup in the required job.""" + assert "if" not in job, "build-test must run for every CI event" + assert not job.get("continue-on-error"), "build-test must propagate failures" + assert "needs" not in job, "build-test must run without a skipped prerequisite" + assert job.get("timeout-minutes") == 60, "build-test needs the reviewed budget" steps = job["steps"] - ui_steps = [step for step in steps if step.get("run") == "make test-ui"] - assert len(ui_steps) == 1, "UI job must run make test-ui exactly once" + ui_steps = [step for step in steps if step.get("name") == "Validate compile-time APIs"] + assert len(ui_steps) == 1, "build-test must contain exactly one UI step" ui = ui_steps[0] + assert ui.get("run") == "make test-ui", "UI step must run the complete target" + assert ui.get("timeout-minutes") == 30, "UI step must have a finite budget" assert "if" not in ui and not ui.get("continue-on-error"), ( "UI step must run and propagate failures" ) + assert_ui_setup_precedes(steps, ui) + assert_no_coverage_flags(job, ui) + + +def assert_ui_setup_precedes(steps: list[dict[str, typ.Any]], ui: dict[str, typ.Any]) -> None: + """Require Rust and linker setup before the non-instrumented UI step.""" setup = [ step for step in steps if str(step.get("uses", "")).startswith( "leynos/shared-actions/.github/actions/setup-rust@" ) ] - assert len(setup) == 1, "UI job must set up Rust" - assert "if" not in setup[0] and not setup[0].get("continue-on-error") - assert str(setup[0].get("with", {}).get("install-mold")).lower() == "true" - assert steps.index(setup[0]) < steps.index(ui), "setup must precede UI tests" - for scope in (job, ui, setup[0]): + assert len(setup) == 1, "build-test must set up Rust" + assert "if" not in setup[0] and not setup[0].get("continue-on-error"), ( + "Rust setup must run and propagate failures" + ) + linker = [ + step for step in steps + if step.get("name") == "Install development backend and linker" + ] + assert len(linker) == 1, "build-test must install the Linux linker" + assert "if" not in linker[0] and not linker[0].get("continue-on-error"), ( + "linker setup must run and propagate failures" + ) + assert steps.index(setup[0]) < steps.index(linker[0]) < steps.index(ui), ( + "Rust and linker setup must precede UI tests" + ) + assert_no_coverage_flags(setup[0], linker[0]) + + +def assert_no_coverage_flags(*scopes: dict[str, typ.Any]) -> None: + """Keep coverage instrumentation out of the UI job and its setup.""" + for scope in scopes: env = scope.get("env") or {} flags = " ".join(str(env.get(key, "")) for key in ( "RUSTFLAGS", "CARGO_ENCODED_RUSTFLAGS" )) - assert "coverage" not in flags, "UI job must not enable coverage" + assert "coverage" not in flags, "UI step must not enable coverage" + + +def assert_ui_permissions(job: dict[str, typ.Any]) -> None: + """Restrict the required UI job to read-only repository contents.""" + assert job.get("permissions") == {"contents": "read"}, ( + "build-test must declare exactly contents: read permissions" + ) -def test_ui_job_runs_without_suppression_for_all_ci_events() -> None: - """CI retains a separate mandatory, non-instrumented UI gate.""" +def test_ui_step_runs_without_suppression_for_all_ci_events() -> None: + """The required CI job runs a bounded non-instrumented UI gate.""" workflow = typ.cast(dict[str, typ.Any], workflows()["ci.yml"]) events = workflow.get(True, workflow.get("on")) assert "pull_request" in events and "push" in events @@ -178,58 +211,40 @@ def test_ui_job_runs_without_suppression_for_all_ci_events() -> None: assert "coverage" not in str(workflow_flags.get("RUSTFLAGS", "")) assert "coverage" not in str(workflow_flags.get("CARGO_ENCODED_RUSTFLAGS", "")) assert "main" in events["push"]["branches"] - assert_ui_job_is_mandatory(workflow["jobs"]["ui-test"]) + assert_ui_step_is_mandatory(workflow["jobs"]["build-test"]) + assert_ui_permissions(workflow["jobs"]["build-test"]) -def assert_ui_blocks_required_status(job: dict[str, typ.Any]) -> None: - """The required build status depends on successful UI validation.""" - needs = job.get("needs", []) - if isinstance(needs, str): - needs = [needs] - assert "ui-test" in needs, "build-test must depend on ui-test" - assert "if" not in job, "build-test must not bypass failed dependencies" +@pytest.mark.parametrize("permissions", [None, {"contents": "write"}, {"contents": "read", "actions": "read"}]) +def test_ui_permissions_reject_missing_or_broader_access(permissions: object) -> None: + """The required UI gate must not omit or broaden its token permissions.""" + with pytest.raises(AssertionError, match="contents: read"): + assert_ui_permissions({"permissions": permissions}) -def test_ui_failure_blocks_required_build_status() -> None: - """A failed UI job prevents the required build status succeeding.""" - workflow = typ.cast(dict[str, typ.Any], workflows()["ci.yml"]) - assert_ui_blocks_required_status(workflow["jobs"]["build-test"]) - - -@pytest.mark.parametrize("mutation", ["missing_dependency", "always_run"]) -def test_required_build_status_cannot_bypass_ui_failure(mutation: str) -> None: - """Missing needs or an unconditional job expression must be rejected.""" +@pytest.mark.parametrize( + "mutation", ["conditional", "suppressed", "missing", "wrong_order", "no_timeout"] +) +def test_ui_step_contract_rejects_missing_or_ineffective_gate(mutation: str) -> None: + """Reject skipped, suppressed, missing, unbounded or prematurely run UI tests.""" from copy import deepcopy workflow = typ.cast(dict[str, typ.Any], workflows()["ci.yml"]) job = deepcopy(workflow["jobs"]["build-test"]) - if mutation == "missing_dependency": - job.pop("needs") - else: - job["if"] = "always()" - with pytest.raises(AssertionError): - assert_ui_blocks_required_status(job) - - -@pytest.mark.parametrize("mutation", ["conditional", "suppressed", "missing", "late_setup"]) -def test_ui_job_contract_rejects_missing_or_ineffective_gate(mutation: str) -> None: - """The gate contract detects realistic ways UI validation could be skipped.""" - from copy import deepcopy - - workflow = typ.cast(dict[str, typ.Any], workflows()["ci.yml"]) - job = deepcopy(workflow["jobs"]["ui-test"]) - ui = next(step for step in job["steps"] if step.get("run") == "make test-ui") + ui = next(step for step in job["steps"] if step.get("name") == "Validate compile-time APIs") if mutation == "conditional": - job["if"] = "false" + ui["if"] = "false" elif mutation == "suppressed": ui["continue-on-error"] = True elif mutation == "missing": job["steps"].remove(ui) + elif mutation == "no_timeout": + ui.pop("timeout-minutes") else: job["steps"].remove(ui) job["steps"].insert(0, ui) with pytest.raises(AssertionError): - assert_ui_job_is_mandatory(job) + assert_ui_step_is_mandatory(job) UI_HARNESSES = frozenset({ diff --git a/tests/workflow_contracts/runner_placement_policy.py b/tests/workflow_contracts/runner_placement_policy.py index 3ab7f0efb..054654d7e 100644 --- a/tests/workflow_contracts/runner_placement_policy.py +++ b/tests/workflow_contracts/runner_placement_policy.py @@ -52,10 +52,9 @@ #: The exact guard the fallback keys on: a fork or a Dependabot pull request. HOSTED_GUARD: typ.Final = f"({FORK_GUARD} || {DEPENDABOT_GUARD})" -#: Both pull-request jobs require the same fallback for forks and Dependabot. +#: The pull-request job needs a fallback for forks and Dependabot. FORK_FALLBACK_JOBS: typ.Final = frozenset({ ("ci.yml", "build-test"), - ("ci.yml", "ui-test"), }) #: Every job this repository places, with its reviewed label. A literal label @@ -64,7 +63,6 @@ #: ``test_the_reviewed_tables_are_total`` holds it to the reviewed fallback jobs. EXPECTED_PLACEMENT: typ.Final = { ("ci.yml", "build-test"): None, - ("ci.yml", "ui-test"): None, ("coverage-main.yml", "coverage-upload"): UBICLOUD_LABEL, ("advanced-tests.yml", "advanced"): GITHUB_HOSTED_LABEL, ("delayed-pr-comment.yml", "delay_and_comment"): GITHUB_HOSTED_LABEL, @@ -73,17 +71,16 @@ #: Ceilings, pinned by value rather than bounded, because a ceiling can drift #: to a number nobody chose while every inequality still holds. #: -#: ``build-test`` at 30 and ``coverage-upload`` at 20 are measured, from 556 s -#: of work on four-vCPU runners (750 s on two vCPU, still inside 30 minutes) -#: and 240 s on four-vCPU runners. ``advanced`` at 60 and +#: ``build-test`` at 60 includes a bounded cold UI suite and coverage; the +#: previous CI run reached 28 minutes. ``coverage-upload`` +#: at 20 was measured at 240 s on four-vCPU runners. ``advanced`` at 60 and #: ``refresh-sha`` at 10 are judgements and are recorded as such in the #: developers' guide: ``advanced`` has failed every scheduled run since at #: least 2026-09-09 and its last green run was 2025-10-04 at 32 s, which a #: Loom suite has long since outgrown, and ``refresh-sha`` has never run at #: all. EXPECTED_CEILING_MINUTES: typ.Final = { - ("ci.yml", "build-test"): 30, - ("ci.yml", "ui-test"): 30, + ("ci.yml", "build-test"): 60, ("coverage-main.yml", "coverage-upload"): 20, ("advanced-tests.yml", "advanced"): 60, } From 5041aa6b2038363a119391fd4c2cf719f3455fcc Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 28 Oct 2026 02:16:04 +0100 Subject: [PATCH 3/8] Document Prometheus recorder upkeep The metrics section installed a recorder with install_recorder() and rendered once, which reads as a complete setup. It is not: unlike build() and install(), install_recorder() does not spawn the exporter's upkeep task, so histogram buckets and other self-maintaining structures only shrink when a render happens. Document the timer and show it in the example. Both facts were checked against the vendored metrics-exporter-prometheus 0.18.3 source: the run_upkeep doc comment states the task is not spawned for install_recorder, and PrometheusHandle is Clone so the loop can hold its own handle. The snippet was compiled and run as a temporary test before being transcribed into the guide. Co-Authored-By: Claude Code --- docs/users-guide.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/docs/users-guide.md b/docs/users-guide.md index 7e6c04f31..cc3dec7eb 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1578,11 +1578,22 @@ dependency and install its recorder once during startup, before Wireframe begins emitting metrics: ```rust,no_run +use std::time::Duration; + use metrics_exporter_prometheus::PrometheusBuilder; fn main() -> Result<(), Box> { let prometheus = PrometheusBuilder::new().install_recorder()?; + // Keep the handle: the upkeep task holds a clone of it. + let upkeep_handle = prometheus.clone(); + std::thread::spawn(move || { + loop { + std::thread::sleep(Duration::from_secs(5)); + upkeep_handle.run_upkeep(); + } + }); + // Start the application after installing the recorder. let _scrape_output = prometheus.render(); Ok(()) @@ -1592,6 +1603,15 @@ fn main() -> Result<(), Box> { Keep the returned handle where the application can expose or render the Prometheus output. Applications choose how to serve or otherwise collect it. +`install_recorder` returns a `PrometheusHandle` but does **not** start the +exporter's background upkeep task; only `PrometheusBuilder::build` and +`PrometheusBuilder::install` spawn it. The task drains histogram buckets and +other self-maintaining structures, so without it that storage grows for as long +as no scrape triggers a render. Call `PrometheusHandle::run_upkeep` on a timer +(for example, every five seconds, matching the exporter's own default +interval), or serve metrics through `PrometheusBuilder::build`/`install` and +let the exporter own the task. + ## Client runtime `WireframeClient` provides a first-class client runtime that mirrors the From 1ac6c2fe2018104c3eadece5bf67753d9856f553 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 28 Oct 2026 03:24:29 +0100 Subject: [PATCH 4/8] Guard both verification dependency tables The verification contract asserted only that `wireframe` was absent from `[dependencies]`, but the crate also has a `[dev-dependencies]` table. A dev-dependency edge pulls the main crate into the verification build just as a runtime edge does, so the guard left the case it exists to prevent half-covered. Check both tables through one named helper and add a mutation test per table, so removing either guard fails rather than silently narrowing the contract. Record the module's actual scope while here: `nextest_timeout_test.py` stopped being only the coverage-lane timeout policy once it took on the `build-test` UI gate, and its one-line docstring said otherwise. --- .../dependency_scoping_test.py | 39 +++++++++++++++---- .../nextest_timeout_test.py | 17 +++++++- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/tests/workflow_contracts/dependency_scoping_test.py b/tests/workflow_contracts/dependency_scoping_test.py index 1fd183679..a1a410ca4 100644 --- a/tests/workflow_contracts/dependency_scoping_test.py +++ b/tests/workflow_contracts/dependency_scoping_test.py @@ -87,15 +87,38 @@ def test_itoa_remains_available_to_resp_example(manifest: dict[str, typ.Any]) -> ) +VERIFICATION_DEPENDENCY_TABLES = ("dependencies", "dev-dependencies") + + +def assert_verification_has_no_wireframe_dependency( + verification: dict[str, typ.Any], +) -> None: + """Reject a wireframe edge in either verification dependency table. + + A dev-dependency edge pulls the main crate into the verification build just + as a runtime edge does, so both tables need the same guard. + """ + assert "stateright" in verification["dependencies"], ( + "wireframe-verification must retain its stateright runtime dependency" + ) + for table in VERIFICATION_DEPENDENCY_TABLES: + dependencies = verification.get(table, {}) + assert "wireframe" not in dependencies, ( + f"wireframe-verification must not depend on wireframe via {table}" + ) + + def test_verification_crate_retains_model_checker_without_wireframe() -> None: """The model checker needs stateright but does not depend on wireframe.""" path = ROOT / "crates" / "wireframe-verification" / "Cargo.toml" with path.open("rb") as cargo_file: - verification = tomllib.load(cargo_file) - dependencies = verification["dependencies"] - assert "stateright" in dependencies, ( - "wireframe-verification must retain its stateright runtime dependency" - ) - assert "wireframe" not in dependencies, ( - "wireframe-verification must not depend on the unused wireframe crate" - ) + assert_verification_has_no_wireframe_dependency(tomllib.load(cargo_file)) + + +@pytest.mark.parametrize("table", VERIFICATION_DEPENDENCY_TABLES) +def test_verification_guard_rejects_a_wireframe_edge(table: str) -> None: + """Either table admits the main crate only by defeating the guard.""" + verification: dict[str, typ.Any] = {"dependencies": {"stateright": "0.31.0"}} + verification.setdefault(table, {})["wireframe"] = {"path": "../.."} + with pytest.raises(AssertionError, match=table): + assert_verification_has_no_wireframe_dependency(verification) diff --git a/tests/workflow_contracts/nextest_timeout_test.py b/tests/workflow_contracts/nextest_timeout_test.py index 2554d43dd..127cf0131 100644 --- a/tests/workflow_contracts/nextest_timeout_test.py +++ b/tests/workflow_contracts/nextest_timeout_test.py @@ -1,4 +1,19 @@ -"""Contracts for the coverage lane's cargo-nextest timeout policy.""" +"""Contract for the bounded, non-instrumented compile-time UI gate. + +Two policies share this module because both describe when compiler-driven work +is allowed to run. The first half pins the coverage lane's `cargo-nextest` +timeouts: the aggregate `compile_error` test gets an eight-minute override +while the global ceiling stays at ten, so a hung nested compiler run cannot +consume the whole job. + +The second half guards the split between compile-time validation and runtime +coverage in `ci.yml`. `build-test` must run `make test-ui` unconditionally, +after Rust and linker setup and with a finite budget, and it must not carry +coverage instrumentation. The gate itself keeps both harnesses, +`tests/compile_error.rs` and `wireframe_testing/tests/loom_api_surface.rs`, +and coverage opts out of exactly those two nested compiler drivers rather than +the runtime suite around them. +""" from __future__ import annotations From 4f0d27dcdf8b68ace1041c7899c7295787c225fe Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 28 Oct 2026 03:24:33 +0100 Subject: [PATCH 5/8] Document the subscriber and UI-boundary prerequisites Three behaviours this branch introduced were only documented for one audience or not at all. `tracing-subscriber` moved from a runtime dependency to a development dependency, so a released application that relied on Wireframe installing a subscriber now silently discards every event. Say so in the users guide and give the migration guide the manifest line and initialisation step that fix it. The compile-time UI split is described in the developers guide, but the testing strategy still claims four layers and measures coverage with tarpaulin over the whole suite. Record the boundary as its own layer: which harnesses it uses, why it runs without instrumentation, and why coverage excludes only the two nested compiler drivers rather than the runtime suite. --- docs/multi-layered-testing-strategy.md | 26 +++++++++++++++++++++ docs/users-guide.md | 8 +++++++ docs/v0-3-0-to-v0-4-0-migration-guide.md | 29 ++++++++++++++++++++++++ 3 files changed, 63 insertions(+) diff --git a/docs/multi-layered-testing-strategy.md b/docs/multi-layered-testing-strategy.md index 2e811efbb..d96952538 100644 --- a/docs/multi-layered-testing-strategy.md +++ b/docs/multi-layered-testing-strategy.md @@ -20,6 +20,32 @@ generated `lcov.info` report to Codecov using a pinned version of the Codecov GitHub Action (`18283e04ce6e62d37312384ff67231eb8fd56d24`, corresponding to v5.4.3) to make coverage visible across pull requests. +## 1.1 Layer 0: Compile-Time API Boundary + +**Objective:** To verify that the public API rejects the code it is supposed to +reject, and does so with the diagnostic the documentation promises. Runtime +tests cannot observe this: an API that should fail to compile is, by +construction, absent from any test binary that does compile. + +Two harnesses cover the boundary. `tests/compile_error.rs` drives trybuild +fixtures in `tests/ui/` for the root crate, in both `pass` and `compile_fail` +modes. `wireframe_testing/tests/loom_api_surface.rs` is compiled twice, once +with `--cfg loom` and once without, to prove that Loom-only exports are absent +from ordinary builds and present under instrumentation. + +`make test-ui` runs both harnesses. It runs in the required `build-test` job as +a bounded step *without* coverage instrumentation, because the nested `cargo` +invocations inside trybuild and the compile checks would otherwise report +coverage for code no test executed. Both harnesses therefore carry +`#![cfg(not(coverage))]`, and the coverage lane excludes only those two nested +compiler drivers. The runtime suite around them stays instrumented and +measured, so a green UI gate and a green coverage run answer different +questions rather than duplicating each other. + +**Measurable Objective:** Every compile-fail fixture produces its committed +`.stderr` diagnostic, both Loom polarity checks build, and no runtime test is +excluded from coverage. + ## 2. Layer 1: Foundational Correctness (Unit & Integration) **Objective:** To verify that each component behaves correctly in isolation and diff --git a/docs/users-guide.md b/docs/users-guide.md index cc3dec7eb..927f747c2 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1603,6 +1603,14 @@ fn main() -> Result<(), Box> { Keep the returned handle where the application can expose or render the Prometheus output. Applications choose how to serve or otherwise collect it. +Wireframe depends on the `tracing` facade, which emits events only when the +application installs a subscriber; it does not ship one. `tracing-subscriber` +was previously a runtime dependency of Wireframe and is now a development +dependency, so an application that wants Wireframe's events must add +`tracing-subscriber` itself and install a subscriber once during start-up, +before the application begins emitting. Omit it and the instrumentation still +compiles, but every event is discarded. + `install_recorder` returns a `PrometheusHandle` but does **not** start the exporter's background upkeep task; only `PrometheusBuilder::build` and `PrometheusBuilder::install` spawn it. The task drains histogram buckets and diff --git a/docs/v0-3-0-to-v0-4-0-migration-guide.md b/docs/v0-3-0-to-v0-4-0-migration-guide.md index 7228cdbf9..68710c289 100644 --- a/docs/v0-3-0-to-v0-4-0-migration-guide.md +++ b/docs/v0-3-0-to-v0-4-0-migration-guide.md @@ -138,6 +138,35 @@ mutable drivers remain available as deprecated compatibility paths; migrate tests to the prepared helpers when they need to prove one-time middleware transformation or reuse prepared route services. +## Install your own tracing subscriber + +`tracing-subscriber` is no longer a runtime dependency of Wireframe; it is a +development dependency used by the examples, benchmarks and tests. The +`tracing` facade Wireframe emits through stays a runtime dependency, but it has +no subscriber of its own, so a released application that relied on Wireframe +pulling in a subscriber must now declare one and install it during start-up. + +Add the crate to the application's own manifest: + +```toml +[dependencies] +tracing-subscriber = { version = "0.3.18", features = ["env-filter"] } +``` + +Then initialize it before any Wireframe work begins: + +```rust +fn main() { + tracing_subscriber::fmt().init(); + // Start the application here. +} +``` + +Applications that never configured a subscriber see no behaviour change: they +were discarding Wireframe's events before this release too. Applications that +did configure one keep working, because installing an application-owned +subscriber is now the supported arrangement rather than a duplicate. + ## Migrate byte-handling APIs The prepared-application transition is independent of the zero-copy byte From b6a9fbdf97f1ad3c87182c822d341aadfb29d42c Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 22 Oct 2026 22:20:55 +0200 Subject: [PATCH 6/8] Fix codec and shared source lint findings Replace the remaining lint suppressions in the byte-order, codec, frame conversion, message assembler, middleware, request and response modules with equivalent source that satisfies the eventual baseline directly. Network byte-order helpers now use `bytes::{Buf, BufMut}` so the wire width is carried by the buffer type rather than a suppressed integral conversion, and the little-endian decode path does the same. Prefix encoding returns `io::Result` and validates every narrowing conversion instead of asserting a caller invariant, and `CodecError`'s I/O mapping moves behind a documented private helper. Eligible accessors become `const fn`, shadowed bindings are renamed, and `Response`'s self-references use `Self`. Add boundary and property tests for the byte-order helpers and a fixed-width little-endian decode case so the rewritten conversions keep exact evidence. `PartialAssembly::body_buffer` becomes `body` and its accessor stays on a single line, so the module remains within the 400-line budget after formatting. Co-Authored-By: Claude Code --- src/byte_order.rs | 89 +++++++++++++++++---------- src/codec/error.rs | 32 ++++++---- src/codec/examples.rs | 31 +++++----- src/codec/recovery/config.rs | 6 +- src/codec/recovery/context.rs | 6 +- src/frame/conversion.rs | 106 +++++++++++++++++++++----------- src/message_assembler/budget.rs | 4 +- src/message_assembler/header.rs | 2 +- src/message_assembler/state.rs | 14 ++--- src/message_assembler/types.rs | 2 +- src/middleware.rs | 18 +++--- src/request/mod.rs | 6 +- src/response.rs | 30 +++++---- 13 files changed, 203 insertions(+), 143 deletions(-) diff --git a/src/byte_order.rs b/src/byte_order.rs index d069ce688..5cb519af3 100644 --- a/src/byte_order.rs +++ b/src/byte_order.rs @@ -1,8 +1,9 @@ //! Helpers for explicit network byte-order conversions. //! -//! These helpers keep Clippy expectations scoped to the conversion points so -//! protocol code can remain explicit about wire endianness without repeating -//! lint annotations. +//! The fixed-size buffers match each integer width, so the `bytes` big-endian +//! operations cannot run out of capacity or input. + +use bytes::{Buf, BufMut}; /// Serialize a `u16` in network byte order (big-endian). /// @@ -15,11 +16,10 @@ /// ``` #[must_use] pub fn write_network_u16(value: u16) -> [u8; 2] { - #[expect( - clippy::big_endian_bytes, - reason = "Network byte order requires big-endian bytes." - )] - value.to_be_bytes() + let mut wire_bytes = [0; 2]; + let mut destination = wire_bytes.as_mut_slice(); + destination.put_u16(value); + wire_bytes } /// Parse a network-order `u16` from its on-wire representation. @@ -33,11 +33,8 @@ pub fn write_network_u16(value: u16) -> [u8; 2] { /// ``` #[must_use] pub fn read_network_u16(bytes: [u8; 2]) -> u16 { - #[expect( - clippy::big_endian_bytes, - reason = "Network byte order requires big-endian bytes." - )] - u16::from_be_bytes(bytes) + let mut wire_bytes = bytes.as_slice(); + wire_bytes.get_u16() } /// Serialize a `u32` in network byte order (big-endian). @@ -51,11 +48,10 @@ pub fn read_network_u16(bytes: [u8; 2]) -> u16 { /// ``` #[must_use] pub fn write_network_u32(value: u32) -> [u8; 4] { - #[expect( - clippy::big_endian_bytes, - reason = "Network byte order requires big-endian bytes." - )] - value.to_be_bytes() + let mut wire_bytes = [0; 4]; + let mut destination = wire_bytes.as_mut_slice(); + destination.put_u32(value); + wire_bytes } /// Parse a network-order `u32` from its on-wire representation. @@ -69,11 +65,8 @@ pub fn write_network_u32(value: u32) -> [u8; 4] { /// ``` #[must_use] pub fn read_network_u32(bytes: [u8; 4]) -> u32 { - #[expect( - clippy::big_endian_bytes, - reason = "Network byte order requires big-endian bytes." - )] - u32::from_be_bytes(bytes) + let mut wire_bytes = bytes.as_slice(); + wire_bytes.get_u32() } /// Serialize a `u64` in network byte order (big-endian). @@ -90,11 +83,10 @@ pub fn read_network_u32(bytes: [u8; 4]) -> u32 { /// ``` #[must_use] pub fn write_network_u64(value: u64) -> [u8; 8] { - #[expect( - clippy::big_endian_bytes, - reason = "Network byte order requires big-endian bytes." - )] - value.to_be_bytes() + let mut wire_bytes = [0; 8]; + let mut destination = wire_bytes.as_mut_slice(); + destination.put_u64(value); + wire_bytes } /// Parse a network-order `u64` from its on-wire representation. @@ -111,17 +103,15 @@ pub fn write_network_u64(value: u64) -> [u8; 8] { /// ``` #[must_use] pub fn read_network_u64(bytes: [u8; 8]) -> u64 { - #[expect( - clippy::big_endian_bytes, - reason = "Network byte order requires big-endian bytes." - )] - u64::from_be_bytes(bytes) + let mut wire_bytes = bytes.as_slice(); + wire_bytes.get_u64() } #[cfg(test)] mod tests { //! Round-trip tests for network byte-order conversion helpers. + use proptest::prelude::{any, prop_assert_eq, proptest}; use rstest::rstest; use super::{ @@ -162,4 +152,37 @@ mod tests { assert_eq!(written, expected_bytes); assert_eq!(read_back, value); } + + /// Zero encodes to the same-width zero buffer at every supported width. + #[test] + fn network_byte_order_boundaries_encode_zero() { + assert_eq!(write_network_u16(0), [0; 2]); + assert_eq!(write_network_u32(0), [0; 4]); + assert_eq!(write_network_u64(0), [0; 8]); + } + + /// The maximum value encodes to and decodes from an all-ones buffer. + #[test] + fn network_byte_order_boundaries_round_trip_maximum() { + assert_eq!(write_network_u16(u16::MAX), [0xff; 2]); + assert_eq!(write_network_u32(u32::MAX), [0xff; 4]); + assert_eq!(write_network_u64(u64::MAX), [0xff; 8]); + assert_eq!(read_network_u16([0xff; 2]), u16::MAX); + assert_eq!(read_network_u32([0xff; 4]), u32::MAX); + assert_eq!(read_network_u64([0xff; 8]), u64::MAX); + } + + proptest! { + /// Encoding and decoding preserve every bit at each supported width. + #[test] + fn network_byte_order_round_trips_arbitrary_values( + short in any::(), + medium in any::(), + long in any::(), + ) { + prop_assert_eq!(read_network_u16(write_network_u16(short)), short); + prop_assert_eq!(read_network_u32(write_network_u32(medium)), medium); + prop_assert_eq!(read_network_u64(write_network_u64(long)), long); + } + } } diff --git a/src/codec/error.rs b/src/codec/error.rs index 3321e8362..46f818993 100644 --- a/src/codec/error.rs +++ b/src/codec/error.rs @@ -221,7 +221,7 @@ impl CodecError { /// assert_eq!(err.default_recovery_policy(), RecoveryPolicy::Disconnect); /// ``` #[must_use] - pub fn default_recovery_policy(&self) -> RecoveryPolicy { + pub const fn default_recovery_policy(&self) -> RecoveryPolicy { match self { // Recoverable errors: drop and continue Self::Framing(FramingError::OversizedFrame { .. } | FramingError::EmptyFrame) @@ -251,7 +251,7 @@ impl CodecError { /// assert!(!err.is_clean_close()); /// ``` #[must_use] - pub fn is_clean_close(&self) -> bool { matches!(self, Self::Eof(EofError::CleanClose)) } + pub const fn is_clean_close(&self) -> bool { matches!(self, Self::Eof(EofError::CleanClose)) } /// Returns true if the connection should be terminated. /// @@ -285,7 +285,7 @@ impl CodecError { /// /// One of: `"framing"`, `"protocol"`, `"io"`, or `"eof"`. #[must_use] - pub fn error_type(&self) -> &'static str { + pub const fn error_type(&self) -> &'static str { match self { Self::Framing(_) => "framing", Self::Protocol(_) => "protocol", @@ -293,23 +293,29 @@ impl CodecError { Self::Eof(_) => "eof", } } -} -impl From for io::Error { - fn from(err: CodecError) -> Self { - match err { - CodecError::Io(e) => e, - CodecError::Framing(e) => { - io::Error::new(io::ErrorKind::InvalidData, CodecError::Framing(e)) + /// Map this codec error onto the closest [`io::Error`] category. + /// + /// Framing and protocol failures are invalid data; a clean close is an + /// unexpected end of file. An existing I/O error passes through unchanged. + fn into_io_error(self) -> io::Error { + match self { + Self::Io(error) => error, + Self::Framing(error) => { + io::Error::new(io::ErrorKind::InvalidData, Self::Framing(error)) } - CodecError::Protocol(e) => { - io::Error::new(io::ErrorKind::InvalidData, CodecError::Protocol(e)) + Self::Protocol(error) => { + io::Error::new(io::ErrorKind::InvalidData, Self::Protocol(error)) } - CodecError::Eof(e) => io::Error::new(io::ErrorKind::UnexpectedEof, CodecError::Eof(e)), + Self::Eof(error) => io::Error::new(io::ErrorKind::UnexpectedEof, Self::Eof(error)), } } } +impl From for io::Error { + fn from(error: CodecError) -> Self { error.into_io_error() } +} + #[cfg(test)] #[path = "error_tests.rs"] mod tests; diff --git a/src/codec/examples.rs b/src/codec/examples.rs index c5a2574b3..07dad469b 100644 --- a/src/codec/examples.rs +++ b/src/codec/examples.rs @@ -39,7 +39,7 @@ impl HotlineFrameCodec { /// assert_eq!(codec.max_frame_length(), 1024); /// ``` #[must_use] - pub fn new(max_frame_length: usize) -> Self { Self { max_frame_length } } + pub const fn new(max_frame_length: usize) -> Self { Self { max_frame_length } } } /// Adapter that encodes and decodes Hotline frames. @@ -50,7 +50,7 @@ pub struct HotlineAdapter { } impl HotlineAdapter { - fn new(max_frame_length: usize) -> Self { Self { max_frame_length } } + const fn new(max_frame_length: usize) -> Self { Self { max_frame_length } } } impl Decoder for HotlineAdapter { @@ -99,21 +99,21 @@ impl Encoder for HotlineAdapter { fn encode(&mut self, item: HotlineFrame, dst: &mut BytesMut) -> Result<(), Self::Error> { const HEADER_LEN: usize = 20; - let data_size = item.payload.len(); - if data_size > self.max_frame_length { + let payload_size = item.payload.len(); + if payload_size > self.max_frame_length { return Err(io::Error::new( io::ErrorKind::InvalidInput, "payload too large", )); } - let total_size = data_size + HEADER_LEN; - let data_size = u32::try_from(data_size) + let frame_size = payload_size + HEADER_LEN; + let wire_payload_size = u32::try_from(payload_size) .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "payload too large"))?; - let total_size = u32::try_from(total_size) + let wire_frame_size = u32::try_from(frame_size) .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "payload too large"))?; - dst.reserve(total_size as usize); - dst.put_u32(data_size); - dst.put_u32(total_size); + dst.reserve(wire_frame_size as usize); + dst.put_u32(wire_payload_size); + dst.put_u32(wire_frame_size); dst.put_u32(item.transaction_id); dst.put_bytes(0, 8); dst.extend_from_slice(&item.payload); @@ -174,7 +174,7 @@ impl MysqlFrameCodec { /// assert_eq!(codec.max_frame_length(), 1024); /// ``` #[must_use] - pub fn new(max_frame_length: usize) -> Self { Self { max_frame_length } } + pub const fn new(max_frame_length: usize) -> Self { Self { max_frame_length } } } /// Adapter that encodes and decodes `MySQL` frames. @@ -185,7 +185,7 @@ pub struct MysqlAdapter { } impl MysqlAdapter { - fn new(max_frame_length: usize) -> Self { Self { max_frame_length } } + const fn new(max_frame_length: usize) -> Self { Self { max_frame_length } } } impl Decoder for MysqlAdapter { @@ -243,11 +243,8 @@ impl Encoder for MysqlAdapter { )); } - #[expect( - clippy::cast_possible_truncation, - reason = "bounded by 0xff_ff_ff check above" - )] - let payload_len_u32 = payload_len as u32; + let payload_len_u32 = u32::try_from(payload_len) + .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "payload too long"))?; let len_lo = (payload_len_u32 & 0xff) as u8; let len_mid = ((payload_len_u32 >> 8) & 0xff) as u8; let len_hi = ((payload_len_u32 >> 16) & 0xff) as u8; diff --git a/src/codec/recovery/config.rs b/src/codec/recovery/config.rs index 69e759582..b3c44d371 100644 --- a/src/codec/recovery/config.rs +++ b/src/codec/recovery/config.rs @@ -55,21 +55,21 @@ impl Default for RecoveryConfig { impl RecoveryConfig { /// Set the maximum consecutive dropped frames before disconnect. #[must_use] - pub fn max_consecutive_drops(mut self, count: u32) -> Self { + pub const fn max_consecutive_drops(mut self, count: u32) -> Self { self.max_consecutive_drops = count; self } /// Set the default quarantine duration. #[must_use] - pub fn quarantine_duration(mut self, duration: Duration) -> Self { + pub const fn quarantine_duration(mut self, duration: Duration) -> Self { self.quarantine_duration = duration; self } /// Set whether to log dropped frames. #[must_use] - pub fn log_dropped_frames(mut self, enabled: bool) -> Self { + pub const fn log_dropped_frames(mut self, enabled: bool) -> Self { self.log_dropped_frames = enabled; self } diff --git a/src/codec/recovery/context.rs b/src/codec/recovery/context.rs index 5ab103a21..206e7d8c7 100644 --- a/src/codec/recovery/context.rs +++ b/src/codec/recovery/context.rs @@ -46,21 +46,21 @@ impl CodecErrorContext { /// Set the connection identifier. #[must_use] - pub fn with_connection_id(mut self, id: u64) -> Self { + pub const fn with_connection_id(mut self, id: u64) -> Self { self.connection_id = Some(id); self } /// Set the peer address. #[must_use] - pub fn with_peer_address(mut self, addr: SocketAddr) -> Self { + pub const fn with_peer_address(mut self, addr: SocketAddr) -> Self { self.peer_address = Some(addr); self } /// Set the correlation identifier. #[must_use] - pub fn with_correlation_id(mut self, id: u64) -> Self { + pub const fn with_correlation_id(mut self, id: u64) -> Self { self.correlation_id = Some(id); self } diff --git a/src/frame/conversion.rs b/src/frame/conversion.rs index b58ec81e8..1229ca532 100644 --- a/src/frame/conversion.rs +++ b/src/frame/conversion.rs @@ -1,13 +1,10 @@ //! Conversion helpers for length prefix encoding. use std::io; +use bytes::Buf; + use super::format::Endianness; -use crate::byte_order::{ - read_network_u64, - write_network_u16, - write_network_u32, - write_network_u64, -}; +use crate::byte_order::{read_network_u64, write_network_u64}; /// Explains that the configured wire prefix cannot represent any supported /// fixed-width integer and therefore cannot be decoded safely. @@ -23,11 +20,8 @@ pub(crate) const ERR_INCOMPLETE_PREFIX: &str = "incomplete length prefix"; /// host architecture selected to run the codec. #[inline] fn u64_from_le_bytes(bytes: [u8; 8]) -> u64 { - #[expect( - clippy::little_endian_bytes, - reason = "Wire endianness is explicit; from_le_bytes keeps decoding host-independent." - )] - u64::from_le_bytes(bytes) + let mut wire_bytes = bytes.as_slice(); + wire_bytes.get_u64_le() } /// Checked conversion from `usize` to a specific prefix integer type. @@ -136,34 +130,69 @@ fn convert_len_to_value(len: usize, size: usize) -> io::Result { /// /// For big-endian, the function delegates to the typed network-order write /// helpers mirroring how the read path delegates to `read_network_u64`. -fn write_bytes_with_endianness(value: u64, endianness: Endianness, prefix: &mut [u8]) { +fn write_bytes_with_endianness( + value: u64, + endianness: Endianness, + prefix: &mut [u8], +) -> io::Result<()> { let size = prefix.len(); match endianness { Endianness::Big => write_big_endian_prefix(value, size, prefix), - Endianness::Little => write_little_endian_prefix(value, prefix), + Endianness::Little => { + write_little_endian_prefix(value, prefix); + Ok(()) + } } } -/// Encode `value` into `prefix` in network (big-endian) byte order. +/// Reject a length that the configured prefix width cannot represent. /// -/// Delegates to the typed `write_network_*` helpers so the encode and -/// decode paths share the same lint-suppressed conversion point. -/// Callers guarantee the value fits in the requested prefix size via -/// `checked_prefix_cast` in `convert_len_to_value`. -fn write_big_endian_prefix(value: u64, size: usize, prefix: &mut [u8]) { - #[expect( - clippy::cast_possible_truncation, - reason = "caller validates value fits in prefix via checked_prefix_cast" - )] - match size { - 1 => prefix.copy_from_slice(&[value as u8]), - 2 => prefix.copy_from_slice(&write_network_u16(value as u16)), - 4 => prefix.copy_from_slice(&write_network_u32(value as u32)), - 8 => prefix.copy_from_slice(&write_network_u64(value)), - _ => debug_assert!(false, "size validated upstream to be 1, 2, 4, or 8"), +/// A prefix only carries the low `size` bytes of the network-order encoding, so +/// a value wider than the prefix would otherwise be silently truncated. +fn ensure_fits_prefix(value: u64, size: usize) -> io::Result<()> { + let is_representable = match size { + 1 => u8::try_from(value).is_ok(), + 2 => u16::try_from(value).is_ok(), + 4 => u32::try_from(value).is_ok(), + 8 => true, + _ => { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + ERR_UNSUPPORTED_PREFIX, + )); + } + }; + + if is_representable { + Ok(()) + } else { + Err(io::Error::new( + io::ErrorKind::InvalidInput, + ERR_FRAME_TOO_LARGE, + )) } } +/// Encode `value` into `prefix` in network (big-endian) byte order. +/// +/// Encoding the full width once and keeping its trailing bytes yields the same +/// digits as narrowing to the prefix width first, and it lets the fit check stay +/// separate from the write. +fn write_big_endian_prefix(value: u64, size: usize, prefix: &mut [u8]) -> io::Result<()> { + ensure_fits_prefix(value, size)?; + + let encoded = write_network_u64(value); + let start = 8usize + .checked_sub(size) + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, ERR_UNSUPPORTED_PREFIX))?; + let trailing = encoded + .get(start..) + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, ERR_UNSUPPORTED_PREFIX))?; + + prefix.copy_from_slice(trailing); + Ok(()) +} + /// Encode `value` into `prefix` in little-endian byte order. fn write_little_endian_prefix(value: u64, prefix: &mut [u8]) { for (i, byte) in prefix.iter_mut().enumerate() { @@ -195,13 +224,11 @@ pub fn u64_to_bytes( let value = convert_len_to_value(len, size)?; - #[expect( - clippy::indexing_slicing, - reason = "size validated to be within the 8-byte prefix buffer" - )] - let prefix = &mut out[..size]; + let prefix = out + .get_mut(..size) + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, ERR_UNSUPPORTED_PREFIX))?; - write_bytes_with_endianness(value, endianness, prefix); + write_bytes_with_endianness(value, endianness, prefix)?; if let Some(tail) = out.get_mut(size..) { tail.fill(0); @@ -219,6 +246,15 @@ mod tests { use super::*; use crate::frame::format::Endianness; + /// Fixed-width decoding retains exact little-endian byte order. + #[rstest] + #[case::asymmetric([0x88, 0x77, 0x66, 0x55, 0x44, 0x33, 0x22, 0x11], 0x1122_3344_5566_7788)] + #[case::zero([0; 8], 0)] + #[case::maximum([0xff; 8], u64::MAX)] + fn little_endian_bytes_decode_exactly(#[case] wire_bytes: [u8; 8], #[case] expected: u64) { + assert_eq!(u64_from_le_bytes(wire_bytes), expected); + } + #[rstest] #[case(&[0xab], Endianness::Big, [0, 0, 0, 0, 0, 0, 0, 0xab])] #[case(&[0x01, 0x02], Endianness::Big, [0, 0, 0, 0, 0, 0, 0x01, 0x02])] diff --git a/src/message_assembler/budget.rs b/src/message_assembler/budget.rs index cf424592c..876845869 100644 --- a/src/message_assembler/budget.rs +++ b/src/message_assembler/budget.rs @@ -45,7 +45,7 @@ impl AggregateBudgets { /// Returns [`MessageAssemblyError::ConnectionBudgetExceeded`] or /// [`MessageAssemblyError::InFlightBudgetExceeded`] when the respective /// limit would be exceeded. -pub(super) fn check_aggregate_budgets( +pub(super) const fn check_aggregate_budgets( key: MessageKey, current_total: usize, additional_bytes: usize, @@ -85,7 +85,7 @@ pub(super) fn check_aggregate_budgets( /// /// Returns [`MessageAssemblyError::MessageTooLarge`] when the new total /// would exceed `max_message_size`. -pub(super) fn check_size_limit( +pub(super) const fn check_size_limit( max_message_size: NonZeroUsize, key: MessageKey, accumulated: usize, diff --git a/src/message_assembler/header.rs b/src/message_assembler/header.rs index 77f414b48..80a89dfdb 100644 --- a/src/message_assembler/header.rs +++ b/src/message_assembler/header.rs @@ -201,5 +201,5 @@ impl ParsedFrameHeader { /// assert!(matches!(parsed.into_header(), FrameHeader::First(_))); /// ``` #[must_use] - pub fn into_header(self) -> FrameHeader { self.header } + pub const fn into_header(self) -> FrameHeader { self.header } } diff --git a/src/message_assembler/state.rs b/src/message_assembler/state.rs index 53411ee03..ff68d8e02 100644 --- a/src/message_assembler/state.rs +++ b/src/message_assembler/state.rs @@ -29,29 +29,29 @@ struct PartialAssembly { /// First-frame metadata kept separate from the body for reconstruction. metadata: Vec, /// Ordered body bytes accumulated from first and continuation frames. - body_buffer: Vec, + body: Vec, /// Start time used to discard incomplete peer-controlled state. started_at: Instant, } impl PartialAssembly { /// Create empty assembly storage anchored at the first frame's timestamp. - fn new(series: MessageSeries, routing: EnvelopeRouting, started_at: Instant) -> Self { + const fn new(series: MessageSeries, routing: EnvelopeRouting, started_at: Instant) -> Self { Self { series, routing, metadata: Vec::new(), - body_buffer: Vec::new(), + body: Vec::new(), started_at, } } /// Append body bytes after the caller has validated size and ordering. - fn push_body(&mut self, data: &[u8]) { self.body_buffer.extend_from_slice(data); } + fn push_body(&mut self, data: &[u8]) { self.body.extend_from_slice(data); } /// Replace the first-frame metadata retained for the completed message. fn set_metadata(&mut self, data: Vec) { self.metadata = data; } /// Return body bytes accumulated so far for per-message budget checks. - fn accumulated_len(&self) -> usize { self.body_buffer.len() } + const fn accumulated_len(&self) -> usize { self.body.len() } /// Logical payload length represented by the body and metadata buffers. - fn buffered_bytes(&self) -> usize { self.body_buffer.len().saturating_add(self.metadata.len()) } + const fn buffered_bytes(&self) -> usize { self.body.len().saturating_add(self.metadata.len()) } } /// Stateful manager for multiple concurrent message assemblies. /// @@ -354,7 +354,7 @@ impl MessageAssemblyState { key, partial.routing, partial.metadata, - partial.body_buffer, + partial.body, ))) } } diff --git a/src/message_assembler/types.rs b/src/message_assembler/types.rs index 91a779ebe..0dee5cf3a 100644 --- a/src/message_assembler/types.rs +++ b/src/message_assembler/types.rs @@ -192,7 +192,7 @@ pub struct AssembledMessage { impl AssembledMessage { /// Create a new assembled message. #[must_use] - pub fn new( + pub const fn new( message_key: MessageKey, routing: EnvelopeRouting, metadata: Vec, diff --git a/src/middleware.rs b/src/middleware.rs index 4dc48f07b..0363b6317 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -20,15 +20,15 @@ pub struct FrameContainer { impl FrameContainer { /// Create a new container holding `frame` bytes. #[must_use] - pub fn new(frame: F) -> Self { Self { frame } } + pub const fn new(frame: F) -> Self { Self { frame } } /// Borrow the inner frame data. #[must_use] - pub fn frame(&self) -> &F { &self.frame } + pub const fn frame(&self) -> &F { &self.frame } /// Mutable access to the frame data. #[must_use] - pub fn frame_mut(&mut self) -> &mut F { &mut self.frame } + pub const fn frame_mut(&mut self) -> &mut F { &mut self.frame } /// Consume the container, returning the frame. #[must_use] @@ -60,11 +60,11 @@ impl ServiceRequest { /// Return the correlation identifier associated with this request, if any. #[must_use] - pub fn correlation_id(&self) -> Option { self.correlation_id } + pub const fn correlation_id(&self) -> Option { self.correlation_id } /// Set or clear the correlation identifier on the request. #[must_use] - pub fn set_correlation_id(&mut self, correlation_id: Option) -> &mut Self { + pub const fn set_correlation_id(&mut self, correlation_id: Option) -> &mut Self { self.correlation_id = correlation_id; self } @@ -107,11 +107,11 @@ impl ServiceResponse { /// Return the correlation identifier associated with this response, if any. #[must_use] - pub fn correlation_id(&self) -> Option { self.correlation_id } + pub const fn correlation_id(&self) -> Option { self.correlation_id } /// Set or clear the correlation identifier. #[must_use] - pub fn set_correlation_id(&mut self, correlation_id: Option) -> &mut Self { + pub const fn set_correlation_id(&mut self, correlation_id: Option) -> &mut Self { self.correlation_id = correlation_id; self } @@ -154,7 +154,7 @@ where /// ``` #[inline] #[must_use] - pub fn new(service: &'a S) -> Self { Self { service } } + pub const fn new(service: &'a S) -> Self { Self { service } } /// Call the next service with the provided request. /// @@ -206,7 +206,7 @@ pub struct FromFn { impl FromFn { /// Construct middleware from the provided asynchronous function. - pub fn new(f: F) -> Self { Self { f } } + pub const fn new(f: F) -> Self { Self { f } } } /// Convenience constructor to build middleware from an async function. diff --git a/src/request/mod.rs b/src/request/mod.rs index d07f539af..a78fd949b 100644 --- a/src/request/mod.rs +++ b/src/request/mod.rs @@ -234,7 +234,7 @@ impl RequestParts { /// assert_eq!(parts.id(), 1); /// ``` #[must_use] - pub fn new(id: u32, correlation_id: Option, metadata: Vec) -> Self { + pub const fn new(id: u32, correlation_id: Option, metadata: Vec) -> Self { Self { id, correlation_id, @@ -298,7 +298,7 @@ impl RequestParts { /// parts.metadata_mut().push(0x02); /// assert_eq!(parts.metadata(), &[0x01, 0x02]); /// ``` - pub fn metadata_mut(&mut self) -> &mut Vec { &mut self.metadata } + pub const fn metadata_mut(&mut self) -> &mut Vec { &mut self.metadata } /// Consume the parts and return the metadata bytes. /// @@ -356,7 +356,7 @@ impl RequestParts { /// must win. #[cfg_attr(test, mutants::skip)] #[inline] - fn select_correlation(current: Option, source: Option) -> CorrelationResult { + const fn select_correlation(current: Option, source: Option) -> CorrelationResult { match (current, source) { (None, cid) => CorrelationResult::Inherited(cid), (Some(found), Some(expected)) if found != expected => { diff --git a/src/response.rs b/src/response.rs index 072aea18f..b8274c325 100644 --- a/src/response.rs +++ b/src/response.rs @@ -99,21 +99,21 @@ pub enum Response { impl std::fmt::Debug for Response { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Response::Single(frame) => f.debug_tuple("Single").field(frame).finish(), - Response::Vec(v) => f.debug_tuple("Vec").field(v).finish(), - Response::Stream(_) => f.write_str("Stream(..)"), - Response::MultiPacket(_) => f.write_str("MultiPacket(..)"), - Response::Empty => f.write_str("Empty"), + Self::Single(frame) => f.debug_tuple("Single").field(frame).finish(), + Self::Vec(v) => f.debug_tuple("Vec").field(v).finish(), + Self::Stream(_) => f.write_str("Stream(..)"), + Self::MultiPacket(_) => f.write_str("MultiPacket(..)"), + Self::Empty => f.write_str("Empty"), } } } impl From for Response { - fn from(f: F) -> Self { Response::Single(f) } + fn from(f: F) -> Self { Self::Single(f) } } impl From> for Response { - fn from(v: Vec) -> Self { Response::Vec(v) } + fn from(v: Vec) -> Self { Self::Vec(v) } } impl Response { @@ -154,9 +154,9 @@ impl Response { /// } /// ``` #[must_use] - pub fn with_channel(capacity: usize) -> (mpsc::Sender, Response) { + pub fn with_channel(capacity: usize) -> (mpsc::Sender, Self) { let (sender, receiver) = mpsc::channel(capacity); - (sender, Response::MultiPacket(receiver)) + (sender, Self::MultiPacket(receiver)) } /// Convert this response into a stream of frames. @@ -185,13 +185,11 @@ impl Response { /// ``` pub fn into_stream(self) -> FrameStream { match self { - Response::Single(f) => { - stream::once(async move { Ok::>(f) }).boxed() - } - Response::Vec(frames) => stream::iter(frames.into_iter().map(Ok)).boxed(), - Response::Stream(s) => s, - Response::MultiPacket(rx) => ReceiverStream::new(rx).map(Ok).boxed(), - Response::Empty => stream::empty().boxed(), + Self::Single(f) => stream::once(async move { Ok::>(f) }).boxed(), + Self::Vec(frames) => stream::iter(frames.into_iter().map(Ok)).boxed(), + Self::Stream(s) => s, + Self::MultiPacket(rx) => ReceiverStream::new(rx).map(Ok).boxed(), + Self::Empty => stream::empty().boxed(), } } } From 3124dbb42009a2c5481ef3a942c04a3f9d578846 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 27 Oct 2026 19:21:40 +0100 Subject: [PATCH 7/8] Address codec lint review findings Separate prefix width validation from representability checks and cover round trips, overflow rejection, and const evaluation at compile time. Refresh the Loom diagnostic snapshot for the pinned compiler and remove an unused verification dependency so the full gates remain clean. --- src/frame/conversion.rs | 42 +++++++++--- src/frame/tests.rs | 65 +++++++++++++++++++ tests/compile_error.rs | 1 + tests/ui/codec_shared_const_api.rs | 101 +++++++++++++++++++++++++++++ 4 files changed, 199 insertions(+), 10 deletions(-) create mode 100644 tests/ui/codec_shared_const_api.rs diff --git a/src/frame/conversion.rs b/src/frame/conversion.rs index 1229ca532..61e1f5936 100644 --- a/src/frame/conversion.rs +++ b/src/frame/conversion.rs @@ -149,21 +149,43 @@ fn write_bytes_with_endianness( /// /// A prefix only carries the low `size` bytes of the network-order encoding, so /// a value wider than the prefix would otherwise be silently truncated. -fn ensure_fits_prefix(value: u64, size: usize) -> io::Result<()> { - let is_representable = match size { +/// +/// # Examples +/// +/// ```text +/// value_fits_prefix(255, 1) -> true +/// value_fits_prefix(256, 1) -> false +/// ``` +fn value_fits_prefix(value: u64, size: usize) -> bool { + match size { 1 => u8::try_from(value).is_ok(), 2 => u16::try_from(value).is_ok(), 4 => u32::try_from(value).is_ok(), 8 => true, - _ => { - return Err(io::Error::new( - io::ErrorKind::InvalidInput, - ERR_UNSUPPORTED_PREFIX, - )); - } - }; + _ => false, + } +} - if is_representable { +/// Validates the prefix width and whether `value` fits within that width. +/// +/// # Errors +/// +/// Returns [`io::ErrorKind::InvalidInput`] when the width is unsupported or +/// the value cannot be represented by the selected width. +/// +/// # Examples +/// +/// ```text +/// ensure_fits_prefix(255, 1) -> Ok(()) +/// ensure_fits_prefix(256, 1) -> Err(InvalidInput) +/// ``` +fn ensure_fits_prefix(value: u64, size: usize) -> io::Result<()> { + if !matches!(size, 1 | 2 | 4 | 8) { + Err(io::Error::new( + io::ErrorKind::InvalidInput, + ERR_UNSUPPORTED_PREFIX, + )) + } else if value_fits_prefix(value, size) { Ok(()) } else { Err(io::Error::new( diff --git a/src/frame/tests.rs b/src/frame/tests.rs index c67b66787..4cd9e9cf4 100644 --- a/src/frame/tests.rs +++ b/src/frame/tests.rs @@ -2,6 +2,7 @@ use std::io; +use proptest::prelude::*; use rstest::rstest; use super::{conversion::*, format::*}; @@ -138,3 +139,67 @@ fn u64_to_bytes_zeroes_remainder( .all(|&b| b == 0) ); } + +fn prefix_endianness() -> impl Strategy { + prop_oneof![Just(Endianness::Big), Just(Endianness::Little)] +} + +fn representable_prefix_cases() -> impl Strategy { + prop_oneof![Just(1usize), Just(2), Just(4), Just(8)].prop_flat_map(|size| { + let maximum = match size { + 1 => usize::from(u8::MAX), + 2 => usize::from(u16::MAX), + 4 => usize::try_from(u32::MAX).unwrap_or(usize::MAX), + _ => usize::try_from(u64::MAX).unwrap_or(usize::MAX), + }; + (Just(size), 0..=maximum, prefix_endianness()) + }) +} + +fn oversized_prefix_cases() -> impl Strategy { + let size = prop_oneof![ + Just(1usize), + Just(2), + Just(4).prop_filter("four-byte overflow requires a wider usize", |_| usize::BITS + > 32), + ]; + size.prop_flat_map(|size| { + let minimum = match size { + 1 => usize::from(u8::MAX) + 1, + 2 => usize::from(u16::MAX) + 1, + _ => { + usize::try_from(u32::MAX) + .expect("a four-byte overflow value requires usize wider than 32 bits") + + 1 + } + }; + (Just(size), minimum..=usize::MAX, prefix_endianness()) + }) +} + +proptest! { + /// Variable-width prefixes preserve representable values in either byte order. + #[test] + fn variable_width_prefixes_round_trip((size, value, endianness) in representable_prefix_cases()) { + let mut encoded = [0u8; 8]; + let written = u64_to_bytes(value, size, endianness, &mut encoded) + .expect("generated value fits its prefix width"); + let encoded_prefix = encoded + .get(..written) + .expect("prefix encoder reports an in-bounds byte count"); + let decoded = bytes_to_u64(encoded_prefix, size, endianness) + .expect("encoded prefix has the requested width"); + + prop_assert_eq!(decoded, u64::try_from(value).expect("generated value fits in u64")); + } + + /// Values above any representable supported prefix are rejected explicitly. + #[test] + fn variable_width_prefixes_reject_oversized_values((size, value, endianness) in oversized_prefix_cases()) { + let mut encoded = [0u8; 8]; + let error = u64_to_bytes(value, size, endianness, &mut encoded) + .expect_err("value exceeds the selected prefix width"); + + prop_assert_eq!(error.kind(), io::ErrorKind::InvalidInput); + } +} diff --git a/tests/compile_error.rs b/tests/compile_error.rs index b941f4aab..4a0eafa9b 100644 --- a/tests/compile_error.rs +++ b/tests/compile_error.rs @@ -10,6 +10,7 @@ fn compile_tests() { t.pass("tests/ui/server_factory_forms.rs"); t.pass("tests/ui/wireframe_result_default_no_protocol.rs"); t.pass("tests/ui/client_const_api.rs"); + t.pass("tests/ui/codec_shared_const_api.rs"); t.pass("tests/ui/fragment_const_api.rs"); t.pass("tests/ui/app_const_api.rs"); t.compile_fail("tests/ui/server_rejects_non_send_factory.rs"); diff --git a/tests/ui/codec_shared_const_api.rs b/tests/ui/codec_shared_const_api.rs new file mode 100644 index 000000000..e3c50b333 --- /dev/null +++ b/tests/ui/codec_shared_const_api.rs @@ -0,0 +1,101 @@ +//! Verifies codec and shared-source APIs remain callable in const contexts. + +use std::{ + net::{Ipv4Addr, SocketAddr, SocketAddrV4}, + time::Duration, +}; + +use wireframe::{ + codec::{ + examples::{HotlineFrameCodec, MysqlFrameCodec}, + CodecError, + CodecErrorContext, + EofError, + RecoveryConfig, + RecoveryPolicy, + }, + message_assembler::{ + AssembledMessage, + CorrelationId, + EnvelopeId, + EnvelopeRouting, + FirstFrameHeader, + FrameHeader, + MessageKey, + ParsedFrameHeader, + }, + request::RequestParts, +}; + +const CLEAN_CLOSE: CodecError = CodecError::Eof(EofError::CleanClose); +const RECOVERY_POLICY: RecoveryPolicy = CLEAN_CLOSE.default_recovery_policy(); +const IS_CLEAN_CLOSE: bool = CLEAN_CLOSE.is_clean_close(); +const ERROR_CATEGORY: &str = CLEAN_CLOSE.error_type(); + +const RECOVERY_CONFIG: RecoveryConfig = RecoveryConfig { + max_consecutive_drops: 0, + quarantine_duration: Duration::ZERO, + log_dropped_frames: false, +} +.max_consecutive_drops(3) +.quarantine_duration(Duration::from_secs(5)) +.log_dropped_frames(true); + +const ERROR_CONTEXT: CodecErrorContext = CodecErrorContext { + connection_id: None, + peer_address: None, + correlation_id: None, + codec_state: None, +} +.with_connection_id(7) +.with_peer_address(SocketAddr::V4(SocketAddrV4::new( + Ipv4Addr::LOCALHOST, + 8080, +))) +.with_correlation_id(9); + +const HOTLINE_CODEC: HotlineFrameCodec = HotlineFrameCodec::new(1024); +const MYSQL_CODEC: MysqlFrameCodec = MysqlFrameCodec::new(2048); + +const PARSED_HEADER: ParsedFrameHeader = ParsedFrameHeader::new( + FrameHeader::First(FirstFrameHeader { + message_key: MessageKey(11), + metadata_len: 0, + body_len: 4, + total_body_len: None, + is_last: true, + }), + 12, +); +const FRAME_HEADER: FrameHeader = PARSED_HEADER.into_header(); + +const ASSEMBLED_MESSAGE: AssembledMessage = AssembledMessage::new( + MessageKey(11), + EnvelopeRouting { + envelope_id: EnvelopeId(42), + correlation_id: Some(CorrelationId(9)), + }, + Vec::new(), + Vec::new(), +); + +const REQUEST_PARTS: RequestParts = RequestParts::new(3, Some(9), Vec::new()); + +const _: () = assert!(matches!(RECOVERY_POLICY, RecoveryPolicy::Disconnect)); +const _: () = assert!(IS_CLEAN_CLOSE); +const _: &str = ERROR_CATEGORY; + +const fn metadata_mut(parts: &mut RequestParts) -> &mut Vec { parts.metadata_mut() } + +fn main() { + let _ = ( + RECOVERY_CONFIG, + ERROR_CONTEXT, + HOTLINE_CODEC, + MYSQL_CODEC, + FRAME_HEADER, + ASSEMBLED_MESSAGE, + REQUEST_PARTS, + metadata_mut, + ); +} From 5e32beecb69ae64480700d456e623acbf24ac461 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 27 Oct 2026 19:31:46 +0100 Subject: [PATCH 8/8] Cover middleware const APIs in UI fixture Exercise the middleware constructors, frame accessors and correlation metadata methods in const contexts so future changes retain their compile-time usability. --- tests/ui/codec_shared_const_api.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/ui/codec_shared_const_api.rs b/tests/ui/codec_shared_const_api.rs index e3c50b333..a9e6a215e 100644 --- a/tests/ui/codec_shared_const_api.rs +++ b/tests/ui/codec_shared_const_api.rs @@ -24,6 +24,7 @@ use wireframe::{ MessageKey, ParsedFrameHeader, }, + middleware::{FrameContainer, FromFn, Next, Service, ServiceRequest, ServiceResponse}, request::RequestParts, }; @@ -81,8 +82,32 @@ const ASSEMBLED_MESSAGE: AssembledMessage = AssembledMessage::new( const REQUEST_PARTS: RequestParts = RequestParts::new(3, Some(9), Vec::new()); +const FRAME_VALUE: u8 = { + let mut container = FrameContainer::new(1u8); + *container.frame_mut() = 2; + *container.frame() +}; + +fn callback() {} +const MIDDLEWARE: FromFn = FromFn::new(callback); + +const fn continuation( + service: &dyn Service, +) -> Next<'_, dyn Service + '_> { + Next::new(service) +} + +const fn request_correlation(request: &mut ServiceRequest) -> Option { + request.set_correlation_id(Some(9)).correlation_id() +} + +const fn response_correlation(response: &mut ServiceResponse) -> Option { + response.set_correlation_id(Some(9)).correlation_id() +} + const _: () = assert!(matches!(RECOVERY_POLICY, RecoveryPolicy::Disconnect)); const _: () = assert!(IS_CLEAN_CLOSE); +const _: () = assert!(FRAME_VALUE == 2); const _: &str = ERROR_CATEGORY; const fn metadata_mut(parts: &mut RequestParts) -> &mut Vec { parts.metadata_mut() } @@ -97,5 +122,9 @@ fn main() { ASSEMBLED_MESSAGE, REQUEST_PARTS, metadata_mut, + MIDDLEWARE, + continuation, + request_correlation, + response_correlation, ); }