Add new deployment flow (direct)#55
Conversation
WalkthroughAdds a backend-version–gated direct-deployment flow (min backend 2.0.6) with per-file SHA1+size computation, expanded exclusions, parallel bounded-concurrency direct uploads (with zip fallback on 404), centralizes exclusions, extends tool registration context with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 3: Update the "version" field in package.json (the version key currently
set to "1.2.10-dev.3") to the next minor prerelease line for this new feature —
e.g., "1.3.0-dev.1" (or increment N if you already have prior 1.3.0-dev
releases) so the package uses the correct X.Y.Z-dev.N prerelease track for a
minor release.
In `@src/shared/tools/deployment.ts`:
- Around line 235-240: The code is erroneously passing successful upload
responses (e.g., 204 No Content) into handleApiResponse(), which always calls
response.json() and will throw on empty bodies; update the Direct Deployment
path around the response handling in deployment.ts so that after you check for
response.status === 404 and throw DirectDeploymentUnsupportedError(), you
short-circuit for successful statuses (e.g., if response.ok or specifically
response.status === 204) and return/continue without calling
handleApiResponse(); only call handleApiResponse(response) for non-success
responses that require JSON error parsing.
- Around line 610-613: The zip command uses root-only -x patterns and will
include nested files like apps/*/.env and packages/*/node_modules; update the
zipping step in the block that builds the command using escapedDir/tmpZip so it
excludes nested paths by either switching to recursive exclude patterns (e.g.
use "**/.env*" or "*/node_modules/*"/"**/dist/**") or, preferably, reuse the
existing JS filter shouldExcludeDeploymentPath() to produce the exact exclude
list or to pre-filter files before zipping (i.e., build the archive from a
filtered file list rather than relying on root-level -x patterns); locate the
zip invocation that references escapedDir and tmpZip and replace the patterns or
wiring to shouldExcludeDeploymentPath() accordingly.
In `@src/shared/tools/index.ts`:
- Around line 175-177: The catch block that throws new Error("Cannot initialize
tools: backend at ... unreachable") should stop treating all failures as
"unreachable"—inspect the original error from
fetchBackendVersion()/health-check: if it's an HTTP response with status >=500
include a message like "backend returned HTTP 5xx (status ...)" and attach the
status/code; if it's a parse/validation error for the health payload use
"malformed health payload" and include the parsing/validation error; if it's a
network/fetch failure keep "unreachable" or "network error" and include the
underlying error; preserve the cause property when rethrowing and update the
thrown message in the throw new Error(...) in src/shared/tools/index.ts to
reflect these cases and include the original error details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecc179f2-137a-4919-9430-5d6e801a4209
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/shared/tools/deployment.tssrc/shared/tools/index.tssrc/shared/tools/types.ts
| if (response.status === 404) { | ||
| throw new DirectDeploymentUnsupportedError(); | ||
| } | ||
|
|
||
| await handleApiResponse(response); | ||
| } |
There was a problem hiding this comment.
Don’t run successful file uploads through handleApiResponse().
handleApiResponse() always calls response.json() (src/shared/response-handler.ts:27-46). If the upload endpoint returns a valid empty success body like 204 No Content, the local direct-deploy path will throw after the file was already uploaded.
Possible fix
if (response.status === 404) {
throw new DirectDeploymentUnsupportedError();
}
- await handleApiResponse(response);
+ if (!response.ok) {
+ await handleApiResponse(response);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (response.status === 404) { | |
| throw new DirectDeploymentUnsupportedError(); | |
| } | |
| await handleApiResponse(response); | |
| } | |
| if (response.status === 404) { | |
| throw new DirectDeploymentUnsupportedError(); | |
| } | |
| if (!response.ok) { | |
| await handleApiResponse(response); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/tools/deployment.ts` around lines 235 - 240, The code is
erroneously passing successful upload responses (e.g., 204 No Content) into
handleApiResponse(), which always calls response.json() and will throw on empty
bodies; update the Direct Deployment path around the response handling in
deployment.ts so that after you check for response.status === 404 and throw
DirectDeploymentUnsupportedError(), you short-circuit for successful statuses
(e.g., if response.ok or specifically response.status === 204) and
return/continue without calling handleApiResponse(); only call
handleApiResponse(response) for non-success responses that require JSON error
parsing.
| ## Step 1: Zip the source directory | ||
| \`\`\`bash | ||
| cd ${escapedDir} && zip -r ${tmpZip} . \ | ||
| -x "node_modules/*" ".git/*" ".next/*" ".env" ".env.local" "dist/*" "build/*" ".DS_Store" "*.log" | ||
| -x "node_modules/*" ".git/*" ".next/*" ".env*" "dist/*" "build/*" ".insforge/*" ".DS_Store" "*.log" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On common Info-ZIP builds, does zip -r archive.zip . -x ".env*" "node_modules/" "dist/" "build/" ".insforge/"exclude nested paths likeapps/web/.env.local, packages/foo/node_modules/react/index.js, and services/api/dist/server.js?
💡 Result:
No, on common Info-ZIP builds, the command zip -r archive.zip . -x ".env*" "node_modules/" "dist/" "build/" ".insforge/" does NOT exclude nested paths like apps/web/.env.local, packages/foo/node_modules/react/index.js, and services/api/dist/server.js. The -x patterns are matched against the full relative paths of files as they would appear in the archive (e.g., "apps/web/.env.local"). Patterns like ".env*" match only files whose full path starts with ".env*" (root-level only), not those ending with it or matching anywhere. "node_modules/" matches only direct contents of a root-level "node_modules/" directory (e.g., "node_modules/foo"), not nested ones like "packages/foo/node_modules/react/index.js", because the pattern lacks path prefix to match the nested location and the wildcard doesn't recurse across multiple directory levels by default in Info-ZIP matching. To exclude nested occurrences, use patterns like "node_modules" or "/.env" (though exact syntax for recursive basename matching varies; common practice is "name" or find alternatives), or "/node_modules/" if supported, but the given patterns fail for nested paths per man page behavior and community examples where "node_modules/" excludes only top-level.
Citations:
- 1: https://man.archlinux.org/man/zip.1.en.raw
- 2: https://linux.die.net/man/1/zip
- 3: https://manpages.ubuntu.com/manpages/xenial/man1/zip.1.html
- 4: https://stackoverflow.com/questions/69006277/how-can-i-exclude-both-the-git-directories-and-the-node-modules-directory-when
- 5: https://stackoverflow.com/questions/59626277/exclude-flag-in-zip-not-excluding-directory-as-expected
- 6: https://gist.github.com/ldong/15ee0b1faa121891e9b53dddcefa0ca0
- 7: https://askubuntu.com/questions/1410521/how-to-zip-a-lot-of-folders-but-exclude-any-subfolder-recursively-called-node
Use recursive patterns or JS-side filters instead of root-level-only zip exclusions.
The -x patterns in the zip command only match files at the root level. Nested paths like apps/web/.env.local, packages/foo/node_modules/**, and services/api/dist/** will still be archived, leaking secrets and bloating the payload. Replace with recursive patterns like "*/.env*", "*/node_modules", "*/dist", "*/build", or better yet, reuse the JS-side shouldExcludeDeploymentPath() filter logic to maintain consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/tools/deployment.ts` around lines 610 - 613, The zip command uses
root-only -x patterns and will include nested files like apps/*/.env and
packages/*/node_modules; update the zipping step in the block that builds the
command using escapedDir/tmpZip so it excludes nested paths by either switching
to recursive exclude patterns (e.g. use "**/.env*" or
"*/node_modules/*"/"**/dist/**") or, preferably, reuse the existing JS filter
shouldExcludeDeploymentPath() to produce the exact exclude list or to pre-filter
files before zipping (i.e., build the archive from a filtered file list rather
than relying on root-level -x patterns); locate the zip invocation that
references escapedDir and tmpZip and replace the patterns or wiring to
shouldExcludeDeploymentPath() accordingly.
tonychang04
left a comment
tonychang04
left a comment
There was a problem hiding this comment.
didn't really check mcp as prioritized cli + skills. but if anyone have problems we can ping and fix
2 participants
Note
Add direct file-based deployment flow to replace zip uploads
deployDirectflow indeployment.tsthat collects files, hashes them (SHA-1), creates a session viaPOST /api/deployments/direct, and uploads files concurrently before callingstart.DirectDeploymentUnsupportedError) or the backend version is below 2.0.6.shouldExcludeDeploymentPathto consistently exclude.env*,node_modules,.git,.next,dist,build,.insforge,.DS_Store, and*.logfiles from both direct and legacy flows.INSFORGE_DEPLOY_UPLOAD_CONCURRENCY.parseCreateDeploymentResponsenow uses strict zod validation and throws on invalid response shapes instead of silently accepting them.Macroscope summarized 6387e7e.
Summary by CodeRabbit
New Features
Chores