Add ts CLI ad-template config diagnostics and browser audit - #823
Add ts CLI ad-template config diagnostics and browser audit#823prk-Jr wants to merge 237 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Incorporate all review feedback (aram356 + jevansnyc): cache contract, consent/GDPR gating, async restructuring detail, CreativeOpportunityFormat schema, glob pattern fix, XSS escaping, win notifications, APS params, timeout config key, defineSlot fix, gpt.rs ownership, KV migration path, Phase 2 sketch - Fix Prettier formatting (format-docs CI) - Add implementation plan (12 tasks, TDD, ordered by dependency)
- Incorporate all review feedback (aram356 + jevansnyc): cache contract, consent/GDPR gating, async restructuring detail, CreativeOpportunityFormat schema, glob pattern fix, XSS escaping, win notifications, APS params, timeout config key, defineSlot fix, gpt.rs ownership, KV migration path, Phase 2 sketch - Fix Prettier formatting (format-docs CI) - Add implementation plan (12 tasks, TDD, ordered by dependency)
Replace the head-injected __ts_bids design with a server-cached bid delivery model fetched by the client via a new /ts-bids endpoint. The auction never blocks page rendering — </head> flushes immediately, body parses without waiting for bids, and the client fetches bids in parallel with content paint. Key changes: - §2 Goal: bid delivery decoupled from page rendering; FCP unchanged from no-TS baseline - §4.3 Auction Trigger: drop buffered/streaming dichotomy; single mode forces chunked encoding on all origins (WordPress, NextJS, etc.) - §4.4 Head Injection: only __ts_ad_slots and __ts_request_id injected at <head> open; bid results moved to /ts-bids endpoint - §4.6 Client Residual: __tsAdInit defines slots immediately, fetches bids via /ts-bids, applies targeting and fires refresh() after resolve - §4.7 (new) Caching Behavior: explicit cacheability table for HTML, JS, CSS, tsjs bundle, bid results; Fastly edge HTTP cache leveraged for origin HTML - §5 Request-Time Sequence: full mermaid diagram covering content + creative + burl flow with cache-hit and cache-miss branches; separate text sequences for cache-hit (~80ms FCP, ~900ms ad-visible) and cache-miss (~250ms FCP, ~1,050ms ad-visible) - §6 Performance Summary: cache-hit and cache-miss columns; FCP added as a tracked metric - §7 Implementation Scope: add bid_cache.rs, /ts-bids endpoint, force chunked encoding step - §8 Edge Cases: origin-agnostic entries; new entries for /ts-bids 404 and client-never-fetches-/ts-bids Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pivot from the /ts-bids fetch endpoint + in-process bid_cache design to
inline __ts_bids injection before </body>. The earlier design relied on
shared state that doesn't reliably survive Fastly Compute's per-request Wasm
isolate model — body injection achieves the same FCP property in a single
response with no shared-state requirement.
Key changes:
- §4.3: replace /ts-bids long-poll with bounded </body> hold tied to
A_deadline. Body content above </body> paints first; close-tag held
until auction completes or A_deadline fires (graceful __ts_bids = {}
fallback).
- §4.3: add auction-eligibility gating (consent, bot UA, prefetch hints,
HEAD method, slot match) so auctions fire on real first-page-load
impressions only.
- §4.4: replace __ts_request_id + /ts-bids machinery with two inline
<script> blocks — __ts_ad_slots at <head> open, __ts_bids before
</body> via lol_html el.on_end_tag().
- §4.5: move both nurl and burl to client-side firing from
slotRenderEnded after hb_adid match. Server-side firing rejected to
avoid billing inflation on bids that never render.
- §4.6: replace fetch+Promise pattern with synchronous __ts_bids read.
Add lazy slim-Prebid loader (post-window.load) for scroll/refresh
auctions and Phase B identity warm-up. Add ts_initial=1 slot-ownership
sentinel.
- §4.7: switch Cache-Control from private, no-store to private,
max-age=0 to preserve browser BFCache eligibility while still
preventing intermediate-cache leaks.
- §4.8 (new): document the EC/KV identity model as load-bearing auction
input — Phase A retrieval at request time, Phase B post-render
enrichment via slim-Prebid userID modules. Add bare-EC first-impression
caveat and auction_eid_count metric. Note federated-consortium
passphrase property and clickstream-compounding speed win.
- §5: update mermaid + cache-hit/miss timelines for bounded body hold;
ad-visible converges to ~870ms (hit) / ~1,020ms (miss).
- §6: drop /ts-bids RTT row; add DCL row; add clickstream-compounding,
TS-overhead, identity-coverage, and confidence-interval framing.
- §7: drop bid_cache.rs and /ts-bids endpoint from scope; add
auction-eligibility gating and slim-Prebid bundle build target. Add
explicit "Deleted" subsection.
- §8: drop /ts-bids edge cases; add SPA/pushState, bare-EC, bot/prefetch,
HEAD, BFCache restoration cases.
- §9.6: server-side GAM downgraded from "Phase 2 commitment" to
aspirational and contingent on Google agreement. §9.8 (slim-Prebid
bundle composition), §9.9 (Privacy Sandbox), §9.10 (per-bidder consent)
added as follow-ups.
Implementation plan at docs/superpowers/plans/2026-04-30-server-side-ad-templates.md
is now stale relative to this spec; needs regenerating before code lands.
…ities.toml Adds the creative_opportunities field to Settings struct to deserialize configuration for the server-side ad auction feature. Includes build.rs stubs for types required during build-time configuration validation. Creates creative-opportunities.toml with example slot configuration and updates trusted-server.toml with the [creative_opportunities] section defining GAM network ID, auction timeout, and price granularity settings. Tests pass with proper TOML parsing of the creative_opportunities section.
…ared auction state
- Add `ad_slots_script: Option<String>` and `ad_bids_state: Arc<RwLock<Option<String>>>` fields to `HtmlProcessorConfig`
- Update `from_settings` to initialize both new fields with safe defaults
- Prepend `ad_slots_script` inside the existing `<head>` handler before integration inserts
- Add `element!("body", ...)` handler that uses `end_tag_handlers()` to inject `__ts_bids` before `</body>`; falls back to empty `{}` when auction state is `None`
- Add `IntegrationRegistry::empty_for_tests()` test helper
- Add three new tests covering all injection paths
…gibility gates; max-age=0 - Make handle_publisher_request async; add orchestrator and slots_file params - Dispatch origin request with send_async before running auction in parallel - Gate auction on GET, no prefetch, no bot, matched slots, TCF purpose-1 consent - Run server-side auction and write bucketed bids to ad_bids_state Arc<RwLock> - Compute ad_slots_script after response headers; set Cache-Control: private, max-age=0 - Fix Stream arm to thread actual ad_slots_script and ad_bids_state through - Add build_auction_request, build_bid_map, build_bids_script, build_ad_slots_script helpers - Update route_tests.rs to pass empty slots_file to route_request
…m slotRenderEnded
- build_bid_map now returns serde_json::Map with full bid objects (hb_pb,
hb_bidder, hb_adid, nurl, burl) instead of a plain CPM string map
- build_bids_script / build_ad_slots_script now emit full <script> tags
using JSON.parse("…") for safe inline embedding; add html_escape_for_script helper
- build_ad_slots_script uses correct property names (gam_unit_path, div_id,
formats, targeting) matching the client-side TSJS bundle expectations
- Replace map_or(false, …) with is_some_and(…) on lines 546, 549, 567
- Add # Panics doc sections to handle_publisher_request and create_html_processor
…nities.toml at startup
… from slotRenderEnded; slim-Prebid lazy loader
- Enable APS and adserver_mock in auction config; set providers and mediator - Increase auction_timeout_ms from 500ms to 3000ms — 500ms was too tight for HTTPS round-trips to mocktioneer, leaving the mediator zero budget - Fix mediation request: send numeric price instead of opaque encoded_price; mocktioneer requires a decoded price field and does not support encoded_price - Expand creative-opportunities slot page_patterns to include /news/**
Define SlotRenderEndedEvent, SlotRenderEvent, and TestWindow types to eliminate all @typescript-eslint/no-explicit-any violations in gpt/index.ts and gpt/index.test.ts. Extend GptWindow with __tsjs_slim_prebid_url so installSlimPrebidLoader avoids the any cast.
Set gam_network_id to 88059007 (autoblog production network). Update atf_sidebar_ad slot to /88059007/autoblog/news with div_id ad-atf_sidebar-0-_r_2_ (desktop ATF sidebar, 300x250); restrict page_patterns to article paths only (/20**, /news/**) since that div does not exist on the homepage. Add homepage_header_ad slot targeting /88059007/autoblog/homepage with ad-header-0-_R_jpalubtak5lb_ for 970x90/728x90/970x250 leaderboard formats. Reduce auction_timeout_ms from 3000 to 500 to cap TTFB at the spec-recommended ceiling.
The bids script set window.__ts_bids but never invoked the __tsAdInit function, leaving GPT slots undefined and server-side targeting (hb_pb, hb_bidder) never applied. Both the winning-bid path (build_bids_script) and the no-auction fallback (html_processor None branch) now guard-call the function after the assignment.
aram356
left a comment
There was a problem hiding this comment.
Summary
Round-3 re-review at 09b7e4a7f. The resolution work is thorough: all 17 blocking findings from the previous review are addressed — 15 verified fixed outright (several by re-running the original reproductions), and most non-blocking carry-overs closed. Requesting changes for what this round introduced or left half-done: the config loader now silently swallows an unparseable [creative_opportunities] and overwrites the operator's slots, the new volatile-collision refusal is page-local and cannot distinguish a re-rendered element from two siblings (probed: a React SSR+hydration publisher generates zero slots), real-world identifiers re-entered the branch (Autoblog in a new spec, a hardcoded rh-gam-kso vendor rule that under-covers its own vendor), the CRLF scanner still mis-tracks triple quotes in comments (probed: a CRLF config silently flips to LF), and the 1024→128 evidence cap turns silent truncation into false --strict drift.
10 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them as commits on the PR branch. Every suggestion was verified in a scratch worktree: applied in isolation and as a batch, withcargo fmt --all -- --check,cargo clippy -p trusted-server-cli --target aarch64-apple-darwin --all-targets -- -D warnings, the full host CLI test suite (420 passed, 0 failed), anddocsprettier all clean, with byte-exact post-verification drift checks. The remaining comments describe fixes in prose because the change spans multiple files, non-contiguous regions, or a design decision.
Blocking
🔧 wrench
- Unparseable
[creative_opportunities]treated as absent — generate overwrites the operator's slots — see inline atcrates/trusted-server-cli/src/commands/audit/mod.rs:323 - Collision refusal is page-local; another page rescues the ambiguous prefix — see inline at
crates/trusted-server-cli/src/commands/audit/generate/gpt_slots.rs:183 - One element under two render tokens is refused as a collision — see inline at
crates/trusted-server-cli/src/commands/audit/generate/gpt_slots.rs:197 - Hardcoded
rh-gam-ksocustomer/vendor identifier under-covers its own vendor — see inline atcrates/trusted-server-cli/src/commands/audit/generate/gpt_slots.rs:220 - Real publisher named in the new spec doc — see inline at
docs/superpowers/specs/2026-08-19-refuse-volatile-div-collisions-design.md:43(suggestion) - Guide contradicts code and spec on out-of-page slots — see inline at
docs/guide/cli.md:410(suggestion) - 128-entry evidence cap: silent truncation reads as
--strictdrift — see inline atcrates/trusted-server-cli/src/commands/audit/ad_template_collector.js:31(suggestion) checkprints unescaped config-derived slot ids — see inline atcrates/trusted-server-cli/src/commands/config/ad_templates.rs:478(suggestion)- CRLF scanner desynchronized by a triple quote in a comment or single-line string — see inline at
crates/trusted-server-cli/src/commands/audit/generate/slot_toml.rs:581(suggestion) - Any two-letter section root is misread as a locale — see inline at
crates/trusted-server-cli/src/commands/audit/generate/crawl_plan.rs:346
❓ question
validate_merge_policyrefuses the first merge onto a hand-written templated config — see inline atcrates/trusted-server-cli/src/commands/audit/generate/mod.rs:1027
Non-blocking
🤔 thinking / ♻️ refactor / ⛏ nitpick
- 🤔 Generate silently follows the root redirect it now accepts — see inline at
crates/trusted-server-cli/src/commands/audit/generate/mod.rs:603(suggestion) - ♻️ Index-document-only sections vanish instead of collapsing to the parent — see inline at
crates/trusted-server-cli/src/commands/audit/generate/crawl_plan.rs:304 - ♻️ Userinfo leaks into stderr notes and the cross-origin refusal — see inline at
crates/trusted-server-cli/src/commands/audit/generate/mod.rs:588 - ♻️ Remaining per-page
report_errorpaths still double-log — see inline atcrates/trusted-server-cli/src/commands/audit/generate/browser_collector.rs:560 - ♻️
consent_stub_activeemitted per page × profile — see inline atcrates/trusted-server-cli/src/commands/audit/generate/browser_collector.rs:563 - ♻️ Legacy
ts audit generateexposes no browser flags or consent opt-out — see inline atcrates/trusted-server-cli/src/commands/audit/mod.rs:303 - ♻️ Known-per-render registry branch seeds the collision map with an unnormalized raw — see inline at
crates/trusted-server-cli/src/commands/audit/generate/gpt_slots.rs:130 - ⛏ Refusal table lacks rows for both new refusal classes — see inline at
docs/guide/cli.md:241(suggestion) - ⛏ Always-on stderr progress output is undocumented — see inline at
docs/guide/cli.md:246(suggestion) - ⛏ CLI manifest dependency ordering — see inline at
crates/trusted-server-cli/Cargo.toml:20(suggestion) - ⛏ Dry-run "no changes" sentence lands on machine-readable stdout; diff computed before the equality check — see inline at
crates/trusted-server-cli/src/commands/audit/generate/mod.rs:744(suggestion)
Cross-cutting / body-level findings
- 🔧 Companion edits the suggestions above cannot carry (fold into the same fixes):
browser.rs:43'sMAX_EVIDENCE_ENTRIES = 1024is now 8× the JS cap with nothing linking the two — align it with__ts_max_entriesor comment the difference; the stale out-of-page claims also live indocs/superpowers/specs/2026-08-18-pr-823-review-resolution-design.md:135-138anddocs/superpowers/plans/2026-06-26-server-side-ad-template-cli.md:1069, 1264; andrh-gam-ksoalso appears throughoutdocs/superpowers/specs/2026-08-19-refuse-volatile-div-collisions-design.mdand its plan. - 🌱 Consent-stub residual risk: the no-op setter fixes strict-mode assignment, but a CMP that installs via
Object.defineProperty(window, "__tcfapi", …)still throws on the non-configurable property, and the stubbed property is non-enumerable unlike a real global. Same class:__ts_install'swindow.googletagaccessor is non-enumerable, soObject.keys(window)no longer lists it — the fingerprint moved rather than disappearing. - 🌱 Dead/stale debt (pre-existing, grouped):
verify_round_trip's mismatch arm is provably dead (instrumented across the full suite) while its doc calls it "the gate", now directly contradicted by the round-3 test comment atunit_template.rs:619;explain'sunknownverdict arm is unreachable (config/ad_templates.rs:334, 385);gam_unit_path_unrenderablefires only for hand-built fixtures (compare.rs:270-279);VerificationReport.warningsis still always empty (audit/ad_templates.rs:163);ExtraEvidence.kinddocs still promisedom/aps(compare.rs:191); theaps_callsplumbing survives with a now-falsedead_codereason (compare.rs:50-53) and a stale JS header comment (ad_template_collector.js:3-4);page.rsstill has zero tests and one unescaped field (final url:); non-derivable-slug refusals report only the generic "several ad-unit paths" reason, so the guide's "the reason is reported" overstates what the operator sees. - ⛏ Grouped nits: the 08-19 spec under-specifies the recognizer (only
inarticle_<n>/overlay_<n>qualify;-containeris stripped first);browser_fixture_availableduplicated verbatim in two test modules; the resource-timing buffer size literal written twice (generate/browser_collector.rs:36vs the inline100000in the init script);GenerateBrowserOptsdropped the DANGEROUS rationale from--danger-accept-invalid-certs's doc;host_cookieevaluates and discardshost_str()and embeds the full URL (query/fragment/userinfo) where only the origin is load-bearing;ControlFlow::Stop's doc still promises to stop a crawl the buffering collector has already finished; the defaultcollect_siteaborts on a root failure while the browser implementor folds it; theLoadingprogress line prints before the pacing sleep;redirectednow fires on fragment-only differences;derive_sectionispubin core with zero consumers;load_file_settingsispubunder#[cfg(test)]; fractional sizes are mislabeled "non-numeric"; several new helpers lack doc comments and several new tests use bare asserts (gpt_slots.rs:211-244,crawl_plan.rs:55, 337-354, and the assert sites listed in the review threads);compare.rs:48cross-references a spec section that now states the opposite; the TOCTOU window between the pre-write re-read andtemp.persist()is fine but deserves the comment that was promised.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS (×2)
- Analyze (rust): PASS
- CodeQL: PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
Local verification at 09b7e4a7f: host CLI suite 420 passed / 0 failed; cargo clippy -p trusted-server-cli --all-targets -- -D warnings clean; core wasm32-wasip1 clippy clean; cargo build -p trusted-server-cli --target wasm32-wasip1 clean.
Blocking:
- Refuse an unreadable `[creative_opportunities]` section instead of reading it
as absent, which let a merge replace the operator's whole slot array.
- Tell one re-rendered element apart from two colliding elements by comparing
what the ephemeral markers did not cover, so a React SSR/hydration pair no
longer refuses itself (a fully per-render publisher generated zero slots).
- Refuse volatile div-id families by token shape rather than a hardcoded vendor
name, covering every placement after the token instead of two.
- Carry the ambiguous-stem verdict site-wide, so a landing page that renders one
member of a refused group cannot resurrect the prefix.
- Read only ISO 639-1 codes as a locale prefix, so `/tv`, `/ai` and `/us` stay
section roots.
- Track line endings past comments and single-line strings, so a stray triple
quote no longer flips a CRLF config to LF.
- Report evidence truncation instead of dropping entries silently, and align the
Rust cap with the collector's.
- Escape config-derived slot ids in `ts config ad-templates check` output.
Non-blocking:
- Adopt an inferred section policy when the config has none: a `{section}` slot
without `section_root` cannot load, so there is no policy to preserve.
- Note a followed root redirect; keep credentials, queries, and origins out of
per-page notes and the cross-origin refusal.
- Report per-page collection failures once and the consent stub once per run.
- Collapse index-document links onto their section.
- Expose the browser flags on `ts audit generate` and its legacy alias.
- Move the dry-run "no changes" sentence to stderr and build the diff lazily.
- Pace the crawl before announcing the page; scope audit cookies by origin.
- Make the consent stub configurable and enumerable so a CMP that installs via
`defineProperty` is not aborted, and the stub is not a fingerprint.
Docs and debt: correct the strict-mode claim for sizeless out-of-page slots,
document both new refusal classes and the stderr progress contract, drop the real
publisher and vendor identifiers from the spec, order the manifest dependencies,
and document the arms and fields that are unreachable or reserved.
A placement that only exists on section pages — a sidebar, an in-article unit —
witnessed no `section_root` of its own, so inference fell through to a literal
decision and refused the slot outright. On a live crawl that dropped
`ad-atf_sidebar-0` from the config even though its five observed ad-unit paths
differ only in the section segment, and the reported reason ("used several
ad-unit paths and none generalized") pointed at the wrong cause.
`SlotAnalysis::RootUnwitnessed` now carries the varying segment, so such a slot
templates against the config-level `section_root` another slot witnessed. That is
safe because the slot's page patterns are derived from the paths it was seen on,
all of which carry a section segment: `{section}` never falls back to the root
for it. A note names the borrowed `section_root`. When *no* slot witnessed a
root, nothing templates, and the diagnostic now says that the crawl never
included a page without a section segment instead of blaming generalization.
Verified against a live crawl: the sidebar is written with
`/{network_id}/autoblog/{section}`, matches only its five sections, and does not
match the root, while the previously written slots are unchanged.
aram356
left a comment
There was a problem hiding this comment.
Summary
Round-4 re-review at 073d5644a. The previous round is fully resolved: all 11 blocking findings and the non-blocking set verified fixed, with the original reproductions re-run (site-wide collision verdicts, residue-based re-render separation, the vendor const replaced by a shape detector, the comment/string-aware CRLF scanner, the ISO-639-1 locale gate, and creative_config refusing present-but-unparseable sections). The branch is clean of real-world identifiers. Requesting changes for a small set this round introduced: the redirect note degenerates to ``from / to `/```` on the http→https case, the legacy flatten publishes seven silently-ignored flags on `ts audit`, `--page-pattern` bypasses the root-less templating feature's safety invariant while its diagnostic asserts the invariant holds, and generate's settle cap silently changed 12s→10s — plus one question on merge policy.
8 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch) to apply them. Every suggestion was verified in a scratch worktree, in isolation and as a batch:cargo fmt --all -- --check,cargo clippy -p trusted-server-cli --target aarch64-apple-darwin --all-targets -- -D warnings, the CLI test suites (402 lib tests; 202 generate tests on the touched paths), anddocsprettier, all clean with byte-exact drift checks. The remaining comments describe fixes in prose because the change spans files or is a design decision.
Blocking
🔧 wrench
- Redirect note renders ``from
/to `/```` for scheme/host changes — see inline at `crates/trusted-server-cli/src/commands/audit/generate/mod.rs:619` (suggestion) - Legacy flatten publishes seven silently-ignored browser flags on
ts audit— see inline atcrates/trusted-server-cli/src/commands/audit/mod.rs:95 --page-patternbypasses the root-less templating safety invariant — see inline atcrates/trusted-server-cli/src/commands/audit/generate/mod.rs:1143- Settle constants dead; generate's cap silently 12s→10s — see inline at
crates/trusted-server-cli/src/commands/audit/generate/browser_collector.rs:26
❓ question
- Explicit
section_segmentadopted silently whensection_rootis unset — see inline atcrates/trusted-server-cli/src/commands/audit/generate/mod.rs:1099
Non-blocking
🤔 thinking / ♻️ refactor / ⛏ nitpick
- ♻️ Per-slot refusal reason still blames "none generalized" on the root-unwitnessed path — see inline at
crates/trusted-server-cli/src/commands/audit/generate/unit_template.rs:184(suggestion) - ♻️
fold_collecteddedupe is unconditional and lossy across profiles — see inline atcrates/trusted-server-cli/src/commands/audit/generate/mod.rs:956 - 🤔
is_per_render_tokenclaims date-prefixed stable segments — see inline atcrates/trusted-server-cli/src/commands/audit/generate/gpt_slots.rs:282 - ⛏
creative_configdefers whole-document syntax errors past the crawl — see inline atcrates/trusted-server-cli/src/commands/audit/mod.rs:349 - ⛏ Nothing enforces
MAX_EVIDENCE_ENTRIES == __ts_max_entries— see inline atcrates/trusted-server-cli/src/commands/audit/browser.rs:45 - ⛏
page.rsescaping test is vacuous for the line it names — see inline atcrates/trusted-server-cli/src/commands/audit/page.rs:131 - ⛏ Spec example still lists the sidebar as omitted — see inline at
docs/superpowers/specs/2026-08-19-refuse-volatile-div-collisions-design.md:70(suggestion) - ⛏ Guide implies
section_segmentalone trips the merge refusal — see inline atdocs/guide/cli.md:279(suggestion) - ⛏ Plan overstates the recognizer's positional rule — see inline at
docs/superpowers/plans/2026-08-19-refuse-volatile-div-collisions.md:75(suggestion) - ⛏
expectmessage form / missing field doc / missing blank line — see inline atcrates/trusted-server-cli/src/commands/audit/generate/mod.rs:1712,:102, andcrates/trusted-server-cli/src/commands/audit/generate/browser_collector.rs:169(suggestions)
Cross-cutting / body-level findings
- 📝 No plan/spec pair accompanies the root-less templating commit (
78c0db453): every other behavior change in this PR ships a dated design pair (contiguous slot tables, review resolutions, pre-navigation cookies, generation progress, volatile-div collisions). This one changesSlotAnalysissemantics, adds a borrowed-section_rootdiagnostic, and rewrites a documented refusal row with no design doc. Add a short pair, or note why the review-resolution plan covers it.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS (×2)
- Analyze (rust): PASS
- CodeQL: PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
Local verification at 073d5644a: CLI suites 402 lib + 232 audit-scoped tests passed, 0 failed; clippy -D warnings clean; fmt clean; docs prettier clean under the locked 3.8.1.
aram356
left a comment
There was a problem hiding this comment.
Summary
Round-5 re-review at 33a654e03. The previous round is fully resolved: the --page-pattern bypass is closed by threading borrowed_section_root out of inference and refusing before any write (with an end-to-end test asserting the config file stays byte-identical), the seven legacy browser flags are hidden behind requires = "legacy_url" with pass-through verified against the built binary, the settle constants are unified behind GENERATE_SETTLE_* restoring the intended 12s cap, the section_segment merge guard and profile-scoped diagnostics landed with tests, config syntax errors fail before any browser launches, and the previously undocumented feature commit now has its design pair. Requesting changes for one bug this round introduced — the malformed-config error prints its seven-line block twice — plus a set of precision and test-hardening items on the new code.
6 of the inline comments below carry a one-click GitHub
suggestion. Every suggestion was verified in a scratch worktree, in isolation and as a batch:cargo fmt --all -- --check,cargo clippy -p trusted-server-cli --target aarch64-apple-darwin --all-targets -- -D warnings, the CLI test suites (411 lib tests; 205 generate-scoped on the touched paths), anddocsprettier, all clean with byte-exact drift checks. The remaining comments describe fixes in prose because the change needs a second edit outside the range or is a judgement call.
Blocking
🔧 wrench
- Malformed-config error prints twice (
report_errorin acli_errorfile) — see inline atcrates/trusted-server-cli/src/commands/audit/mod.rs:413
Non-blocking
♻️ refactor / ⛏ nitpick
- ♻️ Borrowed-root refusal ignores fragment-skipped slots and over-refuses pattern sets — see inline at
crates/trusted-server-cli/src/commands/audit/generate/mod.rs:1154(suggestion) - ⛏ Crawl-gap refusal reason can contradict the run-level ambiguity diagnostic — see inline at
crates/trusted-server-cli/src/commands/audit/generate/unit_template.rs:182(suggestion) - ⛏ cli.md example comment describes the borrowed-root case as an exception, not a failure — see inline at
docs/guide/cli.md:265(suggestion) - ⛏ Parent-flag rejection test asserts only
is_err()— see inline atcrates/trusted-server-cli/src/run.rs:440(suggestion) - ⛏ No
Command::debug_assert()over the 13requiresstring ids — see inline atcrates/trusted-server-cli/src/run.rs:438(suggestion) - ⛏ Cap-invariant test coupled to JS punctuation, not the value — see inline at
crates/trusted-server-cli/src/commands/audit/browser.rs:936(suggestion) - ⛏
page/verifysettle defaults remain two unlinked sources of truth — see inline atcrates/trusted-server-cli/src/commands/audit/collector.rs:46 - ⛏ Hidden
<LEGACY_URL>surfaces in the rejection message — see inline atcrates/trusted-server-cli/src/commands/audit/mod.rs:59 - ⛏ Grouped: undocumented sibling
browserfield; help test pins 3 of 7 flags; test name understates — see inline atcrates/trusted-server-cli/src/commands/audit/mod.rs:236
Note: the two run.rs suggestions target adjacent ranges (line 438 and lines 440-451) — apply them as a batch so the second anchors cleanly.
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS (×2)
- Analyze (rust): PASS
- CodeQL: PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- format-docs: PASS (required)
- format-typescript: PASS (required)
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
Local verification at 33a654e03: CLI 411 lib + integration suites passed, 0 failed; clippy -D warnings clean; fmt clean; docs prettier clean.
Stop the malformed-config error printing twice. `creative_config` used `report_error`, which logs the message and returns it for the top-level `[ts]` printer to log again, so the whole seven-line `toml::de::Error` block was emitted twice. It now returns a plain `format!` like its sibling branch, names the config file from the path the caller already holds, and leads with the guidance so the multi-line parse error trails unbroken. Blame the crawl for an unwitnessed `section_root` only when the crawl gap is what stopped inference. The per-slot reason rewrite was gated on `root_witness_missing` alone while the run-level note was gated on `diagnostics.is_empty()`, so a run that stopped on segment ambiguity told the operator to widen the crawl when the remedy is pinning `section_segment`. Give the page audit's settle defaults one source of truth: `BrowserOpts` and `BrowserCollector::new` now share `PAGE_SETTLE_*` instead of holding independent literals that nothing pinned equal. Also: the borrowed-root refusal says `div id(s)`, matching what it prints and distinguishing it from the run-level diagnostic's slot ids; the `--page-pattern` docs say the run fails rather than implying inference is retained; the hidden legacy alias carries `value_name = "URL"` so its rejection does not name a field absent from `--help`; and the `ad-templates generate` browser field is documented like its twin. Test hardening: a `Command::debug_assert` over the crate's `requires` argument ids, the parent-flag rejection pins `MissingRequiredArgument` rather than any error, the JS/Rust evidence cap test parses the declared value instead of matching punctuation, the hidden-flag help test covers all seven flags, and `audit_page_subcommand_parses` is renamed for the settle defaults it now protects.
…plates Conflict: `publisher.rs` imports. This branch added the ad-stack gate diagnostics (`AdStackGateInput`, `RuntimeAdStackExpected`, `evaluate_ad_stack_gate`) while main added the ESI template-cache work (`AssemblyMode`, `CreativeOpportunitiesConfig`) on the same `use`. Both sets are still referenced after the merge, so the resolution is their union.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed the locked head revision 4c9777d14bf1f541f25cedd37770aa800e8668c6. CI and local validation pass. I found two medium-priority correctness issues in generation/merge behavior and one documentation-policy issue; details are inline.
| &mut |url, collected| { | ||
| match collected { | ||
| Ok(page) => { | ||
| let final_url = page.final_url().unwrap_or_else(|_| url.clone()); |
There was a problem hiding this comment.
Cross-origin redirects from non-root crawl pages are folded into generated config
The origin check protects only the first root navigation. Subsequent section pages pass their post-redirect final_url directly to fold_collected; the later-profile path does the same, including that profile's root page. A same-origin article URL that redirects elsewhere can therefore contribute foreign paths, GPT slots, formats, and ad-unit paths to template inference and the in-place rewrite. Host-scoped cookies are not leaked, but the generated config may bid against inventory from the redirect destination.
Apply origin_changed to every collected page before incrementing success counts or folding evidence. Please also add coverage for a redirected section page and a later-profile root redirect.
| .collect(); | ||
| let mut prefix_claims: BTreeMap<usize, BTreeSet<String>> = BTreeMap::new(); | ||
| for mut slot in discovered_slots { | ||
| if let Some(index) = matching_slot_index(&merged, &slot) { |
There was a problem hiding this comment.
A newly appended slot can incorrectly absorb a later discovered slot
matching_slot_index searches the entire mutable merged list. Once this loop appends a discovered slot, its exact div_id becomes a prefix candidate for later discoveries. For example, first-seen ad-top absorbs later ad-top-sidebar; the later slot's GAM path and provider state are discarded, while only formats and patterns are unioned. The result depends on discovery order, and no broad-prefix diagnostic appears because claims are recorded only for indices from the original config.
Restrict prefix reconciliation to the original existing_slots slice, and match current-run additions only by exact identity or deduplicate them before merging. Please add an order-sensitive regression test with an unrelated existing slot plus ad-top and ad-top-sidebar.
| `ts audit ad-templates generate` currently collects each page only after its | ||
| initial settle. Unlike `ts audit page` and `ts audit ad-templates verify`, it | ||
| cannot request the deterministic scroll pass that triggers lazy ad inventory. | ||
| On Autoblog this produced fewer observable frames than a scrolled page audit. |
There was a problem hiding this comment.
Use a fictional publisher in this design document
This document names Autoblog here and again at line 95. CLAUDE.md requires fictional information in docs and prohibits real customer or publisher names. Please replace it with a fictional publisher or describe the behavior generically.
Apply the requested origin boundary to every collected page, not just the root navigation. A section page that redirected off the audited origin previously folded its slots, formats and ad-unit paths into the generated config, and a later device profile's own root redirect was never checked at all. Both sites now skip such a page with a path-only note, and the later profile stops counting it towards profile coverage, so the existing zero-coverage refusal still fires when every page is lost. Restrict slot prefix reconciliation to the operator's original configured slots. matching_slot_index searched the whole mutable merged list, so a slot appended during this run became a prefix candidate for later discoveries: ad-top absorbed a later ad-top-sidebar, discarding its unit path and provider state while emitting no broad-prefix diagnostic. Run additions now match by exact identity instead, making the result order independent. Replace the real publisher named in the scroll and staleness design document with generic wording, per the documentation policy in CLAUDE.md. Tests cover a redirected section page, a later-profile root redirect, and an order-sensitive merge with an unrelated existing slot alongside ad-top and ad-top-sidebar. Reverting the two production changes fails exactly these three tests and nothing else.
Summary
[creative_opportunities]) configuration: static path/slot diagnostics viats config ad-templates …, and browser-backed live verification viats audit …(local Chrome/Chromium over CDP).ts audit ad-templates generate <url>to bootstrap[creative_opportunities]from a live site. One run crawls the publisher's sections (sitemap viarobots.txt, else navigation links), samples a landing page and an article per section, reconciles each slot across the pages it appeared on, and writes the result into an existingtrusted-server.tomlin place, preserving every other section and comment.{network_id}/{section}ad-unit template plus thesection_root/section_segmentpolicy it depends on, instead of pinning each slot to the one literal path it happened to be scraped from. A wrong template makes a publisher bid against inventory that does not exist, so inference refuses rather than guesses — see the table below.Settings::from_toml, the same load path the runtime uses at startup, on the--dry-runpath too. An unloadabletrusted-server.tomlis a full-site outage once pushed, not a degraded ad stack.chromiumoxide) are excluded from thewasm32-wasip1build, and the runtime ad-stack gate is shared withpublisher.rsso the CLI cannot drift from server behavior.closes #701
Changes
trusted-server-core/src/creative_opportunities.rs[creative_opportunities]config types,match_slots, sharedevaluate_ad_stack_gate;compile_page_patternas the single glob definition;derive_sectionmade public so tooling checks inference against the runtime's own derivation rather than a second implementationtrusted-server-core/src/publisher.rsshould_run_server_side_ad_stackthrough the shared gate (behavior-preserving)trusted-server-cli/src/commands/config/ad_templates.rsts config ad-templates {lint,match,check,explain}static diagnosticstrusted-server-cli/src/app_config.rstrusted-server-cli/src/ad_templates/{expected,compare,output}.rstrusted-server-cli/src/commands/audit/{mod,page,collector,browser,ad_templates}.rs,commands/audit/ad_template_collector.jsts audit page+ts audit ad-templates verify: chromiumoxide collector, read-only GPT/APS/DOM init script, verifier orchestration, cross-origin refusaltrusted-server-cli/src/commands/audit/generate/crawl_plan.rstrusted-server-cli/src/commands/audit/generate/evidence.rstrusted-server-cli/src/commands/audit/generate/unit_template.rs{network_id}/{section}inference with positional network binding, a single-varying-segment rule, the witness rule, and replay through the runtime's own renderertrusted-server-cli/src/commands/audit/generate/page_patterns.rs/newsand/news/*) without extrapolating past a witnessed sectiontrusted-server-cli/src/commands/audit/generate/validate.rsSettings::from_tomlbefore it replaces the file; a pre-existing failure downgrades to a warning so an already-broken config can still be updatedtrusted-server-cli/src/commands/audit/generate/{mod,gpt_slots}.rs_R_/_r_ids,-container, hex UUIDs)trusted-server-cli/src/commands/audit/generate/{browser_collector,collector,analyzer}.rstrusted-server-cli/src/run.rs,src/lib.rsauditnamespacetrusted-server-cli/Cargo.tomledgezero-core+serde_jsondeps (cfg-gated off wasm, like the existing browser deps)docs/guide/cli.mdts audit ad-templates generatedocumented: crawl behavior, refusal table, consent platforms, proxy auditing, and the deploy-ordering contractdocs/superpowers/{specs,plans}/2026-06-26-server-side-ad-template-cli*Test plan
Per CLAUDE.md, a bare
cargo test/cargo clippy --workspacefails at the workspace root — the repo has multiple wasm runtimes with runtime-specific SDKs, so the target-matched aliases are the real gate.cargo fmt --all -- --checkcargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcargo clippy -p trusted-server-cli --target <host-triple> --all-targets --all-features -- -D warningscargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spincargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity(13 passed)cargo test -p trusted-server-cli --target <host-triple>— 347 passedcd crates/trusted-server-js/lib && npx vitest run(829 passed)cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1(pluscargo build -p trusted-server-cli --target wasm32-wasip1— browser deps stay out of wasm)./scripts/test-cli.sh) for evidence collection and scroll-phase attributionts dev proxy— template and section policy inferred, per-render div-id fragments refused, generated config loads throughSettings::from_tomlNotable fixture-based coverage, all offline: crawl planning (cross-origin rejection on links and sitemap entries, utility/asset filtering, query/fragment collapsing, budget truncation), evidence reconciliation (format union, network-id conflict, fragment detection with a co-occurrence false-positive guard), and one test per template-inference refusal case.
How to use
Configure slots
In your (gitignored)
trusted-server.toml— fictional values shown:Generate slots from a live site (needs local Chrome/Chromium)
Re-running merges: a slot seen again keeps its hand-tuned fields and gains this run's patterns and newly observed formats, and a hand-written
gam_unit_pathtemplate is preserved.--replacediscards existing slots, including any template written by hand.Consent platforms. Publishers gate slot definition behind their consent platform, and the audit runs in a throwaway profile with no consent cookie — so such a site would define no slots at all and look identical to a site with no ad stack. The crawl therefore answers the two IAB interfaces every compliant platform exposes (TCF v2 and US Privacy) as a consenting, out-of-scope reader, before any page script runs. This changes only what the audit browser sees.
--no-assume-consentobserves the un-consented page instead.Auditing a production hostname served locally.
ts dev proxyserves a production hostname from a local Trusted Server; auditing through it keeps the page's origin, cookie scope, and any origin checks in the ad stack matching production rather thanlocalhost:Note that a local Trusted Server injects its own configured slots, so a run through the proxy can rediscover config it already has; slot ids absent from the current config are the publisher's own.
When generation keeps literal paths, and when it refuses
section_rootis unknownStatic diagnostics (no browser)
Browser-backed audit (needs local Chrome/Chromium)
Shared config flags (all of the above)
Exit behavior
verifyis auditor-assist: exits0even with missing/partial evidence.--strictexits 1 when a confirmable matched slot is missing or partially confirmed; video, native, and out-of-page slots areunconfirmableand do not fail the gate. A page-level navigation failure, or a redirect that leaves the requested origin, also exits non-zero.[auction].enabled = false) mark a page "skipped" so--strictdoes not fail it.Local live test (deterministic, no external site)
Many large ad publishers block headless/non-evasive browsers, so
verifyagainst them sees a challenge page rather than the article (this tool does not evade bot detection —--cookieforwards a clearance a human already earned, and--headfulruns a visible browser). When a page comes back without slots, the run now reports GPT's observable state — whether the library reachedapiReady, how many queued commands never drained, how many scripts ran — which distinguishes "the library never loaded" from "this page has no ads".To exercise the full pipeline reliably without an external site, serve a local fixture:
For a realistic end-to-end generation run,
ts dev proxyin front of a local Trusted Server is the reliable path — see the proxy example above.Checklist
unwrap()in production code — useexpect("should ...")println!/eprintln!in library code (CLI output useswriteln!; errors uselog)