chore: add job agent selector field to oapi schemas#929
Merged
adityachoudhari26 merged 2 commits intomainfrom Apr 7, 2026
Merged
chore: add job agent selector field to oapi schemas#929adityachoudhari26 merged 2 commits intomainfrom
adityachoudhari26 merged 2 commits intomainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded an optional Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,240,255,0.5)
participant Client
end
rect rgba(200,255,220,0.5)
participant API
end
rect rgba(255,230,200,0.5)
participant DB
end
Client->>API: POST /workspaces/:id/deployments (body with jobAgentId or jobAgentSelector)
API->>API: validate body (UUID for jobAgentId), derive selector if needed
API->>DB: INSERT/UPSERT deployment (jobAgentSelector, jobAgentConfig conditional)
DB-->>API: persist result
API->>API: formatDeployment (parseSelector -> expose jobAgentSelector/jobAgentConfig)
API-->>Client: 201 Created (deployment with jobAgentSelector)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
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 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
Adds support for a deployment-level jobAgentSelector (CEL expression) across OpenAPI schemas/types and validates the behavior via new E2E coverage. This aligns deployment creation/upsert with the newer “selector-driven job agent selection” model.
Changes:
- Add
jobAgentSelectorto deployment schemas in API + workspace-engine OpenAPI sources and regenerated artifacts (TS + Go). - Update API deployment routes to persist/return
jobAgentSelectorand ensurejobAgentConfigis always present in responses. - Add E2E tests covering custom selector, defaulting selector from
jobAgentId, and selector updates via upsert; update e2e OpenAPI generation input path.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/tests/api/deployments.spec.ts | Adds E2E coverage for create/upsert/get behavior around jobAgentSelector. |
| e2e/package.json | Points openapi-typescript generation at apps/api/openapi/openapi.json. |
| e2e/api/schema.ts | Regenerated E2E OpenAPI TS types including jobAgentSelector. |
| apps/workspace-engine/pkg/oapi/oapi.gen.go | Regenerated Go types to include jobAgentSelector on Deployment. |
| apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet | Adds jobAgentSelector to workspace-engine deployment schema source. |
| apps/workspace-engine/oapi/openapi.json | Regenerated workspace-engine OpenAPI JSON including jobAgentSelector. |
| apps/web/app/api/openapi.ts | Regenerated web OpenAPI TS types including jobAgentSelector (and order query type). |
| apps/api/src/types/openapi.ts | Regenerated API OpenAPI TS types including jobAgentSelector. |
| apps/api/src/routes/v1/workspaces/deployments.ts | Persists and returns jobAgentSelector; ensures jobAgentConfig defaults to {} in responses. |
| apps/api/openapi/schemas/deployments.jsonnet | Adds jobAgentSelector to API deployment schema source with descriptions. |
| apps/api/openapi/openapi.json | Regenerated API OpenAPI JSON including jobAgentSelector. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
231
to
248
| const isValid = validResourceSelector(body.resourceSelector); | ||
| if (!isValid) throw new ApiError("Invalid resource selector", 400); | ||
|
|
||
| const id = uuidv4(); | ||
|
|
||
| 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", | ||
| jobAgentSelector: | ||
| body.jobAgentSelector ?? | ||
| (jobAgentId ? `jobAgent.id == "${jobAgentId}"` : "false"), | ||
| jobAgentConfig, |
There was a problem hiding this comment.
jobAgentSelector is accepted and persisted without any CEL validation, unlike resourceSelector. This can allow invalid expressions to be stored and later fail during evaluation. Consider validating body.jobAgentSelector (and possibly body.jobAgents[].selector) with the existing CEL parser (validResourceSelector or a renamed generalized helper) and returning 400 on parse failure with a clear error message.
Comment on lines
+315
to
+322
| ...(body.jobAgentSelector != null | ||
| ? { jobAgentSelector: body.jobAgentSelector, jobAgentConfig } | ||
| : jobAgentId != null | ||
| ? { | ||
| jobAgentSelector: `jobAgent.id == "${jobAgentId}"`, | ||
| jobAgentConfig, | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
In the upsert conflict update branch, when body.jobAgentSelector is provided but jobAgentConfig is omitted, jobAgentConfig is computed as {} and will overwrite any existing config. If clients are allowed to update the selector independently, consider only updating jobAgentConfig when it is explicitly provided (or when jobAgents is provided), to avoid clobbering existing configuration.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/tests/api/deployments.spec.ts (1)
315-329: Optionally assert the second upsert response status for clearer failures.You already validate the final state via GET; adding a direct status check here improves diagnosability when upsert fails.
Suggested test refinement
- await api.PUT( + const updateRes = await api.PUT( "/v1/workspaces/{workspaceId}/deployments/{deploymentId}", { params: { path: { workspaceId: workspace.id, deploymentId }, }, body: { name, slug: name, jobAgentSelector: updatedSelector, jobAgentConfig: { env: "production" }, }, }, ); + expect(updateRes.response.status).toBe(202);🤖 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 315 - 329, Capture the response returned by the second api.PUT call to update the deployment and assert its HTTP status (e.g., status === 200) before proceeding to the GET; specifically, replace the fire-and-forget api.PUT invocation around the block that passes params.path { workspaceId, deploymentId } and body { name, slug, jobAgentSelector: updatedSelector, jobAgentConfig } with a stored response (e.g., const res = await api.PUT(...)) and add an assertion that res.status equals the expected success code to make failures more diagnosable.
🤖 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 245-247: The CEL selector currently injects jobAgentId directly
into the string via jobAgentSelector which allows quotes/backslashes to break
the expression; add a helper (e.g., escapeCelString or similar) that escapes
backslashes and double quotes (replace \ with \\ and " with \") and use it
wherever jobAgentId is interpolated (the jobAgentSelector assignment at
jobAgentSelector, and the other occurrences around lines 301-303 and 319-320) so
the constructed CEL expression is always a safely-quoted string.
---
Nitpick comments:
In `@e2e/tests/api/deployments.spec.ts`:
- Around line 315-329: Capture the response returned by the second api.PUT call
to update the deployment and assert its HTTP status (e.g., status === 200)
before proceeding to the GET; specifically, replace the fire-and-forget api.PUT
invocation around the block that passes params.path { workspaceId, deploymentId
} and body { name, slug, jobAgentSelector: updatedSelector, jobAgentConfig }
with a stored response (e.g., const res = await api.PUT(...)) and add an
assertion that res.status equals the expected success code to make failures more
diagnosable.
🪄 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: 8a4d6c0b-8890-47a6-b52c-e26741b3a744
📒 Files selected for processing (11)
apps/api/openapi/openapi.jsonapps/api/openapi/schemas/deployments.jsonnetapps/api/src/routes/v1/workspaces/deployments.tsapps/api/src/types/openapi.tsapps/web/app/api/openapi.tsapps/workspace-engine/oapi/openapi.jsonapps/workspace-engine/oapi/spec/schemas/deployments.jsonnetapps/workspace-engine/pkg/oapi/oapi.gen.goe2e/api/schema.tse2e/package.jsone2e/tests/api/deployments.spec.ts
Comment on lines
+245
to
+247
| jobAgentSelector: | ||
| body.jobAgentSelector ?? | ||
| (jobAgentId ? `jobAgent.id == "${jobAgentId}"` : "false"), |
Contributor
There was a problem hiding this comment.
Escape jobAgentId before embedding it into CEL.
The derived selector is built with raw string interpolation, so IDs containing quotes/backslashes can break or alter the CEL expression.
🔧 Proposed fix
const parseSelector = (raw: string | null | undefined): string | undefined => {
if (raw == null || raw === "false") return undefined;
return raw;
};
+
+const deriveJobAgentSelector = (
+ explicitSelector: string | null | undefined,
+ jobAgentId: string | null | undefined,
+): string =>
+ explicitSelector ??
+ (jobAgentId != null ? `jobAgent.id == ${JSON.stringify(jobAgentId)}` : "false");
...
await db.insert(schema.deployment).values({
id,
name: body.name,
description: body.description ?? "",
resourceSelector: body.resourceSelector ?? "false",
- jobAgentSelector:
- body.jobAgentSelector ??
- (jobAgentId ? `jobAgent.id == "${jobAgentId}"` : "false"),
+ jobAgentSelector: deriveJobAgentSelector(body.jobAgentSelector, jobAgentId),
jobAgentConfig,
metadata: body.metadata ?? {},
workspaceId,
});
...
.values({
id: deploymentId,
name: body.name,
description: body.description ?? "",
resourceSelector: body.resourceSelector ?? "false",
- jobAgentSelector:
- body.jobAgentSelector ??
- (jobAgentId ? `jobAgent.id == "${jobAgentId}"` : "false"),
+ jobAgentSelector: deriveJobAgentSelector(body.jobAgentSelector, jobAgentId),
jobAgentConfig,
metadata: body.metadata ?? {},
workspaceId,
})
...
...(body.jobAgentSelector != null
? { jobAgentSelector: body.jobAgentSelector, jobAgentConfig }
: jobAgentId != null
? {
- jobAgentSelector: `jobAgent.id == "${jobAgentId}"`,
+ jobAgentSelector: `jobAgent.id == ${JSON.stringify(jobAgentId)}`,
jobAgentConfig,
}
: {}),Also applies to: 301-303, 319-320
🤖 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 245 - 247, The
CEL selector currently injects jobAgentId directly into the string via
jobAgentSelector which allows quotes/backslashes to break the expression; add a
helper (e.g., escapeCelString or similar) that escapes backslashes and double
quotes (replace \ with \\ and " with \") and use it wherever jobAgentId is
interpolated (the jobAgentSelector assignment at jobAgentSelector, and the other
occurrences around lines 301-303 and 319-320) so the constructed CEL expression
is always a safely-quoted string.
This was referenced Apr 8, 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 #916
Summary by CodeRabbit