Conversation
jevansnyc
left a comment
jevansnyc
left a comment
There was a problem hiding this comment.
Review of the design spec. No code changes here, so this is internal consistency plus whether the stated contracts hold against what is in the repo today (gpt_diagnostics.rs, publisher.rs, auth.rs, prebid_eids.rs, and the TS diagnostics store/types).
Seven issues inline. The first three change the design rather than the wording:
- The
ts-auc-token shape does not match the existing producer, so correlation never joins. TraceGptDiagnosticsV1as specified cannot satisfy acceptance criterion 8.- Trace paths terminate ahead of authentication, which carves an exemption out of the
^/_tsnamespace thatauth.rssays should not exist.
The remaining four are bounded-scope corrections to the cookie lifetime claim, the TSJS gate, a capture_status gap, and redaction consistency.
Mechanical checks came back clean: cookie names (ts-ec, ts-eids, ts-tester, __Host-ts-console), the 8 KiB ts-eids cap (MAX_EIDS_COOKIE_BYTES), and the callback-issue reason values all match what the spec assumes.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
A design-only PR adding a single 1852-line spec for a /_ts/trace mobile
diagnostics endpoint. The document is unusually well-grounded: field names,
constants, and several non-obvious hazards (the AuctionRequest.id leak, the
JA4/H2 fingerprint exclusion, bootstrap-fallback argument tolerance, the
unpopulated asn) are verbatim correct against the code. All seven findings
from the previous round are genuinely resolved in e3f371f8c, and I re-verified
each rather than re-raising it.
The blocking findings below are places where the spec mandates behavior the
pinned platform cannot express, or where it assumes adapter defaults that do the
opposite of what sections 8, 12.3, and 13 require. Because this is a design
document, each one is cheaper to fix now than after it becomes four
implementation PRs.
6 of the inline comments below carry a one-click GitHub
suggestion—
use Commit suggestion (or Add suggestion to batch) to apply them. The
remaining comments describe the change in prose because the fix is a new
validation hook or spans more than one contiguous range.
Blocking
🔧 wrench
- Two-second request-body deadline is unimplementable — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:494 HEAD /_ts/traceis proxied to the publisher origin — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:468- Router-level 405 carries no
Allowand no hardening headers — see
inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:506 - The mandated configuration validation cannot fire — see
Cross-cutting below
Non-blocking
🤔 thinking / 📝 note
AuctionSlot.extpresented as an existing type — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:908- Slot tokens described as existing; verbatim-comparison rule conflicts with
normalizedAuctionId— see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:819 - Inherited and new cookie caps are presented as one list — see inline
at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:654 - CSP blocks the favicon, and leaves
blob:and inline styles unaddressed
— see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1326 - Auth contract overstates rule composition; existing JA4 route is an
auth-bypass precedent — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1364 - Container-nesting cap has zero headroom — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1092 sourceenum diverges from the existingAuctionSource— see inline
at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:841- Deprecated
/__ts/page-bidsalias is uncovered — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:996 - GPT projection prose is not a usable allowlist — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:715
Cross-cutting / body-level findings
-
🔧 The configuration validation this spec mandates cannot fire as a
validatorrule. Section 8 (line 452) requires that
“trace_page_enabled = truerequiresenabled = true; invalid
combinations fail configuration validation”, and section 14.1 (line 1433)
requires a test that configuration “rejects trace-page enablement without
GPT diagnostics”.One correction first, in fairness to the design: adding
trace_page_enabled
to TOML today is already a hard error on both the enabled and disabled
paths, becausevalidate_disabled_schema
(crates/trusted-server-core/src/settings.rs:247-253) forgives only errors
beginning"missing field ", and an unknown-field error is not forgiven.
That part is correctly fail-closed.The real gap is narrower but still real. Once
trace_page_enabledis a
legitimate field,IntegrationSettings::get_typedreturns before validation:// crates/trusted-server-core/src/settings.rs:348-350 if !config.is_enabled() { return Ok(None); } config.validate().map_err(|err| { /* :352 */ })?;
So the exact combination the spec wants rejected —
enabledomitted (serde
defaultfalse) together withtrace_page_enabled = true— resolves to
Ok(None)and a#[validate(schema(...))]rule never runs. Note
#[validate(schema(...))]does otherwise work in this crate
(settings.rs:2737), so the failure mode is silent rather than obvious.The precedent that fits is
validate_js_asset_proxy_config
(crates/trusted-server-core/src/config.rs:280-299): it reads the raw JSON
and callsvalidate()outside the enabled gate, runs from both the deploy
(config.rs:250) and runtime (config.rs:271) paths, and is proven by
validate_rejects_invalid_disabled_js_asset_proxy_assets
(config.rs:1424). Naming that pattern here would keep an implementer from
writing a rule that never fires.On sourcing: the “invalid enabled config must not be silently
logged-and-disabled” rule is not actually inAGENTS.mdor
CONTRIBUTING.md. Its canonical statement is a HIGH-severity finding in
docs/superpowers/specs/2026-03-11-production-readiness-report-design.md:217-233.
If this spec relies on it as normative, that is worth making explicit, since
it was never promoted into the contributor docs. -
📝 Section 12.4's cache invariant is already implemented, in two
places on Fastly. The spec asks that “tests must prove that late
response-header handlers cannot make traced content publicly cacheable”
(line 1355). That guarantee exists today:
apply_response_headers_with_cache_privacy
(crates/trusted-server-core/src/response_privacy.rs:163-172) skips operator
response_headersentries forCache-Controland edge-cache header names
whenever the response is already uncacheable. On Fastly there is a second
layer after it —apply_terminal_response_effects
(crates/trusted-server-adapter-fastly/src/main.rs:371-392) re-runs the
privacy guards, because EC finalize and filter effects can addSet-Cookie
later; the regression test is
late_filter_effects_cannot_make_an_assembled_response_public. Citing both
would let the implementation plan reuse the mechanism instead of rebuilding
it. Note also thatapply_finalize_headersis terminal on Axum, Cloudflare,
and Spin but not on Fastly. -
📝 A reserved-namespace classifier already exists, at the fallback
boundary. Section 8 requires rejecting trailing slashes, extra segments,
repeated separators, encoded separators, and ambiguous dot segments beneath
the reserved namespace (lines 509-514).deny_admin_diagnostic_fallback
(crates/trusted-server-core/src/ec/admin.rs:179-196) already solves that
shape for/_ts/admin, including a bounded percent-decode-to-fixed-point
withMAX_PERCENT_DECODE_ROUNDS = 4. It runs first inside each adapter's
fallback rather than at the front door, but it is the pattern to lift
forward rather than re-derive. -
📝 Adapter-parity caveat for section 8. Several existing
/_ts/*
routes (/_ts/api/v1/*,/_ts/set-tester,/_ts/clear-tester,
/_ts/debug/ja4) are registered only on Fastly. Section 8 requires the trace
namespace on all four adapters, so trace routing cannot follow that
precedent — worth stating, since it affects the sequencing in section 17.
CI Status
All checks passing on e3f371f8c.
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CLAUDE.md symlink guard: PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Second review pass, against head 7a82a944a (base main). The previous pass
raised twelve findings; 14c274cba addresses all twelve, and I verified each
fix against the code at this head rather than taking the replies at face value.
Two are worth calling out as substantively verified rather than merely
annotated. The new section 9.3 allowlist table is genuinely exhaustive: diffing
it mechanically against GptDiagnosticsRequestCycle gives 30 real members, 28
allowlisted, and exactly adManager and previousCreativeId excluded, with no
listed name that does not exist on the real type; the coverage keys, counters,
metadata, binding, and durations rows all match their interfaces exactly.
And the container-nesting cap moved 8 to 10, which restores two levels of
headroom over the deepest legitimate path (gpt_diagnostics.slots[].requests[].requestedSizes[][w], level 8).
One new blocking finding, introduced by the body-handling rewrite itself: the
replacement mechanism cannot produce the 413 the same bullet mandates, cites
a precedent that uses a different API, and misreads an empty body on the one
adapter that streams it. Details inline.
1 of the inline comments below carries a one-click GitHub
suggestion.
The other two are prose: one names a trigger condition, one corrects an
explanation.
Blocking
🔧 wrench
- Body-emptiness mechanism cannot return
413and misreads streamed bodies
— see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:516
Non-blocking
📝 note / ⛏ nitpick
- Blob revoke trigger is undefined — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1432 - Favicon suppression is attributed to CSP rather than the
<link>element
— see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1428
Cross-cutting / body-level findings
-
📝 Prior-round findings verified as resolved. Recorded so the next
pass does not re-litigate them: the unimplementable two-second body deadline
and one-byte read are gone; configuration validation now specifies a
raw-config hook modelled onvalidate_js_asset_proxy_config
(crates/trusted-server-core/src/config.rs:280-299) and correctly explains
why a schema validator never fires for a disabled integration;HEADnow
requires explicit registration on all four adapters, citing
dispatch_head_on_named_get_route_falls_through_to_publisher_fallback; the
405 contract now requires the trace responder to supplyAllowand the
section 12.3 hardening itself rather than inheriting a router error; the CSP
gainedimg-src data:and a class-toggle-only styling rule; the auth section
now states first-match-wins and that the fail-closed backstop covers only
/_ts/admin; the deprecated/__ts/page-bidsalias is covered, and the TSJS
retry it refers to is real (crates/trusted-server-js/lib/src/integrations/gpt/index.ts:1675-1692);
and the slot-token, cookie-cap,AuctionSlot.ext, andsource-enum
paragraphs now distinguish new design from existing code. I re-verified the
underlying code claims at this head. -
📝 One claim in the rewrite is accurate and load-bearing enough to
keep. The statement that Spin buffers the body while Axum buffers only JSON
checks out against the pinned dependency: Spin reads the full body into
Body::Onceunconditionally (edgezero-adapter-spin/src/request.rs:74-80),
and Axum branches on content type
(edgezero-adapter-axum/src/request.rs:21-35). That asymmetry is what drives
the blocking finding above, so it is worth keeping the sentence even after
the mechanism changes.
CI Status
No failing checks. Several are still running against the merge commit pushed
shortly before this review; they are recorded as pending rather than treated as
findings.
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CLAUDE.md symlink guard: PASS
- Analyze (actions): PASS
- Analyze (rust): PENDING
- Analyze (javascript-typescript): PENDING
- integration tests: PENDING
- browser integration tests: PENDING
- CodeQL: SKIPPED
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Third review pass, against head c4d664aab (base main). Commit 155d3b09f
resolves all three findings from the previous round, and I verified each fix
against the pinned edgezero v0.0.8 source rather than against the replies.
The body-validation rewrite is the substantive one. It now matches both Body
variants explicitly, requires a clean EOF to prove a streamed body empty, and
names both traps from the last round — telling implementers not to copy
into_bytes().unwrap_or_default() and not to propagate
into_bytes_bounded errors as the response. I checked that this is actually
implementable: Body is a public, non-#[non_exhaustive] enum
(edgezero-core/src/body.rs:14-17), core already matches both variants in the
exact prescribed shape (crates/trusted-server-core/src/publisher.rs:201-211),
and async handlers are available on all four adapters, with Fastly driving them
under block_on. The favicon causality and the blob-revoke timing are both
correct now.
Two findings remain. Neither is a defect in the new text; both are places where
the spec is not yet self-contained. Since this document is the contract four
implementation PRs will be built from, closing them here is cheaper than
discovering them during implementation.
Both inline comments carry a one-click GitHub
suggestion.
Blocking
🔧 wrench
- Section 13 omits every body-validation status code — see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:1516
Non-blocking
📌 out of scope
- Deferred-cleanup rule diverges from the console export that ships today
— see inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:253
Cross-cutting / body-level findings
-
📝 Prior-round findings verified as resolved, recorded so a fourth
pass does not re-litigate them. The body-validation mechanism no longer
depends on an API that cannot produce the mandated413, and it no longer
inherits theinto_bytes().unwrap_or_default()behavior that would read a
non-empty streamed body as empty — which matters, because a bodiless
fetch()POST carries noContent-Typeand Axum streams exactly that case
(edgezero-adapter-axum/src/request.rs:24-35, pinned by its own tests at
:97-118and:167-181). Favicon suppression is now attributed to
<link rel="icon">withimg-src data:only permitting the load. Blob
cleanup is now a 1000 mssetTimeoutwith per-download scheduling, and the
accompanying test lines are implementable:vi.useFakeTimersis already used
across nine JS test files, andcreateObjectURL/revokeObjectURLare
already mocked atapi.test.ts:540-551. -
📝 One concern investigated and dismissed, noted so it does not
resurface as a finding later. The 1000 ms revoke timer raises the question of
what happens if the document is torn down before it fires. No specified flow
navigates during a pending download: the section 6.2 storage-failure download
happens on the publisher page, which that section says "remains in place",
and the section 6.3 download happens on/_ts/trace, whose own
Copy/Share/Download and clear actions do not navigate. Line 1729's "same-tab
navigation occurs only after a successful write" governs the earlier handoff,
before any download exists. If a document were torn down anyway, the object
URL is reclaimed with it. No change needed.
CI Status
All 20 checks passing on c4d664aab.
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- CLAUDE.md symlink guard: PASS
| - Disabled route after authentication: local privacy-safe `404`. | ||
| - Unsupported method: local `405`; never publisher fallback. | ||
| - Rejected activation/end POST: local `403` with no state mutation. |
There was a problem hiding this comment.
🔧 wrench — Section 13 is missing every body-validation status code, and this round widened the gap.
This section is the spec's failure catalogue: it names a status for each trace failure mode, and every other status in section 8 has a counterpart here — 401 (line 1512), 404 (1514), 405 (1515), 403 (1516). Three are absent:
413for a non-empty enable/end body, or a positive/invalidContent-Length, orTransfer-Encoding(section 8, lines 503-505). This one was already missing before this round.400for a stream read error on those same paths (line 509) — added this round.400for an encoded separator or ambiguous dot segment beneath the reserved namespace (line 554).
Line 1516 covers only the Origin/Sec-Fetch-Site rejection, so it does not stand in for any of these. The behavior is fully specified in section 8 and routed through the section 12.3 hardening by line 518, so this is a completeness problem rather than a behavioral contradiction — but section 13 is where an implementer looks to answer “what do we return when this fails”, and three answers are not there.
The suggestion adds the two missing bullets and keeps the existing ordering.
| - Disabled route after authentication: local privacy-safe `404`. | |
| - Unsupported method: local `405`; never publisher fallback. | |
| - Rejected activation/end POST: local `403` with no state mutation. | |
| - Disabled route after authentication: local privacy-safe `404`. | |
| - Unsupported method: local `405`; never publisher fallback. | |
| - Reserved-namespace path that is a trailing slash, extra segment, unsupported | |
| asset name, repeated separator, or lookalike: local `404`; an encoded | |
| separator or ambiguous dot segment: local `400`. Never publisher fallback. | |
| - Non-empty or unreadable activation/end body: local `413` for any body bytes, | |
| positive/invalid `Content-Length`, or `Transfer-Encoding`, and local `400` | |
| for a stream read error, both with no cookie mutation. | |
| - Rejected activation/end POST: local `403` with no state mutation. |
| Version one also consumes `GptDiagnosticsExportV1` through TS Console's public | ||
| export contract and projects it into a distinct redacted | ||
| `TraceGptDiagnosticsV1`. It does not read TS Console internals or create a | ||
| second GPT attribution engine. The existing recorder emits an exact-token | ||
| `TraceSlotCorrelationV1` sidecar when it binds an opportunity to a request | ||
| cycle. The server-auction, correlation, and GPT projections are separate sibling | ||
| contracts in `TraceReportV1`; none is treated as a substitute for another. |
There was a problem hiding this comment.
📌 out of scope — The new deferred-cleanup rule silently diverges from a download path that ships today.
Section 12.3 now requires URL.revokeObjectURL on a 1000 ms setTimeout and explicitly forbids revoking synchronously after click(). TS Console's existing export does exactly what that rule forbids:
// crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/api.ts:254-259
try {
anchor.click();
} finally {
anchor.remove();
this.window.URL.revokeObjectURL(objectUrl);
}That path is live, not dead code — it is wired to the console's export() action at api.ts:148, and api.test.ts:566 currently pins the synchronous behavior. So once this design ships, the product has two download implementations with opposite cleanup rules, and the older one keeps the truncation risk the trace viewer was just corrected to avoid.
Fixing it is not this issue's job — section 5.5 already puts TS Console's own surface under #1081. The ask is only that the divergence be named here, so it reads as a decision rather than an oversight, and so whoever picks up #1081 knows the corrected pattern exists. A follow-up issue against the console export would close it.
| Version one also consumes `GptDiagnosticsExportV1` through TS Console's public | |
| export contract and projects it into a distinct redacted | |
| `TraceGptDiagnosticsV1`. It does not read TS Console internals or create a | |
| second GPT attribution engine. The existing recorder emits an exact-token | |
| `TraceSlotCorrelationV1` sidecar when it binds an opportunity to a request | |
| cycle. The server-auction, correlation, and GPT projections are separate sibling | |
| contracts in `TraceReportV1`; none is treated as a substitute for another. | |
| Version one also consumes `GptDiagnosticsExportV1` through TS Console's public | |
| export contract and projects it into a distinct redacted | |
| `TraceGptDiagnosticsV1`. It does not read TS Console internals or create a | |
| second GPT attribution engine. The existing recorder emits an exact-token | |
| `TraceSlotCorrelationV1` sidecar when it binds an opportunity to a request | |
| cycle. The server-auction, correlation, and GPT projections are separate sibling | |
| contracts in `TraceReportV1`; none is treated as a substitute for another. | |
| The deferred object-URL cleanup in section 12.3 deliberately diverges from TS | |
| Console's existing export, which revokes synchronously in a `finally` block | |
| immediately after `anchor.click()` | |
| (`crates/trusted-server-js/lib/src/integrations/gpt_diagnostics/api.ts`, | |
| reachable through the console's `export()` action). That older path keeps the | |
| truncation risk this design avoids. Version one does not change it; correcting | |
| it belongs to TS Console under #1081, and until then the two download paths | |
| intentionally differ. |
This branch has not been deployed
3 participants
Summary
Changes
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.mdCloses
Closes #1108
Implementation remains tracked by #1050. Related observability and timing follow-ups remain tracked by #1081 and #1076.
Test plan
cargo test-fastly && cargo test-axum— not run; documentation-only changecargo clippy-fastly && cargo clippy-axum— not required for the documentation-only changecargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run— 45 files and 893 tests passed under pinned Node 24.12.0cd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1— not run; documentation-only changefastly compute serve— not applicablegit diff --check main...HEADpassedChecklist
unwrap()in production code — no production code changedtracingmacros (notprintln!) — no logging code changed