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 206 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.
A live crawl produced fourteen slots where four were real. Ten were two placements repeated: an ad stack built its div ids from a per-render token, so the same placement arrived under a new key on every page. Written verbatim those ids match nothing at runtime, and the fragmentation also starves template inference, which needs to observe a slot more than once. Detect it from evidence rather than by pattern-matching token shapes, since each stack invents its own and the previous two forms already needed separate handling. Candidates share an identical ad-unit path and identical formats; what separates a fragmented placement from two legitimate siblings on one unit is co-occurrence. Real siblings appear together on a page, while fragments never do, because each page yields exactly one of them. Fragments are reported and skipped rather than written. The report names the observed ids and the stable prefix they share, so the operator can add the placement once with a prefix they know survives a render. That prefix is deliberately not written as a `div_id`: it reaches only as far as the observed tokens happen to agree, so it would match this crawl's ids and miss the next render's. Verified live: the run that previously wrote fourteen slots now writes the four real ones and explains the two it declined.
Four options landed after the command was first documented and were never written up: request pacing, a headful browser, the consent answer, and auditing through a local proxy. Each exists because a live audit of a protected publisher failed without it, so the reason belongs alongside the flag. Consent gets its own section because the failure is silent. A publisher gates slot definition behind its consent platform, the audit runs in a throwaway profile with no consent cookie, and the result is a page that appears to have no ad stack at all — indistinguishable from one that genuinely has none. Also record that an empty slot registry now reports GPT's observable state, which is what separates "the library never loaded" from "this page has no ads". Proxy auditing gets a section because `ts dev proxy` is how a production hostname is served locally, and matching the production origin matters for cookie scope and for origin checks inside the ad stack. Note the caveat that a local Trusted Server injects its own configured slots, so a run through the proxy can rediscover config it already has. Finally, describe how per-render div ids are detected and reported, including why the suggested prefix is offered but never written.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated Review:
Requested severity: P1 (merge-blocking correctness)
Summary
The device-profile safety check does not collect the root page with profiles after the first. On a site for which the crawl plan has no additional targets, the generated configuration can contain the first profile's literal GAM unit path and silently bid on a nonexistent unit for the other profile.
CI
GitHub currently reports all required checks passing, including Rust/JS analysis, CodeQL, formatting, CLI/native tests, browser and integration tests, and Vitest.
Existing review feedback
I checked the current inline comments and prior reviews. The earlier TOML-write, page-pattern validation, and redirect-default findings have follow-up fixes; this root-page device-profile gap is separate and is not duplicated by the existing feedback.
The multi-profile crawl repeated the first collector for the root page instead of the profile being walked, so every later profile recorded the root as the first device. On a site whose root offers no crawl targets that hid the device split entirely, and the first profile's literal GAM unit path was written as if both devices agreed with it.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
This adds a large, genuinely useful operator surface — static ad-template diagnostics, browser-backed verification, and crawl-based config generation — and the runtime-facing core change (should_run_server_side_ad_stack routed through evaluate_ad_stack_gate) is verified behavior-preserving by its own exhaustive test. Requesting changes for a set of correctness and data-integrity issues concentrated in the new CLI code: the in-place TOML splice can lose or corrupt operator config, refused slots end up bidding on fabricated ad-unit paths, two page-controlled inputs crash or empty the pipeline (non-ASCII div ids; out-of-u32 sizes), --dry-run prints operator secrets to stdout, and the verify path diverges from generate in ways that produce false CI failures. Details are in the inline comments; cross-cutting and lower-severity items follow below.
Blocking
🔧 wrench
ts config ad-templatesships with zero user documentation: the four-subcommand namespace (lint,match,check,explain) and all its flags (--details,--expected-slot,--expect-no-slots,--allow-extra-slots,--method,--non-navigation,--prefetch,--bot,--consent-denied,--edgezero-enabled) are absent fromdocs/guide/cli.md.checkis the CI assertion wrapper — the subcommand most likely to be reached for from a doc and least likely to be discovered from--help. Please add a section covering the four subcommands, and state that--app-config/--manifest/--no-envapply to everyts config ad-templatesandts audit ad-templatescommand (currently documented only forconfig init).
Non-blocking
🤔 thinking
- SRA gampad fallback misattributes multi-slot requests: the
gampad/adsURL parser takes the comma-joinediu_partsas one path and credits the first div with every slot's unioned sizes (generate/gpt_slots.rs:260-285). The~-joined registry form is guarded (is_multi_slot_div) but comma-separateddidsis not — returnNonewhendidshas more than one entry. - Fragment detection can drop two genuinely distinct placements: co-occurrence is the only discriminator, so two slots sharing a unit path and format set that appear on disjoint page subsets (e.g. landing-only vs article-only) are declared fragments and skipped from output (
generate/evidence.rs:217-241; thealpha-1111/beta-2222test shows the prefix guard firing on ids with nothing in common). Require a shared prefix or ≥3 fragments before removing slots; otherwise downgrade to a note. - The gate refactor adds a heap allocation to a per-request hot path:
evaluate_ad_stack_gatepushes into aVecon every blocked request (creative_opportunities.rs:982viapublisher.rs:1817), and blocked (non-matching path, bot, prefetch) is the majority path — where main had a zero-allocation boolean AND. - Synchronous page analysis runs inside the current-thread runtime driving the CDP event pump:
sink → fold_collected → analyze_collected_pagedoes a fullscraperHTML parse in the async crawl loop (generate/browser_collector.rs:465,generate/mod.rs:791-803), stalling the websocket pump while it runs. explainfolds a provider check into the ad-stack verdict; the runtime andverifydo not: with[auction].providers = []the runtime runs the stack,verifyreportsunknown, andexplainprintsno(config/ad_templates.rs:283, 307). Keep the verdict equal togate.expectedand print the provider state as its own advisory line.- The "witness rule" check is a tautology:
witnessed()cannot returnfalseonce reached — the single-page case it guards is stopped upstream bystructural_check(generate/unit_template.rs:237-263). Delete it (moving the explanation intoanalyse_slot's doc) or make it an independent check that could fail. Relatedly,crawl_planhard-codes section = path segment 0 (generate/crawl_plan.rs:289-294), so thesection_segment = 1locale inference is unreachable from a real crawl — the test feeding it is hand-built evidence the crawler cannot produce. Either make the crawler section-index-aware or document that locale-prefixed sites need explicit URLs.
♻️ refactor
AdStackGateName/blocking_gates()is dead public API: both CLI consumers read only.expectedand rebuild their gate lists by hand (commands/audit/ad_templates.rs:187,commands/config/ad_templates.rs:298) — the duplication the type was meant to eliminate still exists. Wire the CLI output throughblocking_gates(), or drop the enum/result struct and returnRuntimeAdStackExpecteddirectly (creative_opportunities.rs:910-1003).compile_page_patternleaksglob::Patterninto core's public API: no external caller uses theOkvalue (.err()and a validation panic are the only uses), and the CLI has noglobdependency to even name the type. Keep thePattern-returning function crate-private and export avalidate_page_pattern(&str) -> Result<(), String>wrapper (creative_opportunities.rs:856).should_run_server_side_ad_stackkeeps the 7-positional-bool signature the input struct exists to remove: delete the wrapper and buildAdStackGateInputwith named fields at the call site (publisher.rs:1808-1830, call at:2722); the test at:5991becomes readable in the process.verifylaunches a fresh Chrome + tokio runtime + temp profile per URL (browser.rs:192-226), discarding any bot-protection clearance between pages of the same run. Add the samecollect_pagesreuse the generate-side trait documents.generatecannot honor--chrome/$CHROME:BrowserOptsis not flattened intoAuditAdTemplatesGenerateArgs, and the crawl collector resolves the binary through a second, divergent discovery path with a different candidate list (generate/browser_collector.rs:932-961vsbrowser.rs:24-30). Collapse the two discovery paths and document--chrome/--settle-quiet-ms/--settle-max-msin the guide (none are documented today).- Exit codes conflate assertion failure with tool error:
checkfailures, missing config, and bad flags all exit 2 (config/ad_templates.rs:208-254), andverify --strictfailure is indistinguishable from "Chrome not installed" (audit/ad_templates.rs:60-64). For CI commands, reserve exit 1 for "ran and found drift" and keep 2 for tool errors; document the mapping. Thecheckfailure text also goes throughlog::error!, so it is suppressible by logger level while the exit code stays. generate's URL is validated at runtime rather than at the clap layer: every sibling URL positional usesvalue_parser = parse_http_url;GenerateArgs.urlis a bareStringchecked later with a message naming the wrong command (generate/mod.rs:79, 168-178). Also the barets auditerror string omitsgenerate(audit/mod.rs:337) —arg_required_else_helpwould keep the list from drifting again.check's required-one-of contract is hand-rolled: use a clapArgGroupfor--expected-slot/--expect-no-slots(restoring standard usage output on error), andconflicts_withfor--allow-extra-slots --expect-no-slots, which is currently accepted and silently ignored (config/ad_templates.rs:42-57, 209-235).- Post-navigation CDP calls are unbounded:
NAVIGATION_TIMEOUTcovers onlygoto/wait_for_navigation;settle()'s loop awaitsresource_countinside the bound check, and the evidence read evaluates a page-controlled function — a page that busy-loops afterloadhangs the CLI despite--settle-max-ms(browser.rs:357-451). Wrap every post-navigation evaluate intokio::time::timeout. Relatedly, the evidence cap runs after the full payload is materialized twice, and the JS caps entry counts but not string lengths — cap payload bytes before decode andString(x).slice(0, 512)at capture sites (browser.rs:409-451). --cookielands at the URL's directory path, notPath=/: setting onlyCookieParam.urlderives the default-path from the URL's directory, so a clearance cookie set via.../news/storydoes not cover/api/...or/— the session partially fails in a way that looks like a flaky challenge (browser.rs:293-299; same construction ingenerate/browser_collector.rs:519-529). Setdomain(host-only) andpath = "/"explicitly. The host-only scoping itself is correct — no cross-origin leak.- The root page is collected in a throwaway browser before the shared crawl session (
generate/mod.rs:548-570): the clearance cookie the batch design exists to keep is discarded exactly when it is needed, and--profilesdoubles the launches. Fold the root into the samecollect_pagescall per profile. media_typeis stringly compared across the compare boundary:format.media_type == "banner"(expected.rs:53-54, 84→compare.rs:232-239) — a rename silently reclassifies every format intounsupported_format→Partial→ strict failure. Hold core'sMediaTypeand render to a string only at the JSON boundary.
🌱 seedling
- The cross-origin refusal fails open: when
page.url()errors or returnsNone, both fallbacks substitute the requested URL, soorigin_changedcompares the URL to itself and off-origin evidence is accepted (browser.rs:328-331). Make the failure explicit — this is the one control between an unrelated origin's slots and a green--strictgate. - Silent degradation paths: a failed init-script build downgrades to "every slot missing" with no cause (
audit/ad_templates.rs:167); the crawl'sGPT_SLOTS_SCRIPT/LINKS_SCRIPT/SITEMAP_SCRIPTevaluate errors are swallowed (Err(_) => Vec::new()+unwrap_or_default) exactly where the challenge-rate refusal needs them (generate/browser_collector.rs:630-664— noteSITEMAP_SCRIPTis an async arrow function, so ifawait_promiseis not set the deserialize failure makes sitemap discovery permanently empty with no diagnostic);lintnever reports page patterns the runtime silently drops, the one config error class the runtime tolerates silently and the reasoncompile_page_patternwas made public (config/ad_templates.rs:109-183); and evidence is collected from the main frame only, so friendly-iframe slots reportmissingwith no indication a frame was skipped (browser.rs:409-422). - Terminal escaping gaps:
is_terminal_controlcovers C0/DEL/C1 but not Unicode bidi overrides (U+202A–U+202E, U+2066–U+2069), which can visually forge output (output.rs:50-53); and the staticconfig ad-templatescommands write config-derived strings (which can arrive via the env overlay or a pushed blob) withoutescape_terminal_textat all. - Exhaustive gate test never sweeps
Noneconsent: the 128-case sweep pinsSome(...), butNoneis what the audit path passes — the load-bearing branch for the CLI ("unknown consent and no matched slots must beNo, notUnknown") has a single test case (creative_opportunities.rs:1077-1101). Add a 64-case sweep withconsent_allows_auction: None. - Consolidated test gaps: no dry-run byte-identical file assertion; no end-to-end
--replacethroughrun_update_slots; no TOML round-trip with trailing comments or multi-line values; no non-ASCII div id anywhere; the template-ambiguity refusal (row 5 of the table) is untested;verify_round_trip'sErrbranch is unreachable by any test;page_patternstests check compilation but never that an emitted pattern matches the path it was derived from;generate/browser_collector.rsgained ~300 lines (device profiles, proxy normalization, consent stub) with zero new tests;page.rstakes the concrete collector type — bypassing the trait built for fake-collector testing — and has no tests, leavingwrite_summary's escaping uncovered; and the newrun.rsparser tests assert onlyCommand::Audit(_), withlint/explain/generate/--profilesparsing untested.
⛏ nitpick
expect()messages missing the"should ..."form:generate/mod.rs:1130, 1390, 1702,generate/validate.rs:67,generate/evidence.rs:226,audit/ad_templates.rs:464, 551, 577, 607.consent_allows_auction: Option<bool>needs three doc lines and a== Some(false)check to explain thatNone ≠ false; aConsentForAuction { Allowed, Denied, Unknown }enum would mirrorRuntimeAdStackExpected(creative_opportunities.rs:945-946).compile_patternsmatchesErr(_)and logs a generic message, discarding the specific errorcompile_page_patternnow produces (creative_opportunities.rs:555-570).crate::creative_opportunities::spelled out three times inside one 13-line function instead of a top-leveluse(publisher.rs:1817-1829).- The crate-level
cfg_attr(test, allow(...))duplicates whatclippy.tomlalready sets repo-wide for test code (trusted-server-cli/src/lib.rs:1-10). Auditmoved out of alphabetical order in theCommandenum, sots --helplistsauthbeforeaudit(run.rs:24-27).scp_shows_prebid: thetest=prebidbranch is dead (subsumed by the barecontains("prebid")), and the bare substring also matches keys likenoprebid=true(generate/gpt_slots.rs:378-381).- Settle knobs are quantized to the 250ms poll (so
--settle-quiet-ms 100behaves as 250) andquiet > maxis not rejected, making the quiet window silently unreachable (browser.rs:33,collector.rs:24-30). - The legacy dispatch arm matches
Some(_), discards the binding, thenexpect()s an impossibility — bind the URL and havelegacy_generate_argsreturnGenerateArgsdirectly (audit/mod.rs:328-338). .html/.htm/.phpare missing fromNON_PAGE_EXTENSIONS, so/index.htmlbecomes a section namedindex.html; percent-encoded paths bypass the noise filters (generate/crawl_plan.rs:50-53, 266-284).dropped_sectionsis joined into one unbounded note — a catalog sitemap can print tens of kilobytes; list the first ~10 and append "and N more" (generate/crawl_plan.rs:225-231).- Observed path segments are interpolated into globs unescaped: a segment containing
[/]/?/*changes pattern meaning, and an unbalanced[aborts the whole run viavalidate_page_patternson page-controlled data — useglob::Pattern::escapefor the literal prefix (generate/page_patterns.rs:40-43). ExtraEvidence.kinddocs promisedom/gpt/apsbut onlygptis ever emitted — unmatched DOM ids and APS calls are discarded entirely (compare.rs:186-187, 376-389; mirrored atoutput.rs:255-256). Narrow the docs or emit the other kinds.SlotResult.phasehardcodesinitial_loadfor a slot with no evidence; make itOptionand skip serializing (compare.rs:348,output.rs:197-198).- The file-scoped
dead_codeallows incompare.rs:10-13andoutput.rs:12-15are stale ("consumed in a later task" — most items are consumed now) and mask the genuinely unusedPageBidsEvidenceandApsFetchBidsEvidence.sizes; annotate the remaining items individually. --methodcomparison is case-insensitive where the runtime's is exact; parse viahttp::Method::from_bytes(config/ad_templates.rs:277).- Human
verifyoutput dropsextra_evidence,gates,matched_slot_count, andruntime_ad_stack_expected—extra_evidenceis the single most useful diagnostic line and is JSON-only today;VerificationReport.warningsis hardcoded empty (audit/ad_templates.rs:136, 387-428). - The "read-only" collector durably marks page-owned objects (
googletag.__tsWrapped, enumerable replaced methods) — a fingerprinting surface working against the--cookieclearance flow; use a closure-localWeakSetand non-enumerable defines, and soften the header claim to "observes without capturing page data" (ad_template_collector.js:68-69, 94-95).
🏕 camp site
- Two orphaned doc comments attach to the struct that follows them, leaving
splice_creative_slots— the riskiest function in the PR — undocumented, and duplicatingrun_update_slots's doc (generate/slot_toml.rs:384-391,generate/mod.rs:484-494). let _ = network_id;discards a binding that exists only as a presence check;if keys.network_id.is_none() { ... }says what it means (generate/slot_toml.rs:462-470).- The deleted
audit_does_not_accept_adapter_optionparser test guarded a claimdocs/guide/cli.md:363still makes ("It has no--adapteroption"); restore it against the new command shape. - Stale-read window: the config is read once, a multi-profile crawl can run for minutes, and the (correctly atomic) rename clobbers any concurrent operator edit; re-read and refuse if changed since the initial read (
generate/mod.rs:541-546, 678).
📌 out of scope
- Partially-invalid
page_patternsstill fail open, silently (pre-existing):validate_runtimeonly requires one surviving pattern, so["/news/*", "["]starts up fine with the bad pattern dropped behind alog::warn!most operators never see. This PR makes the fix a one-liner (validate every pattern throughcompile_page_patterninvalidate_runtime), and the asymmetry is now sharper:generaterefuses to write a pattern the runtime would drop, while the runtime accepts it (creative_opportunities.rs:453-458, 551-572).
📝 note
- The PR description says bare
ts audit <url>is "a hidden alias forts audit page <url>" — the code anddocs/guide/cli.mdagree it aliases togenerate(the behavior-preserving choice). Only the PR body needs correcting.
CI Status
- fmt: PASS
- clippy (all six target-matched lints): PASS
- rust tests (fastly / axum / cloudflare / spin / parity / CLI): PASS
- js tests + formats: PASS
- browser integration tests: PASS (but see the inline comment on
scripts/test-cli.sh— the new browser fixture step likely asserts nothing on the runner)
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review Summary
Requesting changes for three high-severity issues in the new ad-template generator: it can disclose supplied session cookies across a redirect, silently change preserved templated slots, and claim multi-device validation after one profile failed. I also found three medium-severity diagnostic/generation contract gaps.
Validated locally with ./scripts/test-cli.sh (350 standard CLI tests plus 3 Chrome-backed browser tests passed).
| &target_url, | ||
| request.cookies, | ||
| &mut |_, root| { | ||
| root_url = root.final_url().unwrap_or_else(|_| target_url.clone()); |
There was a problem hiding this comment.
P1 — Cross-origin redirects re-scope supplied cookies. The root page's final URL becomes the crawl root without an origin check. collect_site then appends the resulting planned URLs, and every subsequent URL receives the original --cookie values through set_browser_cookies. A redirect to an attacker-controlled origin therefore receives the operator's session/clearance cookie and can supply evidence used for the config rewrite. Keep the requested origin as the trust boundary: reject cross-origin final URLs before planning or folding (or add explicit opt-in), and never install supplied cookies for a derived foreign origin.
| &existing, | ||
| &slot_toml::CreativeSectionKeys { | ||
| network_id: network_id.as_deref(), | ||
| section_root: policy.as_ref().map(|policy| policy.section_root.as_str()), |
There was a problem hiding this comment.
P1 — Merge mode silently changes preserved templated slots. merge_render_slots_with_diagnostics preserves existing slots and their hand-tuned gam_unit_path, but this path overwrites the one global section_root / section_segment policy whenever this crawl infers one. A pre-existing {section} slot can therefore start rendering a different GAM unit path after an ordinary merge. In non---replace mode, reject a conflicting inferred policy (or require an explicit migration); do not overwrite policy used by preserved slots.
| return Ok(collector::ControlFlow::Stop); | ||
| } | ||
| } | ||
| Err(error) => notes.push(format!("skipped `{url}` on {profile_label}: {error}")), |
There was a problem hiding this comment.
P1 — A failed secondary device profile still permits generation. Collection errors here are only logged; no failed observation enters EvidenceTable, so guard_challenge_rate can succeed using desktop-only evidence. Consequently --profiles desktop,mobile can write a desktop-specific unit path after every mobile navigation failed, defeating the option's safety guarantee. Refuse the run when a selected profile cannot collect the required pages, or track profile coverage/errors and block writes until each selected profile is represented.
| if explicit { | ||
| validate_page_patterns(request.page_patterns)?; | ||
| } | ||
| let section_segment = policy.map_or(0, |policy| policy.section_segment); |
There was a problem hiding this comment.
P2 — Static slots get overly broad locale-root patterns. When no unit template is inferred, this defaults to segment 0 and ignores CrawlPlan.section_segment. For a crawl rooted at /en, a static slot observed only at /en/news/story is emitted with /en and /en/*, so it runs for every English section rather than only /en/news{,/*}. Pass the crawl plan's section segment as the fallback when policy is absent, and add a static locale-root regression test.
| return Ok(()); | ||
| }; | ||
|
|
||
| let matched = match_slots(&config.slot, &path); |
There was a problem hiding this comment.
P2 — explain and check can report slots that runtime will omit. These commands use raw match_slots, but the publisher filters matching slots whose dynamic GAM path exceeds the request-specific 100-byte render limit. A valid config with a long request-derived section can therefore pass CI check and show an eligible ad stack here while production injects no slots and dispatches no auction. Share the runtime's renderable-slot filter with the diagnostics, or explicitly distinguish configured matches from runtime-renderable ones.
| manual_review.push(integration); | ||
| Some(AuditSubcommand::AdTemplates(AuditAdTemplatesCommand::Generate(gen_args))) => { | ||
| gen_args.browser.validate()?; | ||
| let loaded = crate::app_config::load_file_settings(&gen_args.config)?; |
There was a problem hiding this comment.
P2 — The advertised already-invalid-config recovery path is unreachable. This fully deserializes/finalizes Settings before run_update_slots runs. Thus an invalid config exits here and can never reach validate::check_candidate's documented baseline-invalid warning path. Resolve/read the raw file before requiring a validated Settings instance (with best-effort existing creative config), while retaining candidate validation before writing.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-review of the resolution round (9841fcc9b..a6a87ce21). The round is substantial and largely successful: of the previous review's 24 blocking findings, 20 are verified fixed — several by re-running the original reproductions (u32 size bound, non-ASCII prefix panic, TOML splice now a structural toml_edit edit with comment/multiline/non-contiguous-table preservation probed byte-for-byte, dry-run reduced to a managed-fields diff with a byte-identity test and notes on stderr, refused slots omitted with reasons, tabs closed on all paths, bounded CDP operations, shared launch configuration, renderability parity with the runtime, exit-code separation, the docs section, CI Chrome wiring, and the fixture scrub). All five questions were acted on.
Requesting changes for 17 findings (inline): four carry-overs that are half-fixed or claimed-fixed-but-not, and thirteen defects introduced by the fixes themselves — including two reproduced data-loss paths in the config writer, two cookie-scoping issues, and a --strict false negative locked in by a test.
Non-blocking
🤔 thinking
verify_round_trip's mismatch arm is dead code, and its doc claims the opposite:analyse_slotalready rejects any slot whose derived section differs from the observed segment, so by the time a slot isTemplatableevery row round-trips by construction — instrumentation across the full CLI suite never hit the arm. Only the byte-limit branch is reachable, and theErr(reason)handler plus its "keeping the literal path" diagnostic are unreachable with it (generate/unit_template.rs:385-413,:187-195). Either delete the arm and correct the doc, or make the round-trip the sole authority and drop the duplicate check.- The audit gate JSON still hand-builds polarity:
to_gateshardcodesPass/Unknownper gate becauseblocking_gates()cannot express per-gateUnknown(commands/audit/ad_templates.rs:326-344). Worth extending the core API with a tri-state per-gate view so the last hand-rolled copy disappears. - In-page caps are not reconciled with the 1 MiB payload limit: 1024 slots x 1024 size pairs x 512-char strings comfortably exceeds
MAX_EVIDENCE_PAYLOAD_BYTES, and overflow discards all evidence (ad_evidence_too_large→ every slot missing). Size the caps so worst-case JSON fits, or re-collect with tighter caps instead of returning nothing (ad_template_collector.js:23-24vsbrowser.rs:45,855-863).
♻️ refactor
- Init-script build failure still silently degrades the run (carry-over, untouched):
build_ad_template_init_script(&config).ok()atcommands/audit/ad_templates.rs:192— onNonethe page is collected with no hooks and every slot reportsmissing, an exit-1 false positive under--strict. Propagate the error;VerificationReport.warnings(still hardcoded empty at:145) is the natural channel. - The APS collection path is now entirely dead but still mutates the publisher's page:
aps_slot_idsis embedded in__TS_CONFIGwith no JS reader,__ts_wrap_apstagstill redefinesapstag.fetchBidson the live page, and the harvested calls decode into a#[allow(dead_code)]type nothing outputs (ad_templates.rs:182-190,ad_template_collector.js:111-143,compare.rs:50-53). Drop the wrapper and config field, or land the consumer. - Unguarded global log-level mutation (
browser.rs:388-410): leaks theErrorlevel permanently ifblock_onpanics, silently overrides an operator's explicitRUST_LOG=debug, and likely no-ops anyway — chromiumoxide 0.9 emits viatracing, notlog. Delete it, or use an RAII guard plus atracingfilter in the logger init. ts audit generateremains the odd one out: URL still validated at runtime rather than the clap layer (generate/mod.rs:79), and neither it nor the legacy alias exposes--chrome,--headful,--browser-proxy, or--no-assume-consent(the consent default is now shared, but the opt-out is not reachable there) (audit/mod.rs:299,308).--method getparses as an HTTP extension method:Method::from_str("get")succeeds but is!= Method::GET, so a lowercase value silently reports the GET gate as blocked instead of erroring (config/ad_templates.rs:81,313). Uppercase in a value parser or use aValueEnum.- CDP pump divergences: generate's handler task
breaks on the first stream error, degrading the rest of the crawl into timeout warnings, where verify's ignores per-event errors (generate/browser_collector.rs:377-383vsbrowser.rs:444); andhandler_task.awaiton the clean-close path is unbounded (:436-439) — abort unconditionally or bound it.
🌱 seedling
- The 250-resource warning is now false on nearly every crawl: the buffer is raised to 100000 pre-navigation, yet
RESOURCE_TIMING_BUFFER_WARNINGstill fires at 250 claiming "some network assets may be missing" — untrue for any page over 250 resources, i.e. most publisher pages (generate/browser_collector.rs:36-38,1002-1005;browser.rs:615-620keeps the same magic number). Key it off the actual buffer size or drop it. NOISE_SEGMENTSandsection_atignore the section depth and disagree on percent-decoding: withsection_segment = 1,/en/aboutand/en/searchpass the noise filter (checked at segment 0 only), andsection_atreads the raw path while the filter reads the decoded one, so/%6Eewsand/newsbecome two sections (generate/crawl_plan.rs:292,304-310).- Page-forgeable warning codes share the collector's namespace: a page can push
{code:"redirected"}into__tsAdTemplateEvidence.warningsand it merges intopages[].warningsindistinguishably; escaping makes this safe for terminals but not for machine consumers keying oncode(ad_templates.rs:231). Consider apage_prefix on decode. - Terminal escaping still skips most config-derived strings in the static commands:
gam_network_id, provider names, slot ids, rendered unit paths, and joined patterns print raw inlint/match/check/explain; only the new lint pattern message escapes (config/ad_templates.rs:148,169-175,396-401,434-442,468-473). - Residual test gaps: no splice test for an operator comment between the last slot and the next section, or a trailing EOF comment (the two cases the original finding named — behavior verified correct by probe, but unpinned); no end-to-end
run_update_slotstest with--replace; noexplaintest on a blocking path, so every gate label is asserted only in itspassstate; no template-ambiguity refusal test;page.rsstill has zero tests despite now taking&dyn AuditCollector; the two audit parser tests still assert onlyCommand::Audit(_), andlint/explain/--profileshave no parser tests;commands::audit::browser::tests::as a hardcoded filter inscripts/test-cli.shexits 0 with "0 passed" if the module is ever renamed. - The design spec §8 was never updated:
output.rsclaims its types mirror the 2026-06-26 design spec, which still enumerates onlyconfirmed/partial/missing, says partial fails strict, and describes the removed APS warnings — JSON consumers written to the spec will break onunconfirmable(output.rs:3-4vs spec §5.6/§8). Update the spec or repoint the module doc.
⛏ nitpick
- Three broken intra-doc links introduced by this round, confirmed by rustdoc:
is_managed_comment_line(deleted with the line splice) inslot_toml.rs:289,merge_slots(now#[cfg(test)]) atslot_toml.rs:716, andcompile_page_pattern(import switched tovalidate_page_pattern) atgenerate/mod.rs:981. Related doc drift: core'scompile_page_patterndoc still describes the public tooling contract it no longer has (plus a dangling empty///),AdStackGateResult's doc still describes a stored list, and theunit_template.rs:9-21module doc still teaches the deleted witness rule ("Three rules..."). RenderSlot::from_evidence's doc still describes the removed write-path-less-refusal behavior and the/{network_id}/{id}fallback (slot_toml.rs:68-72); the stale self-contradicting comment atexpected.rs:258-260says the config is rejected while the test asserts the slot is filtered;gam_unit_path_unrenderableincompare.rs:270-279is now unreachable in production.explaincan never print its documentedunknownverdict (consent is alwaysSome, the arm is dead —config/ad_templates.rs:328,375vscli.md:88) and prints zero gate lines whencreative_opportunitiesis unconfigured, contradicting "print every runtime ad-stack gate" (:299-302).- A dry run with zero changes prints a single blank line —
similarrenders an empty diff as an empty string (generate/mod.rs:714-728); print an explicit "no managed changes" line. - The accepted http→https upgrade emits no note: the
redirectedwarning fires only on path inequality and normalization drops the scheme, so the upgrade the resolution design said would carry "a redirect note" is silent (ad_templates.rs:150-168,232-237). - Non-enumerable wrapping is itself a new fingerprint: redefining
defineSlot/fetchBidswithenumerable:falsechanges the descriptor (plain assignment would not), hiding the method fromObject.keys— a cheaper detection signal than the removed__tsWrappedmarker, and the test asserts it (ad_template_collector.js:90-137,collector.rs:212-216). Carry the original descriptor's enumerability. Also: out-of-u32-range sizes are reported underfluid_size_ignored/ "non-numeric", which mislabels the overflow case the bound exists for (:46,61-64). UUID_SEGMENT.or_else(HEX_HASH_SEGMENT)short-circuits instead of taking the earliest marker, unlike theREACT_USE_IDhandling one line up (generate/gpt_slots.rs:235-242).CrawlPlan::section_segmentis write-only outside its test — slot rendering takes the depth from the inferred policy instead (crawl_plan.rs:100-101,251); wire it in or drop it.- Format union keys on
(w, h, media_type), so an existing(300,250)video format re-observed as a banner gains a duplicate(300,250)entry (slot_toml.rs:212-216) — consistent with add-only, worth a note in the merge doc. similaris a new workspace dependency pulled in solely for the dry-run preview, and it is inserted out of alphabetical order in[workspace.dependencies](rootCargo.toml:94).- Implicit-header configs (slots without a
[creative_opportunities]table, or a dotted root key) get the whole creative section relocated to end-of-file by the position sort — valid and value-preserving, but undocumented in the contiguity design doc. --method's lowercase behavior aside, missing doc comments and bare asserts on several new helpers/tests (push_slot_preserving_collisions,percent_decode_for_filtering,hex_value; stale "first path segment" docs onsection_at/PlannedSection::segment; new tests atpage_patterns.rs:107,evidence.rs:373,503,gpt_slots.rs:879,899without assertion messages); the runtimecompile_patternswarn still logsErr(_)without the specific glob error the helper now produces (creative_opportunities.rs:557-564).
🏕 camp site
upsert_key_in_sectionandis_table_headerare now#[cfg(test)]-only production leftovers with a full doc and a dedicated test asserting behavior no shipped path exercises; the comment insplice_inserts_section_policy_keys_a_config_does_not_have_yetstill says "The whole point ofupsert" (slot_toml.rs:658-702,960,1042-1063). Delete them.run_checkis dead in production —run_ad_templatesinterceptsCheckbefore the writer dispatch, so the wrapper survives only for tests (config/ad_templates.rs:127,237-241).- The duplicated
run_update_slotsdoc block is still glued toUpdateSlotsRequest(generate/mod.rs:484-493), unchanged from the previous head. collect_site's browser implementation now buffers every page's(url, CollectedPage)until the browser closes, whilecollector.rs:43-46still documents the streaming contract it was written for ("drop the page's HTML immediately"); a full-budget crawl holds every DOM serialization at once. Hand the sink through towith_browser, or rewrite the trait doc to describe the buffering and why.
📝 note
- The residual TOCTOU between the pre-write re-read and
temp.persist()is microseconds and acceptable; noting so the "refuses concurrent edits" claim is read as bounded, not eliminated (generate/mod.rs:729-740). enc_prev_iusremains unread in the SRA fallback; the comma-didsguard closes the practical case, but the joinediu_partsorder assumption stands (generate/gpt_slots.rs:291-305).
CI Status
- All 19 GitHub checks: PASS on
a6a87ce21 - Local verification: CLI suites 182 + 159 + 24 passed, 0 failed; core
creative_opportunities103 passed; clippy-D warningsclean (native and wasm32-wasip1 targets);cargo build -p trusted-server-cli --target wasm32-wasip1clean; fmt clean
| slot.div_id, | ||
| reasons.join("; ") | ||
| )); | ||
| continue; |
There was a problem hiding this comment.
🔧 wrench — --replace plus an all-refused crawl silently wipes the operator's entire slot array and exits 0. Refusal now (correctly) drops the slot, but nothing downstream guards an empty merge: with --replace, merge_render_slots_with_diagnostics returns the discovered set verbatim, so an all-refused crawl yields merged == []; render_slots(&[]) emits only the comment lines, the splice serializes no slot key at all, and check_candidate passes because CreativeOpportunitiesConfig::slot is #[serde(default)] ("empty vec = feature disabled"). The run prints Wrote 0 slot(s) to ... and returns success — the operator's hand-tuned slots are gone. Reproduced; a_root_only_site_is_still_collected_on_every_device_profile already encodes exactly this shape (one slot, device-split refusal, zero slots written) with replace: false.
Fix: refuse to write (naming the refused slots) when merged.is_empty() while the pre-existing config had slots — or more conservatively whenever merged.is_empty() and the discovered set was non-empty.
|
|
||
| /// Whether `document` uses CRLF line endings (so edits preserve them). | ||
| fn uses_crlf(document: &str) -> bool { | ||
| document.contains("\r\n") |
There was a problem hiding this comment.
🔧 wrench — Content-based CRLF detection corrupts or dead-ends operator files. uses_crlf inspects the entire document text, including string values. Two reproduced cases:
- An LF-terminated file containing
other = """a\r\nb"""matches, and the blanketreplace("\r\n","\n").replace('\n',"\r\n")rewrites every line ending in the file to CRLF — a wholesale mutation of a file only the managed section was supposed to touch. - A CRLF-terminated file containing a bare
\ninside a multi-line string gets that string's value mutated to\r\n;ensure_only_managed_fields_changedcorrectly catches the value change and the command dies withrefusing to update config because fields outside the managed creative-opportunities keys would change— an error that points at nothing actionable, permanently blockinggenerateon that file.
Fix: derive CRLF-ness from line terminators only (e.g. compare the first line terminator, or count \r\n vs bare \n terminators), and apply the conversion only to the newly generated region.
| } | ||
| } | ||
|
|
||
| fn push_slot_preserving_collisions( |
There was a problem hiding this comment.
🔧 wrench — The collision-retention path breaks the dedup contract and can rewrite a good slot to a volatile id. Three problems, first two reproduced:
- The collision arm never updates
seen_divs, so after one collision every later slot with the same normalized stem is pushed unconditionally — including an exact repeat of a raw div already stored. Probe with rawsad-x-aaaaaaaaaaaaaaaa-0,ad-x-bbbbbbbbbbbbbbbb-1,ad-x-bbbbbbbbbbbbbbbb-1produced three slots: two with an identicaldiv_idplus a fabricated-2id — contradicting this function's own doc ("Slots are deduplicated by div id in first-seen order"). - The registry loop and request loop share
seen_divs: when a React/hash token changes betweendefineSlotand the ad request (a mid-page re-render), the collision branch rewrites the already-correct registry slot'sdiv_id/idto its raw volatile value and pushes a second slot. Before this change the request was simply skipped and the stable stem survived. - The retained placements carry raw per-render div ids into the generated config with no diagnostic — the resolution design promised a note; none is emitted, and prefix matching on those ids cannot match the next render.
Fix: track raw divs per stem in the collision arm (skip true repeats), only take the collision branch within the same source (registry vs request), and emit a note whenever raw volatile ids are written.
|
|
||
| /// File extensions that are assets rather than pages. | ||
| const NON_PAGE_EXTENSIONS: &[&str] = &[ | ||
| ".jpg", ".jpeg", ".png", ".gif", ".webp", ".avif", ".svg", ".ico", ".css", ".js", ".json", |
There was a problem hiding this comment.
🔧 wrench — Adding .html/.htm/.php to NON_PAGE_EXTENSIONS blinds the crawler to whole classes of publishers. same_origin_page_url rejects any candidate ending in these, so a site serving /news/story-abc.html or /section.php contributes zero section candidates: plan.sections is empty, only the root is audited, and every slot degrades to a literal path with page_patterns=["/"]. The round-1 finding was only about /index.html being mistaken for a section name.
Fix: reject only directory-index filenames (index/default/home + extension), or exclude these paths from becoming sections while still allowing them as articles.
| sitemap_locs: &[String], | ||
| budget: CrawlBudget, | ||
| ) -> CrawlPlan { | ||
| let section_segment = usize::from(segment_count(root) == 1); |
There was a problem hiding this comment.
🔧 wrench — The section-depth heuristic lacks path containment and misfires on one-segment roots. Two problems:
same_origin_page_urlenforces origin but never path containment, so with root/ena link to/fr/newsyields sectionnewsand can supply the landing/article for that candidate (record_urlis first-wins) — cross-locale pages then fold into one section's evidence table, exactly the cross-property mixing the network-id guard exists to prevent within a domain.usize::from(segment_count(root) == 1)fires on any one-segment root: an operator pointing the crawl athttps://publisher.example/news(a section) makes every article slug a "section" and samplesmax_sectionsarbitrary articles; conversely a two-segment root like/en/newsyields depth 0 and the locale becomes the section.
Fix: require candidates to share the root's path prefix when section_segment > 0, and gate depth-1 inference on evidence (locale-shaped first segment, or a root redirect) rather than segment count alone.
| #[arg(long, default_value_t = 750)] | ||
| pub page_delay_ms: u64, | ||
| #[command(flatten)] | ||
| pub browser: BrowserOpts, |
There was a problem hiding this comment.
🔧 wrench — --browser-profile is accepted and silently ignored by ts audit ad-templates generate. Flattening BrowserOpts here drags the flag into --help right next to --profiles, but with_browser_options copies chrome/headful/consent/proxy/certs/settle and never reads options.profile — the device profile comes exclusively from --profiles. So generate --browser-profile mobile runs a desktop crawl with no warning, while docs/guide/cli.md (added this round) states generation uses --profiles and verification uses --browser-profile. Verified live in --help output.
Fix: split profile out of the flattened struct for this subcommand, or reject the combination in validation with a message pointing at --profiles.
| callback({ version: 1, uspString: "1---" }, true) | ||
| } | ||
|
|
||
| const pin = (name, value) => { |
There was a problem hiding this comment.
🔧 wrench — The non-configurable pin can break the publisher's real CMP — and this round turned the stub on by default in verify and page. Object.defineProperty(window, name, {writable:false, configurable:false}) at document start means a CMP that later does window.__tcfapi = ... from a strict-mode/module bundle — which is how virtually every CMP installs itself — throws TypeError, aborting the CMP script and potentially the ad stack behind it. In verify, a CMP the stub broke and a page with genuinely no ad stack produce the same all-missing verdict — the command's one job is telling those apart.
Fix: pin with a getter/setter where the setter is a silent no-op (observably "stubbed" without throwing), or default assume_consent off for verify, or at minimum attach a warning to the page result whenever the stub is active so a broken CMP is diagnosable. Also worth restoring the rationale comment lost when this moved out of Rust (the surviving catch comment describes the opposite of the original intent), and accepting the TCF v2 4th argument.
|
|
||
| /// Skips optional local runs, but makes the scripted/CI contract fail loudly. | ||
| fn browser_fixture_available() -> bool { | ||
| if find_chrome().is_ok() { |
There was a problem hiding this comment.
🔧 wrench — The fixture gate checks find_chrome(), ignoring CHROME — the exact variable CI wires. .github/workflows/test.yml exports CHROME from setup-chrome, and the collectors resolve through resolve_chrome, which honors it; this gate searches only PATH names and well-known install paths. If the action exposes Chrome only via its output path (not on PATH), CI fails with "set CHROME to its executable" — advice that cannot fix the failure, because this function never reads it. Conversely a bogus CHROME makes the gate and the launcher disagree. Secondary: scripts/test-cli.sh filters on commands::audit::browser::tests::, so a rename of those tests would make the --ignored run exit 0 with "0 passed".
Fix: gate on resolve_chrome(None).is_ok() so the gate and the launcher share one resolution path.
| | --------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Only one page was crawled | Literal path. One observation cannot distinguish a literal from a template. | | ||
| | The ad unit never varied by section | Literal path. | | ||
| | A section's slug is not derivable from its URL (`/site-news` requesting `.../sitenews`) | Literal path; the round-trip check catches it. | |
There was a problem hiding this comment.
🔧 wrench — The refusal table still contradicts the code (carry-over; the resolution design said docs and tests would "consistently describe these cases as refusal"). This row and the next ("No root page was seen → Literal path rather than a guessed fallback") both describe outcomes that literal_decision refuses whenever the slot observed more than one unit path — which both situations guarantee — and refusal now omits the slot entirely. The tests still assert Refuse under "literal" names (a_slug_the_path_cannot_reproduce_stays_literal, an_unwitnessed_root_does_not_template), the test comment still credits round-trip verification for a rejection analyse_slot makes first, and the operator-facing diagnostics at unit_template.rs:149,160,190 say "keeping the literal path" in exactly the cases that omit the slot.
Fix: correct both table rows to "slot omitted, with the reason reported", rename the two tests, and reword the diagnostics to match what is written to the file.
|
|
||
| - [ ] **Step 2: Replace sensitive-looking fixtures and stale assertions** | ||
|
|
||
| Replace `88059007`, `autoblog`, `car-research`/`carresearch`, and distinctive div tokens introduced by this PR with `123456789`, `publisher`, `/site-news`/`sitenews`, and neutral `ex_...` values. Update comments to describe shapes rather than customers. |
There was a problem hiding this comment.
🔧 wrench — This line re-commits the real customer tokens the scrub removed. The code/doc scrub landed everywhere else (verified: no other hits in this PR's files), but the plan text spells out the real GAM network id and customer name verbatim — permanently, in a tracked file — against CLAUDE.md's "do not write or commit real domains, customer names, ... in comments, tests, docs, or examples".
Fix: reword to describe the substitution without repeating the originals, e.g. "Replace the customer GAM network id, site name, section slugs, and distinctive div tokens introduced by this PR with 123456789, publisher, ...".
4 participants
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)