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"
Comment on lines +240 to +258
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n 'DeploymentJobAgent|jobAgents|DeploymentAgentsSelector|JobAgentsSelector' -g '*.{go,ts,tsx,jsonnet}' --max-count=50

Repository: ctrlplanedev/ctrlplane

Length of output: 48932


🏁 Script executed:

# First, examine the deployment agent selector implementation
head -150 apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector.go

Repository: ctrlplanedev/ctrlplane

Length of output: 3431


🏁 Script executed:

# Check the jsonnet schema definition
cat apps/workspace-engine/oapi/spec/schemas/deployments.jsonnet

Repository: ctrlplanedev/ctrlplane

Length of output: 3342


🏁 Script executed:

# Check test patterns for empty/missing 'if'
grep -A 5 -B 5 "If: \"\"" apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector_test.go | head -40

Repository: ctrlplanedev/ctrlplane

Length of output: 48


🏁 Script executed:

# Look at specific test cases to understand behavior
sed -n '185,290p' apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector_test.go

Repository: ctrlplanedev/ctrlplane

Length of output: 3631


🏁 Script executed:

# Check the generated DeploymentJobAgent type
sed -n '315,330p' apps/workspace-engine/pkg/oapi/oapi.gen.go

Repository: ctrlplanedev/ctrlplane

Length of output: 620


🏁 Script executed:

# Verify the key logic: confirm how empty If is handled and what ref points to
grep -n "deploymentJobAgent.If\|deploymentJobAgent.Ref" apps/workspace-engine/pkg/workspace/jobagents/deployment_agent_selector.go

Repository: ctrlplanedev/ctrlplane

Length of output: 437


🏁 Script executed:

# Check if the file is using Prettier formatting
head -20 apps/workspace-engine/oapi/openapi.json

Repository: ctrlplanedev/ctrlplane

Length of output: 653


Make DeploymentJobAgent.if optional and document ref.

Line 87 of deployment_agent_selector.go shows that empty if expressions are treated as valid (the agent is included). The current schema requires if, but JSON clients that omit it will default to an empty string—which works fine in the implementation. Similarly, ref has no description. Align the schema with actual behavior by removing if from the required array, adding a default value, and documenting what ref references.

🧭 Suggested schema tweaks
        "if": {
           "description": "CEL expression to determine if the job agent should be used",
-          "type": "string"
+          "type": "string",
+          "default": "true"
        },
        "ref": {
-          "type": "string"
+          "description": "Reference to the job agent (JobAgent.id)",
+          "type": "string"
        }
...
        "required": [
           "ref",
-          "config",
-          "if"
+          "config"
        ],
📝 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": {
"properties": {
"config": {
"$ref": "#/components/schemas/JobAgentConfig"
},
"if": {
"description": "CEL expression to determine if the job agent should be used",
"type": "string"
},
"ref": {
"type": "string"
}
},
"required": [
"ref",
"config",
"if"
],
"type": "object"
"DeploymentJobAgent": {
"properties": {
"config": {
"$ref": "#/components/schemas/JobAgentConfig"
},
"if": {
"description": "CEL expression to determine if the job agent should be used",
"type": "string",
"default": "true"
},
"ref": {
"description": "Reference to the job agent (JobAgent.id)",
"type": "string"
}
},
"required": [
"ref",
"config"
],
"type": "object"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/workspace-engine/oapi/openapi.json` around lines 240 - 258, Update the
OpenAPI schema for the DeploymentJobAgent object: remove "if" from the required
list so the CEL expression is optional, add a "default" (empty string) to the
"if" property to reflect that omission is treated as an empty expression by
deployment_agent_selector.go, and add a descriptive "description" to the "ref"
property explaining what it references (e.g., the identifier/name of the job
agent implementation or registry entry). Ensure the schema still references
JobAgentConfig for "config" and keep types unchanged for compatibility with
existing code.

},
"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
Copy Markdown
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