Add Phase 5 cross-adapter verification suite (PR 18)#725
Conversation
Adds 10 tests to tests/routes.rs covering every explicitly registered route plus the tsjs catch-all wildcard. Assertions are scoped to routing only (not 404) for handlers that require live settings or outbound connections, matching the pattern established by the existing auction test.
Verifies that /admin/* routes return 401 without credentials, include a WWW-Authenticate: Basic realm=... header, and reject wrong credentials; also confirms /.well-known and /auction are not gated by admin auth.
…enchmarks - Add golden_script_tag_injected_at_head_start: verifies script tag is the first child of <head> with nothing between the opening tag and the injected <script>. - Add golden_url_rewriting_replaces_origin_in_href: verifies origin host is fully replaced by proxy host in href/src attributes. - Add golden_integration_script_is_not_double_injected: verifies the /static/tsjs= script tag appears exactly once. - Add response_size_does_not_grow_disproportionately: verifies processed HTML stays within 2× of input size to catch buffer/double-processing bugs. - Add Criterion benchmark (html_processor_bench) for process_chunk at 10 KB and 100 KB payload sizes.
The build script writes trusted-server-out.toml to ../../target/ relative to crates/trusted-server-core/. When the test-parity CI job builds this crate as a dependency from crates/integration-tests/ (workspace-excluded), the workspace-root target/ directory may not yet exist, causing a panic. Add fs::create_dir_all for the parent path before the write to handle this case robustly.
The renamed tests duplicated coverage already provided by admin_route_with_wrong_credentials_returns_401. Auth middleware rejects any wrong credentials with 401 regardless of body content, so the extra variants added no unique signal.
The previous comment described the wrong divergence (authenticated path). For unauthenticated requests both adapters return 401. Add the missing assert_eq!(axum_status, 401) and assert_eq!(axum_status, cf_status) so the parity claim is actually verified for both adapters.
crates/integration-tests is workspace-excluded so cargo clippy --workspace is blind to it. Add an explicit step so lint regressions in parity.rs are caught on every PR.
2.0 was a magic number. Named constant with comment makes the bound self-documenting: 2× covers injected script tag plus URL rewrites.
The setup-rust-toolchain action does not guarantee clippy is installed when restoring a shared cache. Explicitly request the component so the Clippy (parity test crate) step can find cargo-clippy.
Matches the convention used by the Axum adapter tests and parity tests. Single-threaded tokio can miss races in middleware that spawns tasks.
Matches the five first-party route tests already present in the Cloudflare adapter test suite. A silently removed route in the Axum adapter now fails the test run instead of going undetected.
- Assert Axum also returns WWW-Authenticate header on 401 (was CF-only)
- Add admin_deactivate_unauthenticated_parity covering the deactivate path
- Rename cookie_behavior_note → publisher_proxy_fallback_parity (name
now reflects what the test actually verifies)
- Fix expect("collect body") → expect("should collect body") per style guide
Adds inline comment so future maintainers know why the version is pinned with `=` rather than a range constraint.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
I verified the three issues locally and opened stacked fix PR #739 with test/CI-only changes.
Blocking summary:
- route smoke tests need to prove explicit registrations, not just non-404 responses behind wildcard fallbacks
- cross-adapter parity tests need to compare critical header values, not just presence
- CI should run clippy for the integration test crate with --all-targets so the test targets are actually linted
Route smoke tests — false positives via wildcard fallback: - Add registered_routes() helper using RouterService::routes() for introspection - Replace 9 individual assert_ne!(404) tests with all_explicit_routes_are_registered() which checks the route table directly; a removed explicit route now fails even when a wildcard fallback would have returned non-404 Cross-adapter parity tests — value equality not just presence: - admin_rotate_unauthenticated_parity: extract both WWW-Authenticate header values and assert they are equal; assert the shared value starts with Basic - admin_deactivate_unauthenticated_parity: same fix - geo_header_parity_on_all_responses: compare X-Geo-Info-Available values across adapters rather than only checking presence; catches true vs false divergence CI clippy: - Add --all-targets to the integration-tests clippy invocation so test targets (tests/parity.rs, tests/routes.rs) are actually linted
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Phase 5 verification adds route smoke tests, cross-adapter parity tests, HTML golden tests, error-correlation unit tests, Criterion benchmarks, and CI gates. The architecture is right and the tests cover important contracts, but several assertions are weaker than they read — they pass against the current code yet would also pass against several plausible regressions the suite is supposed to catch. CI is green across all 11 checks; findings below address test rigor, not breakage.
A prior review by ChristianPavilonis (CHANGES_REQUESTED) flagged three issues. A stacked fix PR (#739) addresses them but is not merged into this branch yet, so those findings are still live. I confirm all three and add four more.
Blocking
🔧 wrench
-
Route smoke tests pass when explicit registration is removed — both adapters end their router with
/{*rest}catch-all GET/POST. Smoke tests asserting!= 404cannot distinguish "explicit handler ran" from "wildcard fallback swallowed the request". Inline detail on cloudflare and axum sides. Same root cause for the basic-auth tests on admin routes: AuthMiddleware short-circuits with 401 regardless of whether the explicit handler is registered, so removing.post("/admin/keys/rotate", ...)would not fail any test in this PR. -
Parity tests check presence, not value equality — three places use
contains_key(...)where parity needsassert_eq!(axum_value, cf_value, ...). Inline detail ongeo_header_parity_on_all_responsesand the admin 401 parity tests. -
CI clippy gate is a no-op —
cargo clippy --manifest-path crates/integration-tests/Cargo.toml -- -D warningsmisses every[[test]]target (no--all-targets), and even with--all-targetsthe crate has no[lints]section so workspace lints (unwrap_used = deny,panic = deny) do not apply. The crate is workspace-excluded solints.workspace = truemay not resolve — verify or inline. -
auth_middleware_runs_in_chain_for_protected_routescannot prove what its name claims — the assertions hold even ifAuthMiddlewareis removed from the chain entirely. Inline detail at cloudflare/tests/routes.rs:43.
Non-blocking
🤔 thinking
-
Tokio exact pin is redundant with the lockfile and silently drifts —
=1.52.3adds nothing the workspace-excludedCargo.lockdoesn't already enforce, but it WILL break when workspace tokio bumps and nobody remembers to edit this line. -
MAX_GROWTH_FACTOR = 2.0is too loose — observed growth is likely ≤1.05× for the test HTML; a regression that doubles script injection would still pass. -
Discovery JSON parity is
is_some == is_some— both adapters could return completely different JSON and the test passes.
♻️ refactor
-
Bench should add no-rewrite and rewrite-dense baselines to separate fixed pipeline overhead from rewrite cost.
-
Parity helpers rebuild the full router per request — fine now; will dominate test time as the suite grows. Consider a
OnceCell<Arc<Router>>per adapter.
📌 out of scope
edgezerogit rev is hardcoded inintegration-tests/Cargo.tomlseparately from[workspace.dependencies]. Worth a follow-up CI check or docs note for the dual-update.
🌱 seedling
- Parity coverage gaps worth a follow-up issue: response-body equality for
/.well-known/trusted-server.jsonwhen both adapters succeed;Set-Cookieattribute parity (HttpOnly, Secure, SameSite, Max-Age);Content-Typeheader parity on success responses. Today's suite covers status + a couple headers; full parity needs the rest.
⛏ nitpick
axum_postandcf_posthelpers are asymmetric — chained.expect()vsoneshot()patterns. Optional symmetrize.
CI Status
- cargo fmt: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (workspace): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- browser integration tests: PASS
- integration tests: PASS
- vitest: PASS
- format-docs / format-typescript: PASS
All gates green. The findings above are about what the gates would catch next, not what's broken now.
- Fix auth_middleware test to use protected route (/admin/keys/rotate)
with 401 + WWW-Authenticate assertion; /auction is not a protected
route and cannot prove AuthMiddleware ran
- Add all_explicit_routes_are_registered() to Axum adapter using
RouterService introspection; status != 404 is a false positive when
/{*rest} catch-all is registered
- Add [lints.clippy] section to integration-tests/Cargo.toml (inlined
from workspace) so clippy --all-targets actually gates test binaries;
drop redundant =1.52.3 exact tokio pin (Cargo.lock already enforces it)
- Fix two clippy::panic violations in parity.rs (unwrap_or_else(panic!))
and pre-existing doc_markdown violations surfaced by the new lint gate
- Add top-level JSON key-set comparison in discovery_route_body_is_json_parity;
is_some == is_some passes even when both adapters return different objects
- Tighten MAX_GROWTH_FACTOR 2.0 → 1.1; observed growth is ≤1.01× and 2×
would not catch double-injection or buffer-leak regressions
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Round-3 review of the Phase 5 cross-adapter verification suite. All blocking findings from the previous review round are genuinely resolved (route-table introspection replaces wildcard-fallback false positives, the auth-middleware test now targets a protected route, geo and WWW-Authenticate parity assert value equality, the clippy gate uses --all-targets with inlined [lints.clippy], and the HTML growth bound is tightened to 1.1×). CI is green on the current head. Approving — remaining items are non-blocking.
This is a test/CI-only change (the sole non-test edit is a build-script create_dir_all guard), so merge risk is minimal.
Non-blocking
⛏ nitpick
- Stale growth-factor comment: comment says "2×" while the constant is 1.1× (html_processor.rs:1286) — inline.
♻️ refactor / 🌱 seedling / 📌 out of scope
- Parity helpers rebuild the router per request (parity.rs:23) — inline.
- Bench measures one input shape — add a zero-rewrite baseline (html_processor_bench.rs:41) — inline.
- Edgezero
revduplicated outside the workspace dep table (integration-tests/Cargo.toml:29) — inline; suggest a follow-up issue.
🤔 thinking
all_explicit_routes_are_registeredis a hand-maintained allowlist: it guards against route removal but won't catch a newly-added route that silently bypasses auth. Acceptable as a regression guard — flagging the asymmetry.
📝 note
- Parity scope: both adapters delegate to shared
trusted-server-core, so this suite proves adapter-wiring equivalence (middleware order, auth gate, geo header), not divergent core logic. That's the right scope — worth stating so the suite isn't over-trusted as full behavioral equivalence.
CI Status
- fmt: PASS
- clippy (incl. parity crate,
--all-targets): PASS - rust tests (axum, cloudflare, core, cross-adapter parity): PASS
- js tests / format / docs format: PASS (no JS/docs changes)
Conflict resolutions: - .github/workflows/test.yml: keep both PR18's benchmark smoke step and PR17's Fastly WASM release build verification step - crates/integration-tests/Cargo.toml: union of PR18's parity test deps and lints with PR17's urlencoding; lock file regenerated - crates/integration-tests/Cargo.lock: regenerated from merged manifest Semantic fixes for the hardened get_settings() brought in by PR17: - Add TrustedServerApp::routes_with_settings() to the Cloudflare adapter (mirroring the Axum seam) and split routes() into build_state_with_settings + build_router - Parity tests build both routers from shared explicit test settings instead of routes(), which now returns the startup error router for the baked placeholder secrets. Previously seven parity tests passed vacuously by comparing identical 500s - Axum and Cloudflare adapter route tests build through the settings seam; route-table introspection now inspects the real route table rather than the startup-error fallback table - Test handler regex ^/(_ts/)?admin covers the adapter-level /admin routes asserted by the 401 tests and the /_ts/admin paths required by settings validation Review follow-ups: - Update stale 2x growth comment to match MAX_GROWTH_FACTOR = 1.1 - Document the dual-bump requirement for the edgezero git rev pinned in the workspace-excluded integration-tests manifest
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review Summary
Requesting changes. I found one merge-blocking CI failure in the newly added parity job, one high-risk auth coverage gap that the new tests currently mask, and one weaker-than-intended parity assertion.
| run: cargo test --manifest-path crates/integration-tests/Cargo.toml --test parity | ||
|
|
||
| - name: Clippy (parity test crate) | ||
| run: cargo clippy --manifest-path crates/integration-tests/Cargo.toml --all-targets -- -D warnings |
There was a problem hiding this comment.
P0 / Blocker — New parity CI job is red
This command currently fails at HEAD. The new [lints.clippy] block in crates/integration-tests/Cargo.toml applies to the whole excluded crate, and --all-targets also lints the pre-existing integration target. CI is failing on existing violations including tests/common/ec.rs:309 (collapsible_if), tests/frameworks/scenarios.rs:538 and :728 (redundant_closure_for_method_calls), and tests/integration.rs:205 (clippy::panic).
Why it matters: this PR cannot merge with the new required parity check failing.
Suggested fix: either fix those clippy violations, or if the intent is to gate only the new parity test target, narrow this to cargo clippy --manifest-path crates/integration-tests/Cargo.toml --test parity -- -D warnings.
| let settings = Settings::from_toml( | ||
| r#" | ||
| [[handlers]] | ||
| path = "^/(_ts/)?admin" |
There was a problem hiding this comment.
P1 / High — Admin auth tests use a broader regex than production validation requires
The tests pass by protecting both /admin/... and /_ts/admin/... with ^/(_ts/)?admin, but the default config and Settings::ADMIN_ENDPOINTS only require coverage for /_ts/admin/.... The Axum/Cloudflare route tables register /admin/keys/* paths, so a production-shaped config that passes settings validation with path = "^/_ts/admin" can leave /admin/keys/rotate and /admin/keys/deactivate outside AuthMiddleware. The Axum test setup has the same issue.
Why it matters: the verification suite gives false confidence for the admin auth parity contract and can miss an unprotected admin alias.
Suggested fix: align routes and validation. Prefer registering/testing the documented /_ts/admin/keys/* paths on Axum/Cloudflare, and either remove /admin aliases or require handler coverage for both aliases. Add a regression test using the production-shaped path = "^/_ts/admin" config.
| let (axum_status, _) = axum_post_headers("/auction", r#"{"adUnits":[]}"#).await; | ||
| let (cf_status, _) = cf_post_headers("/auction", r#"{"adUnits":[]}"#).await; | ||
|
|
||
| assert_ne!(axum_status, 401, "Axum /auction must not 401"); |
There was a problem hiding this comment.
P2 / Medium — Auction parity test does not assert status parity
This test only proves neither adapter returns 401. It would still pass for important divergences such as Axum returning 400 while Cloudflare returns 500, or one adapter accidentally falling through to 404.
Why it matters: /auction is a core adapter contract, and this module's stated goal is same-route status parity.
Suggested fix: add assert_eq!(axum_status, cf_status, ...) here, and preferably also assert neither side returned 404 so routing regressions are explicit.
3 participants
Summary
paritytest binary incrates/integration-teststhat drives the Axum and Cloudflare adapters with identical requests in-process, proving behavioral equivalence before cutoverChanges
crates/trusted-server-adapter-cloudflare/tests/routes.rscrates/trusted-server-adapter-axum/tests/routes.rscrates/trusted-server-adapter-cloudflare/Cargo.tomlbase64dev-dependencycrates/trusted-server-adapter-axum/Cargo.tomlbase64dev-dependencycrates/integration-tests/Cargo.toml[[test]] paritybinary + adapter path deps + edgezero git depscrates/integration-tests/tests/parity.rscrates/trusted-server-core/src/platform/http.rsPlatformResponse::backend_nameunit tests (error-correlation, pre-EdgeZero #213)crates/trusted-server-core/src/html_processor.rscrates/trusted-server-core/Cargo.toml[[bench]] html_processor_benchentrycrates/trusted-server-core/benches/html_processor_bench.rs.github/workflows/test.ymltest-parityCI job + benchmark smoke step intest-axumjobdocs/superpowers/plans/2026-05-20-pr18-phase5-verification.mdCloses
Closes #499
Test plan
cargo fmt --all -- --checkcargo clippy-axumcargo test-axum(16 tests pass)cargo test-cloudflare(22 tests pass)cargo test --manifest-path crates/integration-tests/Cargo.toml --test parity(8 tests pass)cargo test --lib -p trusted-server-core(956 tests pass)cargo bench -p trusted-server-core --bench html_processor_bench -- --test(smoke passes)cargo test-fastly(Viceroy-based — not run locally; covered by existing CI job)Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)