Fix/response clone#141
Closed
godronus wants to merge 15 commits into
Closed
Conversation
Feature/purge upgrade
Feature/purge upgrade
Bump the StarlingMonkey submodule from 0.3.0 (9dda8ba) to the tip of godronus/gcore/integration (84f5d52), which carries two pending upstream patches on top of 0.3.0: - feat(fetch): implement Response.clone() (PR #312) - fix(fetch): body.blob() sets Blob.type from Content-Type header (issue #311) Add context/PATCHES.md documenting the applied patches, their upstream PR/issue links, and the rebase procedure for future StarlingMonkey bumps. Update CONTEXT_INDEX.md and CLAUDE.md to make the file discoverable.
…on tests Implement Response.clone() in the StarlingMonkey runtime and add blob.type propagation from Content-Type headers. Both fixes are applied via the godronus/gcore/integration fork branch (SHA 84f5d52) pending upstream review of PR #312 and issue #311. Expose Response.clone() in types/globals.d.ts (instance method, previously commented out). Update KNOWN_LIMITATIONS.md to reflect the implementation. Restructure integration-tests/test-application from a single flat JS file into a typed, extensible architecture: - Convert to TypeScript with Hono routing - Split into handlers/ (WASM context) and checks/ (Node.js, auto-discovered) - routes.ts as single source of truth for route paths and test names - Build artefacts consolidated under dist/ (one .gitignore entry) - Add temporary Response.clone() prod-invocation guard — remove when PR #312 merges upstream and submodule is rebased Add pnpm test:app:build and test:app:check scripts for local development. Add context/PATCHES.md documenting the integration branch, rebase procedure, and test guard removal checklist.
…k runner Add tsconfig.json for the test application so fastedge-build resolves SDK types directly from ../../types rather than the missing installed package. Pass --tsconfig in build-test-app.js and create-test-app.js. Fix run-test-app-checks.js: remove --bundle=false (incompatible with --external), switch to --bundle so relative imports (routes.ts) are inlined into each check file, and fix ESLint errors (regex u flag, await-in-loop, no-plusplus, catch param naming, dynamic import comment).
…ation tests Add four test scenarios covering the full clone surface: - Constructed response text() on both branches (basic correctness) - Constructed response getReader() + mutation guard: drain original, fill(0) all chunks, verify clone reads independently - Incoming fetch() text() on both branches (HttpIncomingBody materialisation) - Incoming fetch() getReader() + mutation guard: the precise original bug scenario — host-backed body, both branches via getReader() Tests 2 and 4 currently fail: the tee implementation enqueues the same Uint8Array reference to both branches rather than cloning each chunk, so fill(0) on the original's chunks corrupts the clone's queued data. This is a separate bug from the materialisation fix in PR #312 and blocks release. Update KNOWN_LIMITATIONS.md and CONTEXT_INDEX.md to document the tee chunk independence bug and mark Response.clone() as partially implemented.
…core/integration)
…-then-clone guards
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the FastEdge SDK’s Fetch API type surface to include Response.clone(), and refactors/extends the production-invocation test application so runtime regressions (notably around Response.clone() behavior) can be validated against a live deployed app.
Changes:
- Added
Response.clone(): Responseto the publictypes/globals.d.tsdeclarations. - Reworked the prod-invocation test app into a TypeScript + Hono structure with auto-discovered Node-based checks and new local runner scripts.
- Updated CI workflow/scripts and repo context docs to reflect the new prod-invocation test harness and StarlingMonkey patch tracking.
Reviewed changes
Copilot reviewed 31 out of 35 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| types/globals.d.ts | Declares Response.clone() in the public Fetch API type surface. |
| scripts/run-test-app-checks.js | Local script to compile and run all prod-invocation check modules against a deployed URL. |
| scripts/build-test-app.js | Local script to build the prod-invocation TypeScript app into a WASM binary via fastedge-build. |
| runtime/fastedge/build.sh | Adjusts StarlingMonkey runtime build parallelism behavior. |
| pnpm-lock.yaml | Locks new dependency set including hono. |
| package.json | Adds hono devDependency and new test:app:* scripts. |
| integration-tests/test-application/types.ts | Adds shared interfaces for handler/check modules. |
| integration-tests/test-application/tsconfig.json | Adds TS config for the prod-invocation test app + typechecking against SDK types. |
| integration-tests/test-application/test-app.ts | New Hono-based entrypoint that registers handler modules. |
| integration-tests/test-application/test-app.js | Removes the previous flat JS test app implementation. |
| integration-tests/test-application/routes.ts | Centralizes route paths and human-readable test names. |
| integration-tests/test-application/README.md | Documents the prod-invocation test app structure, usage, and temporary guard policy. |
| integration-tests/test-application/handlers/secret.ts | Handler to validate secret injection. |
| integration-tests/test-application/handlers/response-clone.ts | Temporary guard handler exercising multiple Response.clone() scenarios. |
| integration-tests/test-application/handlers/outbound-fetch.ts | Handler to validate outbound fetch behavior. |
| integration-tests/test-application/handlers/multi-chunk-source.ts | Helper route serving multi-chunk streaming data for clone/tee validation. |
| integration-tests/test-application/handlers/env.ts | Handler to validate env var access (BUILD_SHA). |
| integration-tests/test-application/handlers/echo.ts | Handler to echo request method/headers/body. |
| integration-tests/test-application/checks/secret.ts | Node check for the secret handler. |
| integration-tests/test-application/checks/response-clone.ts | Node check validating the Response.clone guard response assertions. |
| integration-tests/test-application/checks/outbound-fetch.ts | Node check for outbound fetch behavior. |
| integration-tests/test-application/checks/env.ts | Node check verifying BUILD_SHA matches the expected SHA. |
| integration-tests/test-application/checks/echo.ts | Node check validating request echo behavior. |
| integration-tests/test-application/.gitkeep | Keeps the directory present in git. |
| context/PATCHES.md | Documents applied StarlingMonkey patches and rebase/retirement procedure. |
| context/KNOWN_LIMITATIONS.md | Adds/updates confirmed runtime limitations, including Response.clone() status. |
| context/development/TESTING_GUIDE.md | Updates testing guide with prod-invocation layer and commands. |
| context/CONTEXT_INDEX.md | Adds references and decision-tree pointers for new context docs and workflows. |
| context/CHANGELOG.md | Records the Response.clone()/blob.type work and new test harness infrastructure. |
| CLAUDE.md | Adds pointer to PATCHES.md for StarlingMonkey submodule bump workflow. |
| .gitignore | Updates ignore rules to ignore the new prod-invocation dist directory. |
| .github/workflows/prod-invocation.yaml | Updates workflow paths/env vars to use dist outputs and new test URL. |
| .github/scripts/prod-invocation/invoke-test-app.js | Invokes auto-discovered compiled check modules instead of hardcoded checks. |
| .github/scripts/prod-invocation/create-test-app.js | Builds TS test app to WASM and compiles check modules to dist for Node import. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
.github/scripts/prod-invocation/invoke-test-app.js:44
- In a
catch (error)block,errorisunknownand not guaranteed to have a.messageproperty. As written, a non-Error throw would cause a secondary exception while reporting, masking the real failure.
} catch (error) {
core.warning(`Attempt ${attempt} failed: ${error.message}`);
if (attempt === MAX_RETRIES) {
throw new Error(
`Test application invocation failed after ${MAX_RETRIES} attempts: ${error.message}`,
);
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.