feat(cli): implement ts dev audit headers cache-header audit (#834) by vasujain00 · Pull Request #1172 · IABTechLab/trusted-server

vasujain00 · 2026-09-16T04:09:34Z

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

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.
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 · 2026-09-21T14:21:40Z

+            if exit_code == 0 {
+                Ok(())
+            } else {
+                process::exit(exit_code)


HIGH - exit code 2 collides with the CLI's existing "command failed" code, which defeats the CI-gating purpose.

main.rs:8 is process::exit(2) for any Err returned by run_from_env(). So all three of these exit 2:

origin returns public, max-age=600 on JS -> "warnings only"

trusted-server.toml missing and no --origin -> resolve_origin errors -> command failed

root fetch times out -> collect_via_discovery errors -> command failed

A CI step written the obvious way, ts dev audit headers || [ $? -eq 2 ], silently green-lights a completely failed audit run.

Compounding it, the mapping is untestable by construction: process::exit inside the subcommand dispatcher means no test can assert it. analyze.rs:272 warn_only_exits_two only asserts AuditReport::exit_code(), not the process-level outcome.

Suggested direction: renumber the warn code away from 2 - e.g. 0 pass / 1 fail / 3 warn, or make warn exit 0 and let a --strict flag promote it - and thread the code back out through run_from_env rather than calling process::exit from inside a dispatcher. The contract is documented in three places that must move together: here, headers/mod.rs:5-7, and headers/mod.rs:22-23.

prk-Jr · 2026-09-21T14:21:40Z

+
+impl OriginClient for ReqwestOriginClient {
+    fn fetch(&self, url: &Url) -> CliResult<OriginResponse> {
+        let response = self


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.

Implements the origin cache-header audit from the IABTechLab#834 spec: 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. - rules.rs: ContentTypeGroup taxonomy (MIME param stripping), unit Verdict enum, per-group cacheability rules. no-store alone passes HTML/RTB per RFC 9111 5.2.2.5; s-maxage honored as CDN TTL; Vary checked on all cacheable groups; Surrogate-Key only on cacheable groups. - fetch.rs: AuditHeadersArgs, OriginClient trait + reqwest blocking impl, config-derived origin resolution, HTML-parse discovery (never probes /_ts/). - analyze.rs: worst-of rollup per group + summary + exit-code mapping. - output.rs: writer-injected human table + JSON (bare-string verdicts). - Adds Audit variant to the existing DevCommand enum (cross-platform, unlike the macOS-only proxy). reqwest scoped to cfg(not(target_arch = wasm32)). Follows the CLI's CliResult<T> string-error and writer-injection conventions. 25 unit tests cover classification, rules, discovery, rollup, and rendering. Refs IABTechLab#834

Blockers (aram356 + prk-Jr): - Cargo.toml: pin reqwest to rustls-tls-webpki-roots-no-provider instead of inheriting the workspace rustls-tls feature (which pulled rustls/ring alongside the proxy's aws-lc-rs and made rustls unable to pick a process CryptoProvider -> ts dev proxy TLS panics, 4 CI failures). Because the proxy (and thus its aws-lc-rs) is macOS-only, the -no-provider variant leaves the audit's own reqwest with no provider on Linux; so also add rustls (aws-lc-rs) to the all-host deps and install_default() it in ReqwestOriginClient::new(). Verified at runtime on Linux: audit runs, proxy stays single-provider. - Exit code: warn-only now exits 3, not 2 (2 is main's generic CLI-error code). - Vary: * now flagged on JS/CSS/image groups. - Discovery restricted to the audited origin (+ --include-cross-origin opt-in); redirects no longer followed (Policy::none). Non-blocking: - private alone now passes HTML (RFC 9111 5.2.2.7). - Cache-Control s-maxage honored as CDN TTL without Surrogate-Control (labeled distinctly so it does not collide with the cacheability verdict). - Surrogate-Control no-store/private/no-cache flagged on cacheable groups. - Directive parsing: trim whitespace around '='; first-valid-occurrence-wins so a trailing malformed/duplicate directive can't null a good value (RFC 4.2.1). - Bounded body reads (2 MiB); explicit-URL path tolerates per-URL failures and hard-errors only if all fail. - Added docs/guide/cli.md section with the exit-code contract. 11 new unit tests + end-to-end runtime verification against a local origin. 445 CLI tests pass, clippy -D warnings clean, fmt clean, Cargo.lock unchanged. Refs IABTechLab#834

vasujain00 · 2026-09-23T15:29:58Z

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

…eaders Round-2 blockers (aram356) — all regressions introduced by the round-1 fix commit, in the rewritten CDN-TTL block and new evaluate_surrogate_disables_caching: 1. Surrogate-Control: no-store no longer borrows Cache-Control s-maxage to report a passing CDN TTL. evaluate_cdn_ttl matches on Surrogate-Control presence: when present it uses only its own max-age/s-maxage (None for a token-only no-store, so no verdict is emitted here); the s-maxage fallback applies only when Surrogate-Control is absent. 2. Two distinct Surrogate-Control findings (CDN-TTL comparison vs disables-caching) no longer collapse. The GroupAccumulator merge now keys on (header, expected), not header alone, so different checks that share a header name both survive while the same check still folds across sampled URLs. 3. Surrogate-Control: max-age=0 on JS/CSS is now flagged. The CDN-vs-browser TTL comparison was extracted into evaluate_cdn_ttl and is now called from evaluate_immutable, not only evaluate_image. 4. Qualified private="…"/no-store="…" is no longer accepted as unqualified. CacheDirectives tracks an `unqualified` set; HTML and RTB body-safety checks use has_unqualified (RFC 9111 §5.2.2.7 — the qualified form restricts a header field, not the body). 5. Cache-Control: public, private now fails HTML as a contradiction rather than passing — intermediaries resolve the conflict inconsistently. Also from the round-2 non-blocking notes: the CDN-TTL verdict sourced from Cache-Control s-maxage is labeled "Cache-Control (s-maxage)" instead of a phantom "s-maxage" header; the now-dead flag_wildcard parameter on evaluate_vary is removed and its stale doc corrected; the redundant double-Option match in evaluate_image is simplified. 7 new tests pin the fixes, including a table-driven Surrogate-Control matrix ({absent, token-only, numeric} x immutable JS) and a merge test asserting both Surrogate-Control findings survive. Verified end-to-end by running the built `ts dev audit headers` against a local origin. 463 CLI tests pass, clippy -D warnings clean, fmt clean. Refs IABTechLab#834

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 · 2026-10-13T07:58:05Z

+        )),
+    }
+
+    verdicts.extend(evaluate_vary(headers));


🔧 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 · 2026-10-13T07:58:05Z

+ts dev audit headers --origin https://origin.publisher.com
+
+# Audit specific URLs instead of crawling.
+ts dev audit headers https://origin.publisher.com/app.js https://origin.publisher.com/


🔧 wrench — origin.publisher.com is a real registrable domain. AGENTS.md requires example/fictional hosts in docs ("for urls use: example.com domains only"), and ts dev lint domains on main (#733) would reject it: its allowlist is example.com/.net/.org plus reserved TLDs.

(prettier --check passes with this applied.)

Suggested change

ts dev audit headers --origin https://origin.publisher.com

# Audit specific URLs instead of crawling.

ts dev audit headers https://origin.publisher.com/app.js https://origin.publisher.com/

ts dev audit headers --origin https://origin.example.com

# Audit specific URLs instead of crawling.

ts dev audit headers https://origin.example.com/app.js https://origin.example.com/

prk-Jr · 2026-10-13T07:58:05Z

+            let immutable = directives.has("public")
+                && directives.has("immutable")
+                && directives
+                    .max_age
+                    .is_some_and(|age| age >= IMMUTABLE_MAX_AGE);


🤔 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 · 2026-10-13T07:58:06Z

+            }) {
+                // Keep the more severe verdict when the same check is seen on
+                // multiple sampled responses in this group.
+                Some(existing) if is_more_severe(incoming.verdict, existing.verdict) => {


🤔 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 · 2026-10-13T07:58:06Z

+
+/// Resolves the origin from `--origin`, else from the config's
+/// `publisher.origin_url`.
+fn resolve_origin(args: &AuditHeadersArgs) -> CliResult<String> {


🤔 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.

- Classify application/woff and application/woff2 as StaticAsset (fonts), not Other, so WOFF responses are evaluated for cache posture. - Duplicate max-age/s-maxage: keep the first valid value for an unparseable or duplicate-identical directive (RFC 9111 §4.2.1, per prk-Jr), but treat two *different* valid values as a contradiction and invalidate the directive (per aram356). Added an `accumulate` helper + per-directive conflict tracking. - RTB/JSON (§3.2, §6.5): origin JSON carries no assumed posture. Bid responses come from the TS edge, not the publisher origin, so the audit no longer FAILs JSON for cache policy; it emits one informational Pass recording the observed policy plus an `add_private` note when `no-store` lacks `private`. Removes a FAIL path, so cacheable origin JSON now exits 0 instead of 1. - Human output: each verdict line now shows the observed `actual` value between the header name and the recommendation (already present in --json). New/updated tests pin all four behaviours. clippy -D warnings clean, fmt clean. Refs IABTechLab#834

Resolve 3-file conflict against current main (d2f0d8c): Cargo.toml keeps rpassword + our reqwest/rustls block (no duplicate rustls key); dev/mod.rs keeps our 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 and the no-assumed-posture RTB/JSON row. 774 CLI tests pass, clippy -D warnings clean.

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.

Prettier (docs format-docs CI) requires a blank line before the '## Operate a self-hosted Prebid Server' heading in guide/cli.md. The separator was lost during an earlier main-merge conflict resolution between the audit-headers and prebid-server sections.

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 · 2026-10-16T15:53:56Z

+            .send()
+            .map_err(|error| format!("failed to fetch {url}: {error}"))?;
+
+        let headers = extract_headers(response.headers());


[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 · 2026-10-16T15:53:56Z

+        let mut s_maxage = None;
+        let mut s_maxage_conflict = false;
+
+        for raw in value.split(',') {


[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 · 2026-10-16T15:53:57Z

+fn discover_asset_urls(base: &Url, html: &str, include_cross_origin: bool) -> Vec<Url> {
+    let document = Html::parse_document(html);


[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 · 2026-10-16T15:53:57Z

+                // strings, so keying on header alone would silently drop the
+                // second — folding *different* checks instead of the same
+                // check across sampled URLs.
+                existing.header == incoming.header && existing.expected == incoming.expected


[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 · 2026-10-16T15:53:57Z

+    struct FakeClient {
+        responses: HashMap<String, OriginResponse>,
+        fetched: RefCell<Vec<String>>,
+    }


[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

aram356 · 2026-10-17T19:38:29Z

+    if let Ok(favicon) = base.join("/favicon.ico")
+        && !urls.contains(&favicon)
+        && urls.len() < MAX_DISCOVERED_URLS
+    {


⛏ nitpick — 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 respected exactly — 50 is 50, 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, would make the guarantee real. Alternatively just soften both comments to say the fallback is best-effort under the cap.

…ders-impl

…to feature/834-audit-headers-impl

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 · 2026-10-21T03:40:13Z

+        let mut s_maxage = None;
+        let mut s_maxage_conflict = false;
+
+        for raw in value.split(',') {


🔧 [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 · 2026-10-21T03:40:14Z

+            let immutable = directives.has("public")
+                && directives.has("immutable")
+                && directives
+                    .max_age
+                    .is_some_and(|age| age >= IMMUTABLE_MAX_AGE);


🔧 [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 · 2026-10-21T03:40:14Z

+    let value = |name: &str| {
+        headers
+            .get(name)
+            .and_then(|value| value.to_str().ok())
+            .map(str::to_owned)
+    };


🔧 [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 · 2026-10-21T03:40:14Z

+) -> Vec<HeaderVerdict> {
+    let mut verdicts = Vec::new();
+
+    let expected = "public, max-age>=31536000, immutable";


🔧 [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 · 2026-10-22T05:02:52Z

+
+    // `Surrogate-Control` wins outright over `s-maxage` at the CDN, so the
+    // `s-maxage` fallback applies only when no `Surrogate-Control` is present.
+    let cdn_ttl = match surrogate_directives.as_ref() {


🔧 wrench — Still present at e918e7a9. A Surrogate-Control that forbids caching and carries a number still reports a passing CDN TTL.

Re-verified by running evaluate against this head:

Cache-Control: public, max-age=86400 Surrogate-Control: no-store, max-age=86400 -> bogus "CDN TTL >= browser TTL" Pass: true Surrogate-Control: private, max-age=86400 -> bogus "CDN TTL >= browser TTL" Pass: true

directives.max_age.or(directives.s_maxage) never asks whether no-store / private / no-cache is present, so once a numeric TTL co-occurs the ? stops short-circuiting and the comparison runs. The CDN stores nothing; the audit affirms the edge TTL is adequate.

The fix from round 3, unchanged and re-verified at this head:

// in CacheDirectives /// Whether these directives forbid caching outright, in unqualified form. fn forbids_caching(&self) -> bool { self.has_unqualified("no-store") || self.has_unqualified("private") || self.has_unqualified("no-cache") } // in evaluate_cdn_ttl let cdn_ttl = match surrogate_directives.as_ref() { Some(directives) if directives.forbids_caching() => None, Some(directives) => directives.max_age.or(directives.s_maxage), None => cache_control .as_ref() .and_then(|directives| directives.s_maxage), }?;

Applied locally it removes the bogus Pass, keeps Surrogate-Control: max-age=30 vs browser 86400 warning, and keeps the Cache-Control (s-maxage) fallback firing. cargo fmt --check and cargo clippy-cli clean.

Still prose rather than a suggestion for the same reason as last round: it fails exactly one existing test, analyze.rs's distinct_surrogate_control_findings_are_not_collapsed, whose fixture is Surrogate-Control: no-store, max-age=30 and therefore depends on this bug to produce its two findings. That fixture needs replacing with one that legitimately emits two distinct verdicts, not a relaxed assertion.

aram356 · 2026-10-22T05:02:53Z

+) -> Option<HeaderVerdict> {
+    let value = headers.surrogate_control.as_deref()?;
+    let directives = CacheDirectives::parse(value);
+    let safe = directives.has("no-store") || directives.has("private");


🔧 wrench — This thread was marked resolved, but the code is unchanged and the bug is live.

The line at this head still reads:

let safe = directives.has("no-store") || directives.has("private");

git diff c71c5d98..e918e7a9 shows no change to this file. Re-verified by running evaluate against this head:

Cache-Control: no-store Surrogate-Control: max-age=600, private="Set-Cookie" -> Surrogate-Control Pass

parse inserts the bare directive name for any name=value form, so the qualified private= satisfies has(). Per RFC 9111 §5.2.2.7 the qualified form restricts only the named field and leaves the body storable and shareable by a shared cache — so the CDN is told to cache personalized HTML for 600 seconds and the audit reports a clean posture. The Cache-Control path next door already guards against this correctly with has_unqualified; this is the same question on the other header.

If the resolve was deliberate and you disagree with the finding, I would genuinely like to hear why on this thread — that closes it just as well. If it was accidental, the suggestion below is the whole fix.

Re-verified at this head with the suggestion applied: qualified private="Set-Cookie" becomes Fail, bare no-store and bare private both stay Pass, cargo fmt --check and cargo clippy-cli clean, 772 CLI tests pass.

Suggested change

let safe = directives.has("no-store") || directives.has("private");

let safe = directives.has_unqualified("no-store") || directives.has_unqualified("private");

(compile-verified and behaviour-checked with the cases above — please re-run ./scripts/test-cli.sh after applying.)

The converse half of the same confusion is still in evaluate_surrogate_disables_caching below, which also uses has(): there a qualified private="x" produces a spurious warning that edge caching is disabled when it is not. Lower stakes, same one-word fix.

aram356 · 2026-10-22T05:02:53Z

+    /// it for warnings would make a warn-only audit indistinguishable from an
+    /// audit that never ran (unreachable origin, bad config). CI gates keying
+    /// on the code can then tell "cache warnings" (3) from "audit failed" (2).
+    pub(crate) fn exit_code(&self) -> i32 {


🔧 wrench — Still present at e918e7a9, and still unanswered. Exit code 3 contradicts both the merged spec and the repo-wide RunOutcome contract.

Spec §59-69 (now on main via #930):

Code Meaning

0 Audit completed; no FAIL verdicts (WARN allowed unless --strict)

1 A FAIL verdict, or a WARN verdict with --strict

2 Audit could not complete: bad arguments, config error, or transport failure

Re-verified against the built binary at this head:

warn-only audit -> exit 3 unreachable origin -> exit 2 ts origin probe-shareability (unreachable) -> exit 2

--strict still does not exist, so a CI gate cannot express "warnings are failures" without special-casing the number 3. And audit/mod.rs:38 still calls process::exit(exit_code), so this command never flows through dispatch and never returns RunOutcome — which is both why 3 was available to take and why it is the one assertion command in the CLI with its own exit vocabulary.

To be fair about the provenance: round 2 is how 3 got here, because I pushed for moving warnings off 2 when main already used 2 for CLI errors. The spec that has since merged answers it better. I am not attached to the spec's answer over yours, but the two should not ship disagreeing.

Proposed fix (apply manually — touches analyze.rs, audit/mod.rs, fetch.rs for the flag, and the exit-code table in docs/guide/cli.md): add --strict, map a FAIL (or a WARN under --strict) to RunOutcome::AssertionFailed and everything else to RunOutcome::Success, and drop the process::exit call.

Note this closes itself if the command moves under ts origin per the placement thread — that group's dispatch already returns RunOutcome.

aram356 · 2026-10-22T05:02:53Z

+
+/// Subcommands of `ts dev audit`.
+#[derive(Debug, clap::Subcommand)]
+pub enum DevAuditCommand {


🔧 wrench — This thread was marked resolved, but the command is still under ts dev audit and the question is still open.

git diff c71c5d98..e918e7a9 shows no change to this file, and ts dev audit headers --help still works while ts origin --help still lists only probe-shareability.

The merged spec (#930, now on main) §34 and §487 place this command at ts origin audit-headers, beside probe-shareability, sharing its HTTP client, user agent, redirect policy, and credential handling. The remaining divergences are placement plus three conventions:

probe-shareability (on main) this PR

Group ts origin ts dev audit

URLs repeatable --url positional [URLS]...

Scheme HTTPS enforced, loopback excepted any scheme accepted

Credentials environment only n/a

I want to be clear that I am not asking you to defer to the spec here. I rewrote that spec three weeks after this branch was written, so this divergence is my doing, not a mistake you made. Either direction closes this thread. If ts dev audit headers is the better home — it is discoverable beside ts dev proxy, and ts origin arguably reads as "questions about the origin" rather than "audits" — say so on this thread and I will amend the spec to match the code, and withdraw the exit-code finding along with it since that one follows from placement.

What I would ask is a reply rather than a silent resolve, because right now the code and the spec on main disagree and the next reader has no way to tell which is authoritative. This is also the finding that gates the most others, so a one-line answer here is worth more than anything else in this review.

aram356 · 2026-10-22T05:02:53Z

+    /// headers are classified on their merits — a long-lived cached 3xx is a
+    /// real misconfiguration this tool should surface, and following would
+    /// silently attribute the final hop's headers to the requested URL.
+    pub(crate) fn new() -> CliResult<Self> {


🔧 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.

aram356 · 2026-10-22T05:02:53Z

+        assert!(
+            surrogate_verdicts.contains(&"CDN TTL >= browser TTL")
+                && surrogate_verdicts.contains(&"should allow CDN caching"),
+            "both Surrogate-Control findings must survive the merge, got {surrogate_verdicts:?}"


🤔 thinking — This thread was marked resolved, but the test is unchanged and still asserts presence rather than verdict values.

git diff c71c5d98..e918e7a9 shows no change to this file. The assertion still collects expected strings and checks the two it wants are in the list, so it would pass even if either finding's severity were wrong — a Pass where a Warn belongs reads identically to it.

That is not hypothetical. The sibling surrogate_control_matrix_on_immutable_js has the same shape (asserting only Verdict::rollup), and it is why the rules.rs:489 blocker survives: for Surrogate-Control: no-store, max-age=31536000 the bogus Pass CDN-TTL verdict coexists with a genuine Warn from disables-caching, so the rollup still reads Warn while a wrong Pass sits inside it. Both tests pass today against code I have shown to be wrong.

Two changes make the class visible:

assert per-(header, expected) verdict values here and in the matrix test, not presence or the rollup

add a no-store, max-age=N row to the matrix, since the token-plus-number combination is exactly where the regression lives and no current row exercises it

And the point from the rules.rs:489 thread applies here: this test's fixture (no-store, max-age=30) depends on that bug to produce two findings, so fixing the bug requires a new fixture rather than a relaxed assertion.

aram356 · 2026-10-22T05:02:53Z

+//!
+//! `ts dev audit headers` fetches directly from the publisher origin. When no
+//! explicit URLs are given, it fetches the origin root, parses the returned HTML
+//! for asset URLs, and audits those. Routes under `/_ts/` are served by the


🤔 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.

<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.

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.

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.

aram356 · 2026-10-22T05:02:53Z

+/// is ever parsed for discovery; every other body is discarded, so this bounds
+/// a fast link from pulling an unbounded payload (a large image or video) that
+/// the timeout alone would not catch.
+const MAX_BODY_BYTES: u64 = 2 * 1024 * 1024;


⛏ nitpick — This thread was marked resolved, but the constant is unchanged.

Still 2 * 1024 * 1024 at this head; spec §175 says 5 MiB, read only for sampled HTML and stylesheets, with every other response closed after its headers.

Two small divergences: the limit, and that the read happens for every response and is then discarded rather than skipped for bodies that are never parsed. The cap makes the second harmless in practice, and the comment explains the reasoning honestly.

I do not think 5 MiB is self-evidently better than 2 MiB — this is mostly about the code and the spec agreeing. Either lift the cap and read only for the parsed content types, or amend the spec to 2 MiB. A reply saying "2 MiB is deliberate, spec should change" closes this.

aram356 · 2026-10-22T05:02:53Z

+    // Ensure at least the favicon is checked when the page links none.
+    if let Ok(favicon) = base.join("/favicon.ico")
+        && !urls.contains(&favicon)
+        && urls.len() < MAX_DISCOVERED_URLS


⛏ 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 · 2026-10-22T05:02:53Z

+        "#;
+        let urls = discover_asset_urls(&base, html, false);
+        assert!(
+            urls.contains(&Url::parse("https://origin.example/app.js").unwrap()),


⛏ 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 · 2026-10-23T18:51:56Z

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:

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.
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.

aram356 assigned vasujain00 Sep 16, 2026

aram356 requested changes Sep 20, 2026

View reviewed changes

aram356 requested review from ChristianPavilonis and prk-Jr September 20, 2026 17:59

prk-Jr requested changes Sep 21, 2026

View reviewed changes

Vasu Jain added 2 commits September 23, 2026 07:55

vasujain00 force-pushed the feature/834-audit-headers-impl branch from da37b83 to 6cf2d03 Compare September 23, 2026 15:26

vasujain00 commented Sep 23, 2026

View reviewed changes

aram356 requested changes Sep 24, 2026

View reviewed changes

aram356 and others added 5 commits September 23, 2026 22:58

Merge branch 'main' into feature/834-audit-headers-impl

2e2213e

Merge branch 'main' into feature/834-audit-headers-impl

25d7cd0

Merge branch 'main' into feature/834-audit-headers-impl

1944b31

Merge branch 'main' into feature/834-audit-headers-impl

b342a31

vasujain00 requested review from aram356 and prk-Jr October 2, 2026 10:17

Merge branch 'main' into feature/834-audit-headers-impl

6cf5e4d

prk-Jr requested changes Oct 13, 2026

View reviewed changes

Vasu Jain added 2 commits October 14, 2026 08:59

vasujain00 requested a review from prk-Jr October 14, 2026 18:02

vasujain00 commented Oct 14, 2026

View reviewed changes

aram356 and others added 2 commits October 15, 2026 08:24

Merge branch 'main' into feature/834-audit-headers-impl

3f09f07

ChristianPavilonis requested changes Oct 16, 2026

View reviewed changes

Merge branch 'main' into feature/834-audit-headers-impl

c71c5d9

aram356 requested changes Oct 17, 2026

View reviewed changes

vasujain00 requested a review from aram356 October 19, 2026 18:06

vasujain00 requested a review from ChristianPavilonis October 19, 2026 18:06

Vasu Jain and others added 3 commits October 19, 2026 11:13

Merge remote-tracking branch 'origin/main' into feature/834-audit-hea…

9333c11

…ders-impl

Merge remote-tracking branch 'fork/feature/834-audit-headers-impl' in…

751d444

…to feature/834-audit-headers-impl

Merge branch 'main' into feature/834-audit-headers-impl

e918e7a

prk-Jr requested changes Oct 21, 2026

View reviewed changes

aram356 requested changes Oct 22, 2026

View reviewed changes

aram356 added 2 commits October 22, 2026 09:59

Merge branch 'main' into feature/834-audit-headers-impl

0313db9

Merge branch 'main' into feature/834-audit-headers-impl

32f041c

		fn discover_asset_urls(base: &Url, html: &str, include_cross_origin: bool) -> Vec<Url> {
		let document = Html::parse_document(html);

-    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))
+    };

	let safe = directives.has("no-store") \|\| directives.has("private");
	let safe = directives.has_unqualified("no-store") \|\| directives.has_unqualified("private");

Code	Meaning
0	Audit completed; no FAIL verdicts (WARN allowed unless `--strict`)
1	A FAIL verdict, or a WARN verdict with `--strict`
2	Audit could not complete: bad arguments, config error, or transport failure

	`probe-shareability` (on main)	this PR
Group	`ts origin`	`ts dev audit`
URLs	repeatable `--url`	positional `[URLS]...`
Scheme	HTTPS enforced, loopback excepted	any scheme accepted
Credentials	environment only	n/a

Conversation

vasujain00 commented Sep 16, 2026

Summary

Changes

Design fidelity

Closes

Test plan

Checklist

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Blocking

🔧 wrench

Non-blocking

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

Cross-cutting / body-level findings

CI Status

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prk-Jr left a comment

Choose a reason for hiding this comment

Verdict

Blockers

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

Verification

Notably good

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasujain00 commented Sep 23, 2026

Uh oh!

vasujain00 left a comment

Choose a reason for hiding this comment

Update — review addressed (rev pushed)

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Blocking

🔧 wrench

Non-blocking

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

Cross-cutting / body-level findings

CI Status

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prk-Jr left a comment

Choose a reason for hiding this comment

Summary

Blocking

🔧 wrench