chore: init schema change and migration for deployment agent selectors#918
chore: init schema change and migration for deployment agent selectors#918adityachoudhari26 merged 6 commits intomainfrom
Conversation
…ment-agent-selectors
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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
Adds first-class deployment-level job agent selector/config fields and a Drizzle migration to backfill them from existing deployment_job_agent links (resolving #915).
Changes:
- Add
job_agent_selectorandjob_agent_configcolumns todeploymentplus a backfill migration. - Update API/TRPC deployment mutation paths to write the new selector/config fields.
- Update Drizzle metadata snapshots/journal and
.gitignore.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/trpc/src/routes/deployments.ts | Updates deployment record with jobAgentSelector/jobAgentConfig when setting a job agent. |
| packages/db/src/schema/deployment.ts | Adds jobAgentSelector + jobAgentConfig columns to the Drizzle schema. |
| packages/db/drizzle/0184_reflective_bishop.sql | Adds the DB columns and backfills them from deployment_job_agent. |
| packages/db/drizzle/meta/0184_snapshot.json | Drizzle snapshot reflecting the new deployment columns. |
| packages/db/drizzle/meta/_journal.json | Registers migration 0184 in the Drizzle journal. |
| apps/api/src/routes/v1/workspaces/deployments.ts | Writes selector/config on create/upsert and adjusts job-agent link insertion behavior. |
| .gitignore | Fixes state/ entry formatting and adds .claude/. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| UPDATE deployment d | ||
| SET job_agent_selector = 'jobAgent.id == "' || dja.job_agent_id || '"', | ||
| job_agent_config = dja.config | ||
| FROM deployment_job_agent dja |
There was a problem hiding this comment.
The backfill UPDATE joins deployment_job_agent directly; if a deployment has multiple job agents, Postgres will update the deployment row multiple times and the final job_agent_selector/job_agent_config values are effectively nondeterministic. Consider restricting the backfill to deployments with exactly one job agent, or joining against a subquery that selects a single deterministic row per deployment (e.g., DISTINCT ON (deployment_id) with an ORDER BY).
| FROM deployment_job_agent dja | |
| FROM deployment_job_agent dja | |
| JOIN ( | |
| SELECT deployment_id | |
| FROM deployment_job_agent | |
| GROUP BY deployment_id | |
| HAVING COUNT(*) = 1 | |
| ) single_dja ON single_dja.deployment_id = dja.deployment_id |
| @@ -281,23 +306,46 @@ const upsertDeployment: AsyncTypedHandler< | |||
| description: body.description ?? "", | |||
| resourceSelector: body.resourceSelector ?? "false", | |||
| metadata: body.metadata ?? {}, | |||
| ...(jobAgentId != null && { | |||
| jobAgentSelector: `jobAgent.id == "${jobAgentId}"`, | |||
| jobAgentConfig, | |||
| }), | |||
| }, | |||
There was a problem hiding this comment.
When body.jobAgents is provided as an empty array (or provided but yields no jobAgentId), this upsert updates the deployment_job_agent links but does not clear deployment.jobAgentSelector / deployment.jobAgentConfig (because the update set only applies when jobAgentId != null). This can leave the deployment row pointing at a stale job agent that is no longer linked. Consider explicitly clearing these fields when body.jobAgents is provided and ends up empty.
| if (jobAgentId) | ||
| await db | ||
| .insert(schema.deploymentJobAgent) | ||
| .values({ | ||
| deploymentId, | ||
| jobAgentId, | ||
| config: jobAgentConfig, | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: [ | ||
| schema.deploymentJobAgent.deploymentId, | ||
| schema.deploymentJobAgent.jobAgentId, | ||
| ], | ||
| set: { config: jobAgentConfig }, | ||
| }); | ||
|
|
||
| if (body.jobAgents != null) | ||
| await db.transaction(async (tx) => { | ||
| await tx | ||
| .delete(schema.deploymentJobAgent) | ||
| .where(eq(schema.deploymentJobAgent.deploymentId, deploymentId)); | ||
|
|
There was a problem hiding this comment.
If both body.jobAgentId and body.jobAgents are provided, the code upserts a single (deploymentId, jobAgentId) row and then (because body.jobAgents != null) deletes all job agents and reinserts only body.jobAgents. If body.jobAgents doesn’t include body.jobAgentId, the deployment’s jobAgentSelector can point to an agent that is no longer linked. Consider reconciling these inputs (e.g., require consistency, or derive selector from the final jobAgents set and upsert within the same transaction).
| const jobAgentId = body.jobAgentId ?? body.jobAgents?.[0]?.ref; | ||
| const jobAgentConfig = | ||
| body.jobAgentConfig ?? body.jobAgents?.[0]?.config ?? {}; | ||
|
|
||
| await db.insert(schema.deployment).values({ | ||
| id, | ||
| name: body.name, | ||
| description: body.description ?? "", | ||
| resourceSelector: body.resourceSelector ?? "false", | ||
| jobAgentSelector: jobAgentId ? `jobAgent.id == "${jobAgentId}"` : "false", | ||
| jobAgentConfig, |
There was a problem hiding this comment.
jobAgentId is interpolated into a CEL expression (jobAgent.id == "${jobAgentId}"). If jobAgentId is not strictly validated/escaped (e.g., UUID), a crafted value containing quotes could break the selector and potentially broaden matching. Consider validating jobAgentId as a UUID (and/or escaping) before building the selector string.
| await ctx.db | ||
| .update(schema.deployment) | ||
| .set({ | ||
| jobAgentSelector: `jobAgent.id == "${jobAgentId}"`, | ||
| jobAgentConfig: config, | ||
| }) | ||
| .where(eq(schema.deployment.id, deploymentId)); |
There was a problem hiding this comment.
jobAgentId is interpolated into a selector string. Since the input schema currently accepts an arbitrary string, a non-UUID (or a value containing quotes) could produce an invalid CEL expression or unexpected selector behavior. Consider validating jobAgentId as a UUID and/or escaping it before building jobAgentSelector.
| jobAgentSelector: text("job_agent_selector").notNull().default("false"), | ||
| jobAgentConfig: jsonb("job_agent_config") | ||
| .default("{}") | ||
| .$type<Record<string, any>>(), |
There was a problem hiding this comment.
jobAgentConfig is defined without .notNull(), so the database allows NULL even though other config JSON columns in the codebase are consistently NOT NULL with a {} default (e.g., deploymentVersion.jobAgentConfig in packages/db/src/schema/deployment-version.ts). Allowing NULL here increases downstream null-handling and can conflict with assuming an object. Consider making this column NOT NULL and backfilling any existing NULLs in the migration.
| .$type<Record<string, any>>(), | |
| .$type<Record<string, any>>() | |
| .notNull(), |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/workspaces/deployments.ts`:
- Around line 286-289: The deployment code computes jobAgentId/jobAgentConfig
from the incoming body before persisting, which can leave
deployment.job_agent_selector or config pointing at removed/changed agents;
instead, derive job_agent_selector and job_agent_config from the actual agent
rows you persist (the final jobAgents set) and use those derived values when
updating the deployment row and the deployment_job_agent row in the same
transaction/operation. Concretely: after you perform the delete-and-insert of
job agent rows (the block that handles body.jobAgents and touches
deployment_job_agent), determine the selector (jobAgentSelector) and config
(jobAgentConfig) from the resulting persisted agent set (not from
body.jobAgentId/body.jobAgentConfig), then use those computed values when
updating the deployment record and when doing any conflict-upsert so both tables
reference the same final agent and config; apply the same change to the other
similar blocks handling jobAgents in this file.
In `@packages/db/drizzle/0184_reflective_bishop.sql`:
- Around line 3-7: The current UPDATE on table deployment using
deployment_job_agent (columns job_agent_selector/job_agent_config from
dja.job_agent_id and dja.config) can collapse multiple agents into an ambiguous
single row; change the migration to either (A) deterministically pick one source
row per deployment (e.g., select the deployment_job_agent row with the smallest
id or newest created_at via a subquery/CTE and use its job_agent_id and config
to set job_agent_selector/job_agent_config) or (B) derive the selector/config
from the full agent set (e.g., aggregate job_agent_id values into a
deterministic list/string and aggregate/merge configs) and update deployment
from that aggregated result; pick one rule, implement it in the UPDATE (using a
CTE or GROUP BY with array_agg/string_agg or a DISTINCT ON/min subselect) and
add a short comment in the migration documenting the chosen rule (which
function/table/columns were used: deployment, deployment_job_agent,
job_agent_id, config, job_agent_selector, job_agent_config).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7651bde-2e8e-40b1-8792-e06e9f908177
📒 Files selected for processing (7)
.gitignoreapps/api/src/routes/v1/workspaces/deployments.tspackages/db/drizzle/0184_reflective_bishop.sqlpackages/db/drizzle/meta/0184_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/deployment.tspackages/trpc/src/routes/deployments.ts
| const jobAgentId = body.jobAgentId ?? body.jobAgents?.[0]?.ref; | ||
| const jobAgentConfig = | ||
| body.jobAgentConfig ?? body.jobAgents?.[0]?.config ?? {}; | ||
|
|
There was a problem hiding this comment.
Derive job_agent_* from the final persisted agent set.
The conflict-update only touches jobAgentSelector / jobAgentConfig when jobAgentId is present, so a PUT with jobAgents: [] clears deployment_job_agent below but leaves the old selector/config on deployment. Also, if body.jobAgentId / body.jobAgentConfig is sent alongside body.jobAgents, the delete-and-reinsert block can remove or overwrite the row you just wrote. That leaves the parent row pointing at a missing or differently-configured agent. Please compute the selector/config from the same final agent set you persist and update both tables together.
Also applies to: 297-312, 316-348
🤖 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 286 - 289, The
deployment code computes jobAgentId/jobAgentConfig from the incoming body before
persisting, which can leave deployment.job_agent_selector or config pointing at
removed/changed agents; instead, derive job_agent_selector and job_agent_config
from the actual agent rows you persist (the final jobAgents set) and use those
derived values when updating the deployment row and the deployment_job_agent row
in the same transaction/operation. Concretely: after you perform the
delete-and-insert of job agent rows (the block that handles body.jobAgents and
touches deployment_job_agent), determine the selector (jobAgentSelector) and
config (jobAgentConfig) from the resulting persisted agent set (not from
body.jobAgentId/body.jobAgentConfig), then use those computed values when
updating the deployment record and when doing any conflict-upsert so both tables
reference the same final agent and config; apply the same change to the other
similar blocks handling jobAgents in this file.
| UPDATE deployment d | ||
| SET job_agent_selector = 'jobAgent.id == "' || dja.job_agent_id || '"', | ||
| job_agent_config = dja.config | ||
| FROM deployment_job_agent dja | ||
| WHERE dja.deployment_id = d.id; No newline at end of file |
There was a problem hiding this comment.
Backfill collapses multi-agent deployments into one ambiguous record.
deployment_job_agent is one-to-many by deployment_id, but this update writes a single job_agent_selector / job_agent_config pair without first reducing that set to one deterministic source row. Any existing deployment with multiple agents can lose data here, and the retained pair is ambiguous. Please choose and document a single backfill rule per deployment, or derive the selector from the full agent set, before shipping this migration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/drizzle/0184_reflective_bishop.sql` around lines 3 - 7, The
current UPDATE on table deployment using deployment_job_agent (columns
job_agent_selector/job_agent_config from dja.job_agent_id and dja.config) can
collapse multiple agents into an ambiguous single row; change the migration to
either (A) deterministically pick one source row per deployment (e.g., select
the deployment_job_agent row with the smallest id or newest created_at via a
subquery/CTE and use its job_agent_id and config to set
job_agent_selector/job_agent_config) or (B) derive the selector/config from the
full agent set (e.g., aggregate job_agent_id values into a deterministic
list/string and aggregate/merge configs) and update deployment from that
aggregated result; pick one rule, implement it in the UPDATE (using a CTE or
GROUP BY with array_agg/string_agg or a DISTINCT ON/min subselect) and add a
short comment in the migration documenting the chosen rule (which
function/table/columns were used: deployment, deployment_job_agent,
job_agent_id, config, job_agent_selector, job_agent_config).
Resolves #915
Summary by CodeRabbit
Release Notes
New Features
Chores