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.
The discovery body parity assertion compared Option<Value> parse results, so two non-JSON error bodies both became None and the assertion passed without comparing the bodies. On the in-process path both adapters return a 500 plain-text error when signing/JWKS data is unavailable, so this was a realistic gap, not a theoretical one. Assert both adapters agree on body type (JSON vs non-JSON), compare parsed JSON values when present, and fall back to raw byte comparison otherwise so diverging error payloads are caught.
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, focusing on the new cross-adapter verification suite, adapter route/auth wiring, and CI gates. CI is green, and the prior parity-test assertion gaps appear addressed. I found one blocking authorization issue: the branch keeps real /admin/keys/* legacy key-management aliases registered while the production-shaped/default auth regex and Settings::ADMIN_ENDPOINTS only protect /_ts/admin/keys/*.
Findings
P0 / Blockers
- See inline comment: legacy admin key aliases can bypass Basic auth under the default production admin handler coverage.
P1 / High
None.
P2 / Medium
None.
P3 / Low
None.
CI / Existing Reviews
GitHub checks are currently green, including fmt, Rust tests, Cloudflare checks, cross-adapter parity, JS tests, docs/TS formatting, and browser/integration tests. Existing review feedback around route-introspection, header/status parity, clippy gating, canonical admin route coverage, and discovery body comparison appears addressed at HEAD.
The Cloudflare and Fastly adapters registered legacy `/admin/keys/*` aliases that routed to the real handle_rotate_key/handle_deactivate_key handlers. The production basic-auth handler regex `^/_ts/admin` does not match those paths, so AuthMiddleware never challenged them and unauthenticated callers could reach the key handlers. Remove the aliases from the Cloudflare router and from both Fastly routers (the EdgeZero NAMED_ROUTES table and the legacy route_request match); unrouted, they fall through to the publisher/organic fallback like any unknown path. The canonical `/_ts/admin/keys/*` routes stay auth-gated. Drop the stale aliases from Cloudflare's route-registration test, retarget the Fastly consent-store admin test to the canonical path, and replace the Fastly alias test with a production-shaped guard asserting the legacy paths are not in NAMED_ROUTES. Add a Cloudflare parity regression test asserting the aliases are not auth-gated admin routes and fall through like an unknown path. Note: the Axum adapter already routes every admin path to AdminNotSupported (501) without reaching a key handler, so it carries no equivalent unauthenticated surface and is left unchanged.
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 current head d5bcd8a0dc5800a893e28be2144ac7159487d9c0, the adapter route/auth changes, the cross-adapter parity suite, core HTML/error-correlation tests, benchmark smoke gate, and CI workflow. GitHub checks are green, and I did not find blocking correctness, security, data-loss, authorization, or severe compatibility issues.
I left one inline P2 finding: the new Cloudflare legacy-admin-alias regression test should assert route-table absence directly, because the current behavioral comparison can still pass if a reintroduced unauthenticated key handler happens to return the same status as the publisher fallback in the no-store test environment.
CI / Existing Reviews
Current PR checks are passing, including fmt, Rust tests, Cloudflare checks, cross-adapter parity, JS tests, docs/TS formatting, and browser/integration tests. Earlier review threads around wildcard route smoke tests, header/status parity, clippy gating, canonical admin path coverage, discovery body comparison, and removal of unauthenticated legacy key aliases appear addressed at HEAD.
| ); | ||
|
|
||
| let (unknown_status, _) = cf_post_headers("/this-route-does-not-exist-abc123", "{}").await; | ||
| assert_eq!( |
There was a problem hiding this comment.
Automated review: P2 — legacy admin alias regression guard can false-pass if the handler returns the fallback status
This test is meant to prove the Cloudflare /admin/keys/* aliases are not registered, because a reintroduced alias would bypass the production-shaped ^/_ts/admin auth regex and reach the real key handlers unauthenticated. The assertions here only compare runtime behavior against an unknown publisher-fallback path. In this no-store/no-origin test environment, a reintroduced key handler can plausibly fail with the same 5xx status as the fallback while also having no WWW-Authenticate header, so the regression this test is guarding against could slip through.
Suggested fix: add a route-table negative assertion for Cloudflare, analogous to the Fastly NAMED_ROUTES guard: collect cf_router().routes() and assert that POST /admin/keys/rotate and POST /admin/keys/deactivate are absent, while the canonical /_ts/admin/keys/* routes remain present. Keep the behavioral fallback check as a secondary assertion if desired.
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!)