Repository navigation
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
Brings #1107 to b4a20f4, its first revision in this RC that carries the trace implementation rather than only the spec. Integration decisions: - EdgeZero moves to 78439fd4 on spec/pre-dispatch-request-fidelity. It contains v0.0.9 (#1175) and the pre-dispatch ingress hooks #1107 and #1251 pin at 965d482f; no released tag carries both. - scheduleInitialAdInit takes (bids, slots, auctionDiagnostics, traceAuctionTransport). One builder emits both payloads, so the RC's diagnostics-only scripts are unchanged and trace moves to the fourth argument. recordTrustedServerOpportunity likewise takes auctionFacts then traceIdentity; #1107's positional tests and the bootstrap fallback were shifted to match. - The Fastly trace terminal path sends through the timed delivery function without Server-Timing, sandbox counters or access telemetry. - build.rs embeds trace assets from #1214's private bundle directory. The viewer rebuilds byte-identical; the pinned source digest changes only because build-all.mjs gained --out-dir. - The trace projection does not copy the RC's auction evidence fields, so the classification test marks them excluded. - The retained diagnostics toolbar resyncs its filter select after selectRequest. - Shim size guard raised to 55.5 KB for diagnostics plus trace.
EdgeZero marks each request from its own listener so it can record the request's inbound origin, and #1107's trace actions require that origin. The marker is crate-private, so the RC's custom timing server produced no origin and every trace action returned 403. Restore main's entry point. The Axum dev server loses the request-timing Server-Timing header until EdgeZero offers a seam to wrap its service.
#1300 replaces #1052, reverted from this RC in the previous commit. Integration decisions: - gpt_diagnostics/store.ts keeps the trace imports beside #1300's creative failure list import. - #1300 adds eight aps_* creative failure reasons that #1107's trace projection does not accept, so any cycle carrying one failed trace capture. The projection now validates against the full diagnostics list and drops the APS reasons; unknown values are still rejected. The viewer bundle is unchanged; the pinned trace source digest moves.
Brings in the squash-merged #1213 (9cf3055), replacing the RC's earlier #1213 branch revision. Conflict resolutions: - #1213's own code (middleware Fastly version header, core build.rs, design spec) takes main's final form. - CHANGELOG keeps #1107's entries beside main's. - Fastly main.rs keeps the RC's timing, telemetry and trace imports. - getting-started keeps #1175's managed ts deploy command and the build-input wording for TRUSTED_SERVER__GIT_VERSION.
Brings #1226 at 5d423a2. Integration decisions: - AGENTS.md lists #1226's wasm clock lint as CI gate 12, after #1254's lint-domains (10) and #1225's documentation proposal scripts (11). - The integration fixture builder keeps #1226's table-customizing app_config_envelope and gains #1107's trace flag as a parameter; integration_app_config_envelope_with_trace and #1226's auction fixture both build on it. - The Cloudflare environment keeps #1226's spawn_with_config_json and shared wrangler launcher, and threads #1107's readiness probe through it, so spawn_with_readiness and the default spawn path still use their own readiness checks. - scripts/lint-wasm-clock.sh passes across the RC's Cloudflare build.
Brings #1260 at 16b6d09 (draft). Integration decisions: - Fastly delivery uses #1260's send_identity_response, extended with the RC's SendContext and post-send steps: identity effects apply after send, pull sync runs only when capture storage is available, and access telemetry always reports the delivered response. - require_identity_graph stays untimed; its doc now names identity capture as a second post-send caller. - The /auction response keeps #1107's trace cache privacy and drops the x-ts-eids headers, as #1260 does. - The integration proxy route reads #1260's integration id from the RC's three-field route tuple. - #1260 removes server-side ts-eids cookie parsing. #1107's trace cookie health now checks the ts-eids format with a local validator that accepts the same shapes the removed parser did. - ec_finalize_kv_lands_before_freeze drives its KV round trip with a SharedID cookie instead of the removed ts-eids ingestion.
Brings in the squash-merged #1175 (a0b00c7), #1239 (d2f0d8c) and #1226 (3e85161), replacing the RC's earlier branch revisions. Conflict resolutions: - EdgeZero stays pinned to 78439fd4. Main pins v0.0.9, which 78439fd4 contains; #1107 and #1251 still need its pre-dispatch hooks, and no released tag carries them yet. The lockfile needed no changes. - Fastly main.rs keeps #1107's ingress-capturing signatures, which already carry #1175's logical store selection. - proxy.rs takes #1239's final redirect-limit passthrough test. - AGENTS.md keeps the RC's gate list, which already includes main's wasm clock lint. - getting-started takes main's final #1175/#1213 deploy wording. - Integration-test config.rs and cloudflare.rs keep the RC's trace-aware envelope and readiness-probe helpers, which carry #1226's spawn_with_config_json.
Brings #1260 to 77dc8c7, which restores browser EIDs and the expired capture fallback. Conflict resolutions: - formats.rs keeps #1260's restored x-ts-eids response header and then applies #1107's trace cache privacy last. - publisher.rs keeps both the chrono and cookie imports. - The Prebid shim guard stays at 55.5 KB; the shim measures 54,603 characters with diagnostics, trace and the EID cookie writer.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Seventh review pass, against head ba06c377c (base main). The head has not
moved since the previous pass, but the situation around it has: main advanced
to a9037770a and the branch went from BEHIND to CONFLICTING.
Five of the six findings from the previous round are resolved, and I verified
each against the code rather than the replies. The build.rs conflict is gone;
a build:trace-assets script now exists; the digest input set was reworked
through a declarative trace-build-inputs.json plus a dedicated
build-trace.mjs, which removes the trace assets' dependency on
build-all.mjs and build-module-options.mjs entirely and so closes both the
over-broad and under-broad halves at the root; HEAD /_ts/trace now runs the
same projection as GET and shares its unavailable_response() path; and
json.ts now passes maximumArrayItems. I also checked the EdgeZero bump in
d821fb319 and confirmed it is a clean fast-forward with no history rewrite.
The new blocking finding is of a different kind from previous rounds, and worth
reading before the inline detail: this PR currently cannot merge for a reason
the author cannot fix in-branch. main migrated EdgeZero to the IABTechLab
org at tag v0.0.9, while this branch pins a stackpop revision that is seven
commits ahead of that tag. The pre-dispatch hook API the whole trace
interception is built on does not exist in v0.0.9, nor in the newer
v0.0.10, so the conflict cannot be resolved by taking either side. It needs
an upstream release in the new org first. Details inline on Cargo.toml.
The second blocking finding is a reproducible test failure whose documented fix
does not work. The third is carried forward unchanged.
Blocking
🔧 wrench
- EdgeZero pin conflicts with
main's org migration, and no published tag
contains the required API — see inline atCargo.toml:59 trace-assets.test.mjsfails with a rawENOENTon a clean checkout; the
documented recovery script does not fix it — see inline at
crates/trusted-server-js/lib/test/trace-assets.test.mjs:27
Non-blocking
🤔 thinking
trusted_client_ipignored on Axum, Cloudflare and Spin — see
inline atcrates/trusted-server-adapter-axum/src/app.rs:739
Cross-cutting / body-level findings
-
📝 The CI signal is stale, for the second round running. The 23
passing checks ran beforemainadvanced toa9037770a, so they do not show
that the merged result builds. That caveat matters more than usual here,
because the conflicting files areCargo.tomlandCargo.lock: the merged
dependency graph is precisely what has not been exercised. The re-run after
the upstream release and rebase is the one that carries information. -
📝 Findings verified as resolved, recorded so an eighth pass does
not re-litigate them.build.rsno longer conflicts (git merge-tree
againsta9037770areports conflicts only inCargo.tomlandCargo.lock).
build:trace-assetsis present inpackage.json. The digest now hashes a
declared input set, the pinned Node version from.tool-versions, and a
resolvedvitedependency subgraph rather than all ofpackage-lock.json;
trace-build-inputs.jsonlistsbuild-trace.mjs,
trace-asset-sources.mjs,tsconfig.jsonandsrc/trace, and neither
build-all.mjsnorbuild-module-options.mjsis an input any more, which is
a cleaner fix than the one suggested last round.trace-build-inputs.test.mjs
adds a determinism regression. TheHEADarm attrace/dispatch.rs:164-184
computes the bounded projection and falls through to the same error path as
GET, so the two can no longer disagree on status.
CI Status
All 23 checks passing on ba06c377c, with the staleness caveat above.
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- mobile trace runtime tests: 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.
The October RC exposed a trace-runtime CI compatibility issue when this PR is combined with the toolchain update in #1123. Please fix the installer in this owning PR so the RC can consume the upstream correction.
The failing RC check is trace runtime and browser acceptance. Cloudflare and Fastly boundary probes pass; Spin times out during startup, before browser acceptance runs.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Eighth review pass, against head ad1b8ef0b (base main). The branch is fully
current with main (the merge-base is origin/main itself), mergeable, and CI
is 24/24.
What this round verified as resolved: trace-assets.test.mjs no longer crashes
on a clean checkout, which I reproduced from a fresh npm ci with no dist/
(the comparison skips locally and runs in CI, and
trace-build-inputs.test.mjs independently regenerates the assets). The merge
resolutions in 944c3eb52 and ffb40704c are clean under --remerge-diff:
settings.rs takes main's #1326 fix wholesale, hex::encode is
output-identical to the previous {:x}, and both per-asset digests are
unchanged, so the frozen v1 bytes hold. The Spin installer from ad1b8ef0b
resolves the finding in review 5606395474: it reads the version from
.tool-versions, fails closed on an unknown pin, and its SHA-256 matches both
the release asset and upstream's published checksums-v4.1.0.txt.
I also re-proved the router invariants at the new EdgeZero rev. The pre-dispatch
hook still runs before find_route and before all middleware, for every method,
and a hook response still short-circuits. The router.rs diff from 965d482f
to 70876c92 is cosmetic, and all four adapters still populate
RequestIngress.
The headline finding is the EdgeZero dependency. It is not fixable in-branch:
the pinned rev sits on an open EdgeZero PR whose head has already changed the
API this PR uses. Details inline on Cargo.toml.
1 of the 7 inline comments carries a one-click
suggestion.
Blocking
🔧 wrench
- EdgeZero pin targets an open PR whose head is already incompatible —
see inline atCargo.toml:64 trusted_client_ipbehaves three different ways across one feature —
see inline atcrates/trusted-server-core/src/trace/dispatch.rs:137- Spec no longer describes asset regeneration; geo and plan drift — see
inline at
docs/superpowers/specs/2026-09-01-mobile-ad-render-trace-endpoint-design.md:570
Non-blocking
🤔 thinking / 🌱 seedling / 📝 note / ⛏ nitpick
- Source digest is over-broad and under-broad — see inline at
crates/trusted-server-js/lib/trace-asset-sources.mjs:32 - No tooling behind “add a version after publication” — see
inline atcrates/trusted-server-js/build.rs:326 - Cloudflare serves path aliases Fastly rejects — see inline at
crates/trusted-server-core/src/trace/routes.rs:207 - Digest assertion does not name the recovery command — see inline at
crates/trusted-server-js/lib/test/trace-assets.test.mjs:21
Cross-cutting / body-level findings
- 📝 Surfaces checked this round and clean, so a ninth pass does not
re-litigate them. The Rust and JS source digests agree: same inputs, byte-order
sort, NUL separators, nestednode_modulesresolution, and both compute
77662b63…against the committed manifest. The digest is
host-independent, because it reads only the lockfile, which lists every
platform optional dependency.authenticated_client_ipmatches Fastly's
resolve_client_ipon comma lists,:port, brackets, zone IDs and
whitespace, all of which fall back to the runtime address. On Fastly it
derives the same IP as the native path, so geo is kept. No Python files or
invocations were added.
CI Status
All 24 checks passing on ad1b8ef0b.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Ninth review pass, against head 20b057537 (base main). The branch is current
with main, mergeable, and CI is 24/24.
Commit 06c86df8f resolves six of the seven findings from the previous round.
I verified each against the code, not the thread replies:
- Client IP.
TraceCaptureGatenow carries the reader address resolved
before header sanitation, andtrace_document_bootstrapprefers it with the
same geo omission. The new publisher test asserts the masked address in the
rendered document, so a regression in the override would fail it. Both docs
pages now agree with each other and with the code. - Digest policy.
source_sha256is enforced only on the prebuilt path, the
Node pin is out of the digest, andcss: { postcss: {} }closes PostCSS
discovery.trace-build-inputs.test.mjsnow fails if either regresses. - Spec and plan now describe the regeneration command, the prebuilt path,
the unpublished-replacement policy, and the geo omission behind a forwarder. - The versioning gap has a tracked follow-up plan, and the Cloudflare alias
divergence is documented intrace/routes.rs.
What remains blocking is the EdgeZero dependency. It is unchanged in substance,
and the author agrees it is a pre-merge blocker. It is re-raised here only
because its previous thread was resolved while the reply said it would stay
open. The other two findings are small.
1 of the 3 inline comments carries a one-click
suggestion.
Blocking
🔧 wrench
- EdgeZero pin still targets open #403; restore the open-thread guard
— see inline atCargo.toml:64
Non-blocking
🤔 thinking / ⛏ nitpick
- Rust half of the digest pair no longer runs in CI — see inline at
crates/trusted-server-js/build.rs:324 - Follow-up link pinned to a PR-branch commit — see inline at
docs/guide/integrations/gpt-diagnostics.md:315
Cross-cutting / body-level findings
- 📝 Surfaces checked this round and clean, so a tenth pass does not
re-litigate them:- The manifest restamp changed only
source_sha256; the frozen v1 bytes and
both per-asset digests are untouched. - chrono's
wasmbindfeature is active in the Cloudflare wasm build
(crates/trusted-server-core/Cargo.toml:63), so the trace paths'Utc::now()
goes throughjs-sysrather than theSystemTime::now()that panics on
wasm32-unknown-unknown. - All 61 review threads were resolved at the time of this review.
- The manifest restamp changed only
CI Status
All 24 checks passing on 20b057537.
| edgezero-adapter-fastly = { git = "https://github.com/IABTechLab/edgezero", rev = "70876c92c52200d37455772f670f33eba760bd9b", default-features = false } | ||
| edgezero-adapter-spin = { git = "https://github.com/IABTechLab/edgezero", rev = "70876c92c52200d37455772f670f33eba760bd9b", default-features = false } | ||
| edgezero-cli = { git = "https://github.com/IABTechLab/edgezero", rev = "70876c92c52200d37455772f670f33eba760bd9b" } | ||
| edgezero-core = { git = "https://github.com/IABTechLab/edgezero", rev = "70876c92c52200d37455772f670f33eba760bd9b", default-features = false } |
There was a problem hiding this comment.
🔧 wrench — Re-raising the EdgeZero dependency so an open thread guards it again. The previous thread was resolved even though its own reply says the blocker stands.
The reply on the earlier thread agrees with the finding: “the required merge/tag and subsequent released-API migration cannot be replaced by another open-PR snapshot … Leaving this thread unresolved … The dependency remains an explicit pre-merge blocker.” The thread is nevertheless resolved, and unresolved threads are part of what gates merge here, so this restores that guard. Nothing about the finding itself has changed:
- Add pre-dispatch hooks and explicit request ingress fidelity edgezero#403 is still open, its review decision is
CHANGES_REQUESTED(renewed this morning atca252139), and no tag pastv0.0.10exists. - Panicking .expect() on regex compilation from user configuration #403's head has moved again, to
7b6f913a, which is now 7 commits past the pinned70876c92(the reply's “current headca252139” is already stale). I re-checked at7b6f913a:Router::pre_dispatch_hookis still generic by value with no blanketimpl PreDispatchHook for Arc<T>, andRequestIngress::newstill takes three arguments and returnsSelf. - The migration that implies has grown on this branch: there are still five
.pre_dispatch_hook(Arc::new(..))calls, and the four-argumentRequestIngress::newcalls are now 13 (up from 10), several of them in the tests added this round.
No change is requested in this branch until #403 settles and is released by tag. At that point, repin to the tag and migrate the call sites above. Please leave this thread open until then.
There was a problem hiding this comment.
Updated the interim dependency pin and all eight lockfile source entries to EdgeZero #403's current head, d05d5526d95c0b8b237cbdb19a42a84040ebecb3, in 9ef51f375, as requested for the moved upstream head. All five shared-hook registrations now use pre_dispatch_hook_arc; all 13 ingress constructors use the three-argument API, with header overrides applied through with_header_overrides where needed.
All eight Clippy gates, all five adapter test aliases, 27 parity tests, the native digest tests/lint, both emitted-script regressions and the prebuilt check pass against that snapshot. Fastly tests used Viceroy 0.21.0.
This addresses the API drift only. I rechecked #403: it remains open with changes requested, and v0.0.10 is still the latest tag. The PR description continues to call out the merge/tag/released-API verification requirement. Leaving this thread unresolved as the merge guard until that requirement is met.
Summary
Add a default-off, same-tab mobile ad-rendering trace workflow with explicit Enable/End controls, bounded browser-local capture, a consolidated viewer and JSON export. Preserve ordinary advertising when transport, correlation, storage or browser APIs are unavailable.
Review corrections
Both setup and traced publisher-document metadata use the authenticated reader IP captured before header sanitation. Runtime geo is omitted when it describes a different address. Ordinary request handling on Axum, Cloudflare and Spin retains its existing runtime client address.
Normal builds compare rebuilt trace assets with committed bytes. Prebuilt builds additionally require matching source provenance. The digest covers dedicated trace inputs and the locked Vite dependency graph, excludes the Node pin, and ignores publisher-only dependencies. Trace CSS builds disable ambient PostCSS discovery. CI now builds into a temporary bundle directory and runs
TSJS_PREBUILT_DIR=<dir> cargo check-axum --locked, exercising the Rust source digest alongside the JS manifest checks. The published guide links to the asset-versioning follow-up onmain. The branch includes currentmainthrough7aa5c993b; asset provenance is restamped against its updated Vite dependency graph.A further review found that refreshed-slot summary cards could combine one request's timing with an earlier request's auction. Auction links now match both slot and representative request number. Two regression cases reproduce the mismatch before the fix and pass afterward. The unpublished v1 JS asset and manifest were regenerated for this correction; the CSS bytes are unchanged. Published asset URLs must still receive a new version instead of replacement bytes.
Runtime path normalization remains adapter-dependent: aliases still visible at dispatch are rejected; Cloudflare may normalize an alias to an exact route before the hook.
Dependency blocker
All EdgeZero dependency and lockfile URLs use the canonical IABTechLab/edgezero repository. The pin is updated to
d05d5526d95c0b8b237cbdb19a42a84040ebecb3from EdgeZero #403. All five shared-hook registrations usepre_dispatch_hook_arc; all 13 ingress constructor calls use the three-argument API, with nonempty header overrides applied separately throughwith_header_overrides.This remains an interim PR snapshot. #403 is still open and the latest released tag remains v0.0.10. The dependency review thread remains unresolved as a merge guard. Before merging, #403 must settle and ship by tag, followed by a consistent dependency/lockfile update and verification against that released API. Updating to another open-PR snapshot does not satisfy this gate.
Validation
cargo check-axum --lockedpath passed; prebuilt embedding used all 13 freshly built modules.Trace remains disabled by default. Physical iOS Safari/Android Chrome, browser session restoration, deployed HTTPS cookie workflows and operator staging/privacy/CDN/CSP acceptance remain release gates. Local automated checks do not establish those results.
Closes #1050
Closes #1108