Skip to content

Add a server-side ad template switch and cache policy - #1008

Open
ChristianPavilonis wants to merge 9 commits into
mainfrom
issue-1007-cache-control
Open

Add a server-side ad template switch and cache policy#1008
ChristianPavilonis wants to merge 9 commits into
mainfrom
issue-1007-cache-control

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add a clear [creative_opportunities].enabled switch for publisher server-side ad-template delivery.
  • Set structurally inactive successful GET publisher documents and their 304 Not Modified revalidations to the issue Improve cache header for html content when SSAT is off #1007 browser-only cache policy (private, max-age=60), replacing origin freshness directives while preserving origin private/no-store policies and CDN-specific cache headers.
  • Keep direct POST /auction available when publisher templates are disabled.

Issue #1007 exposed that publisher HTML caching was tied to whether the server-side ad stack ran, while the global auction setting also controlled unrelated auction behavior. This change separates publisher template delivery from the direct auction API and makes the cache behavior explicit.

Changes

File Change
crates/trusted-server-core/src/creative_opportunities.rs Adds the default-true enabled configuration field and serialization coverage.
crates/trusted-server-core/src/settings.rs Hides creative-opportunity slots from runtime handlers when template delivery is disabled and tests environment overrides.
crates/trusted-server-cli/tests/config_env_overlay.rs Verifies the typed CLI applies the new environment override on pushed configurations.
crates/trusted-server-core/src/config.rs Verifies compatibility for omitted defaults and explicit disabled values.
crates/trusted-server-core/src/publisher.rs Gates publisher HTML and page-bids template delivery, records the disabled-template reason, and tests cache behavior.
crates/trusted-server-core/src/response_privacy.rs Centralizes shared-storage policy checks and keeps cookie privacy stronger than the inactive browser policy.
crates/trusted-server-adapter-fastly/src/middleware.rs Verifies late cookie finalization downgrades the generated inactive browser policy.
crates/trusted-server-core/src/auction/endpoints.rs Proves direct POST /auction still dispatches when templates are disabled.
trusted-server.example.toml Documents the new setting in the example configuration.
docs/guide/configuration.md Documents the switch, cache policy, and environment override.
docs/superpowers/plans/2026-08-06-server-side-ad-templates-cache-control.md Records the implementation decisions, compatibility rules, and verification plan.
CHANGELOG.md Records the new switch and cache behavior.
crates/trusted-server-js/lib/src/core/index.ts Clarifies browser defaults when template delivery is gated.
crates/trusted-server-js/lib/src/integrations/gpt/index.ts Clarifies page-bids behavior when template delivery is disabled.
crates/trusted-server-js/lib/test/integrations/gpt/spa_hook.test.ts Updates the related regression-test explanation.

Scope

The change is limited to configuration, core publisher/page-bids execution, direct-auction regression coverage, browser comments, and documentation. Existing adapter routes already use the centralized settings accessor, so no divergent adapter-specific switch was needed. Active server-side templates retain private, no-store; inactive 200 OK GET document HTML and its 304 Not Modified revalidations use exactly private, max-age=60, intentionally replacing origin freshness directives per #1007 while preventing storage by shared caches that use Cache-Control. Origin private/no-store policies, other response statuses, non-GET requests, and non-document responses remain unchanged. Request-scoped privacy finalization still takes precedence, and validators plus CDN-specific headers remain unchanged. An empty slot list could disable delivery rollback-safely, but the dedicated switch preserves configured slot definitions for reversible operations; because explicit enabled = false is serialized, the guide documents the required config re-push before rolling back to a pre-field binary.

Closes

Closes #1007

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: cargo test-cloudflare, cargo test-spin, focused publisher tests, and all configured native/WASM clippy targets

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses project logging macros, not println!
  • New code has tests
  • No secrets or credentials committed

@ChristianPavilonis ChristianPavilonis self-assigned this Aug 6, 2026
@ChristianPavilonis ChristianPavilonis changed the title Use a short browser cache policy for non-SSAT HTML Use a browser cache policy for non-SSAT HTML Aug 6, 2026
@ChristianPavilonis ChristianPavilonis changed the title Use a browser cache policy for non-SSAT HTML Add a server-side ad template switch and cache policy Aug 7, 2026

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Separating publisher template delivery from the global [auction].enabled kill switch is the right boundary, and the direct-/auction regression test proves the separation holds. The configuration half of this PR is well covered.

The cache-policy half needs work. The new else branch does not just preserve origin policy when templates are inactive — it overwrites it, and its only protection is a two-token check for private/no-store. I verified against this branch (throwaway probes in ssat_cache_policy_tests, cargo test -p trusted-server-core) that origin no-cache, max-age=0, must-revalidate and s-maxage=0 are all replaced with max-age=60, and that 500/404/503 HTML responses become cacheable for 60 seconds. Details inline.

Blocking

🔧 wrench

  • Origin revalidation directives silently overwritten — the private/no-store guard misses no-cache, max-age=0, must-revalidate, s-maxage=0. A personalized page marked no-cache by origin, on a repeat visit that emits no Set-Cookie (so the adapter cookie-privacy net does not fire), gets max-age=60 with no private and no Vary — shared-cacheable and replayable to other users for 60s. (crates/trusted-server-core/src/publisher.rs:2986)
  • Non-2xx HTML becomes cacheable for 60s — no status gate on the new branch, so a transient origin 5xx is pinned in every browser and intermediary for a minute past recovery. (crates/trusted-server-core/src/publisher.rs:2961)
  • Rollback with enabled = false is a site-wide 500, and is undocumenteddeny_unknown_fields makes an older binary reject the blob, and load_settings_from_config_store() failing returns 500 for every request (crates/trusted-server-adapter-fastly/src/main.rs:112-118). The plan file states the "fail loud" intent, but neither docs/guide/configuration.md:1315 nor the CHANGELOG.md:12 entry warns operators what "loud" means here. (crates/trusted-server-core/src/config.rs:333)

❓ question

  • Absent [creative_opportunities] section also gets its cache policy rewrittenis_some_and makes "never configured" behave like "explicitly disabled", so deployments that never enabled server-side ad templates have their origin Cache-Control replaced with max-age=60. On main those responses passed through untouched. Intended blast radius? (crates/trusted-server-core/src/publisher.rs:2643)

Non-blocking

🤔 thinking

  • max-age=60 is an unexplained magic constant — no derivation in the CHANGELOG, configuration guide, or plan file, and not operator-tunable. (crates/trusted-server-core/src/publisher.rs:2992)
  • The empty-slot disable already existed and was rollback-safe — worth stating in the PR body why the new field's rollback cost was accepted. (crates/trusted-server-core/src/creative_opportunities.rs:206)

♻️ refactor

  • ad_templates_enabled / ad_templates_disabled are not complements — both are false when the section is absent; an explicit three-state enum would make that unmissable. (crates/trusted-server-core/src/publisher.rs:2644)

⛏ nitpick

  • should_run_server_side_ad_stack still takes 7 arguments — the new struct absorbed only 2 of the 8 flags, leaving 6 positional bools at every call site. (crates/trusted-server-core/src/publisher.rs:1765)
  • Test name contradicts its assertiondisabled_creative_opportunities_flag_is_visible_to_legacy_schema asserts expect_err, i.e. the legacy schema rejects the field. ..._is_rejected_by_legacy_schema would read correctly. (crates/trusted-server-core/src/config.rs:333)

📝 note

  • Cache-policy test matrix has the same gap as the codenavigation_without_matched_slots_preserves_private_origin_cache_policy covers "private, max-age=0" and "No-Store" only. Once the two wrench findings are settled, no-cache and a non-200 status belong in the same loop, or the regressions will not be caught. (crates/trusted-server-core/src/publisher.rs:5071)

👍 praise

  • Direct /auction regression testTemplateSwitchProbeProvider counts real provider invocations rather than asserting a status code, so it would actually fail if the template flag were later threaded into handle_auction. (crates/trusted-server-core/src/auction/endpoints.rs:707)
  • Rollback-compatible serialization of the defaultskip_serializing_if keeping default true out of pushed blobs matches the existing section_root precedent and keeps the no-opt-in case safe. (crates/trusted-server-core/src/creative_opportunities.rs:204)

CI Status

All 19 GitHub checks pass on 58054463.

  • fmt: PASS
  • clippy (fastly / axum / cloudflare native+wasm / spin native+wasm): PASS
  • rust tests (fastly, axum native, cloudflare, spin, cross-adapter parity, ts CLI): PASS
  • js tests (vitest): PASS
  • format-typescript / format-docs: PASS
  • integration + browser integration tests: PASS

The findings above are behavioral gaps that the current test matrix does not exercise, not CI failures.

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/config.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/creative_opportunities.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/auction/endpoints.rs
Comment thread crates/trusted-server-core/src/creative_opportunities.rs

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The switch mechanics are solid: default-true serde field with rollback-aware serialization, consistent accessor/handler gating, POST /auction independence proven by a provider-probe test, and validation still runs when disabled. The blocking concern is concentrated in the new cache-clamp branch, which overrides origin freshness directives beyond the private/no-store preserve-guard.

Blocking

🔧 wrench

  • Cache clamp overrides origin freshness directives beyond private/no-store: origins sending no-cache, must-revalidate, or max-age=0 get replaced with max-age=60 (crates/trusted-server-core/src/publisher.rs:2988 — see inline comment)

Non-blocking

🤔 thinking

  • Clamp blast radius: applies to all HTML, all methods, and publishers with no [creative_opportunities] section at all (crates/trusted-server-core/src/publisher.rs:2980 — see inline comment)
  • enabled = false config blobs break not-yet-upgraded binaries: the explicit-false rollback hazard is codified in a test but undocumented for operators (crates/trusted-server-core/src/config.rs:333 — see inline comment)

🌱 seedling

  • Hardcoded 60-second TTL: likely needs to become configurable when SSAT is re-architected for cacheability (crates/trusted-server-core/src/publisher.rs:2992 — see inline comment)

CI Status

  • fmt: PASS
  • clippy (all targets): PASS
  • rust tests (fastly/axum/cloudflare/spin/parity/CLI): PASS
  • js tests (vitest): PASS
  • browser integration tests: PASS

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/config.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
@aram356 aram356 added this to the 202608 milestone Aug 13, 2026

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The update resolves four of the five round-one concerns: the inactive policy is now correctly narrowed to 200 OK GET document HTML (with regression coverage for 206/404/500/503, POST, and non-document fetches), the enabled = false rollback hazard is documented with the correct re-push sequencing in the guide/example/CHANGELOG, GPT-diagnostics privacy now takes precedence via the finalize_response reordering, and the 60s TTL source is documented. The direct-auction probe test survived the rebase intact.

One blocking concern remains — and it was introduced by the round-one fix itself: the preserve guard was removed entirely rather than extended, so origin private/no-store HTML is now rewritten to a shared-cacheable policy.

Blocking

🔧 wrench

  • Removing the preserve guard makes origin private/no-store HTML shared-cacheable: inactive 200 OK GET documents replace origin privacy directives with bare max-age=60, and the test at crates/trusted-server-core/src/publisher.rs:5240 pins that in (crates/trusted-server-core/src/publisher.rs:2995 — see inline comment)

CI Status

  • fmt: PASS
  • clippy (all targets): PASS
  • rust tests (fastly/axum/cloudflare/spin/parity/CLI): PASS
  • js tests (vitest): PASS
  • browser integration tests: PASS

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
aram356 added a commit that referenced this pull request Aug 16, 2026
#1010)

Resolved publisher.rs to keep #1008's inactive-SSAT cache policy;
datadome protection.rs resolved to main's final #992 squash.

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Round-two pass, scoped to what the update introduces rather than re-litigating round one. The status/method/document narrowing and the finalize_response reordering both look right, and I verified that nothing per-user is injected on the inactive path — so the residual risk in the new branch is entirely about origin-declared policy and cache-variant mixing, not injected identity.

Two new blocking findings, neither of which overlaps the outstanding preserve-guard concern. The first is additive to it: even with origin private/no-store preserved, a no-cache origin still exposes an ad-free variant. The second is an operator-facing documentation/test defect in the new environment override.

Blocking

🔧 wrench

  • Request-scoped suppression makes the ad-free variant the only shared-cacheable one: the inactive branch has no !is_bot / !is_prefetch / consent gate, so bot, prefetch, and consent-denied navigations of an ad-eligible URL get bare max-age=60 while consenting humans get private, no-store — with no Vary on the publisher path (crates/trusted-server-core/src/publisher.rs:2995, see inline comment for probe output).
  • The documented environment override for the new switch silently no-ops on the real deploy path: Settings::from_toml_and_env is #[cfg(test)]-only, and the EdgeZero overlay cannot create missing TOML leaves (docs/guide/configuration.md:1381, see inline comment).

Non-blocking

🤔 thinking

  • max-age=60 is documented only where its largest affected audience won't read it: the policy also applies with enabled = true, with no [creative_opportunities] section at all, and on bot/prefetch/consent-denied requests (docs/guide/configuration.md:1350, trusted-server.example.toml:184-188).
  • Undocumented, untested exception — first-visit navigations never get max-age=60: enforce_set_cookie_cache_privacy rewrites any Set-Cookie-bearing response to private, max-age=0 (crates/trusted-server-core/src/publisher.rs:2999).

📝 note

  • Verified: nothing per-user is injected when the ad stack is inactive. The </body> bids/adInit() script is skipped (crates/trusted-server-core/src/html_processor.rs:373-377), every integration head_inserts implementation ignores the request context and serializes config values only (gpt.rs:489, prebid.rs:1077, sourcepoint.rs:1021, didomi.rs:340, datadome.rs:768), and the injected bundle src is a content hash rather than a per-user token (crates/trusted-server-core/src/tsjs.rs:5-9). Recording this so the blocking discussion stays on the actual mechanism: origin-declared policy replacement plus variant mixing, not leaked identity in the markup.
  • No Vary is written anywhere on the publisher HTML path. crates/trusted-server-core/src/publisher.rs contains no Vary writes; the existing writers all target other response types (static/tsjs bundles, /identify, proxied GPT and Sourcepoint assets, the fingerprint debug endpoint). Only an origin-supplied Vary survives into the max-age=60 response.

👍 praise

  • The finalize_response reordering is pinned by a test that asserts ordering, which is what actually regressed (crates/trusted-server-core/src/publisher.rs:5337).
  • The narrowing tests lock the blast radius round one asked about: 206/404/500/503, POST, and non-document coverage all assert origin-policy preservation rather than just a status code.

CI Status

  • fmt: PASS
  • clippy (fastly / axum / cloudflare native + wasm / spin native + wasm): PASS
  • rust tests (fastly, axum, cloudflare, spin, cross-adapter parity, ts CLI): PASS
  • js tests (vitest): PASS
  • browser + Fastly EC lifecycle integration tests: PASS

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread docs/guide/configuration.md Outdated
Comment thread docs/guide/configuration.md
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Adds a dedicated [creative_opportunities].enabled switch so publisher HTML and page-bids template delivery can be turned off independently of [auction].enabled, and applies the issue #1007 Cache-Control: max-age=60 policy to successful GET publisher documents when the server-side ad stack is structurally inactive. The structural-vs-request-scoped split is the right design and the test matrix around it is thorough. One blocking concern on the emitted directive and one question on scope.

Blocking

🔧 wrench

  • max-age=60 is shared-cacheable, not a browser-only policy: a bare max-age=60 with no private authorizes shared caches, contradicting the "browser-facing" framing in the code comment and the configuration guide. An origin no-cache on a personalized document becomes 60s of shared cacheability for any intermediary that does not read the CDN-specific headers (crates/trusted-server-core/src/publisher.rs:3054).

❓ question

  • Absent [creative_opportunities] adopts the new policy too: deployments that never configured the feature now have origin Cache-Control replaced on every successful GET HTML navigation, with no opt-out. Intended? (crates/trusted-server-core/src/publisher.rs:3033)

Non-blocking

♻️ refactor

  • Duplicated request-eligibility gate: the cache branch re-spells the request-scoped half of should_run_server_side_ad_stack; a future gate added to one will not reach the other (crates/trusted-server-core/src/publisher.rs:3037).
  • Third copy of the "already uncacheable" check: the same lowercase + contains pattern now exists in publisher.rs and twice in response_privacy.rs (crates/trusted-server-core/src/publisher.rs:3050).

🤔 thinking

  • ServerSideAdStackConfig bundles 2 of 8 gates: six positional bools remain, which is where mis-ordering actually bites (crates/trusted-server-core/src/publisher.rs:1804).
  • Rollback failure mode is documentation-only: enabled = false plus deny_unknown_fields means an older binary fails to load settings and every request fails; consider a ts config push warning alongside the guide's warning block (crates/trusted-server-core/src/creative_opportunities.rs:206).

🌱 seedling

  • Publisher-HTML body coverage: disabled_ad_templates_use_short_browser_cache_policy asserts headers only; the page-bids path has an equivalent body assertion, the publisher path does not (crates/trusted-server-core/src/publisher.rs:5278).

⛏ nitpick

  • Redundant Option walks: the new creative_opportunities local is bound and then re-derived on the next line (crates/trusted-server-core/src/publisher.rs:2708).
  • Unrelated test fixture weakened: enforce_set_cookie_cache_privacy is untouched by this PR, but its fixture lost the origin-public scenario it was written for (crates/trusted-server-adapter-fastly/src/middleware.rs:432).

👍 praise

  • Structural vs request-scoped split, with bot/prefetch/consent-denied retaining the origin policy, and a test matrix covering non-200, non-GET, non-document, and mixed-case No-Store (crates/trusted-server-core/src/publisher.rs:5433).
  • Moving gpt_diagnostics::finalize_response below the cache block so diagnostics privacy wins, pinned by a test (crates/trusted-server-core/src/publisher.rs:3060).
  • Direct /auction regression test with a probe provider, proving the PR's central premise end to end (crates/trusted-server-core/src/auction/endpoints.rs:738).

CI Status

All 19 checks pass on 38c9636.

  • fmt: PASS
  • clippy / cargo check (fastly, axum, cloudflare native + wasm, spin native + wasm): PASS
  • rust tests (fastly, axum, cloudflare, spin, ts CLI, cross-adapter parity): PASS
  • integration tests (browser, Fastly EC lifecycle): PASS
  • js tests (vitest) and format (typescript, docs): PASS

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-adapter-fastly/src/middleware.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/auction/endpoints.rs

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The structural-vs-request-scoped split and its test matrix are sound, and the round-three concerns remain resolved on this head. One blocking issue remains in the emitted directive itself: bare max-age=60 authorizes shared caches, which both contradicts the documented "browser-facing" intent and reintroduces a cross-user vector for origins that serve personalized HTML with bare no-cache. The fix is one word. Non-blocking cleanups accompany it.

Blocking

🔧 wrench

  • Emit private, max-age=60, not bare max-age=60: a bare max-age authorizes shared caches and removes the revalidation obligation no-cache origins relied on (crates/trusted-server-core/src/publisher.rs:3054 — see inline comment)

Non-blocking

♻️ refactor

  • Duplicated request-eligibility gate: the cache branch re-spells the request-scoped half of should_run_server_side_ad_stack (crates/trusted-server-core/src/publisher.rs:3033)
  • Third copy of the "already uncacheable" check: the lowercase + contains pattern now exists here and twice in response_privacy.rs (crates/trusted-server-core/src/publisher.rs:3050)

🤔 thinking

  • Six positional bools remain on should_run_server_side_ad_stack (crates/trusted-server-core/src/publisher.rs:1804)
  • Rollback failure mode is documentation-only: a ts config push warning would catch it at the point of action (crates/trusted-server-core/src/creative_opportunities.rs:206)

🌱 seedling

  • Publisher-HTML body coverage: the disabled-templates test asserts headers only (crates/trusted-server-core/src/publisher.rs:5278)

📝 note

  • Absent-section scope is confirmed intended: answered in an earlier round, documented in the guide, and pinned by a dedicated test (crates/trusted-server-core/src/publisher.rs:5484)

⛏ nitpick

  • Redundant Option walks (crates/trusted-server-core/src/publisher.rs:2707)
  • Middleware fixture lost its origin-public scenario (crates/trusted-server-adapter-fastly/src/middleware.rs:432)

CI Status

  • fmt: PASS
  • clippy (all targets): PASS
  • rust tests (fastly/axum/cloudflare/spin/parity/CLI): PASS
  • js tests (vitest): PASS
  • browser integration tests: PASS

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/creative_opportunities.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-adapter-fastly/src/middleware.rs Outdated
# Conflicts:
#	crates/trusted-server-cli/tests/config_env_overlay.rs
@ChristianPavilonis
ChristianPavilonis requested review from prk-Jr and removed request for prk-Jr August 20, 2026 16:20

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Clean separation of publisher template delivery from the global [auction].enabled switch, with thorough table-driven coverage of the new cache policy across status codes, request kinds, and origin policies. One blocking issue: the private, max-age=60 cap is defeated on revalidation, because the inactive-stack path preserves origin validators but does not carry the policy onto the resulting 304.

None of the inline comments below carry a one-click GitHub suggestion — each proposed fix either spans more than one hunk (the code change plus its new test) or is a design question rather than an edit, so every fix is described in prose and has to be applied manually.

Blocking

🔧 wrench

  • 304 revalidation replays origin freshness, defeating the 60s cap — see inline at crates/trusted-server-core/src/publisher.rs:3054

Non-blocking

🤔 thinking

  • No escape hatch for deployments that never configured SSAT — see inline at crates/trusted-server-core/src/publisher.rs:3048
  • private diverges from the issue text — see inline at crates/trusted-server-core/src/response_privacy.rs:27
  • Policy now varies per request with no Vary — see inline at crates/trusted-server-core/src/publisher.rs:3053

♻️ refactor

  • Extension marker has no end-to-end coverage — see inline at crates/trusted-server-core/src/response_privacy.rs:30

👍 praise

  • Quoted-string-aware Cache-Control directive parsing — see inline at crates/trusted-server-core/src/response_privacy.rs:32

Cross-cutting / body-level findings

  • 📝 PR description file table is incomplete — three changed files are missing from the table in the description: crates/trusted-server-adapter-fastly/src/middleware.rs (the late-cookie downgrade test now loops over both policies), crates/trusted-server-cli/tests/config_env_overlay.rs (the new TRUSTED_SERVER__CREATIVE_OPPORTUNITIES__ENABLED overlay assertion), and docs/superpowers/plans/2026-08-06-server-side-ad-templates-cache-control.md (the plan doc). The plan doc matches the established docs/superpowers/plans/ convention, so this is only about the table being a complete map of the diff.

  • 📝 Checked and found clean — recording these so the next reviewer does not have to re-derive them: prepare_runtime still calls compile_slots / compile_unit_templates / validate_runtime for a config with enabled = false, so the switch cannot be used to smuggle an invalid slot config past startup validation and then flip it on. The EC identity cookie is not refreshed for returning users (ec/finalize.rs:91), so cookie-privacy finalization does not silently downgrade every navigation to private, max-age=0 and leave max-age=60 unreachable in practice. Moving gpt_diagnostics::finalize_response after the cache-policy block is header-neutral on the active path, since both it and enforce_synthesized_html_cache_privacy land on private, no-store and both strip the CDN cache headers. The GeneratedInactiveAdStackBrowserCachePolicy extension does survive to every adapter's finalize — no path between handle_publisher_request and the adapter rebuilds the response head (see the ♻️ comment for the coverage gap that follows from this being untested).

CI Status

All 19 reported checks PASS; 4 are required under branch protection.

  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • format-typescript: PASS (required)
  • format-docs: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): 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

Comment thread crates/trusted-server-core/src/publisher.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/response_privacy.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/response_privacy.rs
Comment thread crates/trusted-server-core/src/response_privacy.rs
@aram356
aram356 self-requested a review August 21, 2026 04:48
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/creative_opportunities.rs
Comment thread crates/trusted-server-core/src/publisher.rs
ChristianPavilonis added a commit that referenced this pull request Aug 21, 2026
# Conflicts:
#	CHANGELOG.md
#	crates/trusted-server-adapter-fastly/src/middleware.rs
#	crates/trusted-server-core/src/publisher.rs
#	crates/trusted-server-core/src/response_privacy.rs

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross-cutting / body-level findings

  • 📝 CI did not run on this head — only Analyze (javascript-typescript) triggered for 8927e136f; the rest of the matrix (fmt, clippy chain, adapter tests, parity, integration, vitest, CodeQL) is not run per the check-suites API. Verified locally in a clean worktree of this head: cargo fmt --all -- --check passes and the full ssat_cache_policy/response_privacy suite (17 tests, including both new 304 revalidation tests) passes under cargo test-fastly on wasm32-wasip1. Recommend re-triggering the workflows (empty-commit push or close/reopen) so the full remote matrix is green before merge.

CI Status

  • Analyze (javascript-typescript): PASS
  • all other checks: not run (workflows never triggered for this push)
  • local: cargo fmt PASS; cargo test-fastly (cache-policy suite, 17/17) PASS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve cache header for html content when SSAT is off

3 participants