refactor: drop old deployment agent table#943
Merged
adityachoudhari26 merged 6 commits intomainfrom Apr 8, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR removes the DeploymentJobAgent model and the deployment_job_agent join table, eliminates Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Web / API Client
participant API as Deployments API
participant DB as Database
rect rgba(100,150,240,0.5)
Client->>API: POST /deployments { jobAgentSelector, jobAgentConfig, ... }
API->>DB: INSERT/UPDATE deployment (no join-table)
DB-->>API: OK (deployment row)
API-->>Client: 201 Created { deployment with jobAgentSelector, jobAgentConfig }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsx (1)
9-10: Minor type inconsistency with API response.The API's
formatDeploymentfunction returnsjobAgentConfig: dep.jobAgentConfig ?? {}, which always produces an object (never null). However, this type allowsnull. Consider aligning with the API:- jobAgentConfig: Record<string, any> | null; + jobAgentConfig: Record<string, any>;Alternatively, if the API can return null in some edge cases, this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsx` around lines 9 - 10, Type mismatch: update the jobAgentConfig type to reflect the API's guarantee (formatDeployment returns an object) by changing the declaration in DeploymentProvider.tsx from "jobAgentConfig: Record<string, any> | null" to "jobAgentConfig: Record<string, any>" and adjust any downstream null-checks/usages (e.g., places referencing jobAgentConfig in this file or components consuming DeploymentProvider) to treat it as an object; alternatively, if you intend to accept null from other sources, keep the union but add a comment referencing formatDeployment to document the discrepancy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsx`:
- Around line 9-10: Type mismatch: update the jobAgentConfig type to reflect the
API's guarantee (formatDeployment returns an object) by changing the declaration
in DeploymentProvider.tsx from "jobAgentConfig: Record<string, any> | null" to
"jobAgentConfig: Record<string, any>" and adjust any downstream
null-checks/usages (e.g., places referencing jobAgentConfig in this file or
components consuming DeploymentProvider) to treat it as an object;
alternatively, if you intend to accept null from other sources, keep the union
but add a comment referencing formatDeployment to document the discrepancy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26866e03-5e44-4a85-8278-82267764c219
📒 Files selected for processing (22)
apps/api/openapi/openapi.jsonapps/api/openapi/schemas/deployments.jsonnetapps/api/src/routes/v1/workspaces/deployments.tsapps/api/src/types/openapi.tsapps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsxapps/web/app/routes/ws/deployments/settings/page.$deploymentId.job-agent.tsxapps/web/app/routes/ws/runners/JobAgentCard.tsxapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/schemas/deployments.jsonnetapps/workspace-engine/pkg/db/deployments.sql.goapps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/queries/deployments.sqlapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/oapi/oapi.gen.goapps/workspace-engine/pkg/selector/langs/cel/cel.gopackages/db/drizzle/0185_steep_scarecrow.sqlpackages/db/drizzle/meta/0185_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/deployment.tspackages/trpc/src/routes/deployments.tspackages/trpc/src/routes/job-agents.tspackages/workspace-engine-sdk/src/schema.ts
💤 Files with no reviewable changes (12)
- apps/web/app/routes/ws/deployments/settings/page.$deploymentId.job-agent.tsx
- apps/web/app/routes/ws/runners/JobAgentCard.tsx
- apps/workspace-engine/pkg/selector/langs/cel/cel.go
- apps/workspace-engine/pkg/db/queries/deployments.sql
- packages/trpc/src/routes/deployments.ts
- apps/workspace-engine/pkg/db/models.go
- apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet
- packages/db/src/schema/deployment.ts
- packages/workspace-engine-sdk/src/schema.ts
- apps/workspace-engine/oapi/openapi.json
- apps/workspace-engine/pkg/db/deployments.sql.go
- apps/workspace-engine/pkg/db/queries/schema.sql
There was a problem hiding this comment.
Pull request overview
Removes the legacy deployment_job_agent join table and the related jobAgentId/jobAgents API surface area, consolidating deployment→agent configuration around jobAgentSelector + jobAgentConfig stored directly on the deployment record.
Changes:
- Drops
deployment_job_agent(Drizzle migration + schema cleanup) and removes query/model code that depended on it (TRPC + workspace-engine sqlc). - Updates OpenAPI specs and generated SDK/types to remove legacy deployment job-agent fields (
jobAgentId,jobAgents,DeploymentJobAgent). - Removes UI/router endpoints that listed deployments per job agent (since the join table is gone).
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/workspace-engine-sdk/src/schema.ts | Updates generated SDK types to remove legacy deployment job agent fields. |
| packages/trpc/src/routes/job-agents.ts | Removes jobAgents.deployments endpoint that depended on deployment_job_agent. |
| packages/trpc/src/routes/deployments.ts | Stops reading/writing deployment job agents via relations/join table. |
| packages/db/src/schema/deployment.ts | Removes deploymentJobAgent table + relations from Drizzle schema. |
| packages/db/drizzle/meta/0185_snapshot.json | Drizzle snapshot update reflecting dropped table. |
| packages/db/drizzle/meta/_journal.json | Registers migration 0185_steep_scarecrow. |
| packages/db/drizzle/0185_steep_scarecrow.sql | Drops deployment_job_agent table. |
| apps/workspace-engine/pkg/selector/langs/cel/cel.go | Removes deployment.jobAgentId from CEL context mapping. |
| apps/workspace-engine/pkg/oapi/oapi.gen.go | Removes JobAgentId and DeploymentJobAgent from generated OAPI models. |
| apps/workspace-engine/pkg/db/queries/schema.sql | Removes deployment_job_agent table from sqlc schema. |
| apps/workspace-engine/pkg/db/queries/deployments.sql | Removes sqlc queries for upserting/deleting deployment job agents. |
| apps/workspace-engine/pkg/db/models.go | Removes DeploymentJobAgent sqlc model. |
| apps/workspace-engine/pkg/db/deployments.sql.go | Removes generated sqlc methods for deployment job agents. |
| apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet | Removes legacy schema fields/types from workspace-engine OAPI spec source. |
| apps/workspace-engine/oapi/openapi.json | Removes legacy fields/types from generated workspace-engine OpenAPI. |
| apps/web/app/routes/ws/runners/JobAgentCard.tsx | Removes UI that fetched/displayed deployments for a job agent. |
| apps/web/app/routes/ws/deployments/settings/page.$deploymentId.job-agent.tsx | Removes per-deployment job agent list UI. |
| apps/web/app/routes/ws/deployments/_components/DeploymentProvider.tsx | Updates deployment context type to use selector/config instead of jobAgents[]. |
| apps/api/src/types/openapi.ts | Updates generated API types to remove legacy deployment job agent fields/types. |
| apps/api/src/routes/v1/workspaces/deployments.ts | Stops populating/accepting legacy job-agent fields and stops joining via deployment_job_agent. |
| apps/api/openapi/schemas/deployments.jsonnet | Removes legacy job-agent fields/types from API OpenAPI schema source. |
| apps/api/openapi/openapi.json | Removes legacy job-agent fields/types from generated API OpenAPI document. |
Comments suppressed due to low confidence (3)
apps/workspace-engine/pkg/oapi/oapi.gen.go:319
Deployment.JobAgentId/DeploymentJobAgentwere removed from the generated OpenAPI types, but there are still Go call sites referencingdeployment.JobAgentId(e.g.pkg/workspace/relationships/property.go,pkg/selector/langs/cel/cel_matches_test.go). The workspace-engine build/tests will fail until those references are removed or migrated to the newjobAgentSelector/jobAgentConfigmodel.
// Deployment defines model for Deployment.
type Deployment struct {
Description *string `json:"description,omitempty"`
Id string `json:"id"`
JobAgentConfig JobAgentConfig `json:"jobAgentConfig"`
// JobAgentSelector CEL expression to match job agents
JobAgentSelector string `json:"jobAgentSelector"`
Metadata map[string]string `json:"metadata"`
Name string `json:"name"`
// ResourceSelector CEL expression to determine if the deployment should be used
ResourceSelector *string `json:"resourceSelector,omitempty"`
Slug string `json:"slug"`
}
apps/workspace-engine/pkg/selector/langs/cel/cel.go:182
- Removing
jobAgentIdfrom the CEL deployment context means any stored CEL expressions that referencedeployment.jobAgentIdmay start erroring at evaluation time (map key no longer present). If backward compatibility is needed, consider keeping the key with anullvalue (or another migration strategy) while consumers transition todeployment.jobAgentSelector/config.
func deploymentToMap(d *oapi.Deployment) map[string]any {
m := make(map[string]any, 8)
m["id"] = d.Id
m["name"] = d.Name
m["slug"] = d.Slug
m["jobAgentConfig"] = d.JobAgentConfig
m["jobAgentSelector"] = d.JobAgentSelector
m["metadata"] = d.Metadata
if d.Metadata == nil {
m["metadata"] = make(map[string]any)
}
if d.Description != nil {
m["description"] = *d.Description
}
if d.ResourceSelector != nil {
m["resourceSelector"] = d.ResourceSelector
}
return m
apps/web/app/routes/ws/deployments/settings/page.$deploymentId.job-agent.tsx:5
deploymentis retrieved fromuseDeployment()but no longer used in the component after removing the job agent list. This will trigger a no-unused-vars / noUnusedLocals error in TS builds; either remove the destructuring or usedeployment(e.g., to display selector/config).
export default function DeploymentJobAgentPage() {
const { deployment } = useDeployment();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
108
to
114
| systemsByDeploymentId.set(link.deploymentId, arr); | ||
| } | ||
|
|
||
| const agentsByDeploymentId = await getJobAgentsByDeploymentId(deploymentIds); | ||
| const items = deployments.map((dep) => ({ | ||
| deployment: { | ||
| ...formatDeployment(dep), | ||
| jobAgents: (agentsByDeploymentId[dep.id] ?? []).map( | ||
| ({ ref, config }) => ({ ref, config }), | ||
| ), | ||
| }, | ||
| deployment: formatDeployment(dep), | ||
| systems: (systemsByDeploymentId.get(dep.id) ?? []).map(formatSystem), | ||
| })); |
There was a problem hiding this comment.
formatDeployment() can omit jobAgentSelector when the stored value is "false" (via parseSelector), but the OpenAPI schema marks jobAgentSelector as required. This will cause responses from listDeployments/getDeployment to violate the contract. Consider always returning the raw selector string (including "false") or updating the OpenAPI schema to make jobAgentSelector optional if omission is intended.
Comment on lines
51
to
57
| "additionalProperties": true, | ||
| "type": "object" | ||
| }, | ||
| "jobAgentId": { | ||
| "type": "string" | ||
| }, | ||
| "jobAgentSelector": { | ||
| "description": "CEL expression to match job agents. Defaults to jobAgent.id == \"<jobAgentId>\" if not provided.", | ||
| "description": "CEL expression to match job agents", | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
This OpenAPI change removes jobAgentId/jobAgents from deployment requests/responses. Please ensure any generated clients/schemas are regenerated and any tests updated accordingly (e.g. the generated TS client types and e2e tests that still reference DeploymentJobAgent / jobAgents).
| @@ -0,0 +1 @@ | |||
| DROP TABLE "deployment_job_agent" CASCADE; No newline at end of file | |||
There was a problem hiding this comment.
Using DROP TABLE ... CASCADE can unintentionally drop dependent objects (views, constraints) without making them explicit in the migration diff. If CASCADE isn't required, prefer dropping the table without it; if it is required, consider documenting which dependencies are expected to be dropped.
Suggested change
| DROP TABLE "deployment_job_agent" CASCADE; | |
| DROP TABLE "deployment_job_agent"; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/relationships/property_test.go (1)
149-150: Strengthen selector-path assertions with value checks.These new cases currently only verify
val != nil; they would still pass if the wrong selector string is returned. Consider asserting the extracted string value too.♻️ Proposed test hardening
@@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { val, err := GetPropertyValue(entity, tt.path) @@ }) } + + t.Run("job_agent_selector value", func(t *testing.T) { + val, err := GetPropertyValue(entity, []string{"job_agent_selector"}) + require.NoError(t, err) + assert.Equal(t, `jobAgent.id == "agent-1"`, extractValueAsString(val)) + }) + + t.Run("jobagentselector alias value", func(t *testing.T) { + val, err := GetPropertyValue(entity, []string{"jobagentselector"}) + require.NoError(t, err) + assert.Equal(t, `jobAgent.id == "agent-1"`, extractValueAsString(val)) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/relationships/property_test.go` around lines 149 - 150, The two test cases named "job_agent_selector" and "jobagentselector" currently only assert that the extracted value (`val`) is non-nil; update those assertions to also check the actual string content returned by the selector extraction. Locate the testcase entries in property_test.go (the slice with {name: "job_agent_selector", path: [...] } and {name: "jobagentselector", ...}) and replace the lone `val != nil` check with a type/assertion to string (or call the existing accessor used elsewhere in the file) and assert it equals the expected selector string (e.g., the canonical selector value you expect for those cases) so the test fails if the wrong selector text is produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/relationships/property_test.go`:
- Around line 149-150: The two test cases named "job_agent_selector" and
"jobagentselector" currently only assert that the extracted value (`val`) is
non-nil; update those assertions to also check the actual string content
returned by the selector extraction. Locate the testcase entries in
property_test.go (the slice with {name: "job_agent_selector", path: [...] } and
{name: "jobagentselector", ...}) and replace the lone `val != nil` check with a
type/assertion to string (or call the existing accessor used elsewhere in the
file) and assert it equals the expected selector string (e.g., the canonical
selector value you expect for those cases) so the test fails if the wrong
selector text is produced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 666d0707-63b6-43c3-900d-c53767cedb2b
📒 Files selected for processing (2)
apps/workspace-engine/pkg/workspace/relationships/property.goapps/workspace-engine/pkg/workspace/relationships/property_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/jobs/factory_test.go (1)
204-204: Minor formatting: missing space after comma.Several call sites have
createTestDeployment(t, deployID,deploymentConfig)without a space after the second comma. This violates gofmt style. Line 571 shows the correct formatting with a space.🧹 Suggested fix
- deployment := createTestDeployment(t, deployID,deploymentConfig) + deployment := createTestDeployment(t, deployID, deploymentConfig)Apply this spacing fix at lines 204, 262, 417, 468, 513, 614, 649, and 678.
As per coding guidelines:
**/*.go: Keep Go code gofmt-compliant and follow existing patterns in the package.Also applies to: 262-262, 417-417, 468-468, 513-513, 614-614, 649-649, 678-678
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobs/factory_test.go` at line 204, The createTestDeployment call sites in the test file are missing a space after the comma (e.g., createTestDeployment(t,deployID,deploymentConfig); update every occurrence of createTestDeployment(...) in this file to use the correct formatting createTestDeployment(t, deployID, deploymentConfig) so they are gofmt-compliant and consistent with existing patterns; ensure you update each test invocation of createTestDeployment to include the space after the comma between arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/jobs/factory_test.go`:
- Line 204: The createTestDeployment call sites in the test file are missing a
space after the comma (e.g., createTestDeployment(t,deployID,deploymentConfig);
update every occurrence of createTestDeployment(...) in this file to use the
correct formatting createTestDeployment(t, deployID, deploymentConfig) so they
are gofmt-compliant and consistent with existing patterns; ensure you update
each test invocation of createTestDeployment to include the space after the
comma between arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cab951a4-388e-469f-b251-2e266d9a01c0
📒 Files selected for processing (4)
apps/workspace-engine/pkg/db/convert_test.goapps/workspace-engine/pkg/selector/langs/cel/cel_matches_test.goapps/workspace-engine/pkg/workspace/jobs/factory_test.goe2e/tests/api/deployments.spec.ts
This was referenced Apr 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #934
Summary by CodeRabbit