feat(sites): experimental deploy functions folder - #212
Conversation
|
@codex review |
🦋 Changeset detectedLatest commit: bf9d87c The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 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 adds experimental site functions, framework-specific build variables, SPA and custom-404 handling, function cleanup, and supporting configuration, documentation, and tests.
Confidence Score: 3/5The PR is not yet safe to merge because function source can become publicly deployable through symlinked path aliases, and an existing function-resource leak remains unresolved. The static uploader compares lexical paths, so deploying a project through a symlink can bypass the functions-directory exclusion and publish function source. The earlier “Created Scripts Can Leak” finding remains partially unfixed because Files Needing Attention: packages/cli/src/commands/sites/uploader.ts, packages/cli/src/commands/sites/functions.ts, packages/cli/src/commands/sites/deploy.ts
|
| Filename | Overview |
|---|---|
| packages/cli/src/commands/sites/deploy.ts | Coordinates function preparation and publication, static-file exclusion, SPA detection, deployment, and JSON output; existing review findings remain relevant to partial failures and incomplete function output. |
| packages/cli/src/commands/sites/functions.ts | Discovers, builds, creates, and publishes site functions; multi-function partial failures can still leave created resources unrecorded. |
| packages/cli/src/commands/sites/uploader.ts | Adds exclusion support for function source trees, but exact lexical path matching can be bypassed through a symlinked deploy path. |
| packages/cli/src/commands/sites/api.ts | Merges function state, applies per-deploy not-found settings, and deletes function resources with sites. |
| packages/cli/src/commands/sites/constants.ts | Adds function records, function URL/environment helpers, and per-deploy not-found modes. |
| packages/cli/src/commands/sites/ci/frameworks.ts | Adds SPA defaults and browser-visible environment prefixes to framework presets. |
| packages/config/src/schema.ts | Extends site configuration with SPA and functions-directory options. |
Sequence Diagram
sequenceDiagram
participant CLI
participant Compute as Edge Scripts API
participant Build
participant Storage
participant Site as Site Pull Zone
CLI->>Compute: Create missing function resources
Compute-->>CLI: Script IDs and hostnames
CLI->>Storage: Persist function records
CLI->>Build: "Run with BUNNY_FUNCTION_*_URL"
CLI->>CLI: Validate deploy directory and target
CLI->>Compute: Upload and publish function code
CLI->>Storage: Upload static deploy files
CLI->>Site: Promote deploy and configure SPA/404
CLI->>Storage: Persist current deploy state
Reviews (3): Last reviewed commit: "feat(sites): experimental deploy functio..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!record) { | ||
| step(`Creating function ${fn.name}...`); | ||
| record = await createFunctionScript(computeClient, state.name, fn); | ||
| } | ||
|
|
||
| const uploaded = force || record.codeHash !== codeHash; | ||
| if (uploaded) { | ||
| step(`Deploying function ${fn.name}...`); | ||
| await uploadScriptCode(computeClient, record.scriptId, code); | ||
| await publishScript(computeClient, record.scriptId); | ||
| record.codeHash = codeHash; | ||
| } | ||
| records[fn.name] = record; |
There was a problem hiding this comment.
A newly created script is not recorded until hostname lookup, upload, and publication all succeed, and it is persisted only after the entire function batch returns. If one of those steps or the state write fails, a retry sees no record and creates another script and linked pull zone. Concurrent deploys can also create the same missing function and retain only one record during state merging. Use idempotent creation or persist and clean up resources across these failure paths.
![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: 4399f1519a
ℹ️ 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".
| const functions = await discoverFunctions( | ||
| root, | ||
| siteConfig?.config.functions?.dir, | ||
| ); |
There was a problem hiding this comment.
When sites.dir is absent, a simple static site deploys the config root/current directory, which also contains the default functions/ directory discovered here. The later collectFiles(dir) recursively includes that non-dot directory, so handler source, package files, and other server-side implementation details are published as static files under /functions/...; exclude the configured functions tree whenever it falls inside the static deploy root.
Useful? React with 👍 / 👎.
| functionResults = await withSpinner("Deploying functions...", (spin) => | ||
| deployFunctions({ | ||
| computeClient, | ||
| state, | ||
| functions, | ||
| force: args.force ?? false, | ||
| onStep: (message) => { | ||
| spin.text = message; | ||
| }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
When a custom deploy ID conflicts with existing content, this publishes the functions before the replacement confirmation later in the handler. If the user declines, the command prints Cancelled. but the new function code is already live alongside the old frontend; invalid or empty static deploy directories similarly mutate production before failing. Perform all directory, content, and confirmation checks first, then publish functions immediately before the site is promoted.
Useful? React with 👍 / 👎.
| if (!record) { | ||
| step(`Creating function ${fn.name}...`); | ||
| record = await createFunctionScript(computeClient, state.name, fn); | ||
| } |
There was a problem hiding this comment.
If a new script is created but its linked hostname is still provisioning, or if upload, publish, or a later function fails, this call rejects before the record reaches state.functions and before the caller writes remote state. The suggested retry therefore has no script ID to resume and attempts another create, leaving the original script and linked pull zone orphaned or hitting a duplicate-name error; persist a recoverable record immediately after creation or delete the resource on failure.
Useful? React with 👍 / 👎.
| configuredSpa = await confirm( | ||
| "This looks like a single-page app. Serve index.html for client-side routes so deep links survive a refresh?", | ||
| { initial: true, optional: true }, | ||
| ); |
There was a problem hiding this comment.
When an undetected project looks like an SPA, this introduces an interactive choice without a corresponding --spa/--no-spa deploy option. Automated callers can only reproduce the choice by editing bunny.jsonc beforehand, contrary to the repository's requirement that every prompt have a flag equivalent; add paired flags and use them to suppress the prompt.
AGENTS.md reference: AGENTS.md:L273-L275
Useful? React with 👍 / 👎.
| export function functionScriptName(site: string, name: string): string { | ||
| return `sites-${site}-${name}`; | ||
| } |
There was a problem hiding this comment.
Both component names can pass their local validators while the generated name exceeds the API limit: a 47-character site plus a 63-character function produces a 117-character name, but AddEdgeScriptModel.Name is capped at 100 characters in packages/openapi-client/specs/compute.json. Such otherwise-valid projects fail only at script creation, so validate the combined length or generate a bounded deterministic name.
Useful? React with 👍 / 👎.
| ...state.deploys, | ||
| ...remote.deploys.filter((d) => !ours.has(d.id) && !removed.has(d.id)), | ||
| ].sort((a, b) => b.createdAt.localeCompare(a.createdAt)); | ||
| state.functions = { ...remote.functions, ...state.functions }; |
There was a problem hiding this comment.
When two deploys update the same function concurrently, the last script publish and the last state write can occur in different orders. This merge always lets the local function record overwrite the concurrent record, so state can store hash A while hash B is actually live; a later deploy of A then sees a matching hash and skips the upload indefinitely. Detect same-name function conflicts or reconcile against the live deployment before retaining the local hash.
Useful? React with 👍 / 👎.
| const pkg = await readPackageJson(path); | ||
| const scripts = pkg?.scripts as Record<string, string> | undefined; | ||
| if (scripts?.build) { | ||
| const pm = await detectPackageManager(path); | ||
| functions.push({ | ||
| name, | ||
| dir: path, | ||
| folder: true, | ||
| build: `${pm} run build`, | ||
| }); |
There was a problem hiding this comment.
When a static or non-JavaScript site contains a folder function whose package.json build script depends on local devDependencies, the generated sites ci init workflow emits no JavaScript setup or dependency-install step because it only considers the site's framework and sites.build. The deploy action then reaches this automatic npm run build on a fresh runner without the function package's dependencies, so deployments that work locally fail in CI; make workflow generation inspect function packages and install with their detected package manager.
Useful? React with 👍 / 👎.
| for (const name of staleFunctions(state, functions)) { | ||
| logger.warn( | ||
| `Function "${name}" is deployed but has no folder here; it keeps serving until its script (${state.functions?.[name]?.scriptId}) is deleted.`, | ||
| ); |
There was a problem hiding this comment.
When a function folder is removed, this warning tells the user to delete its script, but doing so does not remove state.functions[name]. Subsequent deploys therefore keep the dead origin rule because convergence still sees the record, and restoring the folder cannot recover because deployFunctions() tries to reuse the deleted script ID; remove the state entry and converge routing as part of a supported function-removal flow rather than directing users to delete only the script.
AGENTS.md reference: AGENTS.md:L471-L471
Useful? React with 👍 / 👎.
| let record = records[fn.name]; | ||
| const created = !record; | ||
| if (!record) { | ||
| step(`Creating function ${fn.name}...`); | ||
| record = await createFunctionScript(computeClient, state.name, fn); |
There was a problem hiding this comment.
The valid function name constructor collides with the inherited Object.prototype.constructor property on the ordinary {} used for state.functions. For that name, records[fn.name] is truthy even on a fresh site, so creation is skipped and the CLI attempts to upload to an undefined script ID; use an own-property check, a null-prototype object, or a Map for record lookup.
Useful? React with 👍 / 👎.
| const functionsJson = functionResults.map((r) => ({ | ||
| name: r.name, | ||
| url: r.url, | ||
| env: functionEnvName(r.name), | ||
| scriptId: r.scriptId, | ||
| created: r.created, | ||
| uploaded: r.uploaded, | ||
| })); |
There was a problem hiding this comment.
sites deploy --output json builds this array only from functions found in the current checkout. Removed local folders intentionally leave their remote functions serving, so the output omits those live functions and their URLs. Automation using this documented output can therefore receive an incomplete list.
348f100 to
1538fc8
Compare
| for (const entry of readdirSync(abs, { withFileTypes: true })) { | ||
| if (shouldSkipEntry(entry.name)) continue; | ||
| const entryAbs = join(abs, entry.name); | ||
| if (exclude.has(entryAbs)) continue; |
There was a problem hiding this comment.
If the deploy directory is accessed through a symlink alias, the functions directory and the walked entry can have different path strings even though they refer to the same directory. This exact-path check then misses the exclusion and uploads server-side function source files as public site content. Canonicalize both paths before comparing them.
1538fc8 to
bf9d87c
Compare
2 participants
No description provided.