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 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.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Tenth review pass, against head 9ef51f375 (base main). The branch is current
with main and has no conflicts. CI is 24/24.
All three findings from the previous round are resolved. I checked each one
against the code and the CI logs, not the thread replies:
- EdgeZero API drift. The pin
d05d5526matches the current head of
IABTechLab/edgezero#403 (34750022) except forCargo.lock. Every
RequestIngress::newcall site and every adapter hook registration now uses
that API. - Rust digest parity in CI. The new "Verify prebuilt trace source digest
parity" step in the Axum job builds the bundles into a temporary directory
and runsTSJS_PREBUILT_DIR=<dir> cargo check-axum --locked. It ran to
completion on this head. - Docs link.
docs/guide/integrations/gpt-diagnostics.md:315now points at
blob/main.
The only remaining blocker is the EdgeZero release. Nothing in this branch has
to change until #403 ships in a tag.
Blocking
🔧 wrench
- EdgeZero pin still targets the unmerged #403: tracked in the open
Cargo.tomlthread, so this review adds no new inline comment.
IABTechLab/edgezero#403 is still open and unmerged, and the newest tag is
v0.0.10. Once #403 is released by tag, repin alledgezero-*dependencies
to that tag, migrate any API changes made before release, and rerun CI.
Please keep that thread open until then.
Cross-cutting / body-level findings
- 📝 Surfaces checked this round and clean, so the next pass does not
re-litigate them:- Ingress migration. All 13
RequestIngress::newsites are test-only,
and none dropped a header override. The Fastly fidelity change at the new
rev touches onlyglobal_field_order.trace/cookies.rsreads only the
octetsandfield_multiplicityaxes, so cookie classification is
unchanged. - Router hook. Hook dispatch is the same at the new rev. Each production
router registers exactly one hook, so the new double-registration panic in
pre_dispatch_hook_arccannot fire. view-model.tscorrelation fix. This fix was not mentioned in the
replies. Correlation links now require a matchingrequestNumberas well
asruntimeSlotNumber, which is what the spec requires. Reverting the fix
fails both new cases intest/trace/view-model.test.ts. The resulting
v1.jsregeneration is allowed while v1 is unpublished, and the CI byte
comparison confirms the committed bytes.- Real-runtime capture.
trace_runtime_boundary_{cloudflare,fastly,spin}
and the browser workflow passed on this head. - Threads. 63 of 64 review threads are resolved. The open one is the
EdgeZero gate above.
- Ingress migration. All 13
CI Status
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS
- format-typescript: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- trace runtime and browser acceptance: PASS
- vitest: PASS
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Eleventh review pass. The head is still 9ef51f375, the same as round 10, but main has moved by three PRs: #1222, #1413 and #1290. The branch now conflicts with main in nine files. GitHub does not run pull_request workflows on a conflicting PR, so the passing checks below predate those merges, and nothing has tested the merged tree.
To see what a textual merge would hide, I resolved the conflicts in a scratch worktree and built and tested the result. Before the semantic fixes below, that produced 12 compile errors and 3 test failures. After them, everything passed:
- 3,277 native core tests
cargo test-axum,test-cloudflare,test-spinandtest-fastly(Viceroy)- 28 parity tests
The three inline comments are the places where a resolution can compile and still be wrong. The EdgeZero release gate from earlier rounds also still stands, and #403 has drifted again.
Blocking
🔧 wrench
- Keep #1413's host validation on whichever source is selected: see inline at
crates/trusted-server-core/src/http_util.rs:190 - #1290's async EOF driver must carry
trace_bootstrap: see inline atcrates/trusted-server-core/src/publisher.rs:1755 - Two process-global loggers in one test binary: see inline at
crates/trusted-server-core/src/trace/routes.rs:802
Cross-cutting / body-level findings
-
🔧 Conflicts with
main; the merged tree has not run CI. In addition to the three inline items, the scratch merge needed these changes:crates/trusted-server-integration-tests/tests/parity.rs: both sides append at the end of the file, so the conflict hunks interleave two unrelated additions. Takemain's file and append this PR'strace_paritymodule.crates/trusted-server-integration-tests/Cargo.toml: the automatic merge produces duplicatebase64andfutureskeys in[dev-dependencies], so Cargo cannot load the workspace. The conflicted lines need bothtemp-envand tokio'snet,rt-multi-threadandsignalfeatures.crates/trusted-server-core/src/settings.rs:mainmoved the test-module imports to the top ofmod tests. Keep only the newtrusted_forwardertests from this side.crates/trusted-server-core/src/publisher.rs:- Take the union of the imports.
- Take the union of the full-body condition:
diagnostics_requires_full_bodyandmain'sasync_html_finish_present && GET && is_html_document_request. - The four
AuctionDispatchliterals this PR adds in tests needmain's newintegration_registryfield. main's synchronous-control test needstrace_bootstrap: None.
CHANGELOG.md:main's entry for #1206 to #1208 says host selection "remainsForwarded(host=), thenX-Forwarded-Host, thenHost" and "Forwarded-header trust policy is unchanged". This PR's entry says the opposite. Keeping both lines would put a contradiction in the unreleased section, somain's entry needs rewriting to the merged order (authenticated forwarder, then ingress or runtime authority, thenHost, then URI authority, all validated).docs/guide/ts-dev-proxy.md: keep this PR's trust-model paragraph. Carry overmain's statements about returning 400 for a malformed host, URI-authority fallback and the emptyHost, but not its forwarded-precedence claim.Cargo.lock: regenerate after the manifest merge.
-
🔧 The EdgeZero dependency still targets the unmerged IABTechLab/edgezero#403, and #403 has drifted again. #403 is open with changes requested, and
v0.0.10is still the newest tag. Its head has moved from34750022tof6d0ed68("tighten pre-dispatch contract"). Compared with the pinnedd05d5526, that head changes three things:- It renames
edgezero_core::requesttoedgezero_core::ingress. This branch has 49 references in 10 files. - It makes
edgezero_adapter_fastly::request::into_core_request_with_ingresspub(crate).crates/trusted-server-adapter-fastly/src/main.rs:4,:486and:1193depend on it. The publicinto_core_request(req)captures ingress during conversion. That is equivalent today, because nothing between the capture atmain.rs:174and the conversion at:486changes the URL orHost. If the adapter needs to keep supplying a pre-mutation snapshot, raise that on #403 before it merges. - It rejects continuing pre-dispatch hooks that change the method or path.
TracePreDispatchHookchanges only headers and extensions on its continuing path, so it already complies.
The requirement is unchanged: repin to the released tag, migrate to the released API, and rerun CI.
- It renames
CI Status
These results are for 9ef51f375 and finished on 2026-10-24, before main moved. No checks have run on the merged result.
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- Analyze (rust): PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- CodeQL: PASS
- docs-proposal scripts: PASS
- format-docs: PASS
- format-typescript: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- trace runtime and browser acceptance: PASS
- vitest: PASS
| .get::<RuntimeRequestOrigin>() | ||
| .map(|origin| &origin.0) | ||
| }); | ||
| let host = runtime_origin |
There was a problem hiding this comment.
🔧 wrench: When this merges with main, #1413's host validation has to apply to whichever source is selected here, including ingress and runtime origin.
#1413 (now on main) made RequestInfo::from_request fallible. It also rejects any selected host that is not literal host syntax, because the host is inserted into script source. This PR adds ingress and runtime origin as the first-priority source, and that authority comes from EdgeZero's InboundOrigin::parse. That parser rejects only @ / ? # \ and then delegates to http::uri::Authority. With http 1.4.2, Authority accepts example.com', example.com;x and example.com(x). #1413's is_valid_request_host rejects all three.
A resolution that keeps #1413's check only on the Host branch, where it sits on main, would let those authorities into rewrite output. I resolved the conflict in a scratch merge with this shape:
let host = match runtime_origin {
Some(origin) => origin.authority().to_owned(),
// Non-text Host is a 400; empty Host falls through to URI authority without userinfo.
None => header_or_uri_host(req)?,
};
if !host.is_empty() && !is_valid_request_host(&host) {
return Err(invalid_request_host());
}The authenticated-forwarder branch can keep returning early, because parse_public_origin is already stricter than is_valid_request_host.
Callers:
publisher.rs: the two production sites take?again, as onmain.proxy.rs: theunwrap_or_elsebecomes amatchwith?.- DataDome
protection.rs:267becomes.and_then(|_| RequestInfo::from_request(req, client_info).ok()). Only the forwarder branch reaches it, so it cannot fail. - Test call sites take
.expect("should ...").
Tests:
- These Resolve request host from URI authority when headers are absent #1413 tests port unchanged and pass:
request_info_uses_uri_authority_without_host_headersrequest_info_validates_uri_authority_before_rewritingrequest_info_keeps_empty_host_without_headers_or_uri_authorityrequest_info_rejects_oversized_dns_names
- These
maintests assume unauthenticated forwarded headers are honored, so they need to change rather than be carried over:- Resolve request host from URI authority when headers are absent #1413's forwarded-precedence
RequestInfotests. publisher_request_rewrites_html_using_uri_authoritysets HTTPS throughX-Forwarded-Proto. Its host assertion passes, but it needs TLS throughClientInfofor the scheme.- Preserve Next.js and Google Tag Manager rewrites in streamed pages #1222's Axum
nextjs_gtm_rejects_malformed_request_hostsshould keep 400 for a hostileHost. ForX-Forwarded-HostandForwardedit should expect 200 with the hostile bytes absent, because those fields are now stripped.
- Resolve request host from URI authority when headers are absent #1413's forwarded-precedence
- Add a runtime-origin case. In the probe, this test passes with the shape above:
#[test]
fn request_info_validates_host_header_and_runtime_origin_sources() {
for host in ["example.com'", "example.com;x", "example.com(x)", "example.com</script>"] {
let mut req = build_request(Method::GET, "/page");
req.headers_mut().insert(
header::HOST,
http::HeaderValue::from_str(host).expect("should build visible host"),
);
assert!(
RequestInfo::from_request(&req, &default_client_info()).is_err(),
"should reject unsafe Host: {host}"
);
if let Ok(origin) = InboundOrigin::parse(
"https",
host,
edgezero_core::request::OriginSource::RuntimeUri,
) {
let mut req = build_request(Method::GET, "/page");
req.extensions_mut().insert(RuntimeRequestOrigin::new(origin));
assert!(
RequestInfo::from_request(&req, &default_client_info()).is_err(),
"should reject unsafe runtime authority: {host}"
);
}
}
}With these changes the merged tree passes 3,277 native core tests, plus the Axum, Cloudflare, Spin, Fastly (Viceroy) and parity suites.
Apply manually: this depends on is_valid_request_host and the fallible signature, which exist only on main.
| @@ -1725,6 +1751,8 @@ pub struct OwnedProcessResponseParams { | |||
| /// Request-scoped conditional diagnostics delivery decision. | |||
| pub(crate) gpt_diagnostics: | |||
| Option<crate::integrations::gpt_diagnostics::GptDiagnosticsRequestDecision>, | |||
| /// Request-only trace bootstrap, excluded from shared templates and ESI. | |||
| pub(crate) trace_bootstrap: Option<String>, | |||
There was a problem hiding this comment.
🔧 wrench: #1290's async EOF driver builds the HTML processor without this field. A merge that fills it with None compiles and silently drops the trace bootstrap.
On main, publisher/html_finalization.rs (HtmlEofDriver::new) constructs HtmlStreamProcessorParams itself. This PR adds trace_bootstrap, so the merge has to fill it in. None compiles, and every existing test still passes with it: I ran the full native core suite with None, and 3,277 passed. But then any deployment with allow_async_html_finish and an async-finishing integration would serve trace-active documents without the bootstrap. The driver has the value in OwnedProcessResponseParams:
trace_bootstrap: params.trace_bootstrap.clone(),A regression test for that combination, using fixtures already on main. I verified it passes with the line above and fails with None:
#[test]
fn async_eof_driver_keeps_trace_bootstrap() {
let settings = crate::publisher::allow_eof_test_settings(create_test_settings());
let calls = Arc::new(AtomicUsize::new(0));
let registry = IntegrationRegistry::from_html_stream_processors(vec![Arc::new(
CountingTerminalFactory(Arc::clone(&calls)),
)]);
let mut params = html_stream_params("", None);
params.content_type = "text/html".to_owned();
params.trace_bootstrap = Some("<script>window.__example_trace=1</script>".to_owned());
let body = EdgeBody::stream(futures::stream::iter(vec![bytes::Bytes::from_static(
b"<html><head><title>Example</title></head><body>publisher</body></html>",
)]));
let mut output = Vec::new();
futures::executor::block_on(stream_publisher_body_async(
body,
&mut output,
&mut params,
&settings,
®istry,
&AuctionOrchestrator::new(settings.auction.clone()),
&noop_services(),
))
.expect("should finish the document");
assert_eq!(calls.load(Ordering::SeqCst), 1, "should use the async EOF driver");
let html = String::from_utf8(output).expect("should produce UTF-8");
assert!(
html.contains("window.__example_trace=1"),
"should keep the trace bootstrap on the async EOF path"
);
}Trace auction terminal states are already covered on that path: the driver reuses this PR's DispatchedAuctionGuard and abandon_hold_auction, so abandoned trace auctions are still recorded there.
Apply manually: html_finalization.rs and the EOF fixtures exist only on main.
| #[test] | ||
| fn trace_auth_failure_logs_a_fixed_category_without_the_path() { | ||
| LOG_INIT.call_once(|| { | ||
| log::set_logger(&LOGS).expect("should install the test logger"); |
There was a problem hiding this comment.
🔧 wrench: After the merge, this test fights #1222's shared test logger for the process-global log slot, and the loser panics.
#1222 (now on main) added test_support::logging::WarningCapture, which installs one process-wide logger through log::set_logger. Once both are in the same test binary, whichever of the two installs second fails with SetLoggerError. In the scratch merge, this test failed in the full native core run with should install the test logger: SetLoggerError(()) and passed when run alone, so the failure depends on test order.
Both messages it checks are log::warn! (auth.rs:127, :129), which WarningCapture captures per thread. I verified this migration passes in the full suite:
// imports: drop `std::sync::{Mutex, Once}` and `log::{LevelFilter, Log, Metadata, Record}`
use crate::test_support::logging::WarningCapture;
// delete `CapturedLogs`, `LOGS` and `LOG_INIT`, then:
let warnings = WarningCapture::start();
// ... the rejected trace request ...
let logs: Vec<String> = warnings
.take()
.into_iter()
.map(|warning| warning.message)
.collect();
// ... the existing `trace_auth_failed` / no-path assertions over `logs`, minus `drop(logs)` ...
// ... the ordinary request ...
assert!(
warnings
.take()
.iter()
.any(|warning| warning.message
== "Basic auth failed for path: /ordinary-example-sentinel"),
"should retain ordinary path logging outside trace"
);Apply manually: WarningCapture exists only on main.
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