Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 580c8ee The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| test("Artifacts defaults to local and only an explicit connection is remote", ({ | ||
| expect, | ||
| }) => { | ||
| const remoteProxyConnectionString = new URL( | ||
| "http://127.0.0.1:8787" | ||
| ) as RemoteProxyConnectionString; | ||
| const converted = convertV4MiniflareOptions({ | ||
| name: "worker", | ||
| script: "export default {};", | ||
| artifacts: { | ||
| LOCAL: { namespace: "local" }, | ||
| REMOTE: { namespace: "remote", remoteProxyConnectionString }, | ||
| }, | ||
| }); | ||
| expect(converted.workers[0].config.env?.LOCAL).toMatchObject({ | ||
| dev: { remote: false }, | ||
| }); | ||
| expect(converted.workers[0].config.env?.REMOTE).toMatchObject({ | ||
| dev: { remote: true }, | ||
| }); |
There was a problem hiding this comment.
🔍 Legacy Artifacts settings select local storage
The v4 converter sets dev.remote: false without an explicit opt-in, selecting the local simulator. That differs from the stated intermediate-stage default for omitted settings.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed at 00bd9ad: legacy conversion preserves omitted dev.remote instead of synthesizing false. This intermediate layer remains remote by default; local still requires explicit V5 dev.remote: false. The local-default switch is reserved for 3/3.
| if (sourceUrl.protocol !== "https:") { | ||
| throw new ArtifactsError( | ||
| "INVALID_INPUT", | ||
| "Invalid source.url: must be an HTTPS URL." | ||
| ); | ||
| } | ||
| return { source, target, sourceUrl }; |
There was a problem hiding this comment.
🟥 Git import reaches private host networks
A Worker can pass an internal HTTPS URL to import. Host Git connects outside workerd's network restrictions, allowing the Worker to reach private services.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is a deliberate local-only boundary: an explicit HTTPS import contacts its specified host from the developer machine, which may include a private-network host. Import redirects are disabled, and the internal README now warns against untrusted URLs. We are not claiming workerd network isolation for host Git; a broader private-destination policy needs an owner decision.
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/runtime-types
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
925153c to
5902a96
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
93a4ff3 to
9e58655
Compare
9e58655 to
ee4756f
Compare
|
Hey folks, here’s a small example if you want to try the local Artifacts simulator quickly: https://github.com/leostera/workers-sdk-playground It uses the Wrangler preview from the stacked 3/3 PR with an Artifacts binding set to |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
| if (!this.#pendingRoots) { | ||
| throw new Error("Artifacts backend requested outside runtime update"); | ||
| } | ||
| this.#pendingRoots.add(root); |
There was a problem hiding this comment.
Nit: should we normalize the path before caching the backend? Relative and absolute paths to the same folder currently start separate backends, and the second one can delete temporary files from an import still running in the first.
|
Addressed the requested PR 2/3 changes in |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
💬 Approved with comments — The opt-in simulator is well structured, stays behind explicit Findings: 2 suggestions The decision is advisory: this comment never approves, blocks or requests changes on the pull request. 🔧 Fix with your agentPaste the following into your coding agent to investigate and fix these issues. 📋 Code Quality: 💬 2 suggestionsWell structured and heavily tested; two small cleanup points in the sidecar shutdown and Git process handling. Recommendation: 💬 Optional Nits
View full reviewFindings
Checked, no issue
✅ All other checks passed🔒 Security: ✅ LGTMThe Git sidecar is the main new trust boundary. These points held up:
HTTPS import can contact private-network hosts. The pull request description says so, and the simulator is a local, opt-in development tool. The security review covered the sidecar, the Git client and the controller. It did not cover 📖 AGENTS.md: ✅ LGTMNo AGENTS.md changes are needed for this pull request. ⚡ Performance: ✅ LGTMNo measurable performance regressions:
📝 Documentation: ✅ LGTMThe internal README and the changeset describe the opt-in behaviour accurately. Reviewed commit: |
3 participants
Artifacts local-development stack — 2 of 3
Part 2/3; the native Git client (#16176) is merged. This branch is rebased onto upstream
main, so its PR diff shows only the opt-in simulator layer. The development-tools integration is PR #16186 (3/3) (incremental 2/3 → 3/3 diff); merge this PR first, then restack 3/3 ontomainand revalidate its exact head. #16092 is a closed full-stack reference, not another PR to merge.What changes in this layer
dev.remote: false. An omitted setting retains existing remote behavior; the final development-tools layer changes the default.The simulator builds on the original prototype by @GregBrimble. Focused local tests establish tested behavior, not parity with the deployed service. The unused live-comparison harness was removed from this PR; no deployed comparison has been run. A future private comparison needs owner authorization for an exclusively reserved disposable namespace. Explicit HTTPS imports can contact the supplied host, including private-network destinations, and do not follow HTTP redirects.
Validation
pnpm -r --filter miniflare... buildpnpm --filter miniflare exec vitest run test/plugins/artifacts test/config/v4-convert.spec.ts— 109 passed, 1 platform-specific test skipped on macOS (including inherited Git-client tests)