refactor: convert controllers to use selector based job agent matching#932
Conversation
📝 WalkthroughWalkthroughRefactors deployment job-agent handling from embedded agent refs to workspace-scoped CEL selector expressions. Adds Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Controller
participant Getter as DB/Getter
participant Selector as Selector (CEL)
participant JobAgent as JobAgent Processor
rect rgba(34, 139, 34, 0.5)
Note over Controller,JobAgent: New selector-based flow
Controller->>Getter: GetDeployment(id) // includes JobAgentSelector
Getter-->>Controller: Deployment (selector, config)
Controller->>Getter: ListJobAgentsByWorkspaceID(workspaceID)
Getter-->>Controller: All workspace agents
Controller->>Selector: MatchJobAgents(selector, agents)
Selector->>Selector: Compile & Evaluate per agent
Selector-->>Controller: Matched agents list
Controller->>JobAgent: Process matched agents (dispatch/jobs)
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 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
Refactors workspace-engine controllers to stop relying on per-deployment job_agents references and instead select job agents via a CEL-based jobAgentSelector, using workspace-scoped job agent listings.
Changes:
- Updated
jobeligibility,jobdispatch, anddeploymentplancontrollers to resolve eligible job agents viaselector.MatchJobAgents(...)overListJobAgentsByWorkspaceID(...). - Added a new
pkg/selectorhelper for CEL evaluation against job agents. - Extended DB/sqlc models and queries to include
deployment.job_agent_selectoranddeployment.job_agent_config, and updated controller tests/mocks accordingly.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/workspace-engine/test/controllers/harness/pipeline.go | Test harness deployment now uses JobAgentSelector/JobAgentConfig instead of JobAgents. |
| apps/workspace-engine/test/controllers/harness/mocks.go | Harness mock getter now supports workspace-scoped job agent listing. |
| apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go | Job creation now matches agents via selector + workspace agent list, with added tracing. |
| apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go | Updated tests to cover selector nil/empty/no-match and selector matching scenarios. |
| apps/workspace-engine/svc/controllers/jobeligibility/getters.go | Added ListJobAgentsByWorkspaceID to the controller Getter interface. |
| apps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.go | Switched deployment fetch to GetDeploymentByID and implemented workspace agent listing. |
| apps/workspace-engine/svc/controllers/jobdispatch/reconcile.go | Dispatch controller now selects agents via selector + workspace agent list. |
| apps/workspace-engine/svc/controllers/jobdispatch/reconcile_test.go | Updated jobdispatch tests to use selector-based matching and workspace agent lists. |
| apps/workspace-engine/svc/controllers/jobdispatch/getters.go | Added ListJobAgentsByWorkspaceID to the dispatch Getter interface. |
| apps/workspace-engine/svc/controllers/jobdispatch/getters_postgres.go | Switched deployment fetch to GetDeploymentByID and implemented workspace agent listing. |
| apps/workspace-engine/svc/controllers/jobdispatch/controller.go | Plumbed workspaceID into reconcile path for agent listing. |
| apps/workspace-engine/svc/controllers/deploymentplan/getters.go | Added ListJobAgentsByWorkspaceID to deployment plan Getter interface. |
| apps/workspace-engine/svc/controllers/deploymentplan/getters_postgres.go | Switched deployment fetch to GetDeploymentByID and implemented workspace agent listing. |
| apps/workspace-engine/svc/controllers/deploymentplan/controller.go | Deployment plan now matches agents by selector and deep-merges config using deployment-level config. |
| apps/workspace-engine/svc/controllers/deploymentplan/controller_test.go | Updated deploymentplan tests for selector-based behavior and config merge expectations. |
| apps/workspace-engine/pkg/selector/jobagents.go | New helper to evaluate job-agent selectors (CEL) against workspace agents. |
| apps/workspace-engine/pkg/db/queries/schema.sql | Added job_agent_selector and job_agent_config columns to deployment schema for sqlc. |
| apps/workspace-engine/pkg/db/models.go | DB model extended with JobAgentSelector and JobAgentConfig. |
| apps/workspace-engine/pkg/db/deployments.sql.go | Queries updated to select/scan new deployment columns; return types adjusted. |
| apps/workspace-engine/pkg/db/convert.go | Deployment conversion now includes selector/config fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const upsertDeployment = `-- name: UpsertDeployment :one | ||
| INSERT INTO deployment (id, name, description, resource_selector, metadata, workspace_id) | ||
| VALUES ($1, $2, $3, $4, $5, $6) | ||
| ON CONFLICT (id) DO UPDATE | ||
| SET name = EXCLUDED.name, description = EXCLUDED.description, | ||
| resource_selector = EXCLUDED.resource_selector, | ||
| metadata = EXCLUDED.metadata, workspace_id = EXCLUDED.workspace_id | ||
| RETURNING id, name, description, resource_selector, metadata, workspace_id | ||
| RETURNING id, name, description, resource_selector, job_agent_selector, job_agent_config, metadata, workspace_id |
There was a problem hiding this comment.
UpsertDeployment now returns job_agent_selector / job_agent_config, but the INSERT/UPDATE statement never sets these columns. As a result, callers cannot persist a selector or config via this upsert (it will always keep defaults/previous values). Add these columns to the INSERT column list + VALUES and to the ON CONFLICT DO UPDATE SET clause, and extend UpsertDeploymentParams accordingly.
| func ToOapiDeployment(row Deployment) *oapi.Deployment { | ||
| d := &oapi.Deployment{ | ||
| Id: row.ID.String(), | ||
| Name: row.Name, | ||
| Metadata: row.Metadata, | ||
| Id: row.ID.String(), | ||
| Name: row.Name, | ||
| Metadata: row.Metadata, | ||
| JobAgentConfig: oapi.JobAgentConfig(row.JobAgentConfig), |
There was a problem hiding this comment.
ToOapiDeploymentWithJobAgents calls ToOapiDeployment with a partially-populated Deployment struct (it omits JobAgentSelector and JobAgentConfig). Since ToOapiDeployment now populates these fields from the input row, this conversion path will drop selector/config data from GetDeploymentWithJobAgentsRow. Consider wiring row.JobAgentSelector / row.JobAgentConfig through so the OAPI deployment is consistent regardless of which query was used.
| if row.Description != "" { | ||
| d.Description = &row.Description | ||
| } | ||
| if row.JobAgentSelector != "" { |
There was a problem hiding this comment.
job_agent_selector defaults to the literal string "false" in the DB schema. This conversion sets Deployment.JobAgentSelector whenever the DB value is non-empty, which will treat the default "false" as an explicitly-configured selector. Downstream controllers only special-case nil/empty, so they’ll do extra work (list agents + match) and may emit different messages for the default case. Consider treating "false" the same as unset here (e.g., only set the pointer when value is not "" and not "false").
| if row.JobAgentSelector != "" { | |
| if row.JobAgentSelector != "" && row.JobAgentSelector != "false" { |
| if selectorIsNil || *deployment.JobAgentSelector == "" { | ||
| msg := fmt.Sprintf("No job agents configured for deployment '%s'", deployment.Name) | ||
| span.AddEvent(msg) | ||
| return r.createFailureJob(ctx, oapi.JobStatusInvalidJobAgent, msg) | ||
| } | ||
|
|
||
| for _, agentRef := range *deployment.JobAgents { | ||
| agentID, err := uuid.Parse(agentRef.Ref) | ||
| if err != nil { | ||
| return fmt.Errorf("parse job agent id: %w", err) | ||
| } | ||
| allAgents, err := r.getter.ListJobAgentsByWorkspaceID(ctx, r.workspaceID) |
There was a problem hiding this comment.
The default DB selector value is the literal string "false" (see schema default). Here you only treat nil/empty as “no agents configured”, so a default "false" selector will fall through, trigger a workspace-wide agent list, and then create an InvalidJobAgent failure with “No job agents matched selector…”. If "false" is intended to mean “no agents configured”, short-circuit it alongside nil/empty to avoid the extra DB query and to keep the failure reason/message consistent.
| if deployment.JobAgentSelector == nil || *deployment.JobAgentSelector == "" { | ||
| return nil, fmt.Errorf("deployment job agent selector is empty") | ||
| } | ||
|
|
||
| jobAgents := make([]oapi.JobAgent, 0) | ||
| for _, jobAgent := range *deployment.JobAgents { | ||
| jobAgent, err := getter.GetJobAgent(ctx, uuid.MustParse(jobAgent.Ref)) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| jobAgents = append(jobAgents, *jobAgent) | ||
| allAgents, err := getter.ListJobAgentsByWorkspaceID(ctx, workspaceID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list job agents: %w", err) | ||
| } | ||
| return jobAgents, nil | ||
|
|
There was a problem hiding this comment.
deployment.JobAgentSelector == "false" is treated by selector.MatchJobAgents as “no matches”, but this code always queries ListJobAgentsByWorkspaceID before matching. Add an early return for selector == "false" (or treat it as empty) so deployments with the default selector don’t incur a full workspace agent list on every reconcile.
| if deployment.JobAgentSelector == nil || *deployment.JobAgentSelector == "" { | ||
| if err := c.setter.CompletePlan(ctx, planID); err != nil { | ||
| return reconcile.Result{}, fmt.Errorf("mark plan completed: %w", err) | ||
| } | ||
| return reconcile.Result{}, nil | ||
| } | ||
|
|
||
| allAgents, err := c.getter.ListJobAgentsByWorkspaceID(ctx, plan.WorkspaceID) | ||
| if err != nil { | ||
| return reconcile.Result{}, fmt.Errorf("list job agents: %w", err) | ||
| } | ||
|
|
||
| matchedAgents, err := selector.MatchJobAgents(ctx, *deployment.JobAgentSelector, allAgents) | ||
| if err != nil { |
There was a problem hiding this comment.
Same as jobdispatch/jobeligibility: if the DB default selector string is "false", this code will still query all workspace agents and then MatchJobAgents will immediately return no matches. Consider short-circuiting selector == "false" alongside nil/empty so plans for deployments with the default selector avoid an unnecessary agent listing query.
| var jobAgentEnv, _ = celutil.NewEnvBuilder(). | ||
| WithMapVariable("jobAgent"). | ||
| WithStandardExtensions(). | ||
| BuildCached(12 * time.Hour) | ||
|
|
There was a problem hiding this comment.
BuildCached can return an error (e.g., cache initialization failure). The current var jobAgentEnv, _ = ...BuildCached(...) ignores that error, which can leave jobAgentEnv nil and cause a panic later on jobAgentEnv.Compile(...). Consider handling the error explicitly (panic in init, or switch to a helper that returns a non-nil env or fails fast).
| d := &oapi.Deployment{ | ||
| Id: row.ID.String(), | ||
| Name: row.Name, | ||
| Metadata: row.Metadata, | ||
| Id: row.ID.String(), | ||
| Name: row.Name, | ||
| Metadata: row.Metadata, | ||
| JobAgentConfig: oapi.JobAgentConfig(row.JobAgentConfig), | ||
| } |
There was a problem hiding this comment.
Deployment.JobAgentConfig is a required field in the OAPI model (no omitempty). If row.JobAgentConfig is nil (possible since job_agent_config is nullable), this conversion will set it to a nil map which will JSON-encode as null instead of {}. Consider defaulting to an empty config map when the DB value is nil to keep API output consistent with the schema.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go (1)
100-102: Consider adding error propagation forListJobAgentsByWorkspaceID.Same as the deploymentplan mock—this always returns
nilerror. For consistency and potential error-path testing, consider adding error support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go` around lines 100 - 102, The mock method mockGetter.ListJobAgentsByWorkspaceID currently always returns m.workspaceAgents with a nil error; change the mock to support returning an error so tests can simulate failure paths. Add an error field (e.g., workspaceAgentsErr) to the mockGetter struct, update ListJobAgentsByWorkspaceID to return (m.workspaceAgents, m.workspaceAgentsErr), and update tests that construct mockGetter to set workspaceAgentsErr when needed (mirroring the deploymentplan mock pattern).apps/workspace-engine/svc/controllers/deploymentplan/controller_test.go (1)
83-85: Mock doesn't propagate errors forListJobAgentsByWorkspaceID.The
ListJobAgentsByWorkspaceIDmock always returnsnilerror, unlike other mock methods (e.g.,GetJobAgent) that can returnm.agentErr. Consider adding an error field for consistent error-path testing.func (m *mockGetter) ListJobAgentsByWorkspaceID(_ context.Context, _ uuid.UUID) ([]oapi.JobAgent, error) { + if m.agentErr != nil { + return nil, m.agentErr + } return m.workspaceAgents, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/deploymentplan/controller_test.go` around lines 83 - 85, The mock ListJobAgentsByWorkspaceID currently always returns nil error; update the mockGetter to support error-path testing by adding an error field (e.g., agentsErr) and change ListJobAgentsByWorkspaceID to return (m.workspaceAgents, m.agentsErr) instead of nil; also update any tests that rely on this mock to set agentsErr when they need to simulate failures (alternatively, if you prefer reuse, return m.agentErr from ListJobAgentsByWorkspaceID to align with GetJobAgent).
🤖 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/workspace-engine/pkg/db/convert.go`:
- Around line 13-23: The ToOapiDeploymentWithJobAgents conversion path is
missing propagation of the new job-agent fields; update the
ToOapiDeploymentWithJobAgents function to set Deployment.JobAgentConfig (using
oapi.JobAgentConfig(row.JobAgentConfig)) and Deployment.JobAgentSelector (as a
pointer when non-empty) just like the other conversion, and also set Description
as a pointer when row.Description != "" so the WithJobAgents path returns the
same complete payload; locate the Deployment construction in
ToOapiDeploymentWithJobAgents and add those field assignments with the same
nil/empty checks used in the existing conversion code.
In `@apps/workspace-engine/svc/controllers/jobdispatch/controller.go`:
- Around line 60-61: Replace the panic-prone uuid.MustParse usage for
item.WorkspaceID (and any similar MustParse for item.JobID) with uuid.Parse and
add proper error handling before calling c.reconcileJob; specifically, parse
item.WorkspaceID (and item.JobID) using uuid.Parse, check the returned error,
log or return the error consistent with other reconcile.Processor
implementations (policyeval, deploymentplan, etc.), and only call
c.reconcileJob(ctx, workspaceID, jobID, job) when parsing succeeds.
---
Nitpick comments:
In `@apps/workspace-engine/svc/controllers/deploymentplan/controller_test.go`:
- Around line 83-85: The mock ListJobAgentsByWorkspaceID currently always
returns nil error; update the mockGetter to support error-path testing by adding
an error field (e.g., agentsErr) and change ListJobAgentsByWorkspaceID to return
(m.workspaceAgents, m.agentsErr) instead of nil; also update any tests that rely
on this mock to set agentsErr when they need to simulate failures
(alternatively, if you prefer reuse, return m.agentErr from
ListJobAgentsByWorkspaceID to align with GetJobAgent).
In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go`:
- Around line 100-102: The mock method mockGetter.ListJobAgentsByWorkspaceID
currently always returns m.workspaceAgents with a nil error; change the mock to
support returning an error so tests can simulate failure paths. Add an error
field (e.g., workspaceAgentsErr) to the mockGetter struct, update
ListJobAgentsByWorkspaceID to return (m.workspaceAgents, m.workspaceAgentsErr),
and update tests that construct mockGetter to set workspaceAgentsErr when needed
(mirroring the deploymentplan mock pattern).
🪄 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: 9bb33e46-90e7-4bfe-bde3-0be2cb06c893
📒 Files selected for processing (20)
apps/workspace-engine/pkg/db/convert.goapps/workspace-engine/pkg/db/deployments.sql.goapps/workspace-engine/pkg/db/models.goapps/workspace-engine/pkg/db/queries/schema.sqlapps/workspace-engine/pkg/selector/jobagents.goapps/workspace-engine/svc/controllers/deploymentplan/controller.goapps/workspace-engine/svc/controllers/deploymentplan/controller_test.goapps/workspace-engine/svc/controllers/deploymentplan/getters.goapps/workspace-engine/svc/controllers/deploymentplan/getters_postgres.goapps/workspace-engine/svc/controllers/jobdispatch/controller.goapps/workspace-engine/svc/controllers/jobdispatch/getters.goapps/workspace-engine/svc/controllers/jobdispatch/getters_postgres.goapps/workspace-engine/svc/controllers/jobdispatch/reconcile.goapps/workspace-engine/svc/controllers/jobdispatch/reconcile_test.goapps/workspace-engine/svc/controllers/jobeligibility/getters.goapps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.goapps/workspace-engine/test/controllers/harness/mocks.goapps/workspace-engine/test/controllers/harness/pipeline.go
| Id: row.ID.String(), | ||
| Name: row.Name, | ||
| Metadata: row.Metadata, | ||
| JobAgentConfig: oapi.JobAgentConfig(row.JobAgentConfig), | ||
| } | ||
| if row.Description != "" { | ||
| d.Description = &row.Description | ||
| } | ||
| if row.JobAgentSelector != "" { | ||
| d.JobAgentSelector = &row.JobAgentSelector | ||
| } |
There was a problem hiding this comment.
Propagate new deployment job-agent fields in the WithJobAgents conversion path too.
You mapped JobAgentSelector/JobAgentConfig here, but ToOapiDeploymentWithJobAgents (Line 28–Line 35) still builds a Deployment without those fields, so this path can return incomplete deployment payloads.
Proposed fix
func ToOapiDeploymentWithJobAgents(row GetDeploymentWithJobAgentsRow) *oapi.Deployment {
d := ToOapiDeployment(Deployment{
ID: row.ID,
Name: row.Name,
Description: row.Description,
ResourceSelector: row.ResourceSelector,
+ JobAgentSelector: row.JobAgentSelector,
+ JobAgentConfig: row.JobAgentConfig,
Metadata: row.Metadata,
WorkspaceID: row.WorkspaceID,
})📝 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.
| Id: row.ID.String(), | |
| Name: row.Name, | |
| Metadata: row.Metadata, | |
| JobAgentConfig: oapi.JobAgentConfig(row.JobAgentConfig), | |
| } | |
| if row.Description != "" { | |
| d.Description = &row.Description | |
| } | |
| if row.JobAgentSelector != "" { | |
| d.JobAgentSelector = &row.JobAgentSelector | |
| } | |
| func ToOapiDeploymentWithJobAgents(row GetDeploymentWithJobAgentsRow) *oapi.Deployment { | |
| d := ToOapiDeployment(Deployment{ | |
| ID: row.ID, | |
| Name: row.Name, | |
| Description: row.Description, | |
| ResourceSelector: row.ResourceSelector, | |
| JobAgentSelector: row.JobAgentSelector, | |
| JobAgentConfig: row.JobAgentConfig, | |
| Metadata: row.Metadata, | |
| WorkspaceID: row.WorkspaceID, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/db/convert.go` around lines 13 - 23, The
ToOapiDeploymentWithJobAgents conversion path is missing propagation of the new
job-agent fields; update the ToOapiDeploymentWithJobAgents function to set
Deployment.JobAgentConfig (using oapi.JobAgentConfig(row.JobAgentConfig)) and
Deployment.JobAgentSelector (as a pointer when non-empty) just like the other
conversion, and also set Description as a pointer when row.Description != "" so
the WithJobAgents path returns the same complete payload; locate the Deployment
construction in ToOapiDeploymentWithJobAgents and add those field assignments
with the same nil/empty checks used in the existing conversion code.
apps/workspace-engine/svc/controllers/jobdispatch/controller.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/workspace-engine/svc/controllers/deploymentplan/controller.go (1)
77-105:⚠️ Potential issue | 🟡 MinorFetch release targets before listing workspace agents.
The new
ListJobAgentsByWorkspaceIDcall runs before the empty-target short-circuit, so a plan with zero release targets can now fail on a transient agent-list error even though there is nothing to fan out. Moving the target lookup/completion check earlier avoids that unnecessary dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/deploymentplan/controller.go` around lines 77 - 105, The code currently calls getter.ListJobAgentsByWorkspaceID and selector.MatchJobAgents before fetching release targets; move the release target lookup and the zero-target short-circuit (call to getter.GetReleaseTargets and the subsequent if len(targets) == 0 block that calls setter.CompletePlan) to run before listing agents so a plan with zero targets completes without depending on ListJobAgentsByWorkspaceID; ensure span.SetAttributes(attribute.Int("targets.count", len(targets))) stays immediately after GetReleaseTargets and only then call getter.ListJobAgentsByWorkspaceID, followed by selector.MatchJobAgents and the existing matchedAgents handling.apps/workspace-engine/pkg/db/deployments.sql.go (1)
217-245:⚠️ Potential issue | 🟠 MajorPersist
job_agent_selectorandjob_agent_configin the upsert.
UpsertDeploymentnow returns these fields, but the INSERT/UPDATE column list andUpsertDeploymentParamsstill never set them. Any deployment written through this path will keep stale selector/config state, so the new controller logic can reconcile against outdated agent matching rules. Please update the source query inapps/workspace-engine/pkg/db/queries/deployments.sqland regenerate sqlc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/db/deployments.sql.go` around lines 217 - 245, The upsert currently returns job_agent_selector and job_agent_config but neither the SQL upsert (upsertDeployment) nor UpsertDeploymentParams include or set these fields, so update the deployments.sql upsert to include job_agent_selector and job_agent_config in the INSERT and UPDATE column lists and placeholders, add corresponding fields (e.g., JobAgentSelector pgtype.Text and JobAgentConfig map[string]interface{} or matching type) to the UpsertDeploymentParams struct, pass those new params into Queries.UpsertDeployment when calling q.db.QueryRow, then regenerate sqlc so the generated queries/structs (UpsertDeploymentParams, upsertDeployment, and Queries.UpsertDeployment) stay in sync.
🧹 Nitpick comments (2)
apps/workspace-engine/svc/controllers/jobdispatch/reconcile_test.go (1)
54-59: Mock should validate the workspace ID to actually test workspace scoping.Right now the mock ignores the
workspaceIDargument, so tests can pass even if the reconciler sends the wrong workspace. Consider asserting the expected ID in the mock.Proposed test-hardening diff
type mockGetter struct { release *oapi.Release releaseErr error deployment *oapi.Deployment deploymentErr error jobAgents map[string]*oapi.JobAgent jobAgentErr error workspaceAgents []oapi.JobAgent + expectedWorkspaceID uuid.UUID verificationPolicies []oapi.VerificationMetricSpec verificationPoliciesErr error } func (m *mockGetter) ListJobAgentsByWorkspaceID( _ context.Context, - _ uuid.UUID, + workspaceID uuid.UUID, ) ([]oapi.JobAgent, error) { + if m.expectedWorkspaceID != uuid.Nil && workspaceID != m.expectedWorkspaceID { + return nil, fmt.Errorf("unexpected workspace id: %s", workspaceID) + } return m.workspaceAgents, nil } func setupGetterWithAgents(agents []oapi.JobAgent) (*oapi.Job, *mockGetter) { ... getter := &mockGetter{ release: release, deployment: deployment, jobAgents: agentMap, workspaceAgents: agents, + expectedWorkspaceID: testWorkspaceID, } return job, getter }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/jobdispatch/reconcile_test.go` around lines 54 - 59, The mock ListJobAgentsByWorkspaceID (_ context.Context, _ uuid.UUID) currently ignores the workspaceID so tests don't verify scoping; update mockGetter.ListJobAgentsByWorkspaceID to check the received workspaceID against the expected test workspace ID (e.g., a field on mockGetter like expectedWorkspaceID or a value captured from the test), and if it doesn't match return an error or t.Fatalf/panic to fail the test; keep returning m.workspaceAgents when it matches. This ensures the reconciler must call ListJobAgentsByWorkspaceID with the correct workspace ID.apps/workspace-engine/svc/controllers/deploymentplan/controller_test.go (1)
212-224: Let the helper accept the selector value directly.
testDeployment(true)hardcodes"true", so most of this suite still only proves the “fan out to every workspace agent” branch. Accepting a selector string here would make it easy to add one selective CEL case and catch a regression whereProcessstops honoring the selector.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/deploymentplan/controller_test.go` around lines 212 - 224, Update the test helper testDeployment to accept the selector value instead of a boolean: change its signature to something like testDeployment(selector *string) (or testDeployment(selector string) where empty string means no selector) and set dep.JobAgentSelector = selector when non-nil/non-empty; update all test callers (previously passing true/false) to pass either a pointer to the desired CEL string (e.g., "true" or a selective expression) or nil/empty to preserve the no-selector case so tests can easily cover both fan-out and selective branches in Process.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/workspace-engine/pkg/db/deployments.sql.go`:
- Around line 217-245: The upsert currently returns job_agent_selector and
job_agent_config but neither the SQL upsert (upsertDeployment) nor
UpsertDeploymentParams include or set these fields, so update the
deployments.sql upsert to include job_agent_selector and job_agent_config in the
INSERT and UPDATE column lists and placeholders, add corresponding fields (e.g.,
JobAgentSelector pgtype.Text and JobAgentConfig map[string]interface{} or
matching type) to the UpsertDeploymentParams struct, pass those new params into
Queries.UpsertDeployment when calling q.db.QueryRow, then regenerate sqlc so the
generated queries/structs (UpsertDeploymentParams, upsertDeployment, and
Queries.UpsertDeployment) stay in sync.
In `@apps/workspace-engine/svc/controllers/deploymentplan/controller.go`:
- Around line 77-105: The code currently calls getter.ListJobAgentsByWorkspaceID
and selector.MatchJobAgents before fetching release targets; move the release
target lookup and the zero-target short-circuit (call to
getter.GetReleaseTargets and the subsequent if len(targets) == 0 block that
calls setter.CompletePlan) to run before listing agents so a plan with zero
targets completes without depending on ListJobAgentsByWorkspaceID; ensure
span.SetAttributes(attribute.Int("targets.count", len(targets))) stays
immediately after GetReleaseTargets and only then call
getter.ListJobAgentsByWorkspaceID, followed by selector.MatchJobAgents and the
existing matchedAgents handling.
---
Nitpick comments:
In `@apps/workspace-engine/svc/controllers/deploymentplan/controller_test.go`:
- Around line 212-224: Update the test helper testDeployment to accept the
selector value instead of a boolean: change its signature to something like
testDeployment(selector *string) (or testDeployment(selector string) where empty
string means no selector) and set dep.JobAgentSelector = selector when
non-nil/non-empty; update all test callers (previously passing true/false) to
pass either a pointer to the desired CEL string (e.g., "true" or a selective
expression) or nil/empty to preserve the no-selector case so tests can easily
cover both fan-out and selective branches in Process.
In `@apps/workspace-engine/svc/controllers/jobdispatch/reconcile_test.go`:
- Around line 54-59: The mock ListJobAgentsByWorkspaceID (_ context.Context, _
uuid.UUID) currently ignores the workspaceID so tests don't verify scoping;
update mockGetter.ListJobAgentsByWorkspaceID to check the received workspaceID
against the expected test workspace ID (e.g., a field on mockGetter like
expectedWorkspaceID or a value captured from the test), and if it doesn't match
return an error or t.Fatalf/panic to fail the test; keep returning
m.workspaceAgents when it matches. This ensures the reconciler must call
ListJobAgentsByWorkspaceID with the correct workspace ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b34a31e9-7bc4-4dc6-a8cb-e60e9d85cf0d
📒 Files selected for processing (10)
apps/workspace-engine/pkg/db/convert.goapps/workspace-engine/pkg/db/convert_test.goapps/workspace-engine/pkg/db/deployments.sql.goapps/workspace-engine/pkg/db/queries/deployments.sqlapps/workspace-engine/svc/controllers/deploymentplan/controller.goapps/workspace-engine/svc/controllers/deploymentplan/controller_test.goapps/workspace-engine/svc/controllers/jobdispatch/controller.goapps/workspace-engine/svc/controllers/jobdispatch/reconcile_test.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go
💤 Files with no reviewable changes (2)
- apps/workspace-engine/pkg/db/queries/deployments.sql
- apps/workspace-engine/pkg/db/convert_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/workspace-engine/pkg/db/convert.go
Resolves #928
Summary by CodeRabbit
New Features
Refactor
Tests