Verify content rewriting pipeline is platform-agnostic (PR 8)#600
Verify content rewriting pipeline is platform-agnostic (PR 8)#600prk-Jr wants to merge 8 commits intofeature/edgezero-pr7-geo-client-infofrom
Conversation
…zero-pr8-content-rewriting
aram356
left a comment
There was a problem hiding this comment.
Summary
Documentation-only PR (plus one intra-doc link fix) that verifies the content rewriting pipeline is platform-agnostic. Core claims are accurate — html_processor, streaming_processor, streaming_replacer, and rsc_flight have zero fastly imports. Good separation of concerns that saves future adapter authors from implementing unnecessary abstractions.
Non-blocking
🤔 thinking
- Broken intra-doc link syntax in
html_processor.rs:12—`impl `[`StreamProcessor`]has an awkward space between backtick-wrappedimpland the link. Consider`impl` [`StreamProcessor`]or plain`impl StreamProcessor`. - Missing blank line before
useinhtml_processor.rs:17-18— The original file had a blank line between the doc block and imports. Lost in the edit; inconsistent withstreaming_processor.rswhich has the blank line.
♻️ refactor
publisher.rscaveat inplatform/mod.rs:31-37— The handler-coupling detail feels like it belongs inpublisher.rsitself rather than the platform module doc. Consider moving it there and keepingplatform/mod.rsfocused on what adapters need to know.
🌱 seedling
- PR number references in doc comments may age poorly — "PR 8", "PR 11", "PR 16/17" will be opaque without EdgeZero migration context. Consider more descriptive labels (e.g., "the HTTP-type migration" instead of "PR 11").
⛏ nitpick
StreamingPipeline::process<R: Read, W: Write>inplatform/mod.rs:27— Angle brackets are plain text, not a link or code. Should be wrapped in backticks or use an intra-doc link.
👍 praise
- Good call not introducing a
PlatformContentRewritertrait — the pipeline is already generic. - Nice catch fixing the unresolved
EdgeRequestdoc link.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- Fix intra-doc link syntax and restore missing blank line in `html_processor` - Replace opaque PR number references with descriptive context labels - Move HTTP-type coupling caveat from `platform` module down to `publisher.rs` - Convert `StreamingPipeline::process` plain-text generics to an intra-doc link
aram356
left a comment
There was a problem hiding this comment.
Summary
Documentation-only PR that verifies the content rewriting pipeline is platform-agnostic and documents this for future adapter authors. The doc comments are well-structured and the separation of concerns (platform-agnostic pipeline vs platform-coupled handler layer) is correctly captured.
Blocking
🔧 wrench
-
Unresolved intra-doc link:
StreamingPipeline::processinplatform/mod.rs:26does not resolve —StreamingPipelineis not in scope from theplatformmodule. This is a newcargo docwarning not present on the base branch. Needs the full path. -
Inconsistent PR number references:
streaming_processor.rsstill uses opaque "PR 8" (line 11) and "PR 16/17" (line 17) whilehtml_processor.rsandplatform/mod.rswere updated to descriptive labels in commit2333227. This file was missed.
Non-blocking
🤔 thinking
- 350-line plan document:
docs/superpowers/plans/2026-03-31-pr8-content-rewriting-verification.mdis an agentic implementation plan with task checkboxes and bash commands — 350 lines of process artifact for ~55 lines of doc comment changes. It also still contains opaque "PR 8"/"PR 11"/"PR 16/17" references. Is this pattern intended to be kept long-term, or should plans be cleaned up after the PR ships?
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
Summary
html_processor,streaming_processor,streaming_replacer,rsc_flight) has zero Fastly imports and requires noPlatformContentRewritertrait — future adapters (PR 16/17) need not implement any content-rewriting interface.platform/mod.rs,html_processor.rs, andstreaming_processor.rsso it is discoverable for Cloudflare, Axum, and Spin adapter authors.publisher.rshandler layer usesfastly::Body/Request/Responseat its function boundaries, but this is an HTTP-type coupling addressed in Phase 2 (PR 11), not a content-rewriting concern.Changes
crates/trusted-server-core/src/platform/mod.rsPlatformContentRewritertrait is neededcrates/trusted-server-core/src/html_processor.rs# Platform notessection confirming zero Fastly importscrates/trusted-server-core/src/streaming_processor.rs# Platform notessection confirmingStreamingPipeline::processis generic overR: Read + W: Writecrates/trusted-server-adapter-fastly/src/platform.rsEdgeRequest→edgezero_core::http::Request) caught bycargo docdocs/superpowers/plans/2026-03-31-pr8-content-rewriting-verification.mdCloses
Closes #489
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)