fix: insert job agents on deployment POST and PUT#922
Conversation
📝 WalkthroughWalkthroughAdded persistence of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
Fixes deployments POST/PUT so that provided job agents are persisted (resolving #921), and updates E2E coverage accordingly.
Changes:
- Insert
jobAgentslinks intodeployment_job_agenton deployment creation (POST). - Rewrite
deployment_job_agentlinks on deployment upsert (PUT) whenjobAgentsis provided. - Update E2E deployment API tests to send
jobAgentsand add per-test cleanup deletes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| e2e/tests/api/deployments.spec.ts | Updates request shape to jobAgents and adds cleanup DELETEs after tests. |
| apps/api/src/routes/v1/workspaces/deployments.ts | Persists jobAgents on POST and rewrites them on PUT via delete+insert. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (body.jobAgents != null && body.jobAgents.length > 0) | ||
| await db.insert(schema.deploymentJobAgent).values( | ||
| body.jobAgents.map((agent) => ({ | ||
| deploymentId: id, | ||
| jobAgentId: agent.ref, | ||
| config: agent.config, | ||
| })), | ||
| ); |
There was a problem hiding this comment.
The deployment ↔ job agent link is inserted using the provided agent.ref without verifying that the referenced job agent belongs to the same workspaceId. Because deployment_job_agent only FK’s to job_agent.id, a client could attach a deployment to a job agent from a different workspace (cross-tenant reference). Consider validating all body.jobAgents[*].ref against schema.jobAgent with workspaceId (and return 400/404 on mismatch) before inserting, ideally within the same transaction as the deployment insert.
| if (body.jobAgents != null && body.jobAgents.length > 0) | ||
| await db.insert(schema.deploymentJobAgent).values( | ||
| body.jobAgents.map((agent) => ({ | ||
| deploymentId: id, | ||
| jobAgentId: agent.ref, | ||
| config: agent.config, | ||
| })), | ||
| ); |
There was a problem hiding this comment.
DeploymentJobAgent in the OpenAPI schema includes a required selector, but this handler silently drops it (only jobAgentId + config are persisted). This makes the API behave inconsistently and loses client-provided data. Either persist selector (schema + query/formatting), or explicitly reject/strip it at validation time and update the OpenAPI schema accordingly.
| await db.insert(schema.deployment).values({ | ||
| id, | ||
| name: body.name, | ||
| description: body.description ?? "", | ||
| resourceSelector: body.resourceSelector ?? "false", | ||
| metadata: body.metadata ?? {}, | ||
| workspaceId, | ||
| }); | ||
|
|
||
| if (body.jobAgents != null && body.jobAgents.length > 0) | ||
| await db.insert(schema.deploymentJobAgent).values( | ||
| body.jobAgents.map((agent) => ({ | ||
| deploymentId: id, | ||
| jobAgentId: agent.ref, | ||
| config: agent.config, | ||
| })), | ||
| ); |
There was a problem hiding this comment.
The deployment row insert and the subsequent deployment_job_agent inserts are not wrapped in a single transaction. If the job agent insert fails (FK violation, duplicate key, etc.), the deployment is still created, leaving partial state. Consider using a transaction encompassing both the deployment insert and the job-agent link inserts so creation is atomic.
| if (body.jobAgents != null) | ||
| await db.transaction(async (tx) => { | ||
| await tx | ||
| .delete(schema.deploymentJobAgent) | ||
| .where(eq(schema.deploymentJobAgent.deploymentId, deploymentId)); | ||
|
|
||
| if (body.jobAgents!.length > 0) | ||
| await tx.insert(schema.deploymentJobAgent).values( | ||
| body.jobAgents!.map((agent) => ({ | ||
| deploymentId, | ||
| jobAgentId: agent.ref, | ||
| config: agent.config, | ||
| })), | ||
| ); |
There was a problem hiding this comment.
Same cross-workspace integrity issue as POST: the code deletes and reinserts links based on agent.ref without checking that each referenced job agent belongs to workspaceId. This allows associating a deployment with another workspace’s job agent if the ID is known. Validate refs against schema.jobAgent scoped to workspaceId before (re)inserting.
| await db | ||
| .insert(schema.deployment) | ||
| .values({ | ||
| id: deploymentId, | ||
| name: body.name, | ||
| description: body.description ?? "", | ||
| resourceSelector: body.resourceSelector ?? "false", | ||
| metadata: body.metadata ?? {}, | ||
| workspaceId, | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: schema.deployment.id, | ||
| set: { | ||
| name: body.name, | ||
| description: body.description ?? "", | ||
| resourceSelector: body.resourceSelector ?? "false", | ||
| metadata: body.metadata ?? {}, | ||
| }, | ||
| }); | ||
|
|
||
| if (body.jobAgents != null) | ||
| await db.transaction(async (tx) => { | ||
| await tx | ||
| .delete(schema.deploymentJobAgent) | ||
| .where(eq(schema.deploymentJobAgent.deploymentId, deploymentId)); | ||
|
|
||
| if (body.jobAgents!.length > 0) | ||
| await tx.insert(schema.deploymentJobAgent).values( | ||
| body.jobAgents!.map((agent) => ({ | ||
| deploymentId, | ||
| jobAgentId: agent.ref, | ||
| config: agent.config, | ||
| })), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This update path performs the deployment upsert and the job-agent link rewrite in separate statements/transactions. If the link rewrite fails after the deployment upsert succeeds, the request returns 202 but leaves the deployment updated with stale/missing links. Consider a single transaction that includes the deployment upsert and the link delete/insert so the update is atomic when jobAgents is provided.
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/deployments/{deploymentId}", | ||
| { params: { path: { workspaceId: workspace.id, deploymentId } } }, | ||
| ); |
There was a problem hiding this comment.
This cleanup DELETE call doesn’t assert the response status. If the delete fails (auth, transient error), the test will still pass and leave leaked deployments that can make later runs flaky. Capture the response and assert status (and optionally confirm the resource is gone).
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/deployments/{deploymentId}", | ||
| { params: { path: { workspaceId: workspace.id, deploymentId } } }, | ||
| ); |
There was a problem hiding this comment.
This cleanup DELETE call doesn’t assert the response status. If the delete fails, the test can still pass while leaking state. Consider asserting the delete status (and optionally use try/finally so cleanup still runs on earlier assertion failures).
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/deployments/{deploymentId}", | ||
| { params: { path: { workspaceId: workspace.id, deploymentId } } }, | ||
| ); |
There was a problem hiding this comment.
This cleanup DELETE call doesn’t assert the response status. If the delete fails, the test can still pass while leaking state. Consider asserting the delete status (and optionally use try/finally so cleanup still runs on earlier assertion failures).
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/deployments/{deploymentId}", | ||
| { params: { path: { workspaceId: workspace.id, deploymentId } } }, | ||
| ); |
There was a problem hiding this comment.
This cleanup DELETE call doesn’t assert the response status. If the delete fails, the test can still pass while leaking state. Consider asserting the delete status (and optionally use try/finally so cleanup still runs on earlier assertion failures).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/routes/v1/workspaces/deployments.ts (2)
267-301:⚠️ Potential issue | 🔴 CriticalKeep the deployment upsert and agent replacement in the same transaction.
Lines 267-285 commit before the delete/reinsert starts. If the insert here fails, the deployment is updated but the old agent set remains; if two
PUTs race, you can also end up with deployment fields from one request andjobAgentsfrom another.💡 Suggested fix
- await db - .insert(schema.deployment) - .values({ - id: deploymentId, - name: body.name, - description: body.description ?? "", - resourceSelector: body.resourceSelector ?? "false", - metadata: body.metadata ?? {}, - workspaceId, - }) - .onConflictDoUpdate({ - target: schema.deployment.id, - set: { - name: body.name, - description: body.description ?? "", - resourceSelector: body.resourceSelector ?? "false", - metadata: body.metadata ?? {}, - }, - }); - - if (body.jobAgents != null) - await db.transaction(async (tx) => { + await db.transaction(async (tx) => { + await tx + .insert(schema.deployment) + .values({ + id: deploymentId, + name: body.name, + description: body.description ?? "", + resourceSelector: body.resourceSelector ?? "false", + metadata: body.metadata ?? {}, + workspaceId, + }) + .onConflictDoUpdate({ + target: schema.deployment.id, + set: { + name: body.name, + description: body.description ?? "", + resourceSelector: body.resourceSelector ?? "false", + metadata: body.metadata ?? {}, + }, + }); + + if (body.jobAgents != null) { await tx .delete(schema.deploymentJobAgent) .where(eq(schema.deploymentJobAgent.deploymentId, deploymentId)); - if (body.jobAgents!.length > 0) + if (body.jobAgents.length > 0) await tx.insert(schema.deploymentJobAgent).values( - body.jobAgents!.map((agent) => ({ + body.jobAgents.map((agent) => ({ deploymentId, jobAgentId: agent.ref, config: agent.config, })), ); - }); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/workspaces/deployments.ts` around lines 267 - 301, Wrap the deployment upsert and the jobAgents delete/insert in a single database transaction so they commit or rollback together: move the await db.insert(schema.deployment)...onConflictDoUpdate(...) call inside the same db.transaction(...) that currently performs the delete/insert of schema.deploymentJobAgent, using the transaction handle (tx) for both the deployment upsert and the jobAgent delete/insert operations (use tx.insert, tx.delete and eq(schema.deploymentJobAgent.deploymentId, deploymentId)); ensure you still check body.jobAgents for null and length and keep deploymentId, body.name, body.description, body.resourceSelector and body.metadata passed into the upsert as before so the entire change is atomic.
234-250:⚠️ Potential issue | 🟠 MajorMake deployment creation atomic with the job-agent inserts.
If the
deploymentJobAgentinsert fails here, the deployment row from Lines 234-241 is already committed and the client gets an error for a partially created deployment. These writes should succeed or fail together.💡 Suggested fix
- await db.insert(schema.deployment).values({ - id, - name: body.name, - description: body.description ?? "", - resourceSelector: body.resourceSelector ?? "false", - metadata: body.metadata ?? {}, - workspaceId, - }); - - if (body.jobAgents != null && body.jobAgents.length > 0) - await db.insert(schema.deploymentJobAgent).values( - body.jobAgents.map((agent) => ({ - deploymentId: id, - jobAgentId: agent.ref, - config: agent.config, - })), - ); + await db.transaction(async (tx) => { + await tx.insert(schema.deployment).values({ + id, + name: body.name, + description: body.description ?? "", + resourceSelector: body.resourceSelector ?? "false", + metadata: body.metadata ?? {}, + workspaceId, + }); + + if (body.jobAgents?.length) + await tx.insert(schema.deploymentJobAgent).values( + body.jobAgents.map((agent) => ({ + deploymentId: id, + jobAgentId: agent.ref, + config: agent.config, + })), + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/routes/v1/workspaces/deployments.ts` around lines 234 - 250, Wrap the deployment insert and the optional deploymentJobAgent inserts in a single database transaction so both inserts commit or roll back together: move the db.insert(schema.deployment).values({...}) and the conditional db.insert(schema.deploymentJobAgent).values(...) (which uses body.jobAgents.map(...)) into a single transactional call (e.g., db.transaction / db.$transaction depending on the DB client) using the same id and workspaceId variables, and ensure any error from the job-agent insert causes the transaction to abort and the error to be returned to the caller.
🧹 Nitpick comments (1)
e2e/tests/api/deployments.spec.ts (1)
135-186: ThisPUTtest still only covers create-via-upsert.Because
deploymentIdis new, the test never exercises the new branch that deletes existingdeploymentJobAgentrows before re-inserting them. Please add a secondPUTagainst the same deployment and assert the agent set is replaced or cleared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/api/deployments.spec.ts` around lines 135 - 186, The test "should upsert a deployment with a job agent" currently only verifies create-via-upsert because it uses a fresh deploymentId; add a second API.PUT against the same "/v1/workspaces/{workspaceId}/deployments/{deploymentId}" (reusing deploymentId) that changes the jobAgents payload (e.g., an empty array or a different ref) to exercise the branch that deletes existing deploymentJobAgent rows before re-inserting, then GET the deployment and assert deployment.jobAgents reflects the replacement/clearing (e.g., expect(jobAgents).toHaveLength(0) or contains the new ref), and finally keep the existing DELETE cleanup. Ensure you reference the same deploymentId and the same PUT/GET endpoints so the update path inside the upsert logic is executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/src/routes/v1/workspaces/deployments.ts`:
- Around line 267-301: Wrap the deployment upsert and the jobAgents
delete/insert in a single database transaction so they commit or rollback
together: move the await db.insert(schema.deployment)...onConflictDoUpdate(...)
call inside the same db.transaction(...) that currently performs the
delete/insert of schema.deploymentJobAgent, using the transaction handle (tx)
for both the deployment upsert and the jobAgent delete/insert operations (use
tx.insert, tx.delete and eq(schema.deploymentJobAgent.deploymentId,
deploymentId)); ensure you still check body.jobAgents for null and length and
keep deploymentId, body.name, body.description, body.resourceSelector and
body.metadata passed into the upsert as before so the entire change is atomic.
- Around line 234-250: Wrap the deployment insert and the optional
deploymentJobAgent inserts in a single database transaction so both inserts
commit or roll back together: move the
db.insert(schema.deployment).values({...}) and the conditional
db.insert(schema.deploymentJobAgent).values(...) (which uses
body.jobAgents.map(...)) into a single transactional call (e.g., db.transaction
/ db.$transaction depending on the DB client) using the same id and workspaceId
variables, and ensure any error from the job-agent insert causes the transaction
to abort and the error to be returned to the caller.
---
Nitpick comments:
In `@e2e/tests/api/deployments.spec.ts`:
- Around line 135-186: The test "should upsert a deployment with a job agent"
currently only verifies create-via-upsert because it uses a fresh deploymentId;
add a second API.PUT against the same
"/v1/workspaces/{workspaceId}/deployments/{deploymentId}" (reusing deploymentId)
that changes the jobAgents payload (e.g., an empty array or a different ref) to
exercise the branch that deletes existing deploymentJobAgent rows before
re-inserting, then GET the deployment and assert deployment.jobAgents reflects
the replacement/clearing (e.g., expect(jobAgents).toHaveLength(0) or contains
the new ref), and finally keep the existing DELETE cleanup. Ensure you reference
the same deploymentId and the same PUT/GET endpoints so the update path inside
the upsert logic is executed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ee52de8-87f7-4a56-a090-21c882508e1f
📒 Files selected for processing (2)
apps/api/src/routes/v1/workspaces/deployments.tse2e/tests/api/deployments.spec.ts
Resolves #921
Summary by CodeRabbit
New Features
Tests