feat(sites): serve sites with edge rules instead of the router - #200
Conversation
🦋 Changeset detectedLatest commit: 519e2d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Greptile SummaryThis PR replaces each site's router Edge Script with convergently managed pull-zone edge rules and adds caller-supplied deployment identifiers.
Confidence Score: 4/5The PR is not yet safe to merge because ordinary static-site root and directory routes can resolve to nonexistent storage objects instead of their The edge rewrite appends the incoming path directly beneath the selected deployment directory, while uploads preserve Files Needing Attention: packages/cli/src/commands/sites/api.ts
|
| Filename | Overview |
|---|---|
| packages/cli/src/commands/sites/api.ts | Replaces router-backed serving and promotion with pull-zone edge rules; the previously reported loss of directory-index resolution remains. |
| packages/cli/src/commands/sites/deploy.ts | Adds custom deployment-ID handling, replacement safeguards, upload orchestration, and promotion-state persistence. |
| packages/cli/src/core/edge-rules.ts | Introduces shared typed helpers for fetching, upserting, and deleting pull-zone edge rules. |
| packages/cli/src/commands/sites/constants.ts | Defines version-two site state and stable descriptions and identifiers for the managed edge rules. |
| packages/cli/src/commands/sites/provision.ts | Updates site provisioning to create storage and pull-zone resources without a router Edge Script. |
Reviews (3): Last reviewed commit: "refactor(sites): drop router-era state c..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Enabled: true, | ||
| ActionType: EdgeRuleAction.OriginUrl, | ||
| // The origin is the zone's own hostname: the request re-enters the CDN, where the gate rule admits it by the hop header. | ||
| ActionParameter1: `https://${systemHost}/${deployPrefix(deployId)}%{Url.Path}`, |
There was a problem hiding this comment.
When a browser requests /, /blog/, or a slashless directory such as /blog, this rewrite appends the request path directly to deploys/<id> without the removed router's index.html expansion and redirect handling, causing valid static-site routes to return storage 404 responses.
Context Used: AGENTS.md (source)
Knowledge Base Used: CLI automation workflows
There was a problem hiding this comment.
I'm not sure this is true. This was old behaviour. However, slashless /blog serves 200 instead of the old 301 to /blog/.
There was a problem hiding this comment.
You're right that the old 301 behavior is not itself required, and my comment overstated that part. The remaining concern is directory-index resolution: at packages/cli/src/commands/sites/api.ts:313, the rewrite maps /blog to /deploys/<id>/blog and /blog/ to /deploys/<id>/blog/; it does not append index.html. If /blog is a real uploaded object or the storage zone is configured with a fallback, a 200 is expected. But for the usual static-site layout containing only blog/index.html, Bunny Storage does not implicitly resolve either path to that file, so the new rule would return 404 rather than the router's index handling (and redirect for the slashless case). So I agree the specific “must 301” claim should be removed, but the loss of directory-index handling remains a compatibility gap unless the intended contract is that sites must upload route objects/fallback configuration themselves.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0b3b2bb9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ActionType: EdgeRuleAction.OriginUrl, | ||
| // The origin is the zone's own hostname: the request re-enters the CDN, where the gate rule admits it by the hop header. | ||
| ActionParameter1: `https://${systemHost}/${deployPrefix(deployId)}%{Url.Path}`, |
There was a problem hiding this comment.
Requests for a conventional static build's / or /docs/ remain directory paths here: %{Url.Path} appends the path verbatim, so the origin lookup becomes deploys/<id>/ rather than deploys/<id>/index.html. Bunny storage does not resolve directory indexes—the removed router explicitly performed that expansion—so newly deployed sites return a storage 404 at their home URL even though index.html was uploaded; the propagation probe also ignores the status and accepts the response header, allowing the deploy to report success. Restore index expansion instead of forwarding slash paths unchanged.
AGENTS.md reference: AGENTS.md:L468-L468
Useful? React with 👍 / 👎.
| if (existing && existing.contentHash !== identity.contentHash) { | ||
| // Drop the record before deleting its files, so no record ever vouches for a prefix mid-rewrite (a concurrent publish re-reads state and refuses an ID without one), and a crashed replacement re-runs as a fresh upload. | ||
| state.deploys = state.deploys.filter((d) => d.id !== deployId); | ||
| etag = await writeRemoteState(connection, state, etag, { |
There was a problem hiding this comment.
When replacing an existing custom ID with different content, the live/rollback checks occurred earlier against the initially read state, but after the confirmation this block deletes using that stale state. If another command publishes this ID during the prompt or upload preparation, writeRemoteState adopts the concurrent current pointer while removing the record, and deleteDeployFiles then empties the now-live prefix. Re-read the state and reject a current or previous ID immediately before removing its record and files, as the deployment delete and publish commands do.
Useful? React with 👍 / 👎.
| // Generate an auth token (interactive default; --no-token opts out) | ||
| const tokenArg = args[ARG_TOKEN]; | ||
| let shouldCreateToken: boolean; | ||
| if (tokenArg !== undefined) { | ||
| shouldCreateToken = tokenArg; | ||
| } else if (isInteractive) { | ||
| shouldCreateToken = await confirm("Create an auth token?", { | ||
| force: false, | ||
| optional: true, | ||
| }); | ||
| } else { | ||
| shouldCreateToken = false; | ||
| } | ||
| const shouldCreateToken = tokenArg ?? isInteractive; |
There was a problem hiding this comment.
isInteractive is only output !== "json", so a default-text bunny db create run with redirected or non-TTY stdin now takes this branch even though no --token was supplied. It silently creates a permanent full-access credential and later prints the raw token to stdout, allowing automation to leak it into logs; even a TTY user no longer explicitly asked to reveal this secret. Gate the default on the real TTY-aware interactivity helper and retain --token as the non-interactive opt-in.
AGENTS.md reference: AGENTS.md:L172-L172
Useful? React with 👍 / 👎.
| const customId = args["deploy-id"]?.trim(); | ||
| if (customId) { | ||
| const problem = deployIdError(customId); |
There was a problem hiding this comment.
An explicitly supplied value is trimmed and then guarded by truthiness, so in CI --deploy-id "$RELEASE_ID" with an unset or whitespace-only variable silently falls back to the git/content-derived ID and publishes successfully. Downstream catalog or rollback operations then refer to the requested release ID that was never created. Distinguish an omitted option from a supplied empty value and reject the latter through the normal deploy-ID validation.
Useful? React with 👍 / 👎.
3 participants
Sites are now served by pull zone edge rules instead of a per-site router Edge Script, and
sites deploy --deploy-idcarries your own release identifier.What changed
ROUTER_VERSION, andsites upgrade-routerare gone.https://<host>/deploys/<id>%{Url.Path}) that stamps a per-site secret request header and anX-Bunny-Deployresponse header; a gate that 403s/deploys/*unless the secret is present (so deploy dirs are reachable only via the CDN's own hop); a_bunny/*block; and two browser-cache rules for asset extensions (the API caps a condition at 5 patterns).X-Bunny-Deployheader instead of the old 404 heuristic, with the settle floor raised to cover the ~6s config-sync bucket.public, max-age=0(browsers pick up new deploys immediately, the goal that started this); assets get a 1-day browser cache; the edge caches everything and is purged on publish._placeholderpath until the first publish). A fresh storage zone can briefly refuse writes, so the create's first storage write (the state file) retries briefly.--deploy-id(relaxed charset, case-sensitive, never aliases onto matching content), replace-with-confirmation for reused IDs (clearing old files first), live/rollback deploys never replaceable in place, case-variant refusal, publish revalidating fresh state before promoting. The validator's leading-alphanumeric rule keeps the_placeholdersentinel unreachable.core/edge-rules.ts(typed action/trigger constants, fetch/upsert/delete) for futurecdn/pullzonecommands.deployanddeployments publishrefuse them with a delete-and-recreate hint, whilelist/show/delete(including script cleanup) still work. State is version 2 (scriptIdoptional, versions above 2 rejected).