feat(auto-scaling): fix autoscaling and add manual deploy#17
Conversation
and cache clearing - Add `clear_cache` column to `deployments` table - Allow specifying `commitSha` during deployment via API and UI - Update `prepareSourceWorkspace` to support explicit git checkout - Add unit tests for scaling engine - Modularize Docker utilities for the scaling engine
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds commit-pinned manual Git deployments with optional cache clearing, threads deployment fields through the API, database, and orchestrator, introduces deployment history and log components, and refactors scaling command execution with updated policy persistence and tests. ChangesManual deploy, commit pinning, cache clearing
Scaling engine and policy updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DeploymentsTab
participant DeploymentsAPI
participant PipelineOrchestrator
participant prepareSourceWorkspace
participant buildWithRailpack
DeploymentsTab->>DeploymentsAPI: Submit commitSha and clearCache
DeploymentsAPI->>PipelineOrchestrator: Create deployment
PipelineOrchestrator->>prepareSourceWorkspace: Fetch and checkout commitSha
prepareSourceWorkspace-->>PipelineOrchestrator: Return workspace
PipelineOrchestrator->>buildWithRailpack: Build with clearCache
buildWithRailpack-->>PipelineOrchestrator: Use cache-bypassed key
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a manual Git redeploy flow (including optional commit pinning and a “clear build cache” flag) and tightens up autoscaling internals by extracting Docker command helpers and adding test coverage for the scaling engine.
Changes:
- Web: Introduces a “Manual Deploy” dialog for Git projects (latest vs specific commit) and surfaces “clear cache” controls.
- API/DB/Orchestrator: Adds
clearCacheto deployment types/storage and plumbs it through the build pipeline (Railpack cache key bypass). - Scaling: Refactors docker command execution into a utility and adds unit tests for scaling behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/components/project/deployments/DeploymentsTab.tsx | Adds manual deploy dialog UI and clear-cache controls for deployments. |
| apps/api/src/types.ts | Extends deployment types with clearCache. |
| apps/api/src/scaling/engine.ts | Uses extracted docker command helpers. |
| apps/api/src/scaling/docker-utils.ts | New helper module for running docker commands. |
| apps/api/src/scaling/tests/engine.test.ts | Adds unit tests for scaling engine behavior. |
| apps/api/src/orchestrator/source.ts | Adds commit-SHA checkout support when preparing Git sources. |
| apps/api/src/orchestrator/runtime.ts | Skips Caddy reloads during tests. |
| apps/api/src/orchestrator/railpack.ts | Adds cache-bypass behavior when clearCache is set. |
| apps/api/src/orchestrator/pipeline.ts | Plumbs commit label + clearCache into build execution. |
| apps/api/src/db/schema.ts | Adds deployments.clear_cache column. |
| apps/api/src/db/repo/scaling.ts | Extends scaling policy upsert fields (cooldown/enabled). |
| apps/api/src/db/repo/deployments.ts | Maps/persists clearCache to/from DB. |
| apps/api/src/db/migrate.ts | Adds migration for deployments.clear_cache. |
| apps/api/src/api/deployments/index.ts | Parses commitSha/clearCache from multipart form when creating deployments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| memoryThresholdPercent: input.memoryThresholdPercent ?? 85, | ||
| cooldownSeconds: input.cooldownSeconds ?? 120, | ||
| enabled: input.enabled ? 1 : 1, | ||
| createdAt: timestamp, |
| if (commitSha) { | ||
| await run('git', ['clone', gitUrl, root]); | ||
| await run('git', ['checkout', commitSha], root); | ||
| } else { |
| <div className="flex items-center gap-2.5 py-1.5 px-3 rounded-lg bg-[#121215]/50 border border-[#222227]/50"> | ||
| <input | ||
| type="checkbox" | ||
| id="clearCacheUpload" | ||
| checked={clearCache} | ||
| onChange={(e) => setClearCache(e.target.checked)} | ||
| className="h-4 w-4 rounded border-zinc-700 bg-zinc-800 text-amber-500 focus:ring-amber-500 focus:ring-offset-zinc-950 cursor-pointer" | ||
| /> | ||
| <label htmlFor="clearCacheUpload" className="text-xs text-zinc-400 cursor-pointer select-none"> | ||
| Clear build cache for this deployment | ||
| </label> | ||
| </div> |
| <Button | ||
| type="button" | ||
| onClick={() => setShowManualDeployDialog(true)} | ||
| className="bg-amber-600 hover:bg-amber-700 text-white font-medium flex items-center gap-2 shadow-lg shadow-amber-500/10" | ||
| > |
| <Button | ||
| type="button" | ||
| onClick={handleManualDeploy} | ||
| disabled={createDeployment.isPending || (deployOption === "commit" && !commitSha.trim())} | ||
| className="bg-amber-600 hover:bg-amber-700 text-white font-medium text-xs h-9 px-4 flex items-center gap-2" | ||
| > |
| <div className="text-right space-y-1"> | ||
| <div className="text-xs text-muted-foreground font-medium uppercase tracking-wider">Branch</div> | ||
| <div className="text-xs bg-[#1a1a20] border border-[#33333b] text-zinc-300 px-2 py-1 rounded font-mono inline-block"> | ||
| {project?.repoBranch || "main"} |
| )} | ||
| > | ||
| <span className="text-xs font-bold">Latest Commit</span> | ||
| <span className="text-[10px] opacity-75 mt-0.5">Deploy HEAD from {project?.repoBranch || "main"}</span> |
| const deployment = await createDeployment({ | ||
| projectId, | ||
| sourceType: "upload", | ||
| sourceRef: uploadPath, | ||
| branch, | ||
| environment, | ||
| clearCache, | ||
| }); |
| {/* Clear Cache Toggle */} | ||
| <div className="flex items-center gap-2.5 p-3 rounded-lg bg-[#121215]/50 border border-[#222227]/50"> | ||
| <input | ||
| type="checkbox" | ||
| id="clearCache" | ||
| checked={clearCache} | ||
| onChange={(e) => setClearCache(e.target.checked)} | ||
| className="h-4 w-4 rounded border-zinc-700 bg-zinc-800 text-amber-500 focus:ring-amber-500 focus:ring-offset-zinc-950 cursor-pointer" | ||
| /> | ||
| <div className="flex flex-col cursor-pointer select-none" onClick={() => setClearCache(!clearCache)}> | ||
| <label htmlFor="clearCache" className="text-xs font-semibold text-zinc-300 cursor-pointer"> | ||
| Clear build cache | ||
| </label> | ||
| <span className="text-[10px] text-zinc-500 leading-normal"> | ||
| Bypasses cached Buildkit stages to force a clean dependency fetch and build. | ||
| </span> | ||
| </div> | ||
| </div> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/components/project/deployments/DeploymentsTab.tsx (1)
94-895: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftFile exceeds the 500-line limit; split into smaller feature files.
The file is well over 500 lines and this PR adds further UI (manual deploy dialog) into it. As per coding guidelines, "No file should be above 500 lines of code; if it is, refactor and split into smaller files with proper feature grouping in a folder rather than scattered across the codebase." Consider extracting the Manual Deployment dialog (lines ~793-892) and the git/upload source-input section into separate components under a
deployments/feature folder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/project/deployments/DeploymentsTab.tsx` around lines 94 - 895, The DeploymentsTab component is now far over the 500-line limit, so split the newly added UI into feature components instead of keeping everything in one file. Extract the Manual Deployment dialog and the git/upload source-input section into separate components under the deployments feature folder, then have DeploymentsTab compose them using their existing state/handlers such as handleManualDeploy, handleDeploy, and handleSwitchToGit.Source: Coding guidelines
apps/api/src/api/deployments/index.ts (1)
189-198: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRedeploy should carry over
commitShaandclearCache. Without them, a manually pinned deployment can redeploy from branch HEAD and ignore the original cache-busting choice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/api/deployments/index.ts` around lines 189 - 198, The redeploy flow in the deployment copy logic is dropping important fields, so a redeployed manual pin can fall back to branch HEAD and lose the original cache choice. Update the createDeployment call in the redeploy path to carry over both commitSha and clearCache from the original deployment, alongside the existing projectId/sourceType/sourceRef/branch/environment fields, so the new deployment preserves the same revision and cache-busting behavior.
🧹 Nitpick comments (7)
apps/api/src/orchestrator/runtime.ts (1)
97-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest-environment branch embedded in production code.
Hardcoding
NODE_ENV === 'test'insidereloadCaddycouples production logic to test infrastructure and is easy to bypass/misconfigure across environments. Prefer mockingrun/reloadCaddyat the test boundary (as already done fordocker-utilsinengine.test.ts) rather than branching on environment inside the implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/orchestrator/runtime.ts` at line 97, The reloadCaddy implementation currently has a test-only branch that returns early when NODE_ENV is 'test', which should not live in production logic. Remove the environment check from reloadCaddy in runtime.ts and let tests mock or stub the run/reloadCaddy boundary instead, following the pattern used for docker-utils in engine.test.ts. Keep the production behavior in reloadCaddy unchanged for all environments and isolate test behavior in the test setup.apps/api/src/scaling/__tests__/engine.test.ts (1)
12-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInline organizational comments in a
.tsfile.Several
// Mock ...header comments (Lines 12, 44, 57, 82, 93, 106, 111) are added throughout the mock setup. As per coding guidelines,**/*.{ts,tsx,js,jsx}files should have "No comments in source code unless absolutely necessary." Consider extracting each mock block into a small named helper function instead (e.g.,setupDockerUtilsMock()), which documents intent without comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/scaling/__tests__/engine.test.ts` around lines 12 - 111, The test setup in engine.test.ts adds several inline “Mock …” comments, which violates the no-comments guideline for source files. Remove those comments and refactor each mock block into small named helper functions or setup helpers (for example, for docker-utils, fs, redis, repo, and docker-bin) so the intent is expressed through function names like setupDockerUtilsMock and setupRedisMock. Keep the existing mock behavior in the helpers and call them from the test module initialization.Source: Coding guidelines
apps/web/src/components/project/deployments/DeploymentsTab.tsx (1)
400-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "clear cache" checkbox markup.
The same checkbox structure/classes/behavior is repeated for the upload form and the manual deploy dialog. Consider extracting a small shared
ClearCacheTogglecomponent.Also applies to: 853-870
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/project/deployments/DeploymentsTab.tsx` around lines 400 - 411, The clear-cache checkbox markup is duplicated between the upload form and the manual deploy dialog, so extract the repeated structure into a shared ClearCacheToggle component and reuse it in DeploymentsTab. Keep the existing checked/onChange behavior, id/htmlFor wiring, and styling props consistent so both call sites render the same toggle without duplicated JSX.apps/api/src/orchestrator/source.ts (1)
41-43: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFull clone required for arbitrary commit checkout — consider shallow fetch if server supports it.
Cloning full history for the
commitShapath can be slow/expensive on large repos. If your git hosting supportsuploadpack.allowReachableSHA1InWant, agit fetch --depth 1 origin <sha>+ checkout would avoid a full clone. This is optional since server support varies and the current approach is universally safe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/orchestrator/source.ts` around lines 41 - 43, The commitSha checkout path in source.ts always performs a full git clone before checkout, which is expensive for large repos. Update the commitSha branch in the source orchestration flow to prefer a shallow fetch/checkout path when the server supports fetching a reachable SHA directly, and keep the existing full clone as the fallback. Use the existing run-based git command flow in source.ts so the logic remains localized and easy to switch based on server capability.apps/api/src/orchestrator/railpack.ts (2)
742-745: 🚀 Performance & Scalability | 🔵 TrivialCache-clear key never gets cleaned up.
Each
clearCachebuild produces a uniquecacheKey(-clear-${Date.now()}) that is never reused, so the previous cache entry under the old key becomes orphaned in the persistent builder cache. Over repeated manual "clear cache" deploys, this can accumulate unbounded disk usage on the builder host with no visible reclamation path in this diff.Consider tracking and periodically garbage-collecting stale cache keys, or reusing/evicting the prior cache key for the same project when clearing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/orchestrator/railpack.ts` around lines 742 - 745, The clearCache branch in railpack.ts is generating a one-off cacheKey suffix with Date.now() but never evicting the prior key, so old cache entries accumulate. Update the cache-key handling around the clearCache path to either reuse or explicitly invalidate the previous project cache key, and add cleanup/garbage-collection logic for stale keys in the same orchestration flow so repeated clean builds don’t leave orphaned entries.
1-832: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFile exceeds the 500-line guideline.
This file is ~832 lines. As per coding guidelines, "No file should be above 500 lines of code; if it is, refactor and split into smaller files with proper feature grouping in a folder." Worth a follow-up split (e.g. separate cache-key derivation, builder lifecycle, and CLI invocation into their own modules).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/orchestrator/railpack.ts` around lines 1 - 832, The railpack orchestration file is far over the 500-line limit and needs to be split into smaller feature-focused modules. Refactor the logic in buildWithRailpack, ensureBuilder, spawnAsync, generateDynamicRailpackJson, rewriteLocalhostBinding, and cleanBuildkitLine into separate files grouped by responsibility (for example builder lifecycle, CLI execution, config generation, and output cleaning). Keep the public buildWithRailpack entrypoint in the current module and move reusable helpers behind imports so the file stays within the guideline.Source: Coding guidelines
apps/api/src/orchestrator/pipeline.ts (1)
57-844: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFile exceeds the 500-line guideline.
This orchestrator file is ~844 lines. As per coding guidelines, "No file should be above 500 lines of code; if it is, refactor and split into smaller files with proper feature grouping in a folder rather than scattered across the codebase." Consider splitting
PipelineOrchestratorinto cohesive modules (e.g. deployment-run logic, rollback logic, reconciliation) in a follow-up, since this PR only touches a small slice of it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/orchestrator/pipeline.ts` around lines 57 - 844, The PipelineOrchestrator file is far above the 500-line guideline and should be split into smaller cohesive modules. Refactor the large PipelineOrchestrator class by extracting related responsibilities such as runDeployment, rollbackTo, and reconcileState into separate feature-grouped files or helper classes, while keeping PipelineOrchestrator as a thin coordinator. Preserve the existing public behavior and unique symbols like PipelineOrchestrator, runDeployment, rollbackTo, reconcileState, and cancelDeployment so call sites stay stable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/api/deployments/index.ts`:
- Around line 65-68: Validate commitSha in the deployments flow before
enqueueing the job, since the value from form.get("commitSha") in
apps/api/src/api/deployments/index.ts is passed through to deployment execution
and ultimately git checkout. Update the commitSha handling in the request
parsing path to reject malformed or option-like values up front, or resolve it
with git rev-parse --verify before creating the deployment task, so only valid
revisions reach the worker.
In `@apps/api/src/db/repo/scaling.ts`:
- Around line 44-45: The scaling policy insert logic in the repo method is
hardcoding enabled to 1, so explicit false values are ignored. Update the create
path in the scaling repository to map input.enabled correctly, matching the
update-path behavior in the same repo method so undefined still defaults to
enabled while an explicit false is stored as disabled. Use the existing scaling
repo insert/update mapping in the method that builds the policy record to locate
the fix.
In `@apps/api/src/orchestrator/source.ts`:
- Around line 37-49: The prepareSourceWorkspace flow currently trusts commitSha
and gitUrl directly before invoking git. In prepareSourceWorkspace, validate
commitSha with a strict hex/length check before calling git checkout, and keep
the clone invocation in the commitSha path fenced with -- so untrusted input
cannot be interpreted as options. Use the prepareSourceWorkspace symbol and its
git clone/git checkout branches to update the argument handling without changing
the deployments route behavior.
In `@apps/api/src/scaling/docker-utils.ts`:
- Around line 10-13: The run() helper in docker-utils currently concatenates
stderr into successful output, which can break JSON consumers like
getContainerStats() in engine.ts. Update the child process close handler so
successful exits resolve with stdout only, and keep stderr attached only for
failures (or return stdout/stderr separately) so JSON.parse() receives clean
JSON.
In `@apps/web/src/components/project/deployments/DeploymentsTab.tsx`:
- Line 807: Remove the non-essential JSX comments in DeploymentsTab so the
component complies with the no-comments guideline. Delete the three redundant
comments near the deploy option selection, commit hash input, and clear cache
toggle sections, since the surrounding JSX already makes the intent clear. Keep
the related logic and identifiers in DeploymentsTab unchanged; only strip the
comment-only lines.
- Around line 254-280: Manual deploy errors are only logged in
handleManualDeploy, so users get no visible failure feedback and the dialog
remains open. Update the handleManualDeploy flow in DeploymentsTab to surface
the mutation error in the UI, such as by setting an error state/toast or inline
message when createDeployment.mutateAsync(form) throws, and only close/reset the
dialog on success. Use the existing handleManualDeploy, createDeployment, and
setShowManualDeployDialog hooks/state to wire the failure path.
- Around line 254-261: The manual deploy flow in handleManualDeploy is
incorrectly reusing the shared environment state, which can carry over a value
from other deploy paths. Update DeploymentsTab so the git/manual redeploy path
uses an explicit environment value or no environment at all unless it is
intentionally set for that action, and avoid reading the shared environment
field here. Check the handleManualDeploy logic and any related deploy form state
so upload/compose selections do not leak into later git redeploys.
---
Outside diff comments:
In `@apps/api/src/api/deployments/index.ts`:
- Around line 189-198: The redeploy flow in the deployment copy logic is
dropping important fields, so a redeployed manual pin can fall back to branch
HEAD and lose the original cache choice. Update the createDeployment call in the
redeploy path to carry over both commitSha and clearCache from the original
deployment, alongside the existing
projectId/sourceType/sourceRef/branch/environment fields, so the new deployment
preserves the same revision and cache-busting behavior.
In `@apps/web/src/components/project/deployments/DeploymentsTab.tsx`:
- Around line 94-895: The DeploymentsTab component is now far over the 500-line
limit, so split the newly added UI into feature components instead of keeping
everything in one file. Extract the Manual Deployment dialog and the git/upload
source-input section into separate components under the deployments feature
folder, then have DeploymentsTab compose them using their existing
state/handlers such as handleManualDeploy, handleDeploy, and handleSwitchToGit.
---
Nitpick comments:
In `@apps/api/src/orchestrator/pipeline.ts`:
- Around line 57-844: The PipelineOrchestrator file is far above the 500-line
guideline and should be split into smaller cohesive modules. Refactor the large
PipelineOrchestrator class by extracting related responsibilities such as
runDeployment, rollbackTo, and reconcileState into separate feature-grouped
files or helper classes, while keeping PipelineOrchestrator as a thin
coordinator. Preserve the existing public behavior and unique symbols like
PipelineOrchestrator, runDeployment, rollbackTo, reconcileState, and
cancelDeployment so call sites stay stable.
In `@apps/api/src/orchestrator/railpack.ts`:
- Around line 742-745: The clearCache branch in railpack.ts is generating a
one-off cacheKey suffix with Date.now() but never evicting the prior key, so old
cache entries accumulate. Update the cache-key handling around the clearCache
path to either reuse or explicitly invalidate the previous project cache key,
and add cleanup/garbage-collection logic for stale keys in the same
orchestration flow so repeated clean builds don’t leave orphaned entries.
- Around line 1-832: The railpack orchestration file is far over the 500-line
limit and needs to be split into smaller feature-focused modules. Refactor the
logic in buildWithRailpack, ensureBuilder, spawnAsync,
generateDynamicRailpackJson, rewriteLocalhostBinding, and cleanBuildkitLine into
separate files grouped by responsibility (for example builder lifecycle, CLI
execution, config generation, and output cleaning). Keep the public
buildWithRailpack entrypoint in the current module and move reusable helpers
behind imports so the file stays within the guideline.
In `@apps/api/src/orchestrator/runtime.ts`:
- Line 97: The reloadCaddy implementation currently has a test-only branch that
returns early when NODE_ENV is 'test', which should not live in production
logic. Remove the environment check from reloadCaddy in runtime.ts and let tests
mock or stub the run/reloadCaddy boundary instead, following the pattern used
for docker-utils in engine.test.ts. Keep the production behavior in reloadCaddy
unchanged for all environments and isolate test behavior in the test setup.
In `@apps/api/src/orchestrator/source.ts`:
- Around line 41-43: The commitSha checkout path in source.ts always performs a
full git clone before checkout, which is expensive for large repos. Update the
commitSha branch in the source orchestration flow to prefer a shallow
fetch/checkout path when the server supports fetching a reachable SHA directly,
and keep the existing full clone as the fallback. Use the existing run-based git
command flow in source.ts so the logic remains localized and easy to switch
based on server capability.
In `@apps/api/src/scaling/__tests__/engine.test.ts`:
- Around line 12-111: The test setup in engine.test.ts adds several inline “Mock
…” comments, which violates the no-comments guideline for source files. Remove
those comments and refactor each mock block into small named helper functions or
setup helpers (for example, for docker-utils, fs, redis, repo, and docker-bin)
so the intent is expressed through function names like setupDockerUtilsMock and
setupRedisMock. Keep the existing mock behavior in the helpers and call them
from the test module initialization.
In `@apps/web/src/components/project/deployments/DeploymentsTab.tsx`:
- Around line 400-411: The clear-cache checkbox markup is duplicated between the
upload form and the manual deploy dialog, so extract the repeated structure into
a shared ClearCacheToggle component and reuse it in DeploymentsTab. Keep the
existing checked/onChange behavior, id/htmlFor wiring, and styling props
consistent so both call sites render the same toggle without duplicated JSX.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 217a1897-fdc4-4b6e-978c-5251bd1c4629
📒 Files selected for processing (14)
apps/api/src/api/deployments/index.tsapps/api/src/db/migrate.tsapps/api/src/db/repo/deployments.tsapps/api/src/db/repo/scaling.tsapps/api/src/db/schema.tsapps/api/src/orchestrator/pipeline.tsapps/api/src/orchestrator/railpack.tsapps/api/src/orchestrator/runtime.tsapps/api/src/orchestrator/source.tsapps/api/src/scaling/__tests__/engine.test.tsapps/api/src/scaling/docker-utils.tsapps/api/src/scaling/engine.tsapps/api/src/types.tsapps/web/src/components/project/deployments/DeploymentsTab.tsx
| const commitSha = | ||
| String(form.get("commitSha") ?? "").trim() || | ||
| undefined; | ||
| const clearCache = form.get("clearCache") === "true"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how commitSha is consumed downstream to assess injection risk
fd source.ts apps/api/src/orchestrator --exec sed -n '1,200p' {}Repository: Lftobs/dequel
Length of output: 3740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect deployment parsing/validation and all commitSha usages
printf '\n== deployments index ==\n'
sed -n '1,260p' apps/api/src/api/deployments/index.ts
printf '\n== commitSha references ==\n'
rg -n "commitSha" apps/api/src -g '!**/node_modules/**'Repository: Lftobs/dequel
Length of output: 8896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether any code normalizes or validates commitSha before it reaches the orchestrator
sed -n '1,140p' apps/api/src/db/repo/deployments.ts
printf '\n== types ==\n'
sed -n '210,255p' apps/api/src/types.ts
printf '\n== pipeline commitSha usage ==\n'
sed -n '300,350p' apps/api/src/orchestrator/pipeline.tsRepository: Lftobs/dequel
Length of output: 7384
🌐 Web query:
git checkout argument starting with hyphen treated as option argv parse -- commit SHA validation best practice
💡 Result:
In Git, the double-hyphen (--) is the standard mechanism to disambiguate arguments [1][2]. It signals the "end of options," instructing Git to interpret all subsequent arguments as pathspecs (files or directories) rather than command-line options, branches, or commit identifiers [1][3][4]. Using -- is essential in the following scenarios: - Files starting with a hyphen: If you have a file named "-filename", running "git checkout -filename" will cause Git to attempt to parse "-f" as an option [1]. The correct syntax is "git checkout -- -filename" [1]. - Disambiguation: If a file name shares the same name as a branch or commit (e.g., you have a file named "master"), using "git checkout master" may switch branches instead of restoring the file [3][5]. "git checkout -- master" explicitly forces Git to treat "master" as a pathspec [3]. Regarding commit SHA validation, while Git natively validates SHAs when you provide them to commands like "git checkout ", there is no built-in "git checkout --validate-sha" flag. Best practice is to rely on Git's internal object database validation: - When you use a partial SHA (a "short SHA"), Git automatically resolves it to a full 40-character object name. If the SHA is ambiguous (matches multiple objects), Git will return an error and suggest the matches. - To programmatically validate a commit SHA before performing an operation, you can use: git rev-parse --verify ^{commit} This command will return the full SHA if valid, or exit with an error code (and print an error to stderr) if the SHA does not exist or does not point to a commit [6]. Note: For modern workflows, it is recommended to use the more specific, explicit commands "git switch" for branch operations and "git restore" for file operations, as they eliminate much of the ambiguity inherent in "git checkout" [7].
Citations:
- 1: https://stackoverflow.com/questions/6561142/difference-between-git-checkout-filename-and-git-checkout-filename
- 2: https://stackoverflow.com/questions/13321458/meaning-of-git-checkout-double-dashes
- 3: https://git-scm.com/docs/git-checkout
- 4: https://git.github.io/htmldocs/git-checkout.html
- 5: https://stackoverflow.com/questions/37671936/how-does-git-resolve-filename-and-branchname
- 6: git/git@be4908f
- 7: https://stackoverflow.com/questions/25322335/git-change-branch-when-file-of-same-name-is-present
Validate commitSha before enqueuing the deployment. It’s stored as-is and later passed to git checkout, so malformed or option-like input will just make the deploy fail. Reject non-revision values up front, or resolve it with git rev-parse --verify.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/api/deployments/index.ts` around lines 65 - 68, Validate
commitSha in the deployments flow before enqueueing the job, since the value
from form.get("commitSha") in apps/api/src/api/deployments/index.ts is passed
through to deployment execution and ultimately git checkout. Update the
commitSha handling in the request parsing path to reject malformed or
option-like values up front, or resolve it with git rev-parse --verify before
creating the deployment task, so only valid revisions reach the worker.
| child.on('close', (code) => { | ||
| if (code === 0) resolve((stdout + '\n' + stderr).trim()); | ||
| else reject(new Error(`${cmd} ${args.join(' ')} failed (${code}): ${stderr}`)); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf 'Files:\n'
git ls-files apps/api/src/scaling/docker-utils.ts apps/api/src/scaling/engine.ts
printf '\nOutline docker-utils.ts:\n'
ast-grep outline apps/api/src/scaling/docker-utils.ts --view expanded || true
printf '\nOutline engine.ts:\n'
ast-grep outline apps/api/src/scaling/engine.ts --view expanded || true
printf '\nRelevant excerpts:\n'
sed -n '1,220p' apps/api/src/scaling/docker-utils.ts
printf '\n--- engine.ts ---\n'
sed -n '1,220p' apps/api/src/scaling/engine.tsRepository: Lftobs/dequel
Length of output: 9962
🏁 Script executed:
set -euo pipefail
printf 'Search docker-utils/engine references:\n'
rg -n "run\\(|docker stats|JSON\\.parse\\(" apps/api/src/scaling -S
printf '\nShow docker-utils.ts with line numbers:\n'
cat -n apps/api/src/scaling/docker-utils.ts
printf '\nShow engine.ts with line numbers:\n'
cat -n apps/api/src/scaling/engine.tsRepository: Lftobs/dequel
Length of output: 14224
🌐 Web query:
Docker CLI writes warnings to stderr on successful commands JSON output stderr stdout parse
💡 Result:
The Docker CLI historically outputs informational messages, including hints ("What's Next" notices) and warnings, to stderr by design, to keep stdout clean for programmatic use [1]. However, this behavior can interfere with machine-readable outputs, such as JSON, when warnings or hints are emitted alongside the intended data [2][3][4]. To manage and minimize these issues, consider the following strategies: 1. Disable CLI Hints: You can suppress the "What's Next" informational hints that are often printed to stderr by setting the environment variable DOCKER_CLI_HINTS to false [1][5]. export DOCKER_CLI_HINTS=false 2. Use --format for Machine-Readable Output: Whenever possible, use the --format flag to specify a structured output format, such as JSON [3][6]. docker inspect --format '{{json.}}' <container_id> 3. Addressing Inconsistent Warnings: Docker has taken steps to remove inconsistent warnings from commands when they are used in non-TTY (piped) environments, such as the previously problematic warning in docker images [3][4]. If you encounter specific commands still emitting noise to stderr, checking the official Docker CLI repository for existing issues or PRs is recommended, as ongoing efforts continue to improve output consistency [3][4][7][8]. 4. Distinguish Streams: If you are building tools or scripts that parse Docker output, ensure your implementation correctly handles stderr as a separate stream from stdout [9][10]. For advanced integration, such as via the Docker SDK, you can access individual streams (stdout, stderr) as distinct properties in the execution result [11]. In some cases, users have reported that certain errors may appear in stdout or that warnings persist in stderr even when using redirection [7][8]. When developing automation, always verify that your parsing logic is robust enough to ignore or filter unexpected lines from stderr if they are not explicitly part of the desired structured output [2][3].
Citations:
- 1: Docker CLI hints writes to STDERR docker/for-win#13602
- 2: isolate rawjson output from other status/error messages docker/buildx#2967
- 3: The "human readability" warning is (a.) annoying and (b.) inconsistent docker/cli#6749
- 4: Fix: Remove inconsistent human readability warning from docker images docker/cli#6754
- 5: https://shawnhooper.ca/2026/03/24/disable-whats-new-notices-in-docker-cli/
- 6: https://devops.stackexchange.com/questions/6241/how-do-i-use-docker-commands-format-option
- 7: Docker version error output going to stdout instead of stderr docker/cli#2777
- 8: Some exec errors are output to stdout instead of stderr docker/cli#4847
- 9: Why docker cli redirect all output to stderr? docker/cli#2386
- 10: https://docs.docker.com/reference/cli/docker
- 11: https://docs.docker.com/reference/api/extensions-sdk/Docker/
Don't merge stderr into successful JSON output. run() returns stdout + '\n' + stderr on exit 0, so any Docker warning on stderr makes apps/api/src/scaling/engine.ts's JSON.parse() fail and getContainerStats() quietly return null. Keep stderr for rejected runs, or return stdout/stderr separately.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/scaling/docker-utils.ts` around lines 10 - 13, The run() helper
in docker-utils currently concatenates stderr into successful output, which can
break JSON consumers like getContainerStats() in engine.ts. Update the child
process close handler so successful exits resolve with stdout only, and keep
stderr attached only for failures (or return stdout/stderr separately) so
JSON.parse() receives clean JSON.
| const handleManualDeploy = async () => { | ||
| const form = new FormData(); | ||
| form.set("sourceType", "git"); | ||
| if (projectId) form.set("projectId", projectId); | ||
| if (project?.repoUrl) form.set("gitUrl", project.repoUrl); | ||
| if (project?.repoBranch) form.set("branch", project.repoBranch); | ||
| if (environment) form.set("environment", environment); | ||
|
|
||
| if (deployOption === "commit") { | ||
| if (!commitSha.trim()) return; | ||
| form.set("commitSha", commitSha.trim()); | ||
| } | ||
|
|
||
| if (clearCache) { | ||
| form.set("clearCache", "true"); | ||
| } | ||
|
|
||
| try { | ||
| await createDeployment.mutateAsync(form); | ||
| setShowManualDeployDialog(false); | ||
| setCommitSha(""); | ||
| setDeployOption("latest"); | ||
| setClearCache(false); | ||
| } catch (err) { | ||
| console.error("Failed to start manual deployment:", err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Manual deploy failures are silently swallowed.
On error, handleManualDeploy only console.errors; the dialog stays open with no visible feedback telling the user the deployment failed to start.
💡 Suggested improvement
} catch (err) {
console.error("Failed to start manual deployment:", err);
+ // surface error to the user, e.g. via a toast/inline message
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/components/project/deployments/DeploymentsTab.tsx` around lines
254 - 280, Manual deploy errors are only logged in handleManualDeploy, so users
get no visible failure feedback and the dialog remains open. Update the
handleManualDeploy flow in DeploymentsTab to surface the mutation error in the
UI, such as by setting an error state/toast or inline message when
createDeployment.mutateAsync(form) throws, and only close/reset the dialog on
success. Use the existing handleManualDeploy, createDeployment, and
setShowManualDeployDialog hooks/state to wire the failure path.
- Add drizzle-kit config for sqlite and generate migrations - Include initial migration snapshot and journal cleanup - Fix upsertScalingPolicy to store enabled as 0/1 and handle undef - Improve git deployment flow: validate commit SHAs and fetch FETCH_HEAD - Introduce frontend deployment UI: history and logs - Refactor deployments tab to use new components - Enable API build in docker-compose for local dev
- Remove legacy migration snapshot at apps/api/src/db/migrations/meta/0002_snapshot.json to clean up migrations. - Add ClearCacheToggle component and integrate into DeploymentsTab and ManualDeployDialog.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/api/src/orchestrator/source.ts (1)
47-50: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
gitUrlis still passed unfenced into the new git commands, reintroducing the previously-flagged argument-injection risk.The prior review asked to fence the clone URL with
--sincegitUrlisn't validated; that concern now also applies togit remote add origin <gitUrl>andgit fetch ... origin <commitSha>in this new commit-pinned path. AgitUrlbeginning with-could be parsed as a git option (e.g.--upload-pack=<cmd>), which is a known argument-injection vector for git commands run without--fencing.🔒 Proposed fix
- await run('git', ['remote', 'add', 'origin', gitUrl], root); + await run('git', ['remote', 'add', 'origin', '--', gitUrl], root);Please confirm
git remote add/git clonehonor--as an end-of-options marker in the version of git used in the build image before relying on it as the sole mitigation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/orchestrator/source.ts` around lines 47 - 50, The commit-pinned git setup in source.ts still passes gitUrl directly into git commands, so update the orchestration path around run(...) to fence untrusted repository URLs with an explicit end-of-options marker or equivalent safe handling. Apply the fix to the git remote add origin and any git clone/fetch path that accepts gitUrl, and verify the git version used in the build image treats -- as the end of options for these commands before relying on it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/db/migrate.ts`:
- Around line 37-42: The migration block in migrate.ts is too broad because the
catch-all around the deployments ALTER TABLE swallows every failure, not just
the expected duplicate-column case. Update the migration logic in the migration
function to only ignore the known “column already exists” error and, for any
other error from db.run(sql`ALTER TABLE deployments ADD COLUMN clear_cache
...`), log the failure with context and rethrow it so real migration issues are
not masked.
- Line 36: The empty catch in addClearCacheColumn is swallowing all ALTER TABLE
errors, not just the duplicate-column case, so narrow the error handling to only
ignore the “column already exists” condition. Update the addClearCacheColumn
migration helper in migrate.ts to catch the specific duplicate-column error from
the ALTER TABLE add column path and rethrow any other failure so startup fails
fast on real schema issues.
In `@apps/web/src/components/project/deployments/deployment-logs.tsx`:
- Around line 114-127: The log styling in the deployment log renderer is
inverted for severity: in deployment-logs.tsx within the logs.map rendering,
CRITICAL entries are currently assigned the dim class. Update the log-line class
logic so the CRITICAL prefix is treated as important/highlighted styling rather
than dimmed, and keep dim only for low-priority placeholder or non-critical
entries.
---
Duplicate comments:
In `@apps/api/src/orchestrator/source.ts`:
- Around line 47-50: The commit-pinned git setup in source.ts still passes
gitUrl directly into git commands, so update the orchestration path around
run(...) to fence untrusted repository URLs with an explicit end-of-options
marker or equivalent safe handling. Apply the fix to the git remote add origin
and any git clone/fetch path that accepts gitUrl, and verify the git version
used in the build image treats -- as the end of options for these commands
before relying on it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: caa7830e-7cb5-498e-96cd-9ccbc4b58888
📒 Files selected for processing (11)
apps/api/drizzle.config.tsapps/api/src/db/migrate.tsapps/api/src/db/migrations/meta/_journal.jsonapps/api/src/db/repo/scaling.tsapps/api/src/orchestrator/source.tsapps/web/src/components/project/deployments/DeploymentsTab.tsxapps/web/src/components/project/deployments/clear-cache-toggle.tsxapps/web/src/components/project/deployments/deployment-history.tsxapps/web/src/components/project/deployments/deployment-logs.tsxapps/web/src/components/project/deployments/manual-deploy-dialog.tsxdocker-compose.yml
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/db/migrations/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/db/repo/scaling.ts
- apps/web/src/components/project/deployments/DeploymentsTab.tsx
Lftobs
changed the title
Lftobs
changed the title
- Harden migration - Improve deployment logs UI
Summary by CodeRabbit