feat(site-deploy): deploy a static build as an immutable release - #1776
feat(site-deploy): deploy a static build as an immutable release#1776pyramation wants to merge 2 commits into
Conversation
Adds @constructive-io/site-deploy: walk/hash a build, upload only the bytes the server does not already have, commit one manifest, and optionally publish or point a preview ref at the resulting commit. Builds on @constructive-io/upload-client for the generic presigned PUT; the site-specific half (CAS keys, manifest, release row, publish/preview) lives here so the UI and CI do not each reimplement it.
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Review complete. 🟡 4 medium 💬 Inline comments (4)
🧹 Nitpicks (2) — 🟢 2 low
This PR introduces the
The main risks are edge-case handling: Reviewed commit: 909e0a6 |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a new site-deploy package that walks a static build directory, content-addresses files, uploads them to a CAS bucket, and publishes a manifest/release via GraphQL, with several edge-case correctness and security gaps.
Key findings
- 🟡 skipIfUnchanged silently skips requested publish/preview — deploy.ts:112
- 🟡 Symlink-following reads arbitrary files into deploy — walk.ts:39
- 🟡 dryRun still performs a server-side write — deploy.ts:140
- 🟡 Empty (0-byte) files fail the whole deploy — manifest.ts:104
| ) { | ||
| onProgress?.({ type: 'unchanged', commitId: existing.commitId }); | ||
| return { | ||
| commitId: existing.commitId, | ||
| storeId: existing.storeId, | ||
| releaseId: existing.id, | ||
| manifest, | ||
| files: manifest.file_count, | ||
| uploaded: 0, | ||
| skipped: manifest.file_count, | ||
| bytesUploaded: 0, | ||
| published: false, | ||
| previewUrl: null, | ||
| unchanged: true, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟡 bug · medium
skipIfUnchanged silently skips requested publish/preview
When skipIfUnchanged matches an existing manifest, deploySite returns early at deploy.ts:112 with published: false and previewUrl: null, never executing the publish or preview pointer moves even when those options were set. A caller that runs deploy({ publish: true, skipIfUnchanged: true }) on an unchanged tree receives a success result while site.activeCommitId is never moved, leaving the site unpublished despite an explicit go-live request.
📋 Prompt for AI Agents
In packages/site-deploy/src/deploy.ts lines 112-127, the skipIfUnchanged early-return bypasses the publish and preview pointer moves. After the unchanged check returns the existing release, if options.publish is true still call publishCommit(api, names, siteId, existing.commitId) and set published: true; if options.preview !== undefined still call pointPreview(...) and set previewUrl. Do not unconditionally hardcode published: false/previewUrl: null in this branch, because a caller requesting publish must not get a silent no-op go-live.
| // Symlinks to files are followed by readFile; anything that is neither a | ||
| // file nor a directory (socket, fifo) has no meaning in a static site. | ||
| if (!entry.isFile() && !entry.isSymbolicLink()) continue; | ||
| yield { path: logical, bytes: new Uint8Array(await readFile(child)) }; |
There was a problem hiding this comment.
🟡 security · medium
Symlink-following reads arbitrary files into deploy
walkDirectory treats any symlink as a file and reads it with readFile (packages/site-deploy/src/walk.ts:39), so a symlink inside the build tree that points outside the directory (e.g. .env or source) is read and uploaded to the site's public bucket. A build influenced by a compromised dependency or untrusted artifact can therefore exfiltrate arbitrary local files into a public release.
📋 Prompt for AI Agents
In packages/site-deploy/src/walk.ts around lines 36-39, stop following symlinks that escape the build directory. Resolve each entry with fs.realpath and verify the resolved path stays within realpath(dir) before reading; skip (or throw a DeployError) for any symlink whose target resolves outside the root. This prevents a malicious build from reading arbitrary local files into the deployed site. Update the comment on lines 36-37 to reflect the new behavior.
| const diff = await diffCas(files, uploadOptions); | ||
| onProgress?.({ | ||
| type: 'diffed', | ||
| files: manifest.file_count, | ||
| toUpload: diff.missing.length, | ||
| skipped: diff.deduplicated.length, | ||
| bytesToUpload: diff.missing.reduce((sum, entry) => sum + entry.file.size, 0), | ||
| }); |
There was a problem hiding this comment.
🟡 bug · medium
dryRun still performs a server-side write
The dryRun path in deploy.ts calls diffCas (line 140) before returning at line 147, and diffCas executes the uploadFiles mutation, which the server documents as creating file rows (graphile/graphile-presigned-url-plugin/src/plugin.ts:346). A run documented as 'hash and diff only' therefore still writes rows on the server, leaving orphaned file rows behind when the bytes are never PUT.
📋 Prompt for AI Agents
In packages/site-deploy/src/deploy.ts, the dryRun branch (around lines 140-147) still calls diffCas, which runs the uploadFiles mutation that creates file rows server-side (see graphile/graphile-presigned-url-plugin/src/plugin.ts:346). Make dryRun side-effect-free: when dryRun is true, skip the diffCas mutation (or derive the diff from a read-only mechanism) so no server write occurs, then return the hashed/diffed result as before. Update the dryRun test to assert no uploadFiles call is made.
| entries[file.path] = { | ||
| hash: file.hash, | ||
| content_type: file.contentType, | ||
| size: file.size, | ||
| }; |
There was a problem hiding this comment.
🟡 bug · medium
Empty (0-byte) files fail the whole deploy
buildManifest accepts a 0-byte file and emits a manifest entry with size: 0, but the server's processSingleFile rejects size <= 0 with INVALID_FILE_SIZE (graphile/graphile-presigned-url-plugin/src/plugin.ts:613), so any empty file in a build aborts the entire deploy via diffCas. A static build can legitimately ship an empty placeholder (e.g. .gitkeep), and the client gives no signal that it is unsupported.
📋 Prompt for AI Agents
In packages/site-deploy/src/manifest.ts inside buildManifest (around lines 100-115), before writing entries[file.path], check if (file.size <= 0) and throw new DeployError('INVALID_PATH', ...) with a message explaining that empty (0-byte) files are rejected by the server's upload surface, so the deploy fails early and clearly rather than surfacing the server's INVALID_FILE_SIZE from diffCas.
1 participant
Summary
New package
@constructive-io/site-deploy: the client half of a static deploy, which the UI and CI would otherwise each reimplement. The server already owns everything below the manifest (merkle commits, refs, time travel, CAS resolution at the edge); what nothing shipped was the four mechanical steps in front of it — hash the build, upload the missing bytes, write one manifest, point something at the commit.The layering follows the generic/specific split:
@constructive-io/upload-clientstays generic and knows nothing about sites (it contributes the presigned PUT); the site-specific parts —cas/sha256/<hash>keys, the manifest, the release row, publish/preview — live here, so the dependency only points one way.Two behaviours are the reason this is a library rather than a snippet:
re-uploads only the changed file).DeployError.code, so an upload failure can never become a published manifest with holes in it, and a failed publish leaves a complete, retryable release + commit.Also non-obvious from the diff:
UNIQUE (site_id), so the first deploy mustcreateSiteReleaseand every later oneupdateSiteRelease— resolved internally by querying for the row, rather than making callers know which they need.databaseIdis therefore only required for a site's first deploy.cas/sha256/<hash>that was asked for; the server does not enforce that form, and a mismatch would publish a manifest pointing at bytes the gateway cannot resolve. Checked before any PUT.commitId/storeIdis a hard failure (RELEASE_NOT_VERSIONED), not a result with empty strings: without a commit it cannot be published, rolled back to, or previewed.deployNames('platform')→uploadPlatformFiles,createPlatformSiteRelease, …) since the surface is generated per plane; preview procedures are unprefixed today and individually overridable...and anything undercas/instead of dropping it, and dotfiles are included (.well-known/…is a real route) — a file skipped by an invisible rule is a silently broken site.Publishing and rollback are the same operation, exported for the rollback direction:
Testing
44 unit tests, no database and no network: a mock server models the two server behaviours the pipeline depends on — dedupe, and the trigger stamping a fresh commit on every manifest write — plus an injectable
putObject. Covered: first-deploy create vs. later-deploy update, dedupe skip, single-file change, batch chunking, key mismatch failing before upload, partial-upload failure leaving no manifest, retry of a transient 5xx, publish failure leaving the release usable, preview ref/hostname,skipIfUnchanged,dryRun, abort, progress events, and a real directory walk.Live-DB proof of deploy → publish → rollback stays in constructive-db, where it already exists (
site-releases-merkle-timetravel,static-release.e2e).Context: constructive-planning#1886 §7.
Link to Devin session: https://app.devin.ai/sessions/ecf6201520ef4ac984f4dd610ceea5b3
Requested by: @pyramation