Repository navigation
Preserve Next.js and Google Tag Manager rewrites in streamed pages - #1222
ChristianPavilonis wants to merge 19 commits into
Conversation
Apply matching script stages in sequence and emit each fragment once. Keep GTM buffering response-local and bounded, and rewrite captured Flight payloads only after reading their original record boundaries. Preserve unsafe originals and recompute decoded record lengths. Cover Pages and App Router data, bootstrap controls, escaped queries, source-order fallback, and complete identity/gzip adapter responses. Refs #1206, #1207, #1208
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Composes script stages instead of letting one integration's Replace overwrite another's, and threads GTM rewriting into captured Flight payloads with recomputed T lengths. The fallback paths are conservative and the boundary-sweep test coverage is excellent; no blocking issues found. A few design questions and minor cleanups below.
3 of the inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches lines outside the diff or needs an author decision.
Non-blocking
🤔 thinking / ⛏ nitpick
- Flight GTM rewriting is broader than the HTML policy (bare origins, rendered content) — see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:194 ContentType::Htmlremoves the last escaping layer for request-host-derived bytes — see inline atcrates/trusted-server-core/src/html_processor.rs:716- Script rewriter selectors are now effectively
script-only — see inline atcrates/trusted-server-core/src/html_processor.rs:682 - Per-character work on every inline script — see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:476 - Redundant
enabledfilter — see inline atcrates/trusted-server-core/src/integrations/nextjs/mod.rs:114 - Assertions without messages — see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:3606andcrates/trusted-server-core/src/html_processor.rs:838
Cross-cutting / body-level findings
-
🏕 Registration order is now load-bearing but undocumented — GTM's
protects_current_flight_fragmentreads a snapshot the Next.js placeholder stage writes for the same fragment, sonextjsmust stay ahead ofgoogle_tag_managerinbuilders()(crates/trusted-server-core/src/integrations/mod.rs). LikewiseNextJsNextDataRewritermust precedeNextJsRscPlaceholderRewriter, which consumes its one-shotnext_data_fragment. A reorder would make GTM read the previous fragment's flag, and only the mixed integration tests would catch it. Suggest a comment alongside the existing "This must remain first" note (apply manually —mod.rsis outside the diff):// `nextjs` must precede `google_tag_manager`: GTM's script stage reads the // Flight-protection snapshot the Next.js stage records for the same fragment.
-
📝 PR description misses the new file — the Changes table doesn't list the new
nextjs/script_lexer.rs(318 lines, the lexical filter that decides executable vs inert__next_fspellings), and Scope says "11-file diff" while the diff is 12 files. Worth adding so reviewers know it exists and that it is intentionally "not a JavaScript parser". -
👍 Atomic capture and fallback discipline — batched capture in
rsc_placeholders.rsdiscovers every range and validates the queue budget before committing anything, fallback always restores untouched originals (enabled_gtm_transform_never_mutates_fallback_originalslocks that in), and the every-split-boundary sweeps plus identity/gzip end-to-end checks through both Axum and Fastly with decoded T-length assertions are exactly the right regression net for this class of streaming bug.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- integration tests: PASS
- cargo test (cross-adapter parity): PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- format-docs: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- format-typescript: PASS (required)
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- prepare integration artifacts: PASS
- vitest: PASS
- cargo test (axum native): PASS
- cargo fmt: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo test: PASS (required)
- CLAUDE.md symlink guard: PASS
- Analyze (actions): PASS
Keep regex literals after statement blocks inert without hiding Flight calls after object, function, or class expressions. Bound brace and pending-class context alongside existing lexical nesting limits. Add fragment-boundary and identity/gzip regressions for origin/GTM rewrites and decoded Flight T lengths. Both reported brace/slash regressions were reproduced before the fix.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Request changes for three reproduced P2 correctness issues:
- A superclass function body confuses the new lexer, allowing GTM rewriting to produce an invalid Flight T-record byte length.
- A trailing
__next_fproperty reference disables rewriting for the current and subsequent Flight scripts. - An ordinary GTM URL before a Flight call is rewritten differently depending on input chunk boundaries.
The inline comments describe the reproductions and proposed remedies.
Validation: 2,876 existing native core tests passed, as did Axum tests, cargo clippy-axum, cargo fmt --all -- --check, and the focused Fastly complete-response regression under Viceroy. Additional scratch regression tests reproduced the issues above. GitHub CI was green at submission.
Reviewed commit: d738d93f70e3a1e79bfa9256cc730a317d14381a.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed the streamed Next.js/GTM composition and the host-validation follow-up at e81e5d98d7518836ac2aad3e80728785b535c9c4. Request changes: the three previously reproduced P2 correctness findings remain unresolved; the affected runtime code is unchanged from the earlier review, and the author confirms the follow-up commits do not address them.
Blocking
🔧 wrench
- [P2] Superclass function bodies confuse pending class state, permitting an invalid Flight T-record length — existing inline finding at
nextjs/script_lexer.rs:279. - [P2] A trailing
__next_fproperty reference disables rewriting for the current and subsequent Flight scripts — existing inline finding atnextjs/rsc_placeholders.rs:113. - [P2] Ordinary GTM source before a captured Flight call is rewritten differently depending on input chunk boundaries — existing inline finding at
google_tag_manager.rs:1134.
The existing inline threads contain the reproductions and proposed remedies. No duplicate inline comments or new findings are included in this re-review.
Validation
Compared the affected runtime code with the previously reviewed commit d738d93f70e3a1e79bfa9256cc730a317d14381a: the lexer and payload-discovery files are byte-identical, and the GTM change is an assertion message only. Reviewed the complete changed files and removed diff code with independent reviewers. Verified the remote CI results at the reviewed head, including all eight target-matched clippy steps; no full local CI rerun was performed.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test (axum native): PASS
- CLAUDE.md symlink guard: PASS
- Analyze (javascript-typescript): PASS
- cargo fmt: PASS (required)
- Analyze (rust): PASS
- format-typescript: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo test: PASS (required)
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- prepare integration artifacts: PASS
- vitest: PASS
- Analyze (actions): PASS
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at b3538b47d. 4d73741 resolves the three P2s from the previous round. I checked each fix against its regression test: the superclass lexer state, trailing __next_f references, and the chunk-invariant ordinary GTM before captured Flight. One correctness issue remains in the same family as the first P2. A lexer false negative still lets GTM corrupt raw Flight T lengths, because protection depends only on the masked-code view. Requesting changes for a fail-closed guard instead of more per-spelling lexer fixes.
Blocking
🔧 wrench
- Lexer false negatives fail open into invalid Flight T lengths. Five valid-JS prefixes reproduce it. A raw-qualifier protection fallback was prototyped locally and fixes all five. See inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:435.
Non-blocking
🤔 thinking
- Underscore host labels now return 400. See inline at
crates/trusted-server-core/src/http_util.rs:242.
Cross-cutting / body-level findings
- 📌 GTM without Next.js still corrupts App Router Flight lengths. With
nextjsdisabled and GTM enabled,<script>self.__next_f.push([1,"1:T64,{\"tag\":\"http://www.googletagmanager.com/gtm.js?id=…\"…}"])</script>is rewritten to declare0x64(100) for a 101-byte body.protects_current_flight_fragmentreturnsfalsewhen no Next.js state exists. By code reading, this path is unchanged frommain, so it is out of scope here. It is worth a follow-up issue, since GTM-only deployments on App Router sites hit it with no Next.js involvement. - 👍 Prior-round fixes are well locked in.
trailing_next_f_references_preserve_current_and_later_flight,ordinary_gtm_before_captured_flight_is_chunk_invariantand the expanded superclass sweep cover every source split plus identity/gzip. TheSuppress→ unprotected change inrewrite_claimed_fragmentfixes chunk variance without touching GTM's raw fallback restoration.
Local validation (PR head b3538b47d)
cargo test -p trusted-server-core --lib --target aarch64-apple-darwin: 2,946 passedcargo test-fastly(Viceroy): pass (core 2,940 passed / 6 ignored; adapter 196 passed)cargo test-axum: passcargo clippy-fastly: passcargo fmt --all -- --check: pass
CI Status
All 19 checks pass, including cargo fmt, cargo test, axum, cloudflare, spin, the parity, CLI and integration suites, vitest, format-docs, format-typescript and CodeQL.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Approving. Script stages now compose instead of overwriting each other, ContentType::Html stops entity-escaping JS, GTM state is per-document, and the raw push-head observer from af69781 closes the fail-open lexer path from round 3. I found no fail-open path in Next.js-emitted spellings. The earlier round-1 and round-3 points are resolved or explicitly deferred.
Verified locally at 514a617: core lib tests (2949 passed), cargo test-spin, cargo test-axum, Fastly adapter under Viceroy (196 passed), and clippy-fastly / clippy-axum / clippy-spin-native / clippy-spin-wasm all clean.
CI note: 7 jobs (integration, browser integration, spin, format-typescript, CodeQL rust/python/js-ts) were cancelled with "job was not acquired by Runner", which is an infra failure, not code. Please re-run them before merge.
Non-blocking follow-ups
- 🤔 Inline-script cost with Next.js enabled (
rsc_placeholders.rs~L434/441/540). This adds numbers to the deferred perf thread. Release build, native arm64, 8 KB chunks, Next.js + GTM: a 2.1 MB non-Flight inline script goes from ~2 ms (base 8048301) to ~36 ms (~18×), and a 1.7 MB Flight push goes from ~8 ms to ~20 ms. WASM will be slower. A cheap pre-filter would avoid most of it: only mask and observe when the fragment plus a short carried tail contains__next_f, or when a head is in progress. Batched trimming instead of a per-chardrainwould also help. Worth a tracked issue if it stays deferred. - 🤔 Host validation applies to every proxied request (
publisher.rs~L4443,http_util.rs~L185), including assets that never reach a script rewriter. An emptyX-Forwarded-Host, aForwardedvalue that is not visible ASCII, or a valuelesshost=now returns 400 instead of falling back to a lower-priority header. Low impact on Fastly and Spin, which strip these headers, but notable for Axum or Cloudflare behind a proxy. Either document it as intended or tolerate an unparseableForwardedvalue that has nohost=. - ⛏ Redundant
RequestInfo::from_requestinhandle_page_bids(publisher.rs~L6985). It duplicates the extraction ~30 lines earlier, and the first binding is still live. - ⛏ About 40 new
assert!/assert_eq!calls lack a"should …"message (AGENTS.md). Examples:adapter-axum/tests/routes.rs:1079,adapter-fastly/src/app.rs:3191,test_support.rs, andnextjs/mod.rs. - 📝 For the deferred Flight GTM policy discussion:
rewrite_gtm_rsc_spanrewrites quotedanalytics.google.comURLs and bare origins without any GTM marker. The ordinary script path deliberately doesn't treat that domain as a trigger.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at 514a617a0. The composition model, per-document GTM state, raw ContentType::Html emission and host validation hold up. Two new correctness bugs remain in the Flight path: a lexer miss can still leave invalid T lengths when the row continues in the next script (Next.js alone, a regression from main), and GTM's whole-span Flight routing can produce http:/integrations/… for a URL split after its scheme. The branch also conflicts with main, and the required format-typescript check never ran.
3 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied.
Blocking
🔧 wrench
- A Flight push the lexer misses still corrupts T lengths when its row continues in the next script: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:475 - Whole-span routing half-rewrites a URL split after its scheme across Flight payload scripts: see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:204 - Conflicts with main: see cross-cutting below
- Required CI never ran: see cross-cutting below
Non-blocking
🤔 thinking / ♻️ refactor / ⛏ nitpick
- Non-null form state and binary pushes disable Flight rewriting for the rest of the page: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:40 - Skip the script handlers when no script rewriter is registered: see inline at
crates/trusted-server-core/src/html_processor.rs:665 - Domain-lint markers for PR-added test hosts: see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:3636andcrates/trusted-server-core/src/integrations/nextjs/mod.rs:1216
Cross-cutting / body-level findings
-
🔧 Conflicts with main.
google_tag_manager.rsconflicts with #1179, which moved GTM's buffer to a per-documentScriptTextAccumulator(registry.rs, re-exported fromintegrations/mod.rs). A resolution that keeps this PR'sGtmScriptStatechecks out locally:- Take the PR side of both hunks (the struct and the top of
rewrite). Main's comment aboveget_or_insert_with, its// allow-domain:comment and its newan_interrupted_document_leaves_no_residue_for_the_next_documenttest merge cleanly. - Restore
Mutexinuse std::sync::{Arc, LazyLock, Mutex};(the auto-merge drops it) and drop the now-unuseduse crate::integrations::ScriptTextAccumulator;(clippy-D warningsfails on it). - Delete
ScriptTextAccumulatorfromregistry.rsand its re-export fromintegrations/mod.rs. After the merge it is public API with no users, and the deletion passes fmt and clippy.
On that merged tree,
cargo fmt --check,clippy-fastly,clippy-axum,test-axum,test-fastlyandtest-fastly-reusepass, as do 2,956 native core tests,template_build_digest(no fixture update needed: the build digest rotates automatically) and both of main's new regression tests against this PR's logic. - Take the PR side of both hunks (the struct and the top of
-
🔧 Required CI never ran. Seven jobs were cancelled with "The job was not acquired by Runner of type hosted even after multiple attempts" and never re-run:
format-typescript(required),integration tests,browser integration tests,cargo check/build/test (spin native + wasm32-wasip1),Analyze (rust),Analyze (javascript-typescript)andAnalyze (python). The ruleset requires the branch to be up to date with main, so the merge above will start a fresh run; please make sure these go green. -
📌 The ordinary GTM script path has the same scheme-split problem, and it predates this PR.
rewrite_gtm_urls(L515-537, called at L1182) triesGTM_WHOLE_URL_PATTERN, which is meant for attribute values, on held script text that can begin at any chunk boundary. A split just after the scheme givesj.src='http:/integrations/google_tag_manager/gtm.js?id='+i+dl, and a split at the opening quote leaves later URLs in the script unrewritten. Main behaves the same, so this is follow-up material: a quoted-only helper for the script path fixes it (the URL is then left unrewritten instead of corrupted). Relatedly, the retained boundary in the PR description ("a hostname split inside its initial six-byte marker prefix") understates the window: every split from just after the opening quote throughgooglmisses, about 17 positions for"https://www.googletagmanager.com. -
🌱 GTM and Next.js now depend on each other. GTM calls
nextjs::protects_current_flight_fragment(google_tag_manager.rs:1125),nextjs/mod.rsreadsGoogleTagManagerConfigand takesrewrite_gtm_rsc_span, and correctness depends onbuilders()order. A third integration that needs Flight protection or a Flight transform would add more cross-imports. A neutral home would fit better as a follow-up: a registry-owned "current fragment holds raw Flight" flag onIntegrationScriptContext, and a span-transform hook such asIntegrationRegistration::with_flight_span_transformthat GTM registers. -
🤔 The lexer keeps producing this bug class. The superclass case, the five round-3 prefixes and the continuation case above all come from
script_lexer.rsmisclassifying publisher code that shares a<script>with a Flight push. Next.js never emits that: each push is its own script. An alternative that removes most of the 513-line lexer is to capture only scripts whose whole content is a sequence of Next.js-shaped pushes, and to treat any other script with a raw qualified data head as unsupported (bypass_rsc), as main did for unsupported pushes. The trade-off is losing rewriting on pages where publisher code and a push share a script. Worth weighing before the next lexer fix. -
🏕 Docs and changelog.
docs/guide/integrations/google_tag_manager.md("1. Script Rewriting") doesn't mention that GTM/GA URLs inside App Router Flight payloads are now rewritten with recomputed T lengths, or that this depends onnextjsbeing enabled. No guide mentions the new 400 for a malformed effective host; main'sdocs/guide/ts-dev-proxy.md:291says core "prefersX-Forwarded-Host, thenHost", omittingForwardedand the 400.docs/guide/creative-processing.md:752still saysremove_node()deletes the script element, which contradicts the newScriptRewriteAction::RemoveNodedoc inregistry.rs.CHANGELOG.mdhas no[Unreleased]entry. -
🏕 Leftover function-local imports.
crates/trusted-server-adapter-axum/tests/routes.rs:883-885still hasuse std::sync::Arc;anduse trusted_server_core::test_support::nextjs_auction;insidenextjs_auction_output_holds_until_the_structural_body_close, now that the PR hoists both to module level. AGENTS.md asks for no local imports. -
♻️ Transform plumbing.
Option<fn(&str) -> String>now runs through eight signatures (rewrite_span, both rewrite functions, the factory and the processor), and the fix for the whole-span bug pushesrewrite_spanto seven arguments, the AGENTS.md limit. A smallSpanRewrite { rewriter, origin_host, request_host, request_scheme, transform }withfn apply(&self, span, record_text)would carry it. The single-payload loop (rsc.rs~L465-513) and the combined loop (~L715-763) differ only in the length function and could share one helper, andrewrite_rsc_scripts_combined_with_limitnow only forwardsNone.
Lower-priority notes (each verified, all minor):
- 🤔
html_processor.rs:667-679: the flat match-flag vector is reset at every<script>start tag and never restored at its end tag. Inside<svg>a<script>can nest, so the outer script's remaining text runs with the inner script's flags (<svg><script>a<script id="target">b</script>c</script></svg>feeds"c"to ascript#targetstage). Negligible in practice; worth a doc line. - 📝
http_util.rs:214-255accepts any single DNS label, includingscript. With that host, bare-host Flight rewriting (host_rewrite.rs) turns an origin-supplied raw</origin.example.com>into</script>. Not a regression (main had no validation, and Next.js never emits a raw<in Flight), but it qualifies the reasoning that validation makes raw emission safe. Not treating<or</before the host as a boundary would close it. - 🤔
http_util.rs:202-203: an emptyHost:now returns 400, while an absentHoststill yields an empty host. RFC 9112 sends an empty Host when the target has no authority, so the two cases mean the same thing. It only affects Axum or proxy setups. - 📝
rsc.rs:223(unchanged by this PR): the unescaped-length model miscounts\xe9,\0, octal escapes,\q, line continuations and\u{…}. Next.js'sJSON.stringifyplushtmlEscapeJsonStringnever emits them, and the GTM transform can't introduce them, so nothing is needed here. - 📝
google_tag_manager.rs:1147-1159: the limit counts held bytes plus the current fragment, so a single final fragment larger thanmax_buffered_script_bytesnow passes through unrewritten, where main rewrote it. Unreachable at the defaults (16 MiB limit, 8 KB chunks); a one-line comment would make the intent clear. - ⛏
google_tag_manager.rs:451-455:GtmScriptStateand its fields have no doc comments, and the release blocks at L1129-1134 and L1154-1159 are identical. Animpl GtmScriptState { fn release(&mut self, content: &str) -> ScriptRewriteAction }would remove the duplication. - ⛏
script_lexer.rs:MAX_TEMPLATE_DEPTH(L6) also caps braces, parentheses and pending classes, soMAX_LEXICAL_DEPTHwould read better;self.word.len() < 10(L130) is a magic number;ScriptLexer(L56) and itsexpression_start/statement_startinvariants are undocumented. Inrsc_placeholders.rs, slicingcodeat offsets taken fromcontentrelies onmaskemitting one#per masked byte; adebug_assert_eq!(code.len(), content.len())would document it. - ⛏
rsc.rs:626(mostly pre-existing): the classifier compares payload bytes with the limit, but the combined rewriter adds 7 marker bytes per boundary, so a group within7 * (n - 1)bytes of the limit is classified rewritable and then skipped with twowarn!lines. Removing the origin-host early exit for GTM-enabled groups makes this more reachable.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: CANCELLED
- browser integration tests: CANCELLED
- CodeQL: SKIPPED
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- prepare integration artifacts: PASS
- format-docs: PASS (required)
- cargo test (axum native): PASS
- CLAUDE.md symlink guard: PASS
- cargo test: PASS (required)
- cargo fmt: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- vitest: PASS
- cargo check/build/test (spin native + wasm32-wasip1): CANCELLED
- format-typescript: CANCELLED (required)
- Analyze (rust): CANCELLED
- Analyze (javascript-typescript): CANCELLED
- Analyze (python): CANCELLED
Brings #1222 at 514a617. Integration decisions: - RequestInfo::from_request keeps #1251's source order (authenticated forwarder, immutable ingress, runtime origin, Host) and applies #1222's host syntax check to whichever host is selected. It returns BadRequest for a malformed or non-text selected host. Raw Forwarded and X-Forwarded-Host are never selected in the RC, so #1222's tests for those headers now assert they are ignored instead of rejected. - RC-only callers (DataDome protection, Fastly compat, Spin, forwarder, orchestrator and publisher tests) handle the fallible signature. DataDome only reads RequestInfo behind an authenticated origin and keeps its request_host fallback. - The first-party sign path prefers the authenticated forwarder scheme (#1251) and propagates #1222's host error. - The GTM rewriter takes #1222's per-document GtmScriptState, which supersedes main's ScriptTextAccumulator use from #1179; state still lives in IntegrationDocumentState, and #1179's two-document residue tests remain.
… state, and apply approved cleanup.
|
@ChristianPavilonis I don't see resolution for the previous review. Can you please address with comment and resolve it? Thanks. |
Resolve merge conflicts preserving streamed rewrites and base regressions; exclude build-only git metadata from legacy settings parsing.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at 6b317b558. Every round-1 blocker is resolved, and each fix was checked against the original repro: the lexer-miss continuation now fails closed with consistent T lengths, a Flight URL split after its scheme is preserved (no bad T header across about 100k sweep cases), non-null form state no longer disables rewriting, the handler guard and lint markers are applied, the branch is up to date with main, and all 22 checks pass.
Requesting changes for what is still open: how far an inert data-head hint should reach, the settings change that came in through a merge commit, the round-1 leftovers (ScriptTextAccumulator, the local imports, docs, changelog and PR description) and a test gap on the new record_text sites.
3 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied.
Requested before merge
- 🤔 Inert
self.__next_f.push([1,text in any script disables all later Flight rewriting: see inline atcrates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:35 - ♻️ Settings change from merge commit d0cfd88 duplicates open PR #1326: see inline at
crates/trusted-server-core/src/settings.rs:3057 - ♻️
ScriptTextAccumulatoris now dead public API: see inline atcrates/trusted-server-core/src/integrations/registry.rs:575 - ⛏ Function-local imports still present: see inline at
crates/trusted-server-adapter-axum/tests/routes.rs:10 - 🏕 Docs, changelog and PR description: see inline at
crates/trusted-server-core/src/integrations/registry.rs:69 - 🏕 Pin every
record_text = falsesite: see inline atcrates/trusted-server-core/src/integrations/nextjs/mod.rs:1322
Other findings
- 🤔 The document-wide Flight fallback is silent: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:586 - ⛏ Restore main's comment that the merge dropped: see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:1135 - ⛏ Say why a combined record body is still safe to route: see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:194 - ♻️ Observer state sprawl: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_stream.rs:73 - ⛏ Oversized pass-through branch is unreachable through lol_html: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:258 - 📌 A removed
<script src>with an inline Flight push fails the whole response (pre-existing): see inline atcrates/trusted-server-core/src/html_processor.rs:684 - 📌 An unanchored T-header lookalike can leave a stale length (pre-existing): see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc.rs:478
Lower-priority notes:
- 📝 A comment or BOM inside a data head (
push(/*c*/[1,orpush(\u{FEFF}[1,) still yields a stale T length, but only when the lexer also misses that push (for example afterfoo: { bar() } /'/.test(s);). Without such a prefix this head handles both,[1.0,and[0x1,correctly, and main is stale on the comment spelling even without a prefix, so this is not a regression. - ⛏ The
22ef7ca07subject is 103 characters and ends with a period (CONTRIBUTING.md asks for about 50 and no period). A squash merge makes this moot.
Round-1 verification
| Round-1 finding | Status at 6b317b558 |
|---|---|
| Lexer miss leaves a stale T length when the row continues in the next script | Fixed, fail-closed: all five prefixes give consistent lengths at chunk 16, 32 and 8192, Next.js only and mixed; the regression test fails when the bypass assignment is removed |
| Whole-span routing half-rewrites a URL split after its scheme | Fixed for the reported shape; see the test-gap comment for the other three span sites |
| Non-null form state disables later rewriting | Fixed; binary channel 3 keeps the document fallback by design, as documented |
| Script handlers registered with zero rewriters | Applied, byte-identical to the suggestion |
| Domain-lint markers | Applied; ts dev lint domains --changed-vs a9037770a reports 0 violations |
| Conflict with main | Resolved; up to date with main, main's #1179 regression tests pass |
| Required CI never ran | Resolved; all checks pass |
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- Analyze (rust): PASS
- Analyze (python): PASS
- cargo test (axum native): PASS
- CLAUDE.md symlink guard: PASS
- cargo fmt: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo test: PASS (required)
- format-typescript: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (actions): PASS
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- prepare integration artifacts: PASS
- vitest: PASS
- Analyze (javascript-typescript): PASS
…nce capture fix, conservative fallback regressions, once-only warnings, span-site tests, documentation/changelog updates, settings rollback, and routine cleanup. No public
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at b5aac6daf. The round-2 code items are addressed: the foreign-reference trigger is fixed (13 foreign-reference shapes now capture fully at every split, with about 250k probe runs and a fuzz pass all producing consistent T rows), the inert data-head fallback is pinned by the JSON-LD regression test and stated in the CHANGELOG, the page-wide fallback now warns once, the new span-site test kills all four record_text mutants, and the settings rollback, restored comment, doc line and local-import cleanup are in. Requesting changes because none of the required checks have run on this head: the branch conflicts with main, so GitHub did not start the test or format workflows. Smaller items are inline and below.
4 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches lines outside the diff and can't be auto-applied.
Blocking
- 🔧 Required CI has not run on this head. Only the two CodeQL analyses ran on
b5aac6daf;cargo test,cargo fmt,format-docsandformat-typescript(all required) and the rest of the suite did not, becausetest.yml/format.ymltrigger onpull_requestand GitHub skips those runs while the PR conflicts with main. The conflict isCHANGELOG.mdonly (#1238 added a### Fixedentry at the same spot); keeping both sides, with this PR's entries first, resolves it. On that merged tree,cargo fmt --check,clippy-fastly,clippy-axum,test-axumand all 3,046 native core tests pass locally (no #1327 failures now that #1326 is on main), and #1238's new header rewriting uses the validated request host. Please merge main and make sure the full CI run goes green.
Still open from earlier rounds
- ♻️
ScriptTextAccumulatoris still dead public API (third round).integrations/registry.rs:219and the re-export atintegrations/mod.rs:40have no users since this PR replaces GTM's use of it. The round-2 thread onregistry.rs:575is still open. - 📝 PR description is stale. It still cites 2,883 native core tests (3,046 on the merged tree); it has no
## Specline, which main's #1385 template asks for ("Not a feature" for a fix); the Changes table leaves outCHANGELOG.mdand the three docs files; "Retained boundaries" still reads per payload and doesn't mention the document-wide fallback triggers, the inert-hint behavior (including Next.js-only pages) or the once-per-document warning; and thersc_placeholders.rsrow leaves out the foreign-reference fix, channel-2 form state and the warning. - 📌 Follow-up issues for the two round-2 pre-existing bugs are not filed (the removed
<script src>placeholder error, and the unanchored T-header lookalike). Their threads are still open. - ♻️ Observer state sprawl (round-2, optional) is unchanged.
Other findings
- 🏕 The warn-once test never runs in CI: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:807 - ⛏ A second, mislabeled fallback warning: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:71 - 🌱 Unguarded process-wide test logger: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:841 - 📝 Fallback paragraph lists two of about ten triggers: see inline at
docs/guide/integrations/google_tag_manager.md:156 - ⛏ Fastly does not use
sanitize_forwarded_headers: see inline atcrates/trusted-server-core/src/http_util.rs:152 - ⛏ CHANGELOG details: see inline at
CHANGELOG.md:37 - ⛏ Stale
expectmessages: see inline atcrates/trusted-server-core/src/integrations/nextjs/mod.rs:794 - ⛏ Commit message. The
b5aac6dafsubject is 243 characters of session residue ("Prepared scoped, uncommitted changes for findings 0–6: …"), not imperative, and cut off at "No public", with no body. CONTRIBUTING.md asks for an imperative subject of about 50 characters, and main's squash merges keep branch commits as body bullets, so this text would land on main. Rewording the commit (or editing the squash body at merge) avoids that, for example "Warn once on document-wide Flight fallback".
Lower-priority notes:
- 📝 A comment or BOM inside a data head (
push(/*c*/[1,,push(\u{FEFF}[1,) still yields a stale T length when the lexer also misses that push, unchanged from round 2; main is stale on the comment spelling even without a lexer miss, so this is not a regression. - 📝 Two
record_textsites (rsc.rsL508 and L758, the remaining spans) survive mutation, but they are equivalent mutants: a remaining span after a T body starts at a row id or a marker and can never match the anchored whole-URL pattern.
Round-2 verification
| Round-2 finding | Status at b5aac6daf |
|---|---|
| Inert data-head text disables later Flight rewriting | Accepted (option a): pinned by json_ld_data_head_preserves_later_flight_and_queued_originals, stated in the CHANGELOG; foreign-reference trigger fixed |
| Document-wide fallback is silent | Fixed with bypass_document(); see the CI-gating and double-warning comments |
record_text test gap |
Fixed; the new test fails under each of the L474, L706, L724 and L735 mutants |
| Restore main's per-document comment | Done |
record_text doc line |
Done |
settings.rs change from a merge commit |
Reverted; identical to main's #1326 after merging |
ScriptTextAccumulator |
Not done |
Function-local imports in routes.rs |
Done |
| Docs, changelog, PR description | Docs and CHANGELOG added (details inline); description not updated |
| Pre-existing 📌 bugs | No follow-up issues yet |
| Observer state sprawl | Not done (optional) |
| Unreachable pass-through branch | Defensive comment added |
CI Status
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- cargo test: not run (required)
- cargo fmt: not run (required)
- format-docs: not run (required)
- format-typescript: not run (required)
Resolve changelog merge conflict while preserving both branches’ changes
…, clarify fallback/security documentation, and apply accepted routine corrections.
Summary
Spec
Not a feature. Fixes #1206, #1207 and #1208.
Changes
Core paths below are relative to
crates/trusted-server-core/src/.html_processor.rshttp_util.rspublisher.rsproxy.rsintegrations/mod.rsScriptTextAccumulatorre-export.integrations/registry.rsScriptTextAccumulatortype and implementation.integrations/prebid.rsintegrations/google_tag_manager.rsintegrations/nextjs/mod.rsintegrations/nextjs/rsc.rsintegrations/nextjs/rsc_placeholders.rsintegrations/nextjs/rsc_stream.rsintegrations/nextjs/script_rewriter.rsintegrations/nextjs/script_lexer.rstest_support.rsCHANGELOG.mddocs/guide/integrations/google_tag_manager.mddocs/guide/integrations/nextjs.mddocs/guide/ts-dev-proxy.mddocs/guide/creative-processing.mdcrates/trusted-server-adapter-axum/tests/routes.rscrates/trusted-server-adapter-fastly/src/app.rsScope
These three issues share the same script-processing defect, so this PR fixes them together. The diff is mostly regression tests and test fixtures covering fragment boundaries, compression, multiple payloads, buffering limits, bootstrap scripts, and escaped queries. Runtime changes stay in the shared Rust processing path; the adapter changes are tests only.
There are no new settings, dependencies, integration-order changes, or adapter error-handling changes. Missing generated placeholders still fail rather than silently hiding a lost rewrite.
Retained boundaries:
googlportion of a GTM URL; the ordinary-script path can also half-rewrite a URL split just after its scheme. The Flight-specific correction does not fix that separate ordinary-script path.self.__next_f.push([1,text in strings, comments, or JSON-LD as a fallback trigger. Pages quoting Flight source can therefore lose later origin/GTM rewrites, including with onlynextjsenabled. This trade-off is pinned by the JSON-LD regression.Deferred review discussion
Closes
Closes #1206
Closes #1207
Closes #1208
Test plan
Current pushed head:
a063906ae77e0c16fa2a665300352b6f7c2f0722, removing the unused accumulator and its re-export. Local validation on this change passes formatting, native core tests with 3,046 passed,test-fastlyincluding 3,040 core tests passed and 6 ignored,test-axum,clippy-fastly, andclippy-axum. GitHub checks for this head have started and are not yet complete.Additional local checks pass on this head:
test-fastly-reuse,test-cloudflare,test-spin, cross-adapter parity with 17 passed, native build-digest tests, all remaining target-matched clippy aliases, core rustdoc on WASM, and the WASM clock lint. Unchanged JS/docs/script checks reuse the passing preceding-head CI evidence.All 23 GitHub checks passed on the preceding head
ad5f5035a6c7121d8d8dc8d9289870a58153bd95, including required test/format checks, integration and browser suites, and adapter checks. Its core WASM CI log confirms the warn-once, foreign-reference, and JSON-LD fallback regressions ran and passed. The native-only gate on the logging regression has been removed.The local checks below record earlier implementation validation. They were not rerun just to reply to review threads; current-head verification uses the CI results above.
Regression failures were recorded before their corresponding fixes, including lost placeholders, invalid JSON, duplicate/reordered fragments, bootstrap bypass, escaped-query misses, and raw-source escaping. The malformed-host adapter regression returns 200 on the original code and 400 after validation. The deferred Flight-policy concern is recorded above.
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npm exec --no -- vitest runusing the installed Vitest without dependency installation.cd crates/trusted-server-js/lib && npm run formatnpm run formatscript passed with a command-local PATH to the existing Prettier 3.8.1, matching the docs lockfile; the ordinary command cannot resolve Prettier because docs-local dependencies are absent.cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve; direct Viceroy client testing was used instead.Earlier local test counts are omitted because they predate the latest push. See the current-head CI results above. Existing target-specific panic/doctest skips remain; native panic counterparts and all CLI browser fixtures ran. Local transport testing used debug WASM and a synthetic HTTP origin, not production TLS.
Remaining review work
ScriptTextAccumulatortype, implementation, re-export, and type-specific imports ina063906ae. This completes Aram's outstanding cleanup request.Checklist
unwrap()in new production code; useexpect("should ...").logmacros rather thanprintln!, as required by AGENTS.md.