[vasujain00]

Summary

Implements the origin cache-header audit designed in the #834 spec (PR #930). ts dev audit headers fetches origin responses — explicit URLs or discovered from the origin root HTML — classifies each by content type, evaluates cache directives, and reports a per-type pass/warn/fail verdict.

Changes

Area	Change
commands/dev/audit/mod.rs	New DevAuditCommand group + exit-code dispatch
commands/dev/audit/headers/rules.rs	ContentTypeGroup taxonomy, unit Verdict enum, per-group cacheability rules
commands/dev/audit/headers/fetch.rs	AuditHeadersArgs, OriginClient trait + reqwest blocking impl, HTML-parse discovery
commands/dev/audit/headers/analyze.rs	Worst-of rollup per group, summary, exit-code mapping
commands/dev/audit/headers/output.rs	Writer-injected human table + JSON
commands/dev/mod.rs	Adds cross-platform Audit variant to existing DevCommand
Cargo.toml	reqwest (blocking) scoped to cfg(not(target_arch = "wasm32"))

Design fidelity

• no-store alone passes HTML/RTB (RFC 9111 §5.2.2.5)
• s-maxage honored as CDN TTL; Vary (User-Agent/Cookie) checked on all cacheable groups
• Surrogate-Key only checked on cacheable groups; MIME params stripped before classification
• Discovery HTML-parses the origin root — never probes edge-only /_ts/ routes
• Audit is available on all host platforms (unlike the macOS-only proxy)

Closes

Refs #834

Test plan

• cargo test --package trusted-server-cli --target <host> — 105 pass (25 new)
• cargo clippy --package trusted-server-cli --all-targets -- -D warnings — clean
• cargo fmt --all -- --check — clean
• ts dev audit headers --help renders

Checklist

• Follows AGENTS.md conventions (CliResult string errors, writer injection)
• No unwrap() in production code
• Uses log macros (not println!) outside the output sink
• New code has tests
• No secrets committed

[aram356]

Summary

Well-structured module: the pure rules.rs / analyze.rs split, the injected OriginClient and Write sink, and 25 colocated tests make the logic genuinely testable without network I/O. The blockers below are about the new dependency's effect on the rest of the CLI, and about cases where the audit reports a clean bill of health on postures it exists to catch.

Every finding below was verified by running code against this head, not by inspection alone.

2 of the inline comments carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch) to apply them. The remaining comments describe the fix in prose because the change spans multiple files or lines outside the diff.

Blocking

🔧 wrench

• Adding reqwest breaks ts dev proxy at runtime and fails CI — see inline at crates/trusted-server-cli/Cargo.toml:28
• Vary: * silently passes on JS, CSS, and images — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:525
• Exit code 2 collides with the CLI's own error code — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/analyze.rs:61
• Discovery fetches arbitrary cross-origin URLs — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:210
• Redirects mask the audited URL's real cache headers — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:78

Non-blocking

🤔 thinking / ♻️ refactor / ⛏ nitpick / 📝 note

• s-maxage is not honored as a CDN TTL, contrary to the PR description — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:393
• Surrogate-Control: no-store on cacheable groups is never flagged — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:397
• max-age = 3600 with spaces loses its value — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:206
• Redundant double-Option match — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:356
• ts dev audit sits alongside an unrelated top-level ts audit — see inline at crates/trusted-server-cli/src/commands/dev/audit/mod.rs:16

Cross-cutting / body-level findings

• 🔧 CI gate cargo test (ts CLI, native) is failing — 196 passed, 4 failed. All four are dev::proxy TLS tests panicking in rustls with "Could not automatically determine the process-level CryptoProvider". This is not flaky and not pre-existing: I built both sides and ran them locally. Base 066ea3c6 passes 175/175; this head fails 4. Root cause and fix are in the Cargo.toml:28 comment. The check is not in branch protection's required list, but AGENTS.md treats the adapter test aliases as PR gates.

• 📝 No documentation for the new command. docs/ has no reference to ts dev audit headers. Given the exit-code contract is meant for CI gating, the codes and their meanings are worth writing down wherever the other ts commands are documented.

CI Status

• cargo test (ts CLI, native): FAIL
• cargo fmt: PASS (required)
• cargo test: PASS (required)
• format-typescript: PASS (required)
• format-docs: PASS (required)
• cargo test (axum native): PASS
• cargo test (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• vitest: PASS
• integration tests: PASS
• browser integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• prepare integration artifacts: PASS
• CodeQL: PASS
• Analyze (rust): PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS
• CLAUDE.md symlink guard: PASS

[prk-Jr]

Staff-level review pass. Read the full diff against origin/main in a clean worktree at da37b83d and verified each finding against the source; everything reported below was reproduced locally rather than reasoned about, except where explicitly noted.

Verdict

Requesting changes on two blockers. The feature itself is well built - the OriginClient trait plus the injected &mut dyn Write sink let the whole pipeline (fetch -> discovery -> classify -> evaluate -> render) run in unit tests with zero network and zero stdout, and 25 new colocated tests come with it. But the PR ships a dependency-feature regression that breaks an unrelated command and fails CI, plus an exit-code contract that collides with the CLI's existing one and so defeats the CI-gating purpose the command exists for.

Blockers

1. CRITICAL - Cargo.toml:28. Inheriting the workspace reqwest pulls rustls/ring alongside the rustls/aws-lc-rs that edgezero-cli already brings in. rustls can then no longer resolve a process-level CryptoProvider, so four pre-existing ts dev proxy tests panic and ts dev proxy itself panics at runtime. The test-cli CI job fails on this PR.
2. HIGH - audit/mod.rs:36. Exit 2 means "warnings only" here, but main.rs:8 already uses exit 2 for "command failed". A CI gate cannot tell a warning origin from a crashed run.

Non-blocking, but worth fixing before this becomes a gate publishers trust

private-alone HTML is graded Fail (RFC 9111 5.2.2.7); Vary: * passes on immutable assets where it is most destructive; discovery fans out to third-party hosts and folds their verdicts into the publisher's rollup; the CDN-undercuts-browser check never fires for s-maxage. Details inline.

Verification

Worktree at da37b83d, host aarch64-apple-darwin:

Gate	Result
cargo fmt --all -- --check	PASS (exit 0, no output)
cargo clippy -p trusted-server-cli --all-targets --all-features -- -D warnings	PASS (no warnings)
cargo test --manifest-path crates/trusted-server-cli/Cargo.toml (CI-equivalent)	FAIL - 196 passed, 4 failed
same, at merge-base 066ea3c6	PASS - 175 passed, 0 failed
same, with the Cargo.toml fix applied	PASS - 200 passed, 0 failed
same, with the rules/fetch fixes applied	196/4 - no new failures; all 25 new audit tests green

One gap I could not close: CI pins the Linux triple for CLI clippy (format.yml:83), and --target x86_64-unknown-linux-gnu fails here for lack of a cross-linker. The DevCommand shape is identical on both targets so I expect it is fine, but this repo has prior history of host-gated CLI modules passing locally and breaking Linux CI jobs - worth watching the actual run rather than assuming.

Notably good

• Testability design. Splitting run_audit_headers out of run purely so the sink and client are injectable is the right call, and the module docs say so explicitly.
• Dependency footprint. Cargo.lock grows by exactly one line - reqwest 0.12.28 was already in the graph via edgezero-adapter-axum, so no new transitive crates. The cfg(not(target_arch = "wasm32")) scoping correctly excludes the repo-default wasm32-wasip1, so no adapter build is touched. The problem is purely the feature selection, not the dependency.
• MAX_DISCOVERED_URLS = 50 is a real cap, enforced both in push_resolved and again on the favicon fallback.
• Discovery never probes /_ts/ edge routes against the origin, and discovery_fetches_root_and_assets_but_never_ts_routes asserts it - a good defensive test for a non-obvious invariant.
• Conventions clean throughout: no unwrap() in production paths, no println! outside the output sink, expect("should ...") form honoured, doc comments and # Errors sections on public items.

[prk-Jr]

LOW - redirect policy is left implicit.

reqwest's default is follow-up-to-10-hops, so the audit reports the final hop's headers under the requested URL and never audits the redirect's own cache headers. An origin root that 301s to /en/ with Cache-Control: public, max-age=31536000 on the 301 itself - a classic misconfiguration that pins users to the wrong locale for a year - is reported as Pass.

Secondarily, redirects are followed without a scheme or host check, so a redirect to a link-local or loopback address is fetched. Low real-world severity given the origin is operator-supplied, but it deserves an explicit decision rather than an inherited default.

Suggest setting an explicit .redirect(...) policy on the builder and either reporting the redirect's own headers or documenting the follow behaviour. Marked as reasoned-not-exercised: I read the code and reqwest's documented default but did not run a live redirect.

[vasujain00]

Thanks @aram356 and @prk-Jr — both reviews were thorough and everything reproduced. Pushed a revision addressing all findings, blocking and non-blocking. Verified two ways: 445 CLI unit tests (+11 new pinning the fixed behaviours), and end-to-end by running the built ts binary against a local origin.

One correction worth surfacing (details in the Cargo.toml thread): the -no-provider reqwest fix alone is not sufficient — it works on macOS because the proxy brings aws-lc-rs, but the proxy is macOS-gated, so on Linux (where the test-cli job runs) the audit's own reqwest is left with no rustls provider and panics No provider set at runtime. Final fix keeps -no-provider (avoids the ring/aws-lc-rs collision) and additionally pins rustls (aws-lc-rs) in the all-host deps + install_default() in the audit client. Cargo.lock is unchanged.

Re-requesting review.

[vasujain00]

Update — review addressed (rev pushed)

Addressed all feedback from @aram356 and @prk-Jr, blocking and non-blocking. The test-cli failure is fixed: reqwest now uses rustls-tls-webpki-roots-no-provider so it no longer pulls rustls/ring alongside the proxy's aws-lc-rs (Cargo.lock unchanged). Running the built binary on Linux surfaced that -no-provider alone leaves the audit's own reqwest with no provider (the proxy's aws-lc-rs is macOS-gated), so rustls (aws-lc-rs) is now pinned in the all-host deps and install_default()-ed in the audit client — single provider on every host. Also: warn-only exits 3 (2 is reserved for CLI errors); Vary: * is flagged on JS/CSS/image; discovery is restricted to the audited origin (--include-cross-origin to opt in) and redirects are no longer followed; private alone passes HTML (RFC 9111 §5.2.2.7); Cache-Control: s-maxage is honored as the CDN TTL; Surrogate-Control: no-store is flagged on cacheable groups; directive parsing tolerates whitespace around = and keeps the first valid value (RFC §4.2.1); body reads are capped at 2 MiB; the explicit-URL path tolerates per-URL failures; and docs/guide/cli.md documents the command and its exit-code contract. Verified: 445 CLI unit tests (+11 new), clippy -D warnings clean, fmt clean, plus end-to-end runs of ts dev audit headers against a local origin.

[aram356]

Summary

Round 2 against 6cf2d03d. Every round-1 finding is fixed, and I re-verified each by running code rather than reading the diff: ring is gone from the dependency tree on macOS and Linux, ts dev proxy starts cleanly, a real HTTPS fetch works, 540 CLI tests pass, and all 19 CI checks are green. The seven new unit tests pin the fixed behaviours.

The blockers below are almost all regressions introduced by the fix commit itself — three of the five come from the rewritten CDN-TTL block and the new evaluate_surrogate_disables_caching. The direction of the fix was right; the new branch structure needs a case the tests don't currently cover: Surrogate-Control present but carrying no numeric TTL.

1 of the inline comments carries a one-click GitHub suggestion. The rest describe the fix in prose because they span multiple call sites or need a design decision.

Blocking

🔧 wrench

• Surrogate-Control: no-store produces an affirmative PASS on CDN TTL — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:408
• Two Surrogate-Control findings collapse into one; the actionable one is dropped — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/analyze.rs:148
• Surrogate-Control: max-age=0 on JS/CSS yields no verdict at all — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:352
• Qualified private="Set-Cookie" is accepted as unqualified — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:271
• public, private passes HTML with no contradiction check — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:267

Non-blocking

🤔 thinking / ♻️ refactor / ⛏ nitpick / 📝 note

• Include the design spec in this PR — see inline at docs/guide/cli.md:653
• Conflicting duplicate max-age takes the first instead of invalidating — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:214
• CDN/browser TTL check is skipped when max-age is absent — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:416
• application/woff and application/woff2 classify as Other — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:61
• RTB accepts bare no-store, diverging from spec §4.5 — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:463
• "s-maxage" is emitted as a header name — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:429
• flag_wildcard is now a dead parameter and its doc comment is stale — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:578
• Redundant double-Option match — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:365
• reqwest is hard-pinned to 0.12 instead of workspace = true — see inline at crates/trusted-server-cli/Cargo.toml:40
• Human output diverges from the spec's table format — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/output.rs:52

Cross-cutting / body-level findings

• 📝 Test coverage gap behind the three regressions. evaluate_image's rewritten TTL block and the new evaluate_surrogate_disables_caching are both exercised only with inputs where Surrogate-Control either carries a number or is absent. The three blocking regressions all live in the third case — present, but token-only (no-store / private / no-cache). A table-driven test over the matrix of {no Surrogate-Control, token-only, numeric} x {Cache-Control has s-maxage, does not} would have caught all three and is worth adding alongside the fixes.

CI Status

• cargo test (ts CLI, native): PASS
• cargo fmt: PASS (required)
• cargo test: PASS (required)
• format-typescript: PASS (required)
• format-docs: PASS (required)
• cargo test (axum native): PASS
• cargo test (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• vitest: PASS
• integration tests: PASS
• browser integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• prepare integration artifacts: PASS
• CodeQL: PASS
• Analyze (rust): PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS
• CLAUDE.md symlink guard: PASS

[prk-Jr]

Summary

Round 3 against 6cf5e4d0. All five round-2 blockers are fixed and each is pinned by a test. The new {absent, token-only, numeric} Surrogate-Control matrix is exactly the coverage that was missing. The rule logic is now in good shape.

This round's blockers are about what the audit actually fetches and reads: HTTP status is discarded, only the first line of repeated headers is read, and Vary is graded on HTML. I found and confirmed all three by running the built ts binary against a local origin. They produce a false FAIL (exit 1) on a correctly configured origin, a false PASS (exit 0) on a bot-wall page, and a false warn (exit 3) on correct personalized HTML, which is a problem for a command meant to gate CI.

7 of the inline comments carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. Each was scratch-verified alone at this head (Rust: fmt, clippy-cli, dev::audit tests; docs: prettier), and the six Rust suggestions also pass applied together. The remaining comments describe the fix in prose because it spans multiple files or lines outside one contiguous range.

Blocking

🔧 wrench

• Non-2xx responses graded as origin posture (false FAIL via unlinked favicon 404; false PASS on 403 bot wall) — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:118
• Only the first line of repeated Vary / Cache-Control headers is read — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:137
• Vary graded on HTML, warning on a correct posture (exit 3) — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:334
• Docs use a real domain (origin.publisher.com) — see inline at docs/guide/cli.md:723

Non-blocking

🤔 thinking / ♻️ refactor / ⛏ nitpick

• No User-Agent sent — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:96
• Duplicates crate::tls::install_crypto_provider(); stale macOS-only comment — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:91
• (header, expected) merge key splits Vary into contradictory rows — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:654
• Immutable posture accepts no-store / private — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:359
• Fonts and module chunks never discovered — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:299
• Merged verdicts lose the URL that produced them — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/analyze.rs:160
• Origin resolution bypasses the shared app_config::load_settings — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:217
• Asset bodies downloaded then discarded; root body-read error swallowed — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:115

Cross-cutting / body-level findings

• 🔧 Merge conflicts with main. GitHub reports the PR as CONFLICTING. Cargo.toml, commands/dev/mod.rs, and docs/guide/cli.md conflict with #1171 (Linux ts dev proxy) and #733 (ts dev lint / install-hooks). When resolving:
  • dev/mod.rs on main now has Lint, InstallHooks, SandboxProbe and a finish exit-code helper, so Audit needs to slot in alongside them.
  • The new Cargo.toml comment ("ts dev proxy only exists on macOS") is no longer true.
  • This PR also removes the trailing newline at the end of Cargo.toml.
• 📝 Round-2 non-blocking items with no reply or change yet, listed so they're decided rather than lost:
  • conflicting duplicate max-age still resolves first-wins instead of invalid (max-age=60, max-age=31536000 → 60 → Warn, re-confirmed);
  • the CDN/browser TTL comparison is skipped when max-age is absent;
  • application/woff / application/woff2 still classify as Other;
  • the RTB no-store vs spec §4.5 divergence;
  • the hard reqwest = "0.12" pin vs the workspace version;
  • human output still omits actual / expected;
  • the design spec still lives only in #930.
• 👍 Tests and directive model. Tracking qualified vs bare directives (unqualified) is the right model for RFC 9111 §5.2.2.7, and the round-2 regressions each got a targeted test.

CI Status

• cargo fmt: PASS (required)
• cargo test: PASS (required)
• format-typescript: PASS (required)
• format-docs: PASS (required)
• cargo test (ts CLI, native): PASS
• cargo test (axum native): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• vitest: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• browser integration tests: PASS
• prepare integration artifacts: PASS
• CodeQL: PASS
• Analyze (rust): PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS
• CLAUDE.md symlink guard: PASS

Locally at this head (aarch64-apple-darwin): 595 CLI lib tests pass, cargo clippy-cli clean, cargo fmt --check clean. Note that CI passes against the PR's merge commit, while GitHub reports the branch as conflicting with current main.

[prk-Jr]

🔧 wrench — Vary is evaluated on HTML, which contradicts the PR description ("Vary (User-Agent/Cookie) checked on all cacheable groups") and warns on a correct posture.

HTML here must be no-store or private, and neither is stored by the CDN, so the hit-ratio argument doesn't apply. For personalized private HTML, Vary: Cookie is the correct companion header. Reproduced with the built binary:

Cache-Control: no-store
Vary: Cookie

⚠ HTML
    ✓ Cache-Control      --
    ⚠ Vary               Vary on User-Agent/Cookie destroys the CDN hit ratio
exit=3

Since exit 3 is the warn signal for CI gates, a correctly configured origin gets a permanent warning it can't fix without making things worse.

Proposed fix (apply manually — it also needs the vary_wildcard_warns_on_html test at rules.rs:793 flipped to assert no Vary verdict, which is outside a single contiguous suggestion range): drop verdicts.extend(evaluate_vary(headers)); from evaluate_html. The cacheable groups keep the check.

[prk-Jr]

🤔 thinking — The immutable posture accepts a Cache-Control that also forbids storage. I confirmed public, max-age=31536000, immutable, no-store rolls up to Pass for a JS bundle, even though neither the browser nor the CDN will store it.

This becomes more likely once multi-line headers are combined (see fetch.rs:137), e.g. a framework's immutable header plus a middleware's blanket no-store line. evaluate_image (rules.rs:405) has the same gap and would want the same two conditions.

(compile-verified only — please re-run ./scripts/test-cli.sh after applying)

Suggested change

	let immutable = directives.has("public")
	&& directives.has("immutable")
	&& directives
	.max_age
	.is_some_and(|age| age >= IMMUTABLE_MAX_AGE);
	// `no-store` / `private` override the long TTL: the bundle is then
	// never stored by the CDN, however long its `max-age`.
	let immutable = directives.has("public")
	&& directives.has("immutable")
	&& !directives.has_unqualified("no-store")
	&& !directives.has_unqualified("private")
	&& directives
	.max_age
	.is_some_and(|age| age >= IMMUTABLE_MAX_AGE);

[prk-Jr]

🤔 thinking — When a worse verdict replaces an existing one, the report keeps its actual value but not the URL it came from. For a group with many sampled URLs, the operator can see that an image failed and what the bad header value was, but not which image.

In the favicon repro on fetch.rs:118, the JSON showed actual: "public, max-age=600" next to urls_sampled: ["/", "/favicon.ico"], and nothing linked the two. Since this is meant as a CI gate, the failing URL is the first thing someone acting on the result needs. Suggest adding a url: String field to HeaderVerdict (set from add's url) and rendering it, at least in --json.

[prk-Jr]

🤔 thinking — Origin resolution hand-parses trusted-server.toml (--config, default ./trusted-server.toml). The other config-reading commands go through crate::app_config::load_settings (AppConfigArgs: edgezero.toml manifest → <app.name>.toml, plus the EdgeZero env overlay). app_config.rs's module doc says that sharing exists so "config-path resolution and the EdgeZero environment overlay stay consistent across command families".

As written, TRUSTED_SERVER__PUBLISHER__ORIGIN_URL is ignored, and a project whose app name isn't trusted-server needs --config here but not elsewhere. Reusing AppConfigArgs + load_settings(...).settings.publisher.origin_url would align it. Apply manually — it changes the CLI flags and imports.

[vasujain00]

@aram356 @prk-Jr — ready for re-review. Rebased/merged onto current main (through d2f0d8c) with conflicts resolved, and the remaining round-2 non-blocking items are now addressed. Note #930 merged, so the design spec is now in main and backs the behavior changes below.

Fixes (squashed into one commit, plus the merge):

• RTB/JSON — now matches the merged spec §3.2/§6.5: origin JSON carries no assumed posture. Bid responses come from the TS edge (POST /auction, GET /_ts/page-bids), not the publisher origin, so the audit no longer treats origin JSON as bid data. ⚠️ Behavior change worth flagging: evaluate_rtb no longer FAILs JSON for cache policy — it emits one informational Pass recording the observed policy, with an add_private note when no-store lacks private (RFC 9111 §5.2.2.5; some CDNs ignore bare no-store and TS itself emits no-store, private). This removes a FAIL path, so cacheable origin JSON now exits 0 instead of 1. The old rule assumed a posture the merged spec explicitly says not to assume.
• Duplicate max-age/s-maxage — reconciled both of your positions: a later unparseable or duplicate-identical directive keeps the first valid value (RFC 9111 §4.2.1, per prk-Jr), but two different valid values are a contradiction and invalidate the directive (per aram356).
• application/woff and application/woff2 now classify as StaticAsset (fonts), not Other.
• Human output now shows the observed actual value on each verdict line (it was already in --json).

Conflict resolution against main (3 files): Cargo.toml keeps rpassword + the existing reqwest/rustls block (dropped a duplicate rustls key); dev/mod.rs keeps the cross-platform Audit variant alongside main's macOS+Linux proxy and lint/install-hooks/sandbox-probe; cli.md keeps both the audit-headers and prebid-server sections.

Verified on the merged tree: 774 CLI tests pass, clippy -D warnings clean, fmt clean. Re-requesting review.

[ChristianPavilonis]

Requesting changes because the audit can report success for unsafe cache policies or incomplete collection.

• Evaluate effective cache policy correctly: unsupported Surrogate-Control tokens must not mask shared TTLs; repeated headers, quoted directives and restrictive asset directives need proper handling.
• Avoid misleading recommendations: reserve immutable caching for fingerprinted URLs and omit cache-hit-ratio warnings for private HTML.
• Preserve HTTP status and configured Host overrides so the audit evaluates the intended resources.
• Make discovery reliable: exclude edge-only routes, honor document base URLs, and expose fetch and root-body read failures in reports and exit status.

The inline comments cover these issues in detail. The Vary-row merge, reserved example hosts and shared test fixture suggestions are non-blocking improvements.

[ChristianPavilonis]

[P2] Preserve HTTP status before applying content rules

Behavioral defect; correctness. send() succeeds for HTTP error responses, but status is discarded. Runtime fixtures showed a 403 challenge page passing with exit 0 and a cacheable HTML 404 failing as personalized content. Discovery's unconditional favicon request exposes the latter on otherwise healthy sites. Carry status into analysis and grade redirects/errors separately, preserving their headers rather than blindly rejecting every non-2xx response. This avoids attributing error-page posture to the intended resource.

[ChristianPavilonis]

[P2] Parse directive separators outside quoted strings

Behavioral defect; correctness. Cache-Control: public, max-age=600, ext="a,no-store,b" manufactures an unqualified no-store directive when split here. The built CLI consequently passed this cacheable HTML with exit 0. Tokenize commas only outside quoted strings, respecting escapes and retaining qualified-directive distinctions. Core's cache_control_value_has_directive already demonstrates quote-aware scanning. This changes parsing, not the intended cache policy.

[ChristianPavilonis]

[P2] Resolve assets against the document's base URL

Behavioral defect; correctness. For <base href="/assets/"><script src="app.js">, discovery requests /app.js, not the browser's /assets/app.js. The fixture observed the wrong request and a passing report while the intended asset remained unaudited. Resolve the first applicable base href against the document URL, then resolve asset references against that effective base. Continue enforcing the original origin restriction so a cross-origin base element does not silently broaden discovery.

[ChristianPavilonis]

[P3] Give the Vary check a consistent merge identity

Minor reporting defect; correctness and architecture, non-blocking. The merge uses (header, expected), but evaluate_vary changes its expected text between passing and warning branches. Two sampled scripts consequently produce separate Pass and Warn Vary rows rather than the documented worst-of merged check; the fixture reproduced this. Use a stable check identity or consistent expected text. Preserve separate Surrogate-Control checks, which genuinely represent different evaluations. The group's overall Warn exit is already correct.

[ChristianPavilonis]

[P3] Share the duplicated in-memory origin client fixture

Maintenance; code-smells lane, Duplicated Code; non-blocking. This fixture and headers/mod.rs:62–94 duplicate the response map, fetched-URL recorder, constructor and OriginClient lookup implementation. Changes to the response contract must update both copies. Extract a small test-only client supporting arbitrary ResponseHeaders, retaining local convenience builders where useful. This removes duplicate fixture mechanics at the cost of a shared test-helper dependency; production behavior is unaffected.

[aram356]

Summary

Round 3 against c71c5d98. Every round-2 code finding is fixed — I re-verified each by running code: public, private now fails, Surrogate-Control: max-age=0 on JS warns, conflicting duplicate max-age invalidates, application/woff2 classifies as a static asset. cargo fmt, cargo clippy-cli, and 772 CLI tests pass locally.

Two caveats on the findings below, because they are not all the same kind of thing.

Two blockers are reopenings. The Surrogate-Control fixes from rounds 1 and 2 each landed on one of two symmetric code paths. Blocker 1 guards a token-only Surrogate-Control but not the token-plus-number case; blocker 2 fixed the has_unqualified requirement on Cache-Control but left the identical body-safety check on Surrogate-Control using has(). Each one produces a clean Pass on a genuinely unsafe or uncached posture, which is the failure this tool exists to prevent.

Four blockers are our fault, not yours. The design spec merged to main three weeks ago in #930, and I am the one who rewrote it. It now specifies a different command surface than this PR implements — ts origin audit-headers, sitting beside the ts origin probe-shareability that has since landed, with a --strict flag instead of a third exit code, HTTPS and private-address refusal, and grading that calls TS's own cache helpers from core. None of that was knowable when this branch was written. Please read findings 4 through 7 as "reconcile the code and the spec, in whichever direction is right" rather than as defects. If you think ts dev audit headers is the better home, say so and I will amend the spec instead — you have built against this surface and have the better view of it.

2 of the inline comments carry a one-click GitHub suggestion. The rest describe the fix in prose because they span several call sites, need a test fixture changed alongside, or need a design decision first.

Blocking

🔧 wrench

• Surrogate-Control: no-store with a numeric max-age still reports a passing CDN TTL — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:489
• Qualified private="Set-Cookie" passes the Surrogate-Control safety check — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:566
• format-docs is failing on this PR's own file — see inline at docs/guide/cli.md:799
• Merged spec places this command at ts origin audit-headers — see inline at crates/trusted-server-cli/src/commands/dev/audit/mod.rs:16
• Exit-code contract contradicts the spec and RunOutcome — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/analyze.rs:66
• Spec-mandated HTTPS and private-address refusal are absent — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:84
• Spec Task 1 is unimplemented: the audit never grades with TS's own cache logic — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:14

Non-blocking

🤔 thinking / ⛏ nitpick / 📝 note

• An invalid or self-contradicting Surrogate-Control TTL emits no verdict at all — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:593
• --include-cross-origin admits non-HTTP schemes from fetched HTML — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:262
• Round-2 regression tests assert rollups, not per-verdict values — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/analyze.rs:331
• No <base href> handling, no not_audited reporting, no browser-like request profile — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:5
• Body cap is 2 MiB where the spec says 5 MiB — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:30
• The favicon fallback is silently dropped at the discovery cap — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:284
• Bare .unwrap() in tests — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:373

Cross-cutting / body-level findings

• 📝 The design spec request from round 2 is satisfied. It landed on main through #930 rather than in this PR, which is fine — the file is now at docs/superpowers/specs/2026-07-14-cache-header-audit-design.md. I am resolving that thread myself; no action needed from you. Note main has also since added a "Require specs for feature PRs" gate (#1385), so the spec being merged separately is what satisfies it.

• 📝 Four of my round-2 threads were fixed but left unresolved, which is why this round started by re-checking them: public, private, Surrogate-Control: max-age=0 on JS/CSS, conflicting duplicate max-age, and application/woff2. All four verified fixed. Resolving a thread when you push the fix makes the next round much cheaper — otherwise a reviewer cannot tell "not addressed" from "addressed, not marked".

• 🤔 The two reopenings share one root cause worth fixing structurally. Both Cache-Control and Surrogate-Control answer the same two questions (does this forbid caching? is the directive qualified?), and the logic is duplicated per header rather than shared. Rounds 1, 2, and 3 have each fixed one copy and missed the other. A single predicate used by both paths — something like CacheDirectives::forbids_caching() built on has_unqualified — would close this class instead of its instances. I verified such a helper works; see the inline comment on rules.rs:489.

CI Status

• format-docs: FAIL (required)
• cargo fmt: PASS (required)
• cargo test: PASS (required)
• format-typescript: PASS (required)
• cargo test (ts CLI, native) (macos-latest): PASS
• cargo test (axum native): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• vitest: PASS
• Analyze (rust): PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS
• lint-domains: PASS
• markdown format: PASS
• integration tests: PENDING
• browser integration tests: PENDING
• integration tests (Fastly EC lifecycle): PENDING
• CodeQL: SKIPPED

[prk-Jr]

Summary

The audit pipeline has four correctness issues: quoted directive parsing can accept shareable HTML, restrictive asset directives can be ignored, repeated fields are lost, and immutable caching is recommended for stable URLs. These should be corrected before merging.

One inline comment carries a one-click GitHub suggestion; the other three describe changes that require broader implementation and regression coverage.

Blocking

• 🔧 [P1] Parse quoted field lists — see inline at rules.rs:224.
• 🔧 [P2] Honor restrictive asset directives — see inline at rules.rs:375–379.
• 🔧 [P2] Preserve repeated headers — see inline at fetch.rs:124–129 (suggestion).
• 🔧 [P2] Restrict immutable recommendations to fingerprinted URLs — see inline at rules.rs:371.

Validation

Reviewed commit e918e7a9584fe95b90149d6061ce2d1e58c4e0a3. All 46 audit tests passed locally both before and after the exact suggestion. Formatting and cargo clippy-cli also passed with the suggestion applied. Separate executable reproductions confirmed the first three findings; the repeated-header reproduction passed after applying the exact suggestion in scratch verification.

CI Status

• integration tests: PENDING
• integration tests (Fastly EC lifecycle): PENDING
• browser integration tests: PENDING
• CodeQL: SKIPPED
• cargo test (ts CLI, native) (macos-latest): PASS
• format-typescript (required): PASS
• cargo test (required): PASS
• cargo fmt (required): PASS
• cargo test (axum native): PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• Analyze (rust): PENDING
• format-docs (required): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• docs-proposal scripts: PASS
• cargo test (cross-adapter parity): PASS
• cargo test (ts CLI, native) (ubuntu-latest): PASS
• CLAUDE.md symlink guard: PASS
• vitest: PASS
• prepare integration artifacts: PASS

[prk-Jr]

🔧 [P1] Parse quoted field lists before splitting cache directives

value.split(',') treats commas inside quoted values as directive separators. For example, Cache-Control: public, max-age=600, private="x,no-store,y" produces an unqualified no-store token and the HTML evaluation returns Pass, although the qualified private only protects the named header fields and leaves the body shareable. This was reproduced against the rules module.

Please use a quoted-string-aware directive parser (including escaped quotes), so only commas outside quoted values separate directives, and add a regression test showing this HTML response fails. Apply manually: the fix needs parser logic and regression coverage rather than a local line replacement.

[prk-Jr]

🔧 [P2] Honor restrictive directives before passing cacheable assets

The positive-token check accepts public, max-age=31536000, immutable, no-store as a passing JS policy. The equivalent image check accepts public, max-age=86400, no-cache. With the other required headers present, both groups roll up to Pass; both outcomes were reproduced. no-store prohibits storage, while no-cache requires validation before reuse, so these policies do not provide the advertised long-lived caching.

Please apply restrictive-directive precedence and flag contradictory policies in both evaluate_immutable and evaluate_image, covering no-store, unqualified private, and no-cache. Apply manually because the fix spans the separate immutable and image checks.

[prk-Jr]

🔧 [P2] Preserve repeated cache-related header fields

HeaderMap::get reads only the first field value. An HTML response with separate Cache-Control: max-age=3600 and Cache-Control: no-store fields is incorrectly graded Fail even though the combined policy forbids storage. Repeated Vary fields can likewise hide a later Cookie or User-Agent value and suppress its warning.

Combine the field values before evaluation (space-separated for Surrogate-Key, comma-separated for the directive lists). Scratch verification confirmed the reproduced HTML case changes from Fail to Pass.

Suggested change

	let value = |name: &str| {
	headers
	.get(name)
	.and_then(|value| value.to_str().ok())
	.map(str::to_owned)
	};
	let value = |name: &str| {
	let values: Vec<&str> = headers
	.get_all(name)
	.iter()
	.filter_map(|value| value.to_str().ok())
	.collect();
	let separator = if name == "surrogate-key" { " " } else { ", " };
	(!values.is_empty()).then(|| values.join(separator))
	};

[prk-Jr]

🔧 [P2] Reserve immutable caching recommendations for fingerprinted URLs

Every JavaScript/CSS/font response is graded against public, max-age>=31536000, immutable, including stable URLs such as /app.js or /style.css. The evaluator receives no URL and therefore cannot distinguish these from content-addressed assets. Following the recommendation for a stable URL can leave clients using old assets after deployment for up to a year.

The repository's cache-header audit design (§6.3) explicitly limits this policy to fingerprinted filenames and recommends validators without immutable for stable URLs. Please pass the sampled URL into evaluation, detect filename fingerprints, and use the appropriate policy for each case. Apply manually because the fix spans the evaluator, its callers, and tests.

[aram356]

Summary

Round 4 against e918e7a9. CI is now fully green, 21 of 21, and cargo fmt, cargo clippy-cli, and 772 CLI tests pass locally.

One commit landed since round 3: 0cb1b00f, the one-line Prettier fix that cleared format-docs. I diffed c71c5d98..e918e7a9 and that blank line is the only non-merge change to the code. So the other six blocking findings from round 3 are still present, and I re-verified each of them by running code at this head rather than assuming they carried over.

Two process points before the findings, because they are the reason this round exists.

First, four of my round-3 threads were marked resolved without a code change: rules.rs:566 (qualified private= on Surrogate-Control), audit/mod.rs:16 (command placement), analyze.rs:334 (rollup-only assertion), and fetch.rs:30 (body cap). I checked the bytes at this head — rules.rs:566 still reads directives.has("no-store") || directives.has("private"), and I re-proved the bug is live. Resolving a thread is the signal that a finding is handled, so resolving one with no change and no reply leaves the next reviewer unable to tell "fixed" from "won't fix, here is why" from "closed by accident". A reply saying "disagree, because X" closes a thread just as well and costs a round less.

Second, none of the nine threads still open from round 3 has a reply, including the two spec questions where I explicitly offered to amend the spec instead of the code. Those two gate most of what follows and need a decision from you, not more review from me.

Worth knowing: prk-Jr reviewed this same head and ChristianPavilonis reviewed 3f09f07e, and between them there are around twenty open threads that overlap several of mine (exit codes, <base href> resolution, third-party grading, /_ts/ exclusion). I have not re-filed theirs. One of prk-Jr's I did verify independently and will credit here rather than duplicate: commas inside a quoted field list (private="Set-Cookie, X-Other") do break the naive split(',') in CacheDirectives::parse. It happens to grade HTML correctly as Fail, because the fragments miss the safe set, so it is a real parsing defect with a mostly benign outcome today.

1 of the inline comments carries a one-click GitHub suggestion. The rest describe the fix in prose because they span several call sites, need a test fixture changed alongside, or need a design decision first.

Blocking

🔧 wrench

• Surrogate-Control: no-store with a numeric max-age still reports a passing CDN TTL — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:489
• Qualified private="Set-Cookie" still passes the Surrogate-Control safety check, and this thread was resolved without a change — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:566
• Exit-code contract still contradicts the spec and RunOutcome — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/analyze.rs:66
• Command placement still diverges from the merged spec, and this thread was resolved without a change — see inline at crates/trusted-server-cli/src/commands/dev/audit/mod.rs:16
• Spec-mandated HTTPS and private-address refusal are still absent — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:84
• Spec Task 1 is still unimplemented: the audit never grades with TS's own cache logic — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:14

Non-blocking

🤔 thinking / ⛏ nitpick

• An invalid or self-contradicting Surrogate-Control TTL still emits no verdict at all — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:593
• --include-cross-origin still admits non-HTTP schemes from fetched HTML — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:262
• This test still asserts presence rather than verdict values, and the thread was resolved without a change — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/analyze.rs:334
• <base href>, not_audited reporting, and the browser-like request profile are still missing — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:5
• Body cap is still 2 MiB where the spec says 5 MiB, and the thread was resolved without a change — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:30
• The favicon fallback is still silently dropped at the discovery cap — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:283
• Bare .unwrap() in tests — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:373

Cross-cutting / body-level findings

• 🔧 The two spec questions are the critical path and are unanswered after a full round. Placement (ts origin audit-headers versus ts dev audit headers) and the exit-code contract (--strict versus exit 3) each have a cheap answer in either direction, and both change what the other findings should look like — if the command moves under ts origin, its dispatch already returns RunOutcome and the exit-code finding closes itself, and probe-shareability's HTTPS handling is right there to share. I said in round 3 that I would amend the spec if you prefer the current surface, and I still will. What I cannot do is guess which way you want it. A one-line reply on either thread unblocks the rest.

• 🤔 The reopening pattern is still unaddressed and is now three rounds old. Cache-Control and Surrogate-Control answer the same two questions — does this forbid caching, and is the directive qualified — with the logic duplicated per header rather than shared. Round 1, round 2, and round 3 each fixed one copy and missed the other; the rules.rs:489 and rules.rs:566 findings in this review are both that same shape. A single predicate used by both paths, something like CacheDirectives::forbids_caching() built on has_unqualified, closes the class instead of its instances. I verified such a helper works; the inline comment on rules.rs:489 has it.

• 📝 Three reviewers are now converging on the same surfaces. With around twenty open threads from prk-Jr and ChristianPavilonis overlapping these, it may be worth agreeing on the two spec decisions first and then doing one consolidated pass, rather than three reviewers each re-verifying the same rules module every round.

CI Status

• cargo fmt: PASS (required)
• cargo test: PASS (required)
• format-docs: PASS (required)
• format-typescript: PASS (required)
• cargo test (ts CLI, native) (macos-latest): PASS
• cargo test (ts CLI, native) (ubuntu-latest): PASS
• cargo test (axum native): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (fastly reusable sandbox): PASS
• vitest: PASS
• Analyze (rust): PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS
• lint-domains: PASS
• markdown format: PASS
• integration tests: PASS
• browser integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• prepare integration artifacts: PASS

[aram356]

🔧 wrench — Still present at e918e7a9. The spec's transport safety rules are not implemented.

Spec §190-191:

• targets must be HTTPS, as in probe-shareability; plain HTTP is accepted only for a loopback development origin
• a target that resolves to a loopback, RFC 1918, or link-local address is refused unless the configured origin itself resolves there (local development)

Neither check exists in this file at this head — no scheme validation, no address check. Re-verified against the built binary:

$ ts dev audit headers http://169.254.169.254/latest/meta-data/
skipping http://169.254.169.254/latest/meta-data/: failed to fetch ...

The request was attempted and only failed because nothing answered. On a CI runner or an EC2 host it generally does answer. Plain HTTP is likewise accepted for any host.

Round 1's same-origin discovery gate still holds and is doing real work — it is why the automatic crawl no longer wanders. The gap is the explicit-URL path, which was exempt by design, plus the missing scheme check.

Proposed fix (apply manually — a validation step over resolved targets, shared by both collection paths): reject a non-HTTPS target unless its host is loopback, and refuse targets resolving into loopback / RFC 1918 / link-local space unless the configured origin resolves there too.

probe-shareability already implements the scheme half of this, so if the command moves under ts origin per the placement thread, that logic is there to share rather than reimplement.

[vasujain00]

Partially addressed at head (b1650a8), with the rest proposed as follow-up. The scheme half of §190 now exists: a shared url_guard::require_credential_safe_transport runs on all three paths — explicit --url, the resolved origin root, and every discovered URL — rejecting non-HTTPS targets unless the host is loopback, and rejecting embedded userinfo. Your exact repro is refused now, since http://169.254.169.254/... is plain HTTP off loopback:

--url requires HTTPS to protect the credentials sent with the request; HTTP is allowed only for loopback development services

The §191 address-resolution refusal is the remaining gap, and I'd like to take it as a follow-up rather than in this PR. url_guard currently inspects the host literally and does no DNS resolution, so an HTTPS target that resolves into loopback / RFC-1918 / link-local space is still fetched. Closing it properly means resolving the host and refusing private/link-local addresses unless the configured origin resolves there too — a contained addition to url_guard, but a distinct SSRF-hardening change with its own tests, and it pairs naturally with the not_audited and <base href> discovery items from your grouped thread. I'll track all of these in the core-aware-grading follow-up so the SSRF check is explicit rather than implied. Flagging it clearly since it's a security property, not cosmetics.

[aram356]

🤔 thinking — Still present at e918e7a9. Three discovery behaviours the spec specifies are not implemented, and this module comment remains the closest thing to a record of them.

1. <base href> is not honoured. Spec §155 wants relative references resolved per WHATWG against the sampled document's URL and its <base href>, not against publisher.origin_url. Discovery resolves against the origin, so a page carrying <base> yields wrong asset URLs. <base> is load-bearing in this repo already — Make creative click rewriting a separate switch #1252 touched <base target> in the HTML rewriter. ChristianPavilonis has an open thread on exactly this.

2. Nothing is reported as not_audited. Spec §163-168 wants /_ts/*, third-party URLs, and auth / image-optimizer asset routes listed as skipped with a reason, so the operator can see what was left out. They are silently absent. This comment and a test assertion are the only places /_ts/ appears — there is no skip logic, and the property holds only because discovery is same-origin, so a /_ts/ path on the origin's own host would still be fetched. ChristianPavilonis also has a thread on this.

3. The request is not browser-like. Spec §175 wants the desktop User-Agent probe-shareability already uses, plus Accept per context and Accept-Encoding: gzip. Neither header is set. Origins routinely vary cache headers by user agent, so verdicts gathered with reqwest's default UA may not describe what a reader actually receives — which undercuts the audit's purpose more than a missing header count suggests.

Grouping these because they share a cause: discovery was written before the spec was rewritten, and these are the parts the rewrite added. All three are cheap next to the blockers.

[vasujain00]

Confirmed — all three are still present at head (b1650a8); this round only closed the correctness blockers and prk-Jr's P1/P2 parsing items. Grouping these with the other discovery/spec-fidelity gaps into the core-aware-grading follow-up, since they share the same cause (discovery predates the spec rewrite) and overlap what that work touches:

(§155) — discovery still resolves against publisher.origin_url, not the sampled document URL + . Agree this is load-bearing (#1252 / ); will resolve per WHATWG there. Linking @ChristianPavilonis's open thread so they converge. not_audited reporting (§163-168) — no skip-with-reason surface exists today; /_ts/*, third-party, and auth/image-optimizer routes are silently absent, and the same-origin gate is the only thing keeping an origin-hosted /_ts/ path from being fetched. Will add an explicit not_audited list in the follow-up (also @ChristianPavilonis's thread). Browser-like request (§175) — still reqwest's default UA, no Accept/Accept-Encoding: gzip. Agree the UA-varying-cache-headers point matters more than the header count; will adopt probe-shareability's desktop UA + per-context Accept so verdicts reflect what a reader actually receives. None block this PR, but I'd rather track them explicitly than leave them implied — the follow-up is noted in the PR description. Thanks for the detailed write-up on each.

[aram356]

⛏ nitpick — Still present at e918e7a9. The favicon fallback is silently dropped on a busy page.

The push is gated on urls.len() < MAX_DISCOVERED_URLS, so once discovery reaches the cap the fallback never happens. Measured:

scripts in HTML	URLs returned	favicon included
49	50	yes
50	50	no
60	50	no

The cap itself is exact, no off-by-one. The issue is that the comment above says "Ensure at least the favicon is checked" and the doc comment promises a fallback, while both are conditional in a way neither states.

Reserving one slot for the favicon, or taking it before the selector loop, makes the guarantee real. Alternatively soften both comments to say the fallback is best-effort under the cap.

[aram356]

⛏ nitpick — Still present at e918e7a9. Bare .unwrap() here where the rest of the module uses expect("should …").

AGENTS.md asks for expect() / expect_err() with "should …" messages instead of unwrap(), and states the same quality bar applies to test code. Every other expect across these files follows that form, so these read as an oversight rather than a choice.

Also at lines 377, 381, and 397.

[aram356]

Hi @vasujain00, this is a status note rather than new findings, plus one request about process.

First, credit where it is due: CI is fully green on 32f041cf, the branch is cleanly merged with main, and the round-1 and round-2 fixes were thorough. I re-verified them by running code and they hold. The Vary: * extension, the exit-code change, same-origin discovery, the redirect policy, public, private, conflicting max-age, and application/woff2 are all genuinely fixed. That was real work and it landed.

Where things stand now. There are 38 open review threads across three reviewers: 10 from @ChristianPavilonis, 14 from @prk-Jr, and 14 from me. Roughly 21 of those are marked blocking by their authors.

I went through every one of them against the code at the current head rather than trusting the thread text, and I could only resolve one: the format-docs blank line, fixed by 0cb1b00f. I also collapsed 7 duplicates of my own, where round 4 re-filed a round-3 finding at the same line, so each finding now has one live thread instead of two. No finding was closed by that cleanup.

The reason I could not resolve more is just the commit history. The branch has five commits from you:

Commit	Addressed
378131d8	original implementation
6cf2d03d	round-1 feedback
f83a5179	round-2 feedback
e299d09d	remaining round-2 items
0cb1b00f	the docs blank line

Everything after 0cb1b00f is a merge from main. So rounds 3 and 4 have not had a code or comment response yet, which is why the thread count keeps growing instead of shrinking.

The request. Could you close each open thread one of two ways: a code change, or a reply explaining why you disagree or are deferring it?

A reply is a completely valid way to close a thread, and sometimes the better one. "This is deliberate, here is why" or "agreed, filing as a follow-up issue" settles a finding just as well as a commit, and it costs a round less than silence does. The thing that is hard for us on the reviewing side is a thread that goes quiet, because we cannot tell "fixed", "won't fix for a good reason", and "missed it" apart, so we re-verify from scratch each round. That is most of why rounds 3 and 4 were as long as they were, and it is avoidable.

One related thing, mentioned gently: four threads were resolved at one point without a code change (rules.rs:566, audit/mod.rs:16, analyze.rs:334, fetch.rs:30). I re-filed those, since the findings were still reproducible. If any of those resolves was deliberate disagreement, I would genuinely rather hear the reasoning than keep re-filing. You may well be right, and I would drop them.

If you only do one thing, do this. Two of my threads are decisions rather than defects, and they gate most of the rest:

1. Command placement. The spec merged in Add cache-header audit design spec for ts origin audit-headers #930 puts this at ts origin audit-headers, but the code has it at ts dev audit headers. This one is on me: I rewrote that spec three weeks after your branch existed, so the divergence is my doing, not a mistake you made. Either direction closes it. If ts dev audit headers is the better home, say so and I will amend the spec to match your code.
2. Exit codes. The spec says 0/1/2 with a --strict flag, while the code uses exit 3. Round 2 is how 3 got there, because I pushed for moving warnings off 2. If the command moves under ts origin, this closes itself, since that group already returns RunOutcome.

A one-line answer on either unblocks a lot. Happy to pair on any of this, or to take the spec amendment myself if that is the direction you prefer. And if any finding looks wrong, please push back. Several have already been narrowed by exactly that.

[vasujain00]

@aram356 @prk-Jr — round-3 addressed, ready for re-review at 53205e5. The feature changes are in two commits (a548c7a, be5b406); the rest of the diff is your own main-merge updates (Rust 1.98.1, dependabot, doc fixes) that I merged in.

Blocking:

1. ✅ Surrogate-Control no-store, max-age=N no longer reports a passing CDN TTL. Added CacheDirectives::forbids_caching() (built on has_unqualified); evaluate_cdn_ttl returns no verdict when the surrogate forbids caching, so only the disables-caching Warn stands. (a548c7a)
2. ✅ Qualified private="Set-Cookie" no longer passes the Surrogate-Control body-safety check — evaluate_surrogate_no_store now uses has_unqualified, matching the Cache-Control path. The converse false-warn in evaluate_surrogate_disables_caching is fixed by the same shared predicate. (a548c7a)
3. ✅ format-docs: blank line before the Prebid heading (landed earlier in the branch).
4. ✅ Moved to ts origin audit-headers beside probe-shareability per the merged spec (#930 §34). commands/dev/audit/headers → commands/origin/audit_headers; positional URLs → repeatable --url. (be5b406)
5. ✅ Exit codes now match spec §59-69: dropped the bespoke code 3 and the process::exit; the report maps to RunOutcome (FAIL, or WARN under the new --strict, → exit 1; else 0), returned through dispatch. Errors return Err, so main exits 2 — a warn-only audit is never confused with one that could not run. (be5b406)
6. ✅ Transport safety: explicit --url, the resolved origin root, and discovered assets all go through the shared url_guard::require_credential_safe_transport used by probe-shareability (HTTPS except loopback, no URL-embedded creds). Discovery is additionally restricted to http/https, so file:/javascript:/data: links from fetched HTML are dropped. The http://169.254.169.254/... case is now refused. (be5b406)
7. ↪️ Core-aware grading (Spec Task 1) is split to a tracked follow-up, noted in the PR description — this PR ships the generic ruleset. Flagging per your round-3 note; happy to pull it in here if you'd rather.

Structural note you raised: both reopenings shared one root cause (Cache-Control and Surrogate-Control each answered "does this forbid caching?" with duplicated logic). forbids_caching() is the single predicate both paths now use, so this closes the class, not just the two instances.

Tests: reworked distinct_surrogate_control_findings_are_not_collapsed (two responses, asserts verdict values not just presence), hardened surrogate_control_matrix_on_immutable_js with the token-plus-number regression cell, added HTML qualified/unqualified private tests. 776 CLI tests pass, clippy -D warnings clean, fmt + prettier clean — all on the pinned Rust 1.98.1.

Non-blocking items (#8 invalid-TTL no-verdict, cross-origin scheme filter beyond discovery, <base href>, 5 MiB body cap, favicon-under-cap, test unwrap()s) not in this revision — can fold into a follow-up alongside #7 if you'd like. Re-requesting review.

[aram356]

Summary

Round 5 against 57b2a174. This round landed the architectural change I asked for, and I verified all of it by running code rather than reading the diff.

Confirmed fixed: audit-headers now sits under ts origin beside probe-shareability; --url is repeatable; --strict exists and is exit-code-only; the contract is RunOutcome (warn 0, warn under --strict 1, could-not-run 2) with no process::exit. HTTPS-except-loopback is enforced through the shared crate::url_guard on all three fetch paths; qualified private="Set-Cookie" now fails on Surrogate-Control; the bogus passing CDN TTL on no-store is gone; discovery is gated to http/https; and the docs exit-code table matches the spec. probe-shareability shows no regression from its signature change. cargo fmt, cargo clippy-cli, and 777 CLI tests pass locally.

Findings from the other reviewers that I verified as fixed, so their threads are stale rather than ignored: @prk-Jr's quoted-field-list P1 is thoroughly closed (split_top_level_commas handles a quoted list in last position, escaped quotes, and unbalanced quotes, and degrades in the safe direction), the repeated-header-field P2 is fixed via get_all, and the immutable-accepts-no-store item now fails as it should. The three new regression tests are non-vacuous; I checked each by computing its pre-fix verdict.

Two new P1 defects came in with this round's changes, which is why this is still a change request. Both reproduce against the built binary, not just in unit tests. One is a regression introduced by the round-4 get_all fix; the other is the fifth instance of a pattern worth addressing structurally.

1 of the inline comments carries a one-click GitHub suggestion. The other is prose because the fix restructures a branch rather than replacing a line.

Blocking

🔧 wrench

• A duplicated Content-Type drops the response from the audit and exits 0 on an unsafe page; see inline at crates/trusted-server-cli/src/commands/origin/audit_headers/fetch.rs:134
• The public plus private contradiction check exists on Cache-Control but not on its Surrogate-Control twin; see inline at crates/trusted-server-cli/src/commands/origin/audit_headers/rules.rs:652

Cross-cutting / body-level findings

• 🔧 The one-of-two-symmetric-paths pattern has now recurred in all five rounds, and is worth retiring structurally rather than instance by instance. Round 1 fixed Vary: * on HTML but not on assets. Round 2 fixed has_unqualified on Cache-Control but not on Surrogate-Control. Round 3 fixed the token-only Surrogate-Control TTL case but not token-plus-number. Round 4 fixed forbids_caching precedence on both asset paths but left the public contradiction check single-sided, which is the second P1 above. Each fix was correct. Each landed on one side of a pair.

  Cache-Control and Surrogate-Control answer overlapping questions (does this forbid caching, is the directive qualified, is the directive set self-contradictory), and the logic is written per header rather than shared. A table-driven test that drives the same directive string through both paths and asserts the verdicts agree, with an explicit exemption list for the cases that should legitimately differ, would catch instance six before review does. That is a smaller change than it sounds, and it is the thing I would most like to see from this round.

• 📝 browser integration tests is failing, and it is not this PR. The job fails at Install Playwright with ERROR: Playwright does not support chromium on ubuntu26.04-x64, then every chromium test errors with a missing executable. PRs #1107, #1222, and #1290 show this check passing, but their runs predate yours: theirs ran 2026-10-24 and 2026-10-26, yours ran 2026-10-27 02:00. The runner image rolled over in between, so this will hit every PR on its next run and wants a separate infrastructure fix. Nothing to do here, and please do not spend time on it.

• 📝 Two of my earlier findings remain open as acknowledged follow-ups, and I am not treating them as blocking this round. Spec §191 private-address refusal (link-local is still reachable over HTTPS, while the §190 scheme half landed) and spec Task 1 (grading through trusted_server_core's fingerprint and template-cache helpers, which the spec itself breaks out as its own task). Your replies saying these are follow-ups are reasonable. If you would rather close them here, say so on the threads and I will hold the review open for them; otherwise I would like them filed as issues before merge so they are recorded rather than implied.

• 📝 Smaller items still open from my earlier rounds, none blocking: an invalid or self-contradicting Surrogate-Control TTL still emits no verdict at all and the group rolls up to Pass; <base href> is still unhandled; nothing is reported as not_audited; there is no browser-like User-Agent; the favicon fallback is still dropped at the discovery cap; and four bare .unwrap() remain in tests where the module otherwise uses expect("should …").

• 📝 Still live from the other reviewers, verified by me and not re-filed. @prk-Jr's crate::tls::install_crypto_provider() duplication is a genuine one-line change: the helper exists and commands/cache/purge.rs and commands/origin/probe.rs both already call it, so the inline install_default() in fetch.rs can simply defer to it. And origin.publisher.com in docs/guide/cli.md is flagged by the repo's own linter (ts dev lint domains docs/guide/cli.md reports two disallowed hosts); lint-domains is not yet a gate on this PR, but it is on main.

• 📝 Stale doc comment. rules.rs:650-651 says the unqualified requirement protects "personalized HTML/RTB", but evaluate_surrogate_no_store has exactly one caller, the HTML path. evaluate_rtb grades only Cache-Control, with no Surrogate-Control and no Vary. Skipping Surrogate-Control on JSON is defensible, but Vary: * on origin JSON going unreported is inconsistent with every other group, and the comment no longer describes the code.

CI Status

• cargo fmt: PASS (required)
• cargo test: PASS (required)
• format-docs: PASS (required)
• format-typescript: PASS (required)
• cargo test (ts CLI, native) (macos-latest): PASS
• cargo test (ts CLI, native) (ubuntu-latest): PASS
• cargo test (axum native): PASS
• cargo check (cloudflare native + wasm32-unknown-unknown): PASS
• cargo check/build/test (spin native + wasm32-wasip1): PASS
• cargo test (cross-adapter parity): PASS
• cargo test (fastly reusable sandbox): PASS
• vitest: PASS
• Analyze (rust): PASS
• Analyze (javascript-typescript): PASS
• Analyze (actions): PASS
• markdown format: PASS
• integration tests: PASS
• integration tests (Fastly EC lifecycle): PASS
• prepare integration artifacts: PASS
• browser integration tests: FAIL (runner image, not this PR; see above)
• CodeQL: SKIPPED

[aram356]

🔧 wrench; A duplicated Content-Type drops the response from the audit entirely, so an unsafe page exits 0.

This closure comma-joins every field, including content-type. That is right for Cache-Control, Surrogate-Control, and Vary, which are comma-separated lists, but Content-Type is single-valued: joining two of them yields text/html, text/html, which classify cannot match because it splits on ; rather than ,. The group becomes Other, and run_analysis drops Other responses, so the response vanishes from the report.

Reproduced against the built binary, same unsafe posture in both runs, only the field count differing:

# Content-Type: text/html  (once)
✗ HTML
    ✗ Cache-Control   public, max-age=3600   HTML served without no-store or private ...
Summary (per content type): 0 pass, 0 warn, 1 fail (1 types audited)
exit=1

# Content-Type: text/html  (twice, identical value)
No auditable content types found.
exit=0

A publicly-cacheable personalized HTML page now passes. --json reports "total_groups": 0 with empty groups, which a CI gate reads as a clean origin. Duplicate Content-Type is not exotic: it comes from a misconfigured proxy or an origin layer that sets the field on top of its framework's.

Which types survive the join is incidental rather than principled:

joined value	classified as
text/html, text/html	Other (dropped)
application/javascript, application/javascript	Other (dropped)
text/css, text/css	Other (dropped)
application/json, application/json	Other (dropped)
text/html; charset=utf-8, text/html; charset=utf-8	Html, by accident (the ; truncates)
image/png, image/png	Image, by accident (starts_with("image/"))

This is a regression from the round-4 get_all change: before it, get(name) returned the first value and classification was correct. The fix was needed for the directive lists but was applied to every field.

The suggestion below splits the closure in two: list-valued fields keep the join, single-valued fields take the first value as a cache would. I verified it at this head; the duplicated case now reports exit=1 with the HTML failure, matching the single-field run, cargo fmt --check and cargo clippy-cli are clean, and 777 CLI tests pass.

Suggested change

	// A field may appear more than once; `get` returns only the first, which
	// can hide a later `no-store` on `Cache-Control` or a later high-cardinality
	// token on `Vary`. Combine every field value so evaluation sees the whole
	// policy. `Surrogate-Key` is a space-separated token list; the directive
	// lists are comma-separated.
	let value = |name: &str| {
	let values: Vec<&str> = headers
	.get_all(name)
	.iter()
	.filter_map(|value| value.to_str().ok())
	.collect();
	let separator = if name == "surrogate-key" { " " } else { ", " };
	(!values.is_empty()).then(|| values.join(separator))
	};
	// A list-valued field may appear more than once; `get` returns only the
	// first, which can hide a later `no-store` on `Cache-Control` or a later
	// high-cardinality token on `Vary`. Combine every value for those so
	// evaluation sees the whole policy. `Surrogate-Key` is a space-separated
	// token list; the directive lists are comma-separated.
	let joined = |name: &str, separator: &str| {
	let values: Vec<&str> = headers
	.get_all(name)
	.iter()
	.filter_map(|value| value.to_str().ok())
	.collect();
	(!values.is_empty()).then(|| values.join(separator))
	};
	// Single-value fields must NOT be joined: a duplicated `Content-Type`
	// would become `text/html, text/html`, which classifies as `Other` and
	// drops the response from the audit entirely. Take the first value, as
	// a cache does.
	let single = |name: &str| {
	headers
	.get(name)
	.and_then(|value| value.to_str().ok())
	.map(str::to_owned)
	};

The struct literal below then becomes content_type: single("content-type"), etag: single("etag"), and joined("cache-control", ", ") / joined("surrogate-control", ", ") / joined("vary", ", ") / joined("surrogate-key", " ") for the rest. A regression test with the field set twice would pin it.

(behaviour-checked against the binary as shown; please re-run ./scripts/test-cli.sh after applying.)

[aram356]

🔧 wrench; The public plus private contradiction check guards Cache-Control but not its Surrogate-Control twin, so a contradictory CDN directive passes.

rules.rs:368 has the check on the Cache-Control path:

let contradictory = directives.has("public") && directives.has_unqualified("private");

This function, which answers the same body-safety question for Surrogate-Control, has no public check at all. has("public") occurs at exactly one site in the file.

Reproduced against the built binary, all three with --strict:

# Cache-Control: public, private
✗ HTML
    ✗ Cache-Control   public, private   HTML advertises both `public` and `private`; ...
exit=1

# Cache-Control: no-store  +  Surrogate-Control: public, private
✓ HTML
    ✓ Cache-Control      no-store          --
    ✓ Surrogate-Control  public, private   --
exit=0

# Cache-Control: no-store  +  Surrogate-Control: public, no-store
✓ Cache-Control      no-store           --
✓ Surrogate-Control  public, no-store   --
exit=0

The verdict is Pass, not a weaker warning, so --strict cannot catch it either.

This matters more on the Surrogate-Control side than on Cache-Control. The reason the Cache-Control recommendation says "intermediaries resolve this inconsistently and may share personalized content" is the shared cache, and Surrogate-Control is the header that shared cache reads by preference. The contradiction is aimed directly at the consumer the warning is about.

Proposed fix (apply manually; it restructures the branch rather than replacing a line, and should land with its test):

Compute the contradiction alongside safe and let it override, mirroring the Cache-Control path:

let safe = directives.has_unqualified("no-store") || directives.has_unqualified("private");
let contradictory = directives.has("public") && directives.has_unqualified("private");
Some(if contradictory {
    HeaderVerdict::flagged(
        "Surrogate-Control",
        Verdict::Fail,
        Some(value.to_owned()),
        "no-store or private, without `public`",
        "Surrogate-Control advertises both `public` and `private`; the shared cache resolves this inconsistently and may store personalized content; drop `public`",
    )
} else if safe {
    // ... existing pass arm
} else {
    // ... existing flagged arm
})

Worth deciding at the same time whether Surrogate-Control: public, no-store deserves the same treatment. I would say yes, for the same reason, but the Cache-Control path does not flag that pairing today either, so matching the twin exactly is the smaller change and still closes the gap I can demonstrate.

[aram356]

@vasujain00 a separate note from the round-5 review, about thread hygiene rather than code.

I understand you cannot resolve threads on this PR, since it is from a fork and resolution needs write access on the base repo. That is on us, not you, and I will do the resolving. What would help is a reply on each open thread, and then I will verify and resolve it.

There are 30 open threads right now: 10 from @ChristianPavilonis, 12 from @prk-Jr, and 8 from me. 27 of them have no reply from you yet.

A reply is genuinely enough, and any of these closes a thread from my side:

• "Fixed in <sha>" and I will verify and resolve it.
• "Deliberate, because X." Your reply on the 2 MiB body cap was exactly this, and it closed that thread. I resolved it on your reasoning, not on a code change.
• "Agreed, filing as a follow-up issue." That is how I am treating spec §191 and Task 1 right now.
• "I think this finding is wrong, because X." Several findings have been narrowed or withdrawn this way already, and I would rather hear it than keep re-filing.

The reason I am asking: with no reply, a reviewer cannot tell "already fixed" from "deliberate" from "not seen yet", so each round re-verifies everything from scratch. That is most of why rounds 3 and 4 were long. This round I re-verified 46 threads by running code, and found that 3 of @prk-Jr's findings were already fixed by your round-4 work. Nobody had said so, so nobody knew.

Two practical notes:

You do not need to answer all 27. Many are small, and several from @ChristianPavilonis and @prk-Jr overlap each other and overlap mine, so one reply often covers a cluster. If it helps, start with the ones marked blocking by their authors.

Please do not spend time on the browser integration tests failure. It is a runner-image break (Playwright does not support chromium on ubuntu26.04-x64) that will hit every PR on its next run, and it needs a separate infrastructure fix.

Thanks for the work this round. Moving the command under ts origin, adopting --strict, and wiring RunOutcome were all substantial, and I verified every one of them against the binary. The two P1s in the review are new code from this round rather than anything you were asked about before.