Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions apps/workspace-engine/oapi/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,12 @@
"jobAgentId": {
"type": "string"
},
"jobAgents": {
"items": {
"$ref": "#/components/schemas/DeploymentJobAgent"
},
"type": "array"
},
"metadata": {
"additionalProperties": {
"type": "string"
Expand Down Expand Up @@ -231,6 +237,26 @@
],
"type": "object"
},
"DeploymentJobAgent": {
"properties": {
"config": {
"$ref": "#/components/schemas/JobAgentConfig"
},
"ref": {
"type": "string"
},
"selector": {
"description": "CEL expression to determine if the job agent should be used",
"type": "string"
}
},
"required": [
"ref",
"config",
"selector"
],
"type": "object"
},
"DeploymentVariable": {
"properties": {
"defaultValue": {
Expand Down
11 changes: 11 additions & 0 deletions apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,22 @@ local openapi = import '../lib/openapi.libsonnet';
description: { type: 'string' },
jobAgentId: { type: 'string' },
jobAgentConfig: openapi.schemaRef('JobAgentConfig'),
jobAgents: { type: 'array', items: openapi.schemaRef('DeploymentJobAgent') },
resourceSelector: openapi.schemaRef('Selector'),
metadata: { type: 'object', additionalProperties: { type: 'string' } },
},
},

DeploymentJobAgent: {
type: 'object',
required: ['ref', 'config', 'selector'],
properties: {
ref: { type: 'string' },
config: openapi.schemaRef('JobAgentConfig'),
selector: { type: 'string', description: 'CEL expression to determine if the job agent should be used' },
},
},
Comment on lines +20 to +28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Check the current state of deployments.jsonnet schema
cd apps/workspace-engine && cat -n oapi/spec/schemas/deployments.jsonnet | sed -n '1,50p'

Repository: ctrlplanedev/ctrlplane

Length of output: 2007


🏁 Script executed:

# Look for the DeploymentJobAgent definition and surrounding context
cd apps/workspace-engine && rg -n "DeploymentJobAgent" oapi/spec/schemas/deployments.jsonnet -A 10 -B 2

Repository: ctrlplanedev/ctrlplane

Length of output: 802


🏁 Script executed:

# Check the generated Go type for DeploymentJobAgent
cd apps/workspace-engine && rg -n "type DeploymentJobAgent struct" pkg/oapi/oapi.gen.go -A 15

Repository: ctrlplanedev/ctrlplane

Length of output: 694


🏁 Script executed:

# Find the test mentioned in the review
cd apps/workspace-engine && rg -n "AllAgentsNoCondition" . --type go -B 3 -A 15

Repository: ctrlplanedev/ctrlplane

Length of output: 1414


🏁 Script executed:

# Check if there are other uses of DeploymentJobAgent in tests
cd apps/workspace-engine && rg -n "DeploymentJobAgent{" . --type go -B 2 -A 5

Repository: ctrlplanedev/ctrlplane

Length of output: 167


🏁 Script executed:

# Search for DeploymentJobAgent struct construction
cd apps/workspace-engine && rg -n "DeploymentJobAgent" test/e2e/engine_deployment_test.go -A 5 -B 2

Repository: ctrlplanedev/ctrlplane

Length of output: 7519


🏁 Script executed:

# Check the specific test to see how it uses DeploymentJobAgent
cd apps/workspace-engine && sed -n '1013,1100p' test/e2e/engine_deployment_test.go

Repository: ctrlplanedev/ctrlplane

Length of output: 3056


🏁 Script executed:

# Search for where Selector is being set (or not set) in deployments
cd apps/workspace-engine && rg -n "Selector" test/e2e/engine_deployment_test.go | grep -i "agent\|deployment"

Repository: ctrlplanedev/ctrlplane

Length of output: 1989


🏁 Script executed:

# Look for all test cases that construct DeploymentJobAgent
cd apps/workspace-engine && rg -n "jobAgents" test/e2e/engine_deployment_test.go -B 5 -A 10

Repository: ctrlplanedev/ctrlplane

Length of output: 48


🏁 Script executed:

# Check how selector is evaluated in the deployment job agent logic
cd apps/workspace-engine && rg -n "selector" pkg/ --type go -i | grep -i "agent\|deployment" | head -20

Repository: ctrlplanedev/ctrlplane

Length of output: 3337


🏁 Script executed:

# Look for where DeploymentJobAgent.Selector is actually evaluated/used
cd apps/workspace-engine && rg -n "\.Selector" pkg/ --type go -B 3 -A 3 | grep -i agent -A 5 -B 5

Repository: ctrlplanedev/ctrlplane

Length of output: 5529


🏁 Script executed:

# Get the full context of the deployment_agent_selector.go logic
cd apps/workspace-engine && sed -n '85,105p' pkg/workspace/jobagents/deployment_agent_selector.go

Repository: ctrlplanedev/ctrlplane

Length of output: 866


🏁 Script executed:

# Check if there are other places where DeploymentJobAgent.Selector is evaluated
cd apps/workspace-engine && rg -n "DeploymentJobAgent" pkg/workspace/jobagents/ --type go -A 8 | grep -i "selector"

Repository: ctrlplanedev/ctrlplane

Length of output: 17722


selector should be optional, not required

The tests (e.g., TestEngine_DeploymentJobAgentsArray_AllAgentsNoCondition) construct DeploymentJobAgent without a Selector field, relying on the Go zero value "". With selector marked required in the schema, the generated type is Selector string (non-pointer), making it impossible to distinguish "no filter" from "empty string" at the API boundary. The application logic in pkg/workspace/jobagents/deployment_agent_selector.go (lines 90–100) treats empty string as "always match"—it skips CEL compilation when Selector != "" is false—but an empty string is not a valid CEL expression.

Making selector optional cleanly represents "omit or leave empty to always use this agent":

♻️ Proposed fix
  DeploymentJobAgent: {
    type: 'object',
-   required: ['ref', 'config', 'selector'],
+   required: ['ref', 'config'],
    properties: {
      ref: { type: 'string', description: 'Job agent ID to use for this configuration' },
      config: openapi.schemaRef('JobAgentConfig'),
-     selector: { type: 'string', description: 'CEL expression to determine if the job agent should be used' },
+     selector: { type: 'string', description: 'CEL expression to determine if the job agent should be used. Omit or leave empty to always use this agent.' },
    },
  },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
DeploymentJobAgent: {
type: 'object',
required: ['ref', 'config', 'selector'],
properties: {
ref: { type: 'string' },
config: openapi.schemaRef('JobAgentConfig'),
selector: { type: 'string', description: 'CEL expression to determine if the job agent should be used' },
},
},
DeploymentJobAgent: {
type: 'object',
required: ['ref', 'config'],
properties: {
ref: { type: 'string', description: 'Job agent ID to use for this configuration' },
config: openapi.schemaRef('JobAgentConfig'),
selector: { type: 'string', description: 'CEL expression to determine if the job agent should be used. Omit or leave empty to always use this agent.' },
},
},
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet` around lines 20
- 28, The schema marks DeploymentJobAgent.selector as required which forces the
generated Go type to be a non-pointer string; remove "selector" from the
required array in the DeploymentJobAgent JSONNet so selector is optional (i.e.,
not required) and the generated Go model becomes *string (or omitempty),
allowing callers to omit it and letting the existing logic in
pkg/workspace/jobagents/deployment_agent_selector.go treat nil/empty as "always
match"; after the change regenerate the OpenAPI/Go client types so the models
reflect the optional selector.


DeploymentWithVariablesAndSystems: {
type: 'object',
required: ['deployment', 'variables', 'systems'],
Expand Down
21 changes: 14 additions & 7 deletions apps/workspace-engine/pkg/db/deployments.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions apps/workspace-engine/pkg/db/models.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 5 additions & 4 deletions apps/workspace-engine/pkg/db/queries/deployments.sql
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,18 @@ WHERE workspace_id = $1
LIMIT COALESCE(sqlc.narg('limit')::int, 5000);

-- name: ListDeploymentsBySystemID :many
SELECT d.id, d.name, d.description, d.job_agent_id, d.job_agent_config, d.resource_selector, d.metadata, d.workspace_id
SELECT d.id, d.name, d.description, d.job_agent_id, d.job_agent_config, d.job_agents, d.resource_selector, d.metadata, d.workspace_id
FROM deployment d
INNER JOIN system_deployment sd ON sd.deployment_id = d.id
WHERE sd.system_id = $1;

-- name: UpsertDeployment :one
INSERT INTO deployment (id, name, description, job_agent_id, job_agent_config, resource_selector, metadata, workspace_id)
VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
INSERT INTO deployment (id, name, description, job_agent_id, job_agent_config, job_agents, resource_selector, metadata, workspace_id)
VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)
ON CONFLICT (id) DO UPDATE
SET name = EXCLUDED.name, description = EXCLUDED.description, job_agent_id = EXCLUDED.job_agent_id,
job_agent_config = EXCLUDED.job_agent_config, resource_selector = EXCLUDED.resource_selector,
job_agent_config = EXCLUDED.job_agent_config, job_agents = EXCLUDED.job_agents,
resource_selector = EXCLUDED.resource_selector,
metadata = EXCLUDED.metadata, workspace_id = EXCLUDED.workspace_id
RETURNING *;

Expand Down
1 change: 1 addition & 0 deletions apps/workspace-engine/pkg/db/queries/schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ CREATE TABLE deployment (
description TEXT NOT NULL DEFAULT '',
job_agent_id UUID,
job_agent_config JSONB NOT NULL DEFAULT '{}',
job_agents JSONB NOT NULL DEFAULT '[]',
resource_selector TEXT DEFAULT 'false',
metadata JSONB NOT NULL DEFAULT '{}',
workspace_id UUID REFERENCES workspace(id)
Expand Down
3 changes: 3 additions & 0 deletions apps/workspace-engine/pkg/db/sqlc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ sql:
- column: "deployment.job_agent_config"
go_type:
type: "map[string]any"
- column: "deployment.job_agents"
go_type:
type: "[]byte"
- column: "deployment.metadata"
go_type:
type: "map[string]string"
Expand Down
Loading
Loading