Skip to content

Add ts CLI ad-template config diagnostics and browser audit - #823

Open
prk-Jr wants to merge 230 commits into
mainfrom
feature/ts-cli-ad-templates
Open

Add ts CLI ad-template config diagnostics and browser audit#823
prk-Jr wants to merge 230 commits into
mainfrom
feature/ts-cli-ad-templates

Conversation

@prk-Jr

@prk-Jrprk-Jr commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Adds operator tooling to verify server-side ad-template ([creative_opportunities]) configuration: static path/slot diagnostics via ts config ad-templates …, and browser-backed live verification via ts audit … (local Chrome/Chromium over CDP).
  • Adds ts audit ad-templates generate <url> to bootstrap[creative_opportunities] from a live site. One run crawls the publisher's sections (sitemap via robots.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 existing trusted-server.tomlin place, preserving every other section and comment.
  • Where the evidence proves it, the run infers a {network_id}/{section} ad-unit template plus the section_root/section_segment policy 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.
  • Everything the run writes is validated before it replaces the file: the candidate goes through Settings::from_toml, the same load path the runtime uses at startup, on the --dry-run path too. An unloadable trusted-server.toml is a full-site outage once pushed, not a degraded ad stack.
  • Lets publishers catch slot/config drift before deploy — match a path to configured slots, assert expected slots in CI, explain runtime ad-stack gating, and confirm configured slots actually render (DOM/GPT/APS evidence) on live pages.
  • Keeps the CLI host-only: browser deps (chromiumoxide) are excluded from the wasm32-wasip1 build, and the runtime ad-stack gate is shared with publisher.rs so the CLI cannot drift from server behavior.

Stacks on the EdgeZero CLI base (server-side-ad-templates-impl). After merging the base back in, the diff against it is feature-only.

Behavior change: bare ts audit <url> remains a hidden alias for ts audit generate <url>; the base branch's draft-artifact generation moved to the explicit ts audit generate <url>.

Deploy ordering — read before pushing generated config. A config carrying section_root or section_segment is not rollback-safe. CreativeOpportunitiesConfig is deny_unknown_fields, so a binary predating ad-unit templating (cee1e20e) rejects the entireSettings load — not just the ad-template section — and every route serves an error. Deploy the template-aware binary first, then push, and do not roll that binary back while the config is live. A run that did not template writes neither key and leaves the config exactly as rollback-safe as it was. The command prints this warning itself.

closes#701

Changes

AreaChange
trusted-server-core/src/creative_opportunities.rs[creative_opportunities] config types, match_slots, shared evaluate_ad_stack_gate; compile_page_pattern as the single glob definition; derive_section made public so tooling checks inference against the runtime's own derivation rather than a second implementation
trusted-server-core/src/publisher.rsRoute should_run_server_side_ad_stack through the shared gate (behavior-preserving)
trusted-server-cli/src/commands/config/ad_templates.rsts config ad-templates {lint,match,check,explain} static diagnostics
trusted-server-cli/src/app_config.rsShared effective app-config loader for the config + audit commands
trusted-server-cli/src/ad_templates/{expected,compare,output}.rsPure expected-slot projection, DOM/GPT/APS evidence comparison, stable JSON model, terminal-safe text escaping
trusted-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 refusal
trusted-server-cli/src/commands/audit/generate/crawl_plan.rsNew. Turns links and sitemap entries into a bounded page set — one landing page and one article per section, ranked by nav/sitemap corroboration, capped by section and page budgets, same-origin enforced on both sources
trusted-server-cli/src/commands/audit/generate/evidence.rsNew. Cross-page slot evidence: formats unioned, divergent unit paths retained, network ids required to agree, and detection of one placement fragmented across per-render div ids
trusted-server-cli/src/commands/audit/generate/unit_template.rsNew.{network_id}/{section} inference with positional network binding, a single-varying-segment rule, the witness rule, and replay through the runtime's own renderer
trusted-server-cli/src/commands/audit/generate/page_patterns.rsNew. Expands observed paths into per-section glob pairs (/news and /news/*) without extrapolating past a witnessed section
trusted-server-cli/src/commands/audit/generate/validate.rsNew. Write-side gate: the candidate config must load through Settings::from_toml before it replaces the file; a pre-existing failure downgrades to a warning so an already-broken config can still be updated
trusted-server-cli/src/commands/audit/generate/{mod,gpt_slots}.rsCrawl orchestration, challenge-rate refusal, per-page diagnostics; slot reconstruction from live GPT plus normalization of ephemeral div-id noise (React _R_/_r_ ids, -container, hex UUIDs)
trusted-server-cli/src/commands/audit/generate/{browser_collector,collector,analyzer}.rsBatch collection on one reused browser, hydrated-DOM link extraction, in-page sitemap fetch, device profiles, request pacing, consent answering, proxy support
trusted-server-cli/src/run.rs, src/lib.rsCommand wiring and parser tests for the audit namespace
trusted-server-cli/Cargo.tomlHost-only edgezero-core + serde_json deps (cfg-gated off wasm, like the existing browser deps)
docs/guide/cli.mdts audit ad-templates generate documented: crawl behavior, refusal table, consent platforms, proxy auditing, and the deploy-ordering contract
docs/superpowers/{specs,plans}/2026-06-26-server-side-ad-template-cli*Design spec and implementation plan

Test plan

Per CLAUDE.md, a bare cargo test/cargo clippy --workspace fails 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 -- --check
  • cargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasm
  • cargo clippy -p trusted-server-cli --target <host-triple> --all-targets --all-features -- -D warnings
  • cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spin
  • cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity (13 passed)
  • Host CLI tests: cargo test -p trusted-server-cli --target <host-triple>347 passed
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run (829 passed)
  • 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 (plus cargo build -p trusted-server-cli --target wasm32-wasip1 — browser deps stay out of wasm)
  • Chrome-gated browser fixtures (./scripts/test-cli.sh) for evidence collection and scroll-phase attribution
  • Manual: full crawl against a live publisher through ts dev proxy — template and section policy inferred, per-render div-id fragments refused, generated config loads through Settings::from_toml

Notable 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

Static ts config ad-templates … commands only read local effective config (no browser). Browser-backed ts audit … commands launch a local Chrome/Chromium.

Configure slots

In your (gitignored) trusted-server.toml — fictional values shown:

[creative_opportunities]
gam_network_id = "123"section_root = "homepage"# required when a template uses {section}
[[creative_opportunities.slot]]
id = "atf"gam_unit_path = "/{network_id}/news/{section}"# or a literal pathdiv_id = "ad-atf-"# treated as a prefix; matches e.g. ad-atf-0page_patterns = ["/", "/news", "/news/*"]
formats = [{ width = 300, height = 250 }]
# optional provider hints
[creative_opportunities.slot.providers.aps]
slot_id = "atf"

Generate slots from a live site (needs local Chrome/Chromium)

# Crawl the site's sections and write [creative_opportunities] in place
ts audit ad-templates generate https://www.example.com/
ts audit ad-templates generate https://www.example.com/ --dry-run # preview, no write
ts audit ad-templates generate https://www.example.com/ --replace # overwrite instead of merge# Bound the crawl
ts audit ad-templates generate https://www.example.com/ --max-sections 20 --max-pages 41
ts audit ad-templates generate https://www.example.com/ --max-pages 1 # single page, as before# Set the patterns yourself; disables pattern inference
ts audit ad-templates generate https://www.example.com/ \
--page-pattern '/' --page-pattern '/news' --page-pattern '/news/*'# Check whether the publisher splits ad units by device
ts audit ad-templates generate https://www.example.com/ --profiles desktop,mobile
# Behind bot protection: carry a session, pace the crawl, or use a visible browser
ts audit ad-templates generate https://www.example.com/ \
--cookie '<NAME>=<VALUE>' --page-delay-ms 1500
ts audit ad-templates generate https://www.example.com/ --headful

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_path template is preserved. --replace discards 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-consent observes the un-consented page instead.

Auditing a production hostname served locally.ts dev proxy serves 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 than localhost:

ts dev proxy --map www.example.com=127.0.0.1:7676 --upstream-plaintext --rewrite-host
ts audit ad-templates generate https://www.example.com/ \
--browser-proxy 127.0.0.1:18080 --danger-accept-invalid-certs

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

SituationResult
Only one page was crawledLiteral path — one observation cannot distinguish a literal from a template
The ad unit never varied by sectionLiteral path
A section's slug is not derivable from its URLRefused slot omitted; the failed round-trip is reported
No root page was seen, so section_root is unknownRefused slot omitted rather than guessing a fallback
Two path segments could both be the sectionNo template; the ambiguity is reported
The unit varies by device, geo, or anything the URL cannot supplyRefused slot omitted with the conflicting evidence reported
Several slots are one placement under per-render div idsFragments reported and skipped, with the stable prefix they share
Crawled pages report different GAM network idsRun fails — the pages are not one property
More than a quarter of crawled pages return no slotsRun fails — the signature of bot protection serving challenge pages

Static diagnostics (no browser)

# Summarize config + deploy-time implications
ts config ad-templates lint
# Which configured slots match a path (or full URL)
ts config ad-templates match /news/story --details
# CI assertion: exact expected slot set
ts config ad-templates check /news/story --expected-slot atf
ts config ad-templates check /weather --expect-no-slots
ts config ad-templates check /news/story --expected-slot atf --allow-extra-slots
# Explain the runtime ad-stack gates for a modeled request
ts config ad-templates explain /news/story --bot --consent-denied --prefetch

Browser-backed audit (needs local Chrome/Chromium)

# Generic read-only page summary
ts audit page https://www.example.com/
# Verify configured slots render on live pages (DOM/GPT/APS evidence)
ts audit ad-templates verify https://www.example.com/news/story
ts audit ad-templates verify https://www.example.com/news/story --json
ts audit ad-templates verify https://www.example.com/ --strict --scroll
# Carry a session past an origin challenge (repeatable)
ts audit ad-templates verify https://www.example.com/ --cookie '<NAME>=<VALUE>'# Allow a known redirect between your own properties (e.g. apex → www)
ts audit ad-templates verify https://example.com/ --allow-cross-origin-redirect
# Point at a specific browser / tune settle timing
ts audit ad-templates verify https://www.example.com/ \
--chrome /path/to/chrome --settle-quiet-ms 1000 --settle-max-ms 15000

Shared config flags (all of the above)

--app-config <path> # default: <app.name>.toml beside edgezero.toml
--manifest <path> # default: edgezero.toml
--no-env # skip the app-config environment overlay

Exit behavior

  • Default verify is auditor-assist: exits 0 even with missing/partial evidence.
  • --strict exits 1 when a confirmable matched slot is missing or partially confirmed; video, native, and out-of-page slots are unconfirmable and do not fail the gate. A page-level navigation failure, or a redirect that leaves the requested origin, also exits non-zero.
  • Runtime gates (e.g. [auction].enabled = false) mark a page "skipped" so --strict does not fail it.

Local live test (deterministic, no external site)

Many large ad publishers block headless/non-evasive browsers, so verify against them sees a challenge page rather than the article (this tool does not evade bot detection — --cookie forwards a clearance a human already earned, and --headful runs a visible browser). When a page comes back without slots, the run now reports GPT's observable state — whether the library reached apiReady, 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:

# 1. Fixture page that stubs just enough GPT (no network), served over HTTP
mkdir -p demo/news
cat > demo/news/story.html <<'HTML'<!doctype html><html><head><metacharset="utf-8"><divid="ad-atf-0"></div><script>(function(){var s=[];var gt={cmd:[]};gt.defineSlot=function(p,z,d){var o={getAdUnitPath:function(){return p},getSlotElementId:function(){return d},getSizes:function(){returnz.map(function(x){return {getWidth:function(){return x[0]},getHeight:function(){return x[1]}}})}};s.push(o);return o};gt.pubads=function(){return {getSlots:function(){return s}}};var op=gt.cmd.push.bind(gt.cmd);gt.cmd.push=function(c){op(c);c()};window.googletag=gt;googletag.cmd.push(function(){googletag.defineSlot('/123/news/atf',[[300,250]],'ad-atf-0')});})();</script></head><body></body></html>HTML
(cd demo && python3 -m http.server 8771 &)
# 2. Verify against it (config from the example above, with page_patterns ["/news/*"])
ts audit ad-templates verify http://127.0.0.1:8771/news/story.html --json
# => slots[0].status == "confirmed"

For a realistic end-to-end generation run, ts dev proxy in front of a local Trusted Server is the reliable path — see the proxy example above.

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • No println!/eprintln! in library code (CLI output uses writeln!; errors use log)
  • New code has tests
  • No secrets or credentials committed

jevansnycand others added 30 commits April 15, 2026 20:47
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
- 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
… 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.

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

Automated Review:

Review Summary

Reviewed the current head (e8f459352330b988e7bebe77028d1522299ebc5f) against main. One non-blocking repository-guidance issue was found in a test fixture.

Findings by P0–P3

  • P0: None
  • P1: None
  • P2: None
  • P3: 1 inline finding

CI / Existing Reviews

The focused CLI generator tests passed locally. GitHub checks are successful except the browser integration test, which is cancelled. Earlier review feedback on prior commits was considered; no review by the authenticated reviewer existed for the current head before this submission.

Comment threadcrates/trusted-server-cli/src/commands/audit/generate/gpt_slots.rs Outdated

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

Automated Review:

Summary

One functional issue remains in the new ad-template generation flow.

Findings by priority

  • P1: The root-redirect trust-boundary check rejects a safe same-host HTTP-to-HTTPS upgrade, so a common canonical redirect prevents generation with no opt-out.

CI & Existing Reviews

The current head passed the reported Rust, adapter, formatting, integration, and Vitest checks; the browser-integration check was cancelled. I also ran the focused update_slots_ CLI tests (9 passed) and Vitest. Existing feedback about cross-origin redirect cookie scoping appears addressed; this finding is limited to the separately safe same-host HTTP-to-HTTPS case.

Comment threadcrates/trusted-server-cli/src/commands/audit/generate/mod.rs Outdated

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

Automated Review:

Review Summary

Reviewed the PR's complete three-dot diff, including the CLI configuration diagnostics, browser evidence collection, audit generation, core configuration changes, and associated tests. No additional high-confidence findings were identified beyond the existing inline feedback, so no duplicate inline comments are posted.

Findings

No additional findings.

CI / Existing Reviews

The current CI checks are successful except for the browser integration test, which is marked cancelled. Existing inline feedback on the current head was considered and is not duplicated by this review.

The known-per-render slot tests embedded a real GAM network id, ad-unit
path, and div-id token. Replace them with the fictional network id already
used elsewhere in the module, a generic placement path, and a synthetic
token.
The `rh-gam-kso` prefix stays: it is the shipped matcher constant under
test, not fixture data. The token keeps the shape the matcher requires
(eight or more leading digits followed by letters), so both the dynamic
and all-digit branches are still exercised.
`Url::origin()` includes the scheme, so an `http://publisher.example/`
entry URL that canonically redirects to `https://publisher.example/` was
refused as a cross-origin root redirect, forcing operators to find and
enter the canonical URL before generation could run.
Reuse the verify path's `origin_changed`, which already permits only the
same-host default-port `http:80` to `https:443` upgrade and still refuses
host changes, port changes, and HTTPS downgrades. The host is the cookie
boundary, so the upgrade leaves the trust boundary intact.

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

Automated Review:

Summary

Reviewed the PR head (09b7e4a7fbbc6a588d862b6c262b4eb0ed953a0d) against main, including the CLI audit/config implementation, redirect and cookie-boundary handling, crawl/generation behavior, and relevant tests. I found no additional high-confidence findings beyond the existing line-level review feedback.

Findings by severity

  • P0: None.
  • P1: None newly reported.
  • P2: None newly reported.

CI / Existing Reviews

  • CI: GitHub reports 19 successful checks for this head.
  • Existing reviews: Current-head inline discussions already cover the actionable concerns identified during this review (including generation merge safety, crawl coverage, cookie scope, verification behavior, and collector lifecycle). To avoid duplicate feedback, no additional inline comments are included.

@aram356aram356 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-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, with cargo 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), and docs prettier 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 at crates/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-kso customer/vendor identifier under-covers its own vendor — see inline at crates/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 --strict drift — see inline at crates/trusted-server-cli/src/commands/audit/ad_template_collector.js:31 (suggestion)
  • check prints unescaped config-derived slot ids — see inline at crates/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_policy refuses the first merge onto a hand-written templated config — see inline at crates/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_error paths still double-log — see inline at crates/trusted-server-cli/src/commands/audit/generate/browser_collector.rs:560
  • ♻️ consent_stub_active emitted per page × profile — see inline at crates/trusted-server-cli/src/commands/audit/generate/browser_collector.rs:563
  • ♻️ Legacy ts audit generate exposes no browser flags or consent opt-out — see inline at crates/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's MAX_EVIDENCE_ENTRIES = 1024 is now 8× the JS cap with nothing linking the two — align it with __ts_max_entries or comment the difference; the stale out-of-page claims also live in docs/superpowers/specs/2026-08-18-pr-823-review-resolution-design.md:135-138 and docs/superpowers/plans/2026-06-26-server-side-ad-template-cli.md:1069, 1264; and rh-gam-kso also appears throughout docs/superpowers/specs/2026-08-19-refuse-volatile-div-collisions-design.md and 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's window.googletag accessor is non-enumerable, so Object.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 at unit_template.rs:619; explain's unknown verdict arm is unreachable (config/ad_templates.rs:334, 385); gam_unit_path_unrenderable fires only for hand-built fixtures (compare.rs:270-279); VerificationReport.warnings is still always empty (audit/ad_templates.rs:163); ExtraEvidence.kind docs still promise dom/aps (compare.rs:191); the aps_calls plumbing survives with a now-false dead_code reason (compare.rs:50-53) and a stale JS header comment (ad_template_collector.js:3-4); page.rs still 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; -container is stripped first); browser_fixture_available duplicated verbatim in two test modules; the resource-timing buffer size literal written twice (generate/browser_collector.rs:36 vs the inline 100000 in the init script); GenerateBrowserOpts dropped the DANGEROUS rationale from --danger-accept-invalid-certs's doc; host_cookie evaluates and discards host_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 default collect_site aborts on a root failure while the browser implementor folds it; the Loading progress line prints before the pacing sleep; redirected now fires on fragment-only differences; derive_section is pub in core with zero consumers; load_file_settings is pub under #[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:48 cross-references a spec section that now states the opposite; the TOCTOU window between the pre-write re-read and temp.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.

Comment threadcrates/trusted-server-cli/src/commands/audit/mod.rs Outdated
Comment threadcrates/trusted-server-cli/src/commands/audit/generate/gpt_slots.rs Outdated
Comment threadcrates/trusted-server-cli/src/commands/audit/generate/gpt_slots.rs Outdated
Comment threaddocs/superpowers/specs/2026-08-19-refuse-volatile-div-collisions-design.md Outdated
Comment threadcrates/trusted-server-cli/src/commands/audit/generate/gpt_slots.rs Outdated
Comment threaddocs/guide/cli.md Outdated
Comment threaddocs/guide/cli.md Outdated
Comment threadcrates/trusted-server-cli/Cargo.toml Outdated
Comment threadcrates/trusted-server-cli/src/commands/audit/generate/mod.rs
prk-Jrand others added 2 commits August 20, 2026 13:20
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.
prk-Jrand others added 2 commits August 20, 2026 16:35
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.

@aram356aram356 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-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), and docs prettier, 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 at crates/trusted-server-cli/src/commands/audit/mod.rs:95
  • --page-pattern bypasses the root-less templating safety invariant — see inline at crates/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_segment adopted silently when section_root is unset — see inline at crates/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_collected dedupe is unconditional and lossy across profiles — see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:956
  • 🤔 is_per_render_token claims date-prefixed stable segments — see inline at crates/trusted-server-cli/src/commands/audit/generate/gpt_slots.rs:282
  • creative_config defers whole-document syntax errors past the crawl — see inline at crates/trusted-server-cli/src/commands/audit/mod.rs:349
  • Nothing enforces MAX_EVIDENCE_ENTRIES == __ts_max_entries — see inline at crates/trusted-server-cli/src/commands/audit/browser.rs:45
  • page.rs escaping test is vacuous for the line it names — see inline at crates/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_segment alone trips the merge refusal — see inline at docs/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)
  • expect message form / missing field doc / missing blank line — see inline at crates/trusted-server-cli/src/commands/audit/generate/mod.rs:1712, :102, and crates/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 changes SlotAnalysis semantics, adds a borrowed-section_root diagnostic, 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.

Comment threadcrates/trusted-server-cli/src/commands/audit/generate/mod.rs
Comment threadcrates/trusted-server-cli/src/commands/audit/mod.rs Outdated
Comment threadcrates/trusted-server-cli/src/commands/audit/generate/mod.rs
Comment threadcrates/trusted-server-cli/src/commands/audit/generate/browser_collector.rs Outdated
Comment threadcrates/trusted-server-cli/src/commands/audit/generate/mod.rs
Comment threaddocs/guide/cli.md Outdated
Comment threaddocs/superpowers/plans/2026-08-19-refuse-volatile-div-collisions.md Outdated
Comment threadcrates/trusted-server-cli/src/commands/audit/generate/mod.rs Outdated
Comment threadcrates/trusted-server-cli/src/commands/audit/generate/mod.rs
@prk-Jr
prk-Jr requested a review from aram356August 21, 2026 10:59
Sign up for freeto 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.

Build a standalone validation tool for configuring creative opportunities

4 participants

@prk-Jr@aram356@ChristianPavilonis@jevansnyc