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.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Round-4 review of the Phase 5 cross-adapter verification suite. The architecture and test coverage are right, but the PR is currently merge-blocked by its own new CI gate, plus two test-fidelity gaps that weaken the parity/auth guarantees the suite is meant to provide.
These overlap with the open ChristianPavilonis review (2026-06-10) — they remain unresolved at HEAD.
Blocking
🔧 wrench
test-parityjob is red at HEAD (.github/workflows/test.yml:141): the new crate-wide[lints.clippy]config +clippy --all-targets -- -D warningslints pre-existing test code for the first time. Reproduced locally — 4 errors:panic(integration.rs:205),collapsible_if(common/ec.rs:309),redundant_closure(frameworks/scenarios.rs:538,:728).- Auction parity asserts no status equality (
parity.rs:382): only checks!= 401per adapter; a real divergence passes silently.
❓ question
- Admin auth tests use a broader regex than production (
adapter-axum/tests/routes.rs:22,cloudflare/tests/routes.rs:24,parity.rs:33): test config^/(_ts/)?adminvs production^/_ts/admin. Registered routes are/admin/keys/*(no_ts), so under production config they would not be auth-gated (handler is inert501, so low real impact). The "auth parity verified" claim is misleading and hides a/admin/*vs/_ts/admin/*path inconsistency withvalidate_admin_coverage(settings.rs:850).
Non-blocking
🤔 thinking
is_some()JSON parity (parity.rs:212) is weaker than the parity goal — both returning arbitrary JSON passes.unwrap_or_else(|_| panic!(...))(build.rs:49) — build-script panic is fine, butexpect("should ...")matches conventions and preserves theio::Error.
📝 note
- The edgezero git
revis duplicated incrates/integration-tests/Cargo.toml:37vs the workspace pin (already documented in an in-file comment; flagging the drift risk).
CI Status
- fmt: PASS
- clippy (fastly/axum): PASS
- cargo test (core): PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): FAIL — see blocking finding above
- vitest / docs / ts format / integration / browser: PASS
…-adapter' into feature/edgezero-pr18-phase5-verification # Conflicts: # crates/trusted-server-adapter-axum/Cargo.toml
The new crate-wide [lints.clippy] block plus clippy --all-targets -D warnings lints pre-existing test code for the first time. Resolve the four violations the new parity CI gate surfaces: - collapsible_if in common/ec.rs - redundant_closure_for_method_calls in frameworks/scenarios.rs (x2) - panic in integration.rs (replace unwrap_or_else+panic with assert)
Resolve the test-fidelity gaps from the round-4 review:
- Register canonical /_ts/admin/keys/* routes on the Axum and Cloudflare
adapters (matching Settings::ADMIN_ENDPOINTS and the Fastly adapter), and
switch the route/parity tests to a production-shaped ^/_ts/admin handler
regex. The auth-gating assertions now target the canonical paths that
production config actually protects; legacy /admin/keys/* aliases are kept
for parity and documented as reaching the handler directly.
- Assert cross-adapter status equality (and non-404 routing) for /auction
instead of only checking each adapter does not 401.
- Strengthen the discovery JSON parity check to compare the full parsed
bodies rather than mere JSON-parsability.
- Use expect("should ...") in build.rs instead of unwrap_or_else+panic.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review:
Review Summary
Reviewed PR #725 against feature/edgezero-pr17-cloudflare-adapter, including the cross-adapter parity suite, adapter route/auth changes, CI workflow updates, and HTML/error-correlation tests. CI is currently green. I did not find blocking correctness, security, data-loss, authorization, or severe compatibility issues. I found one remaining medium-confidence test-fidelity gap where a parity assertion can still pass without comparing response bodies.
Findings
P0 / Blockers
None.
P1 / High
None.
P2 / Medium
- Discovery body parity still passes when both responses are non-JSON —
crates/integration-tests/tests/parity.rs:213- Issue: The test parses both bodies as
Option<Value>and compares those options. If both adapters return non-JSON error bodies, both parse results areNone, so the assertion passes without comparing the actual bodies. With the current in-process settings/runtime services,handle_trusted_server_discoverycan fail before producing the discovery JSON because the request-signing config store/JWKS are unavailable, making this a realistic path rather than only a theoretical edge case. - Why it matters: This suite is intended to prove adapter parity. A regression where Axum and Cloudflare return different discovery error payloads, or both stop returning the JSON discovery document, can still pass this test.
- Suggested fix: Either seed the test runtime with signing/JWKS data and assert both parsed JSON values are
Someand equal, or compare raw body bytes/content type when parsing fails soNone == Noneis not treated as body parity.
- Issue: The test parses both bodies as
P3 / Low
None.
CI / Existing Reviews
GitHub checks are green on the current head, including fmt, Rust tests, Cloudflare checks, cross-adapter parity, JS tests, docs/TS formatting, and browser/integration tests. Prior blocking review threads around route introspection, auth regex/path coverage, clippy gating, and status/header parity appear addressed at HEAD.
| // must be identical across adapters (not merely both parsable as JSON). | ||
| let axum_json: Option<Value> = serde_json::from_slice(&axum_body_bytes).ok(); | ||
| let cf_json: Option<Value> = serde_json::from_slice(&cf_body_bytes).ok(); | ||
| assert_eq!( |
There was a problem hiding this comment.
Automated review: Discovery body parity still passes when both responses are non-JSON.
This compares Option<Value> parse results, so if both adapters return non-JSON error bodies, both sides become None and the assertion passes without comparing the response body at all. That can happen on the current in-process discovery path when JWKS/config-store data is unavailable, so this does not fully prove body parity.
Please either seed signing/JWKS data and assert both parsed JSON values are Some and equal, or compare raw body bytes/content type when parsing fails so None == None is not treated as parity.
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!)