Add cache-header audit design spec for ts dev audit headers - #930
Add cache-header audit design spec for ts dev audit headers#930vasujain00 wants to merge 3 commits into
Conversation
Technical spec for epic IABTechLab#834: origin cache-header audit with per-content-type cacheability diagnostics. Covers content-type taxonomy, cacheability rules for all five directives (Cache-Control, Surrogate-Control, Surrogate-Key, Vary, ETag), per-type verdict rollup, CLI integration into the ts dev group, and a 7-task implementation breakdown. Closes IABTechLab#835
aram356
left a comment
There was a problem hiding this comment.
Summary
The spec is a solid starting shape for ts dev audit headers (clear taxonomy, verdict rollup, task breakdown), but several load-bearing claims about the codebase are stale or incorrect — ts dev is already a subcommand tree, there is no CliError enum, reqwest is not a CLI dependency, and the discovery section probes edge-only /_ts/ routes against the publisher origin. Two cacheability rules also fail the strictest correct configuration, and the doc itself is what's breaking the format-docs CI check.
Details are in the inline comments (8 blocking 🔧, 5 🤔, 1 ♻️, 2 🌱, 2 ⛏). Cross-cutting items below.
Blocking
🔧 wrench
- format-docs CI failure: prettier rejects this file's tables —
cd docs && npm run format:write(line 30) - Dependency graph must be mermaid: docs use vitepress-plugin-mermaid; ASCII art won't render (line 391)
- Stale
ts devpremise:DevCommandsubcommand tree already exists on main; noServeto default to (lines 254, 289) CliErrordoesn't exist: CLI errors areCliResult<T> = Result<T, String>(line 271)reqwestis not a CLI dep + target scoping unaddressed: workspace default target iswasm32-wasip1; spec must say where deps are scoped and that audit is cross-platform, unlike the macOS-only proxy (line 323)- Discovery probes edge routes against origin:
/_ts/*paths 404 onpublisher.origin_url(line 131) - Wrong issue link: #293 text links to /issues/834 (line 7)
- HTML/RTB rules fail
no-storealone: RFC 9111's strongest posture would FAIL the audit (lines 75, 106)
Non-blocking
📌 out of scope
- Plan document and epic breakdown still pending: issue #835 asks for a spec and a plan under
docs/superpowers/plans/plus a task breakdown in epic #834; this PR delivers the spec only (Section 10 is a partial plan). Also, the PR body retains the unfilled template (empty Summary/Changes tables, danglingCloses #).
CI Status
- format-docs: FAIL (caused by this file)
- All other checks (fmt, clippy targets, Rust/JS tests, integration, parity, CodeQL): PASS
|
|
||
| ### Arguments | ||
|
|
||
| | Argument | Description | |
There was a problem hiding this comment.
🔧 wrench — This file is what's failing the format-docs CI check: npx prettier --check rejects every markdown table in the doc (unpadded cells).
Fix: cd docs && npm run format:write and commit the result.
|
|
||
| ## 11. Dependency Graph | ||
|
|
||
| ``` |
There was a problem hiding this comment.
🔧 wrench — The dependency graph should be a mermaid diagram, not ASCII box art. The docs site uses vitepress with vitepress-plugin-mermaid, and existing specs (including the ts-dev-proxy spec referenced by #835 as the template) use mermaid; this block will render as a dead code block.
Fix:
```mermaid
flowchart TD
T1["Task 1: CLI restructure"] --> T6["Task 6: E2E wiring"]
T2["Task 2: Rules engine"] --> T3["Task 3: Origin fetch"] --> T4["Task 4: Analysis"] --> T5["Task 5: Output"] --> T6
T2 --> T4
T6 --> T7["Task 7: Docs"]
```|
|
||
| ### 8.1 `ts dev` restructure | ||
|
|
||
| `ts dev` is currently a flat command wrapping `fastly compute serve`. It must be restructured into a subcommand tree: |
There was a problem hiding this comment.
🔧 wrench — Stale premise: ts dev is not "a flat command wrapping fastly compute serve". On main, DevCommand is already a clap::Subcommand enum with a macOS-only Proxy variant (crates/trusted-server-cli/src/commands/dev/mod.rs), and local serving lives in the separate top-level ts serve (edgezero). There is no DevArgs and no Serve variant to default to — implementing this section as written would invent a Serve behavior ts dev never had.
Fix: rewrite Section 8.1 (and Task 1, line 289) against current main: add an Audit(DevAuditCommand) variant to the existing DevCommand enum and drop the Serve default / backward-compat requirement. Task 1's "modifies existing code on feature/ts-cli" is also stale — the CLI landed on main.
|
|
||
| ### 8.2 Error handling | ||
|
|
||
| Add `CliError::HeaderAudit` variant to `error.rs`. |
There was a problem hiding this comment.
🔧 wrench — There is no CliError enum to add a variant to. crates/trusted-server-cli/src/error.rs defines CliResult<T> = Result<T, String> plus helper functions.
Fix: either follow the existing String-error pattern, or explicitly propose introducing a typed error enum for the CLI — which is a much larger change than this one-liner implies and should be called out as such.
| **Type:** Net-new (`dev_audit/fetch.rs`) | ||
| **Dependencies:** Task 2 (uses `ContentTypeGroup`) | ||
|
|
||
| Fetch responses from origin using `reqwest` (already a dep). Support explicit URL list and config-derived discovery. |
There was a problem hiding this comment.
🔧 wrench — reqwest is not "already a dep" of trusted-server-cli; it's defined at the workspace level but not consumed by the CLI crate. More importantly, the workspace's default build target is wasm32-wasip1, which is exactly why the dev-proxy's tokio/rustls/ring stack is cfg-scoped to macOS in the CLI's Cargo.toml. This spec's own design decision says explicit URLs exist "for CI" (i.e. Linux) — but today DevCommand has zero variants off macOS.
Fix: state where the HTTP-client dependencies get scoped (presumably cfg(not(target_arch = "wasm32"))) and that ts dev audit, unlike ts dev proxy, must be available on all host platforms.
|
|
||
| ```rust | ||
| pub enum ContentTypeGroup { Html, JavaScript, Image, StaticAsset, RtbJson, Other } | ||
| pub enum Verdict { Pass, Warn(String), Fail(String) } |
There was a problem hiding this comment.
♻️ refactor — Verdict::Warn(String) / Fail(String) embed a message that duplicates HeaderVerdict.recommendation, and the JSON example (line 172) shows "verdict": "Fail" as a bare string — which payload-carrying variants can't serialize to without a custom serde impl.
Fix: make Verdict a unit enum (Pass, Warn, Fail) and keep messages on HeaderVerdict.
|
|
||
| ## 4. Cacheability Rules | ||
|
|
||
| Each content-type group has an expected caching posture. Rules are evaluated against: `Cache-Control`, `Surrogate-Control`, `Surrogate-Key`, `Vary`, `ETag`. |
There was a problem hiding this comment.
🌱 seedling — s-maxage is ignored throughout. It's the shared-cache TTL Fastly honors between Surrogate-Control and max-age, so the CDN-vs-browser comparison in 4.3 (and the RTB/HTML shared-cache rules) should account for it.
|
|
||
| ## 3. Content-Type Taxonomy | ||
|
|
||
| Responses are classified into groups by their `Content-Type` response header: |
There was a problem hiding this comment.
🌱 seedling — Two gaps worth noting: (1) Vary is listed among the five audited headers but only evaluated for HTML — Vary: User-Agent or Vary: Cookie on JS/CSS/images are classic hit-ratio killers; (2) classification must strip Content-Type parameters (text/html; charset=utf-8) before matching, which the spec doesn't state.
| | `Html` | `text/html` | | ||
| | `JavaScript` | `application/javascript`, `text/javascript`, `application/x-javascript` | | ||
| | `Image` | `image/*` | | ||
| | `StaticAsset` | `text/css`, `font/*`, `application/font-*`, `application/woff*` | |
There was a problem hiding this comment.
⛏ nitpick — application/woff* isn't a real MIME pattern: fonts are font/woff2 (covered by font/*) or legacy application/font-woff (covered by application/font-*). The entry is redundant.
| # Technical Specification: Origin Cache-Header Audit (`ts dev audit headers`) | ||
|
|
||
| **Status:** Draft | ||
| **Author:** @vasujai |
There was a problem hiding this comment.
⛏ nitpick — @vasujai doesn't match the author's GitHub login (vasujain00).
Technical spec for epic #834: origin cache-header audit with per-content-type cacheability diagnostics. Covers content-type taxonomy, cacheability rules for all five directives (Cache-Control, Surrogate-Control, Surrogate-Key, Vary, ETag), per-type verdict rollup, CLI integration into the ts dev group, and a 7-task implementation breakdown.
Closes #835
Summary
Changes
Closes
Closes #
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)