Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a job-eligibility reconciliation subsystem: event enqueueing, a controller worker, release-target parsing, Getter/Setter interfaces with Postgres adapters, eligibility/retry evaluators with context propagation, job construction/persistence and dispatch enqueueing, extensive tests, and related DB/query schema changes. Changes
Sequence DiagramsequenceDiagram
rect rgba(200,220,255,0.5)
participant Queue as Event Queue
end
rect rgba(200,255,200,0.5)
participant Controller as Job Eligibility Controller
end
rect rgba(255,230,200,0.5)
participant Reconciler as Reconcile Logic
end
participant Getter as Data Access (Getter)
participant Setter as Job Persist/Dispatch (Setter)
Queue->>Controller: job-eligibility event (deployment:env:resource)
Controller->>Controller: parse ReleaseTarget scope
Controller->>Getter: ReleaseTargetExists?
Getter-->>Controller: exists / missing
alt Target exists
Controller->>Reconciler: Reconcile(workspaceID, getter, setter, rt)
Reconciler->>Getter: GetDesiredRelease / GetJobs / GetPolicies
Getter-->>Reconciler: release, jobs, policies
Reconciler->>Reconciler: evaluate (concurrency + retry/backoff)
alt Eligible
Reconciler->>Getter: GetDeployment / GetJobAgent / GetEnvironment / GetResource
Getter-->>Reconciler: deployment, agents, env, resource
Reconciler->>Setter: CreateJob(release, job)
Setter-->>Reconciler: created
Reconciler->>Setter: EnqueueJobDispatch(workspaceID, jobID)
Setter->>Queue: job-dispatch event
else Not eligible
Reconciler-->>Controller: NextReconcileAt (request requeue)
end
else Target missing
Controller->>Controller: short-circuit (no-op)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 7
🧹 Nitpick comments (3)
apps/workspace-engine/svc/controllers/jobeligibility/releasetarget.go (1)
12-51: Document the exported release-target API.
ReleaseTarget,NewReleaseTarget, andToOAPIare exported and currently undocumented.As per coding guidelines, "document exported functions/types/methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/jobeligibility/releasetarget.go` around lines 12 - 51, Add Go doc comments for the exported type and its functions: document ReleaseTarget with a one-line summary of what a ReleaseTarget represents (including its WorkspaceID/DeploymentID/EnvironmentID/ResourceID), document NewReleaseTarget to explain the expected input format of the key (deploymentID:environmentID:resourceID) and the errors returned on parse failure, and document ToOAPI to state that it converts the ReleaseTarget into the oapi.ReleaseTarget DTO; place comments immediately above the ReleaseTarget type, NewReleaseTarget function, and ToOAPI method.apps/workspace-engine/svc/controllers/jobeligibility/setters.go (1)
9-12: Document the exportedSetterinterface.
Setteris part of the public package surface, but it has no doc comment yet.As per coding guidelines, "document exported functions/types/methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/jobeligibility/setters.go` around lines 9 - 12, Add a clear doc comment above the exported type Setter describing its role in the package (e.g., "Setter defines methods to persist jobs and enqueue dispatches for a workspace") and what implementations are expected to do; also ensure exported methods CreateJob and EnqueueJobDispatch each have short doc comments describing their behavior and parameters (context, job, workspaceID, jobID) so the public surface is fully documented.apps/workspace-engine/pkg/reconcile/events/jobeligibility.go (1)
13-48: Document the exported enqueue API.
JobEligibilityParams,ScopeID,EnqueueJobEligibility, andEnqueueManyJobEligibilityare exported without doc comments.As per coding guidelines, "document exported functions/types/methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/reconcile/events/jobeligibility.go` around lines 13 - 48, Add Go doc comments for the exported API: document the JobEligibilityParams type (what each field represents and expected format), the ScopeID() method (describe the returned string format "deploymentID:environmentID:resourceID"), EnqueueJobEligibility (describe purpose, required params, and error behavior), and EnqueueManyJobEligibility (explain batching behavior, empty-slice no-op, log side-effect, and error return). Place concise comments immediately above the declarations for JobEligibilityParams, func (JobEligibilityParams) ScopeID, EnqueueJobEligibility, and EnqueueManyJobEligibility so tooling and godoc pick them up.
🤖 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/svc/controllers/jobeligibility/controller.go`:
- Around line 42-49: NewReleaseTarget returns a ReleaseTarget with
deployment/environment/resource parsed but missing WorkspaceID, so calling
getter.ReleaseTargetExists(ctx, rt) uses a partially-initialized object; before
invoking ReleaseTargetExists (in Reconcile), set the WorkspaceID on the
ReleaseTarget (e.g., rt.WorkspaceID = item.WorkspaceID) so the existence check
uses the same fully-populated ReleaseTarget as the rest of the reconciliation
path.
In `@apps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.go`:
- Around line 14-45: The PostgresGetter returned by New() is wired but all
methods (ReleaseTargetExists, GetDesiredRelease, GetJobsForReleaseTarget,
GetPoliciesForReleaseTarget, GetDeployment, GetJobAgent, GetEnvironment,
GetResource) are stubs that return "not implemented", causing immediate
failures; either implement these methods fully or stop wiring PostgresGetter:
change the factory New() to return an error or a nil/default getter (or return a
fallback/mock) until implementations exist, and ensure callers of New() handle
the error/absence so Process doesn't call ReleaseTargetExists on a
non-functional getter.
In `@apps/workspace-engine/svc/controllers/jobeligibility/getters.go`:
- Around line 11-19: The Getter interface currently forces policy lookup via
*oapi.ReleaseTarget (GetPoliciesForReleaseTarget) which causes reconcile.go to
down-convert your internal *ReleaseTarget and lose scope data; change the
interface method signature to accept the internal type
(GetPoliciesForReleaseTarget(ctx context.Context, rt *ReleaseTarget)
([]*oapi.Policy, error)), then update all concrete implementations of Getter and
any callers (notably reconcile.go) to pass the original *ReleaseTarget instead
of converting to *oapi.ReleaseTarget so the additional scope fields are
preserved during policy resolution.
In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go`:
- Around line 139-155: The code treats attemptCount==0 as "first attempt" even
when mostRecentJob exists and is in a terminal non-retryable state, causing
finished releases to be reopened; modify the logic so that if mostRecentJob !=
nil and its status is terminal and non-retryable (use the same status-check
helper you're using elsewhere—e.g., the function or enum that determines
retryability) you should not treat this as a first attempt: either return false,
nil, "terminal non-retryable job; not reopening" immediately, or increment/set
attemptCount>0 before the existing attemptCount==0 check so the function returns
not-to-reopen; reference the variables/methods mostRecentJob, mostRecentTime,
attemptCount and the retryability/status-check helper when implementing the
change.
- Around line 173-195: The loop over deployment.JobAgents that calls
r.getter.GetJobAgent, r.buildJob, r.setter.CreateJob and
r.setter.EnqueueJobDispatch per-agent can leave a partial rollout if a later
CreateJob/EnqueueJobDispatch fails; change the reconcile to either (A) build the
full set of jobs for all agents first (use r.buildJob to assemble []job) and
then persist/enqueue them atomically via a batch API/transaction (add a new
setter method like CreateJobsAndEnqueueBatch or wrap
CreateJob/EnqueueJobDispatch calls in a DB transaction), or (B) implement
idempotent resume logic: before creating/enqueuing each job check for existence
(query via r.getter or a new GetJobsForDeployment method) and only
create/enqueue missing jobs and ensure a durable rollout marker is written so
subsequent reconciles complete remaining agents; update the reconcile flow to
use these new batch/existence checks instead of doing per-agent side-effecting
calls directly in the loop.
- Around line 115-137: The loop in reconcile.go counts every agent job toward
attemptCount, but we need to count distinct reconcile attempts (one per
reconcile, not per agent); change the loop that iterates r.jobs (and the
attemptCount logic) to deduplicate by a reconcile identifier (e.g.,
job.ReconcileId or other per-reconcile field) using a map[string]bool
(seenReconiles) and increment attemptCount only the first time you observe that
reconcile id while still computing mostRecentJob/mostRecentTime across all jobs;
this ensures buildAndDispatchJob fan-out per agent doesn't inflate retries.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/reconcile/events/jobeligibility.go`:
- Around line 13-48: Add Go doc comments for the exported API: document the
JobEligibilityParams type (what each field represents and expected format), the
ScopeID() method (describe the returned string format
"deploymentID:environmentID:resourceID"), EnqueueJobEligibility (describe
purpose, required params, and error behavior), and EnqueueManyJobEligibility
(explain batching behavior, empty-slice no-op, log side-effect, and error
return). Place concise comments immediately above the declarations for
JobEligibilityParams, func (JobEligibilityParams) ScopeID,
EnqueueJobEligibility, and EnqueueManyJobEligibility so tooling and godoc pick
them up.
In `@apps/workspace-engine/svc/controllers/jobeligibility/releasetarget.go`:
- Around line 12-51: Add Go doc comments for the exported type and its
functions: document ReleaseTarget with a one-line summary of what a
ReleaseTarget represents (including its
WorkspaceID/DeploymentID/EnvironmentID/ResourceID), document NewReleaseTarget to
explain the expected input format of the key
(deploymentID:environmentID:resourceID) and the errors returned on parse
failure, and document ToOAPI to state that it converts the ReleaseTarget into
the oapi.ReleaseTarget DTO; place comments immediately above the ReleaseTarget
type, NewReleaseTarget function, and ToOAPI method.
In `@apps/workspace-engine/svc/controllers/jobeligibility/setters.go`:
- Around line 9-12: Add a clear doc comment above the exported type Setter
describing its role in the package (e.g., "Setter defines methods to persist
jobs and enqueue dispatches for a workspace") and what implementations are
expected to do; also ensure exported methods CreateJob and EnqueueJobDispatch
each have short doc comments describing their behavior and parameters (context,
job, workspaceID, jobID) so the public surface is fully documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4ea0dfb1-ec36-405b-b096-5766c557b993
📒 Files selected for processing (9)
apps/workspace-engine/main.goapps/workspace-engine/pkg/reconcile/events/jobeligibility.goapps/workspace-engine/svc/controllers/jobeligibility/controller.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/releasetarget.goapps/workspace-engine/svc/controllers/jobeligibility/setters.goapps/workspace-engine/svc/controllers/jobeligibility/setters_postgres.go
Comment on lines
+42
to
+49
| rt, err := NewReleaseTarget(item.ScopeID) | ||
| if err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, err.Error()) | ||
| return reconcile.Result{}, fmt.Errorf("parse release target: %w", err) | ||
| } | ||
|
|
||
| exists, err := c.getter.ReleaseTargetExists(ctx, rt) |
Contributor
There was a problem hiding this comment.
Populate WorkspaceID before ReleaseTargetExists.
NewReleaseTarget only parses deployment/environment/resource, so Line 49 sends a partially initialized ReleaseTarget into the getter. Every later query gets WorkspaceID populated inside Reconcile, which makes the existence check inconsistent with the rest of the data path.
Suggested change
import (
"context"
"fmt"
"runtime"
"time"
"workspace-engine/svc"
"github.com/charmbracelet/log"
+ "github.com/google/uuid"
"workspace-engine/pkg/reconcile"
"workspace-engine/pkg/reconcile/events"
"workspace-engine/pkg/reconcile/postgres"
@@
rt, err := NewReleaseTarget(item.ScopeID)
if err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, err.Error())
return reconcile.Result{}, fmt.Errorf("parse release target: %w", err)
}
+ workspaceID, err := uuid.Parse(item.WorkspaceID)
+ if err != nil {
+ span.RecordError(err)
+ span.SetStatus(codes.Error, err.Error())
+ return reconcile.Result{}, fmt.Errorf("parse workspace id: %w", err)
+ }
+ rt.WorkspaceID = workspaceID
exists, err := c.getter.ReleaseTargetExists(ctx, rt)📝 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
| rt, err := NewReleaseTarget(item.ScopeID) | |
| if err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, err.Error()) | |
| return reconcile.Result{}, fmt.Errorf("parse release target: %w", err) | |
| } | |
| exists, err := c.getter.ReleaseTargetExists(ctx, rt) | |
| rt, err := NewReleaseTarget(item.ScopeID) | |
| if err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, err.Error()) | |
| return reconcile.Result{}, fmt.Errorf("parse release target: %w", err) | |
| } | |
| workspaceID, err := uuid.Parse(item.WorkspaceID) | |
| if err != nil { | |
| span.RecordError(err) | |
| span.SetStatus(codes.Error, err.Error()) | |
| return reconcile.Result{}, fmt.Errorf("parse workspace id: %w", err) | |
| } | |
| rt.WorkspaceID = workspaceID | |
| exists, err := c.getter.ReleaseTargetExists(ctx, rt) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/jobeligibility/controller.go` around
lines 42 - 49, NewReleaseTarget returns a ReleaseTarget with
deployment/environment/resource parsed but missing WorkspaceID, so calling
getter.ReleaseTargetExists(ctx, rt) uses a partially-initialized object; before
invoking ReleaseTargetExists (in Reconcile), set the WorkspaceID on the
ReleaseTarget (e.g., rt.WorkspaceID = item.WorkspaceID) so the existence check
uses the same fully-populated ReleaseTarget as the rest of the reconciliation
path.
apps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.go
Outdated
Show resolved
Hide resolved
Comment on lines
+11
to
+19
| type Getter interface { | ||
| ReleaseTargetExists(ctx context.Context, rt *ReleaseTarget) (bool, error) | ||
| GetDesiredRelease(ctx context.Context, rt *ReleaseTarget) (*oapi.Release, error) | ||
| GetJobsForReleaseTarget(ctx context.Context, rt *ReleaseTarget) ([]*oapi.Job, error) | ||
| GetPoliciesForReleaseTarget(ctx context.Context, rt *oapi.ReleaseTarget) ([]*oapi.Policy, error) | ||
| GetDeployment(ctx context.Context, deploymentID uuid.UUID) (*oapi.Deployment, error) | ||
| GetJobAgent(ctx context.Context, jobAgentID uuid.UUID) (*oapi.JobAgent, error) | ||
| GetEnvironment(ctx context.Context, environmentID uuid.UUID) (*oapi.Environment, error) | ||
| GetResource(ctx context.Context, resourceID uuid.UUID) (*oapi.Resource, error) |
Contributor
There was a problem hiding this comment.
Don't force policy lookup through *oapi.ReleaseTarget.
Line 15 is the odd one out. It makes reconcile.go down-convert *ReleaseTarget before policy resolution, which drops the extra scope data carried on the internal type.
Suggested change
type Getter interface {
ReleaseTargetExists(ctx context.Context, rt *ReleaseTarget) (bool, error)
GetDesiredRelease(ctx context.Context, rt *ReleaseTarget) (*oapi.Release, error)
GetJobsForReleaseTarget(ctx context.Context, rt *ReleaseTarget) ([]*oapi.Job, error)
- GetPoliciesForReleaseTarget(ctx context.Context, rt *oapi.ReleaseTarget) ([]*oapi.Policy, error)
+ GetPoliciesForReleaseTarget(ctx context.Context, rt *ReleaseTarget) ([]*oapi.Policy, error)
GetDeployment(ctx context.Context, deploymentID uuid.UUID) (*oapi.Deployment, error)
GetJobAgent(ctx context.Context, jobAgentID uuid.UUID) (*oapi.JobAgent, error)
GetEnvironment(ctx context.Context, environmentID uuid.UUID) (*oapi.Environment, error)
GetResource(ctx context.Context, resourceID uuid.UUID) (*oapi.Resource, error)
}📝 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
| type Getter interface { | |
| ReleaseTargetExists(ctx context.Context, rt *ReleaseTarget) (bool, error) | |
| GetDesiredRelease(ctx context.Context, rt *ReleaseTarget) (*oapi.Release, error) | |
| GetJobsForReleaseTarget(ctx context.Context, rt *ReleaseTarget) ([]*oapi.Job, error) | |
| GetPoliciesForReleaseTarget(ctx context.Context, rt *oapi.ReleaseTarget) ([]*oapi.Policy, error) | |
| GetDeployment(ctx context.Context, deploymentID uuid.UUID) (*oapi.Deployment, error) | |
| GetJobAgent(ctx context.Context, jobAgentID uuid.UUID) (*oapi.JobAgent, error) | |
| GetEnvironment(ctx context.Context, environmentID uuid.UUID) (*oapi.Environment, error) | |
| GetResource(ctx context.Context, resourceID uuid.UUID) (*oapi.Resource, error) | |
| type Getter interface { | |
| ReleaseTargetExists(ctx context.Context, rt *ReleaseTarget) (bool, error) | |
| GetDesiredRelease(ctx context.Context, rt *ReleaseTarget) (*oapi.Release, error) | |
| GetJobsForReleaseTarget(ctx context.Context, rt *ReleaseTarget) ([]*oapi.Job, error) | |
| GetPoliciesForReleaseTarget(ctx context.Context, rt *ReleaseTarget) ([]*oapi.Policy, error) | |
| GetDeployment(ctx context.Context, deploymentID uuid.UUID) (*oapi.Deployment, error) | |
| GetJobAgent(ctx context.Context, jobAgentID uuid.UUID) (*oapi.JobAgent, error) | |
| GetEnvironment(ctx context.Context, environmentID uuid.UUID) (*oapi.Environment, error) | |
| GetResource(ctx context.Context, resourceID uuid.UUID) (*oapi.Resource, error) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/jobeligibility/getters.go` around lines
11 - 19, The Getter interface currently forces policy lookup via
*oapi.ReleaseTarget (GetPoliciesForReleaseTarget) which causes reconcile.go to
down-convert your internal *ReleaseTarget and lose scope data; change the
interface method signature to accept the internal type
(GetPoliciesForReleaseTarget(ctx context.Context, rt *ReleaseTarget)
([]*oapi.Policy, error)), then update all concrete implementations of Getter and
any callers (notably reconcile.go) to pass the original *ReleaseTarget instead
of converting to *oapi.ReleaseTarget so the additional scope fields are
preserved during policy resolution.
apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go
Outdated
Show resolved
Hide resolved
Comment on lines
+139
to
+155
| if attemptCount > int(maxRetries) { | ||
| return false, nil, fmt.Sprintf("retry limit exceeded (%d/%d attempts)", attemptCount, maxRetries) | ||
| } | ||
|
|
||
| // Backoff check | ||
| if attemptCount > 0 && backoffSeconds != nil && *backoffSeconds > 0 && mostRecentJob != nil { | ||
| backoffDuration := calculateBackoff(attemptCount, *backoffSeconds, backoffStrategy, maxBackoffSeconds) | ||
| nextAllowed := mostRecentTime.Add(backoffDuration) | ||
| if time.Now().Before(nextAllowed) { | ||
| return false, &nextAllowed, fmt.Sprintf("waiting for retry backoff (%ds remaining)", int(time.Until(nextAllowed).Seconds())) | ||
| } | ||
| } | ||
|
|
||
| if attemptCount == 0 { | ||
| return true, nil, "first attempt" | ||
| } | ||
| return true, nil, fmt.Sprintf("retry allowed (%d/%d attempts)", attemptCount, maxRetries) |
Contributor
There was a problem hiding this comment.
A terminal non-retryable job is reopened as a first attempt.
If the newest same-release job is Successful (or any other non-retryable terminal status), the loop above exits with attemptCount == 0, and Line 153 returns "first attempt". That allows duplicate job creation for a release that already finished.
🤖 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.go` around
lines 139 - 155, The code treats attemptCount==0 as "first attempt" even when
mostRecentJob exists and is in a terminal non-retryable state, causing finished
releases to be reopened; modify the logic so that if mostRecentJob != nil and
its status is terminal and non-retryable (use the same status-check helper
you're using elsewhere—e.g., the function or enum that determines retryability)
you should not treat this as a first attempt: either return false, nil,
"terminal non-retryable job; not reopening" immediately, or increment/set
attemptCount>0 before the existing attemptCount==0 check so the function returns
not-to-reopen; reference the variables/methods mostRecentJob, mostRecentTime,
attemptCount and the retryability/status-check helper when implementing the
change.
Comment on lines
+173
to
+195
| for _, agentRef := range *deployment.JobAgents { | ||
| agentID, err := uuid.Parse(agentRef.Ref) | ||
| if err != nil { | ||
| return fmt.Errorf("parse job agent id: %w", err) | ||
| } | ||
|
|
||
| jobAgent, err := r.getter.GetJobAgent(ctx, agentID) | ||
| if err != nil { | ||
| return fmt.Errorf("get job agent %s: %w", agentRef.Ref, err) | ||
| } | ||
|
|
||
| job, err := r.buildJob(ctx, jobAgent, deployment) | ||
| if err != nil { | ||
| return fmt.Errorf("build job: %w", err) | ||
| } | ||
|
|
||
| if err := r.setter.CreateJob(ctx, job); err != nil { | ||
| return fmt.Errorf("create job: %w", err) | ||
| } | ||
|
|
||
| if err := r.setter.EnqueueJobDispatch(ctx, r.workspaceID.String(), job.Id); err != nil { | ||
| return fmt.Errorf("enqueue job dispatch: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Per-agent create/enqueue can strand a partial rollout.
This loop persists and dispatches each agent independently. If one agent succeeds and a later CreateJob / EnqueueJobDispatch fails, some jobs are already live; the next reconcile then sees those active jobs at Lines 69-72 and never creates the missing agents. This needs atomic batch semantics or idempotent resume logic.
🤖 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.go` around
lines 173 - 195, The loop over deployment.JobAgents that calls
r.getter.GetJobAgent, r.buildJob, r.setter.CreateJob and
r.setter.EnqueueJobDispatch per-agent can leave a partial rollout if a later
CreateJob/EnqueueJobDispatch fails; change the reconcile to either (A) build the
full set of jobs for all agents first (use r.buildJob to assemble []job) and
then persist/enqueue them atomically via a batch API/transaction (add a new
setter method like CreateJobsAndEnqueueBatch or wrap
CreateJob/EnqueueJobDispatch calls in a DB transaction), or (B) implement
idempotent resume logic: before creating/enqueuing each job check for existence
(query via r.getter or a new GetJobsForDeployment method) and only
create/enqueue missing jobs and ensure a durable rollout marker is written so
subsequent reconciles complete remaining agents; update the reconcile flow to
use these new batch/existence checks instead of doing per-agent side-effecting
calls directly in the loop.
apps/workspace-engine/svc/controllers/jobeligibility/setters_postgres.go
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go (1)
151-174:⚠️ Potential issue | 🟠 MajorPer-agent job creation lacks atomicity guarantees.
The loop creates and enqueues jobs one agent at a time. If
CreateJoborEnqueueJobDispatchfails mid-iteration, some jobs will be persisted/enqueued while others won't. On the next reconcile, the concurrency gate atreleasetargetconcurrency.NewEvaluatorwill see the already-created active jobs and block further creation, leaving the deployment in a partial state.Consider either:
- Batch all job creations in a single transaction
- Implement idempotent resume logic that detects and creates only missing agent jobs
🤖 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.go` around lines 151 - 174, The loop over deployment.JobAgents in reconcile.go creates and enqueues jobs per-agent (using r.buildJob, r.setter.CreateJob and r.setter.EnqueueJobDispatch) which can leave a partial state if a mid-loop error occurs; fix by making job creation/enqueue atomic for the whole set or adding idempotent resume logic: either (A) collect all jobs (call r.buildJob for each agent via getter.GetJobAgent) and perform a single transactional batch persist+enqueue so either all jobs are created/enqueued or none, or (B) prior to creating, query existing active jobs for this deployment/workspace (use releasetargetconcurrency.NewEvaluator behavior) and only create/enqueue missing agent jobs so retries are safe; update the code around the loop to use the chosen approach (replace per-agent r.setter.CreateJob/r.setter.EnqueueJobDispatch calls) and ensure errors roll back or are handled to avoid partial creation.apps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.go (1)
36-49:⚠️ Potential issue | 🟠 MajorStub methods will cause runtime failures.
GetDesiredReleasereturns an error that will immediately fail any reconciliation attempt (seereconcile.golines 33-37).GetJobsForReleaseTargetandGetJobsInProcessingStateForReleaseTargetreturnnil, which may cause silent failures or unexpected behavior in evaluators that expect a non-nil map.If
PostgresGetteris wired into production code paths before these are implemented, the controller will be non-functional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.go` around lines 36 - 49, These stub methods must be implemented to avoid runtime failures: replace the placeholder error in GetDesiredRelease by querying the desired_release DB (use the existing desired_release query or a new g.db.ListDesiredRelease/GetDesiredReleaseByTarget helper) and return the resulting *oapi.Release with any DB error; implement GetJobsForReleaseTarget and GetJobsInProcessingStateForReleaseTarget to call the ListJobsByReleaseTarget query (filtering by processing state for the latter), ensure they return an initialized map[string]*oapi.Job (empty map on no results) instead of nil, and propagate DB errors appropriately so callers receive concrete results/errors rather than panics or silent nils.
🧹 Nitpick comments (1)
apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go (1)
233-234: Input mutation:rt.WorkspaceIDis set from theworkspaceIDparameter.Line 234 mutates the input
rtby settingWorkspaceID. If callers expectrtto remain unchanged, this could cause subtle bugs. Consider documenting this behavior or copying theReleaseTargetbefore mutation.🤖 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.go` around lines 233 - 234, The code currently mutates the incoming ReleaseTarget (rt) by setting rt.WorkspaceID via r.rt.WorkspaceID = r.workspaceID which can surprise callers; to fix, avoid mutating the input by creating a shallow copy of ReleaseTarget before assigning WorkspaceID (e.g., newRT := *rt; newRT.WorkspaceID = workspaceIDUUID) and use that copy when constructing the reconciler (reconciler{workspaceID: workspaceIDUUID, getter: getter, setter: setter, rt: &newRT}), or alternatively document in the reconciler/ReleaseTarget API that rt is modified by reconciler construction; update tests or call sites if they rely on rt remaining unchanged.
🤖 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/svc/controllers/jobeligibility/reconcile_test.go`:
- Around line 21-86: The compile error is because mockGetter is missing the
GetJobsInProcessingStateForReleaseTarget(ctx context.Context, releaseTarget
*oapi.ReleaseTarget) map[string]*oapi.Job method required by the Getter (via
concurrencyGetter); add a field on mockGetter (e.g., jobsInProcessing
map[string]*oapi.Job) and implement GetJobsInProcessingStateForReleaseTarget to
return that map (or an empty map if nil) so mockGetter satisfies the interface —
update the mockGetter struct and add the method named
GetJobsInProcessingStateForReleaseTarget to resolve the assertion involving
Getter and concurrencyGetter.
- Around line 55-57: The mockGetter must match the retry.Getters and embedded
releasetargetconcurrency.Getters signatures: change GetJobsForReleaseTarget to
func (m *mockGetter) GetJobsForReleaseTarget(ctx context.Context, releaseTarget
*oapi.ReleaseTarget) map[string]*oapi.Job (convert your stored jobs into a map
keyed by job ID and remove the error return), and add the missing method
GetJobsInProcessingStateForReleaseTarget(ctx context.Context, releaseTarget
*oapi.ReleaseTarget) map[string]*oapi.Job with similar behavior for
processing-state jobs; update the mockGetter fields (e.g., store jobs as
map[string]*oapi.Job or provide a helper to build that map from a slice) so both
methods can return the required map types and satisfy the interfaces.
---
Duplicate comments:
In `@apps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.go`:
- Around line 36-49: These stub methods must be implemented to avoid runtime
failures: replace the placeholder error in GetDesiredRelease by querying the
desired_release DB (use the existing desired_release query or a new
g.db.ListDesiredRelease/GetDesiredReleaseByTarget helper) and return the
resulting *oapi.Release with any DB error; implement GetJobsForReleaseTarget and
GetJobsInProcessingStateForReleaseTarget to call the ListJobsByReleaseTarget
query (filtering by processing state for the latter), ensure they return an
initialized map[string]*oapi.Job (empty map on no results) instead of nil, and
propagate DB errors appropriately so callers receive concrete results/errors
rather than panics or silent nils.
In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go`:
- Around line 151-174: The loop over deployment.JobAgents in reconcile.go
creates and enqueues jobs per-agent (using r.buildJob, r.setter.CreateJob and
r.setter.EnqueueJobDispatch) which can leave a partial state if a mid-loop error
occurs; fix by making job creation/enqueue atomic for the whole set or adding
idempotent resume logic: either (A) collect all jobs (call r.buildJob for each
agent via getter.GetJobAgent) and perform a single transactional batch
persist+enqueue so either all jobs are created/enqueued or none, or (B) prior to
creating, query existing active jobs for this deployment/workspace (use
releasetargetconcurrency.NewEvaluator behavior) and only create/enqueue missing
agent jobs so retries are safe; update the code around the loop to use the
chosen approach (replace per-agent
r.setter.CreateJob/r.setter.EnqueueJobDispatch calls) and ensure errors roll
back or are handled to avoid partial creation.
---
Nitpick comments:
In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go`:
- Around line 233-234: The code currently mutates the incoming ReleaseTarget
(rt) by setting rt.WorkspaceID via r.rt.WorkspaceID = r.workspaceID which can
surprise callers; to fix, avoid mutating the input by creating a shallow copy of
ReleaseTarget before assigning WorkspaceID (e.g., newRT := *rt;
newRT.WorkspaceID = workspaceIDUUID) and use that copy when constructing the
reconciler (reconciler{workspaceID: workspaceIDUUID, getter: getter, setter:
setter, rt: &newRT}), or alternatively document in the reconciler/ReleaseTarget
API that rt is modified by reconciler construction; update tests or call sites
if they rely on rt remaining unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a23dd38-66ff-4a94-9bcc-31f033789dda
📒 Files selected for processing (16)
apps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/getters.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/environmentprogression/jobtracker.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/releasetargetconcurrency/releasetargetconcurrency.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/getters.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/retry/retry.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/getters.goapps/workspace-engine/pkg/workspace/releasemanager/policy/evaluator/versioncooldown/versioncooldown.goapps/workspace-engine/svc/controllers/desiredrelease/policyeval/policyeval_test.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.goapps/workspace-engine/svc/controllers/jobeligibility/controller.goapps/workspace-engine/svc/controllers/jobeligibility/getters.goapps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.goapps/workspace-engine/svc/controllers/jobeligibility/postgres_test.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.goapps/workspace-engine/test/controllers/harness/mocks.go
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/workspace-engine/svc/controllers/jobeligibility/controller.go
apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Blocked evaluator result incorrectly overridden by pending
- I changed eligibility resolution to return blocked outcomes before pending ones so permanent denials are not requeued.
- ✅ Fixed: Unused context variable is dead code
- I removed the unused
context.Background()assignment and suppression since the variable was never used.
- I removed the unused
Or push these changes by commenting:
@cursor push 92af228a50
Preview (92af228a50)
diff --git a/apps/workspace-engine/svc/controllers/jobeligibility/controller.go b/apps/workspace-engine/svc/controllers/jobeligibility/controller.go
--- a/apps/workspace-engine/svc/controllers/jobeligibility/controller.go
+++ b/apps/workspace-engine/svc/controllers/jobeligibility/controller.go
@@ -97,7 +97,6 @@
MaxRetryBackoff: 10 * time.Second,
}
- ctx := context.Background()
kind := events.JobEligibilityKind
queue := postgres.NewForKinds(pgxPool, kind)
enqueueQueue := postgres.New(pgxPool)
@@ -115,6 +114,5 @@
log.Fatal("Failed to create job eligibility reconcile worker", "error", err)
}
- _ = ctx
return worker
}
diff --git a/apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go b/apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go
--- a/apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go
+++ b/apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go
@@ -124,12 +124,12 @@
}
}
+ if hasBlocked {
+ return false, nil, reason
+ }
if hasPending {
return false, earliestNextTime, reason
}
- if hasBlocked {
- return false, nil, reason
- }
return true, nil, reason
}| if hasBlocked { | ||
| return false, nil, reason | ||
| } | ||
| return true, nil, reason |
There was a problem hiding this comment.
Blocked evaluator result incorrectly overridden by pending
Medium Severity
In checkEligibility, when multiple evaluators run and one returns a "pending" (backoff) result while a later one returns a "blocked" (denied) result, both hasPending and hasBlocked become true. Since hasPending is checked first after the loop, the method returns a requeue time instead of a permanent denial. A blocked result (e.g., retry limit exceeded) is more restrictive and should take priority over a pending/backoff result.
Additional Locations (1)
| log.Fatal("Failed to create job eligibility reconcile worker", "error", err) | ||
| } | ||
|
|
||
| _ = ctx |
There was a problem hiding this comment.
Unused context variable is dead code
Low Severity
A context.Background() is created on line 100 and explicitly suppressed with _ = ctx on line 118. This appears to be leftover scaffolding from a planned feature that was never wired in — it serves no purpose and adds confusion about whether it was meant to be passed somewhere (e.g., to the worker or controller initialization).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go (1)
152-174:⚠️ Potential issue | 🔴 CriticalPer-agent create/enqueue can still strand a rollout half-issued.
Once any agent job is persisted, a later
CreateJoborEnqueueJobDispatchfailure exits early. The next reconcile then sees the already-persisted pending job and blocks, so the missing agents are never created. This still needs batch atomicity or idempotent resume logic.🤖 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.go` around lines 152 - 174, The loop persists and enqueues each agent job one-by-one which can leave a partial rollout if a later CreateJob or EnqueueJobDispatch fails; change to build all jobs first using jobs.NewFactoryFromGetters(...).CreateJobForRelease for each agentRef, then persist and enqueue in a recovery-safe way (either perform a single batched persist/enqueue or make r.setter.CreateJob and r.setter.EnqueueJobDispatch idempotent/resumable by checking for an existing pending job for the same agent/job Id before creating/enqueuing); update the reconcile loop around deployment.JobAgents to 1) collect jobs, 2) check for existing jobs via r.getter.GetJob or query by agent, and 3) create/enqueue only missing jobs (or use a batch API) so a partial failure on one agent does not permanently block others.
🧹 Nitpick comments (5)
apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go (1)
143-145: Make the new enqueue side effect observable in this mock.This stub always returns
niland records nothing, so the suite cannot assert that a successful reconcile actually schedules job-eligibility work or uses the right workspace/release target. That leaves the new handoff path effectively untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go` around lines 143 - 145, The EnqueueJobEligibility stub on mockReconcileSetter currently returns nil and records nothing; change it to record calls so tests can assert the handoff happened: add observable state on mockReconcileSetter (e.g., a slice or last-enqueued record and optionally a sync.Mutex/counter) and have EnqueueJobEligibility(ctx context.Context, workspace string, rt *ReleaseTarget) append or set that state (and return nil); update tests to inspect that recorded workspace and ReleaseTarget to verify the enqueue behavior.apps/workspace-engine/pkg/workspace/jobs/factory.go (1)
20-24: Document the new exported API.
GettersandNewFactoryFromGettersare new exported surface area, but neither has a Go doc comment.✍️ Suggested comments
+// Getters resolves release-target entities needed to build jobs and dispatch contexts. type Getters interface { GetDeployment(ctx context.Context, deploymentID uuid.UUID) (*oapi.Deployment, error) GetEnvironment(ctx context.Context, environmentID uuid.UUID) (*oapi.Environment, error) GetResource(ctx context.Context, resourceID uuid.UUID) (*oapi.Resource, error) } @@ +// NewFactoryFromGetters creates a job factory backed by the provided entity getters. func NewFactoryFromGetters(getters Getters) *Factory {As per coding guidelines, "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods."
Also applies to: 38-42
🤖 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.go` around lines 20 - 24, The exported type Getters and the exported constructor NewFactoryFromGetters lack Go doc comments; add concise package-comment style documentation above the Getters interface and above NewFactoryFromGetters explaining their purpose and contract (what GetDeployment/GetEnvironment/GetResource do, expected inputs/outputs and error behavior), and describe NewFactoryFromGetters’ behavior, parameters, return value, and any side effects or concurrency guarantees so users understand how to use them.apps/workspace-engine/pkg/workspace/jobs/getters_store.go (1)
18-20: Add a Go doc comment forNewStoreGetters.This is a new exported constructor, so it should carry a short doc comment.
✍️ Suggested comment
+// NewStoreGetters creates a store-backed Getters implementation. func NewStoreGetters(store *store.Store) *storeGetters {As per coding guidelines, "Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/pkg/workspace/jobs/getters_store.go` around lines 18 - 20, Add a Go doc comment immediately above the exported constructor NewStoreGetters describing what it does and its parameters/return (e.g., "NewStoreGetters returns a new storeGetters backed by the given *store.Store"); ensure the comment starts with "NewStoreGetters" (godoc style) and briefly documents the returned value (storeGetters) and the input store parameter so the exported function is properly documented.apps/workspace-engine/pkg/workspace/jobs/factory_test.go (2)
114-128: Add malformed-ID coverage if the UUID contract is intentional.The error-path section exercises missing deployment/environment/resource rows, but not the new parse failures introduced by the UUID parsing in
Factory. A small table-driven test with invalid deployment/environment/resource IDs would lock those branches down.Also applies to: 558-619
🤖 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` around lines 114 - 128, The test currently only covers "not found" database rows but misses the UUID parse failure branches in Factory.CreateJobForRelease; update TestFactory_CreateJobForRelease_DeploymentNotFound (or add a new table-driven test alongside it) to include cases where release.DeploymentID, release.EnvironmentID, and release.ResourceID are malformed (e.g. "invalid-uuid") and assert that NewFactory(...).CreateJobForRelease(ctx, release, jobAgent, nil) returns an error indicating a parse/invalid UUID rather than a not-found error; include separate subtests for each malformed ID to lock down the parsing error paths.
625-877: Delete the commented-out workflow test block.Keeping hundreds of lines of disabled tests in-tree makes this file harder to maintain, and they will silently drift as the factory API changes. If workflow coverage needs to return later, restore it in a follow-up PR instead of checking in the commented implementation.
🤖 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` around lines 625 - 877, Remove the entire commented-out workflow test block (the large commented region containing setupWorkflowStore and all TestFactory_CreateJobForWorkflowJob_* tests, including TestFactory_CreateJobForWorkflowJob_DispatchContextHasNoReleaseFields and the related deep-merge tests) from factory_test.go so the file no longer contains hundreds of lines of disabled tests; delete the commented lines cleanly, ensure no remaining dangling references to setupWorkflowStore or the removed test names (e.g., TestFactory_CreateJobForWorkflowJob_Success, TestFactory_CreateJobForWorkflowJob_BuildsDispatchContext, TestFactory_CreateJobForWorkflowJob_MergesConfig, TestFactory_CreateJobForWorkflowJob_AgentNotFound, TestFactory_CreateJobForWorkflowJob_WorkflowRunNotFound, TestFactory_CreateJobForWorkflowJob_WorkflowNotFound), and run the test suite to confirm nothing else depends on them.
🤖 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/workspace/workflowmanager/manager_test.go`:
- Around line 3-435: The PR removed critical workflow manager tests by
commenting out TestWorkflowManager_CreatesNewWorkflowRun,
TestWorkflowManager_DispatchesAllJobsConcurrently, TestWorkflowView_IsComplete
and multiple TestMaybeSetDefaultInputValues cases; restore these tests (or
replace them with equivalent tests) so CreateWorkflowRun,
maybeSetDefaultInputValues, and CEL-based job gating are covered: re-enable the
commented test functions (TestWorkflowManager_CreatesNewWorkflowRun,
TestWorkflowManager_DispatchesAllJobsConcurrently, TestWorkflowView_IsComplete,
and the TestMaybeSetDefaultInputValues variants) or add new tests that exercise
WorkflowManager.CreateWorkflowRun, WorkflowRun view completeness via
NewWorkflowRunView, and manager.maybeSetDefaultInputValues, ensuring assertions
verify job creation, JobAgentConfig merging, job If-expression behavior, and
default input population.
In `@apps/workspace-engine/pkg/workspace/workflowmanager/manager.go`:
- Around line 82-135: The CreateWorkflowRun method currently returns (nil, nil)
which silently no-ops; restore its implementation in Manager.CreateWorkflowRun
so it actually looks up the workflow via m.store.Workflows.Get, sets default
inputs via m.maybeSetDefaultInputValues, creates and upserts a WorkflowRun,
iterates workflow.Jobs applying m.evaluateJobTemplateIf, creates WorkflowJob
records and upserts them (m.store.WorkflowJobs.Upsert), uses
m.factory.CreateJobForWorkflowJob to create concrete jobs and upserts them
(m.store.Jobs.Upsert), and dispatches each job with m.jobAgentRegistry.Dispatch;
preserve and propagate errors (return non-nil err) instead of swallowing them so
callers relying on err can detect failures.
In `@apps/workspace-engine/svc/controllers/desiredrelease/setters_postgres.go`:
- Around line 19-20: The PostgresSetter's Queue field is left nil by
NewPostgresSetter causing a panic when events.EnqueueJobEligibility dereferences
it; update NewPostgresSetter to accept a reconcile.Queue parameter and assign it
to the PostgresSetter.Queue (and similarly ensure any code paths that construct
a PostgresSetter set Queue, including the zero-value case referenced around the
alternative constructor lines ~85-91), and make NewPostgresSetter return an
error if the provided queue is nil so the package fails fast rather than
panicking when events.EnqueueJobEligibility is called.
In `@apps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.go`:
- Around line 54-85: The function GetJobsForReleaseTarget (and the sibling
method block at lines 87-119) must not swallow parse/query failures by returning
nil map; change their signatures to return (map[string]*oapi.Job, error),
replace the logged-only error paths (uuid.Parse failures and
ListJobsByReleaseTarget error) to return (nil, err) after logging, and on
success return (jobs, nil); update callers to handle the error. Ensure you
update the usages of GetJobsForReleaseTarget and the other getter so they
propagate and handle the error instead of treating nil as "no jobs."
In `@apps/workspace-engine/svc/controllers/jobeligibility/postgres_test.go`:
- Around line 66-70: The test TestPostgresSetter_CreateJob_Stub currently treats
CreateJob as a stub by expecting an error; change it to assert the real happy
path: instantiate a PostgresSetter backed by a test DB (or proper test fixture)
and a mockQueue, call PostgresSetter.CreateJob, require.NoError(t, err), and
then verify persistence side-effects (e.g., query the jobs table or call a
repository method to assert the Job with the given Id/ReleaseId exists and/or
that mockQueue received the expected enqueue call). Ensure the test uses
PostgresSetter and CreateJob identifiers to locate the implementation and
replace require.Error with require.NoError plus an assertion that the job was
persisted/enqueued.
In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go`:
- Around line 492-494: The tests use setupHappyPath and then set getter.jobs =
[]*oapi.Job{activeJob}, but releasetargetconcurrency reads
GetJobsInProcessingStateForReleaseTarget so the concurrency path isn't
exercised; update the failing test cases (the blocks that call
setupHappyPath(rt, release) and then set getter.jobs) to instead populate
getter.processingJobs (or the mock backing
GetJobsInProcessingStateForReleaseTarget) with []*oapi.Job{activeJob} so the
processing-state concurrency gate is actually hit — apply the same change to the
other occurrence of this pattern around the second test case.
In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go`:
- Around line 103-134: The loop currently marks pending before checking for a
terminal deny, causing a later terminal deny to be ignored; change the order
inside the evals loop in reconcile.go so you immediately handle a terminal deny
(if !result.Allowed && !result.ActionRequired) by returning false, nil,
result.Message (or at minimum setting hasBlocked and breaking) before you set
hasPending/reason/earliestNextTime for result.ActionRequired; use the existing
symbols (evals, result.ActionRequired, result.Allowed,
result.NextEvaluationTime, earliestNextTime, reason) to locate and reorder the
checks so denies are not downgraded to “requeue later.”
In `@apps/workspace-engine/svc/controllers/jobeligibility/setters_postgres.go`:
- Around line 45-60: Wrap the two separate inserts into a single database
transaction so they succeed or fail atomically: begin a transaction (e.g., using
your DB/queries transaction helper or db.BeginTx), perform q.InsertJob(...) and
q.InsertReleaseJob(...) inside the same transactional queries object, and
commit; if either insert returns an error rollback and return that error
(preserving the existing error wrapping). Ensure you use the same ctx and the
same jobID and release.Id values inside the transaction and replace the
standalone calls to InsertJob and InsertReleaseJob with the transactional
equivalents so no orphan job rows can be left behind.
---
Duplicate comments:
In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile.go`:
- Around line 152-174: The loop persists and enqueues each agent job one-by-one
which can leave a partial rollout if a later CreateJob or EnqueueJobDispatch
fails; change to build all jobs first using
jobs.NewFactoryFromGetters(...).CreateJobForRelease for each agentRef, then
persist and enqueue in a recovery-safe way (either perform a single batched
persist/enqueue or make r.setter.CreateJob and r.setter.EnqueueJobDispatch
idempotent/resumable by checking for an existing pending job for the same
agent/job Id before creating/enqueuing); update the reconcile loop around
deployment.JobAgents to 1) collect jobs, 2) check for existing jobs via
r.getter.GetJob or query by agent, and 3) create/enqueue only missing jobs (or
use a batch API) so a partial failure on one agent does not permanently block
others.
---
Nitpick comments:
In `@apps/workspace-engine/pkg/workspace/jobs/factory_test.go`:
- Around line 114-128: The test currently only covers "not found" database rows
but misses the UUID parse failure branches in Factory.CreateJobForRelease;
update TestFactory_CreateJobForRelease_DeploymentNotFound (or add a new
table-driven test alongside it) to include cases where release.DeploymentID,
release.EnvironmentID, and release.ResourceID are malformed (e.g.
"invalid-uuid") and assert that NewFactory(...).CreateJobForRelease(ctx,
release, jobAgent, nil) returns an error indicating a parse/invalid UUID rather
than a not-found error; include separate subtests for each malformed ID to lock
down the parsing error paths.
- Around line 625-877: Remove the entire commented-out workflow test block (the
large commented region containing setupWorkflowStore and all
TestFactory_CreateJobForWorkflowJob_* tests, including
TestFactory_CreateJobForWorkflowJob_DispatchContextHasNoReleaseFields and the
related deep-merge tests) from factory_test.go so the file no longer contains
hundreds of lines of disabled tests; delete the commented lines cleanly, ensure
no remaining dangling references to setupWorkflowStore or the removed test names
(e.g., TestFactory_CreateJobForWorkflowJob_Success,
TestFactory_CreateJobForWorkflowJob_BuildsDispatchContext,
TestFactory_CreateJobForWorkflowJob_MergesConfig,
TestFactory_CreateJobForWorkflowJob_AgentNotFound,
TestFactory_CreateJobForWorkflowJob_WorkflowRunNotFound,
TestFactory_CreateJobForWorkflowJob_WorkflowNotFound), and run the test suite to
confirm nothing else depends on them.
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go`:
- Around line 20-24: The exported type Getters and the exported constructor
NewFactoryFromGetters lack Go doc comments; add concise package-comment style
documentation above the Getters interface and above NewFactoryFromGetters
explaining their purpose and contract (what
GetDeployment/GetEnvironment/GetResource do, expected inputs/outputs and error
behavior), and describe NewFactoryFromGetters’ behavior, parameters, return
value, and any side effects or concurrency guarantees so users understand how to
use them.
In `@apps/workspace-engine/pkg/workspace/jobs/getters_store.go`:
- Around line 18-20: Add a Go doc comment immediately above the exported
constructor NewStoreGetters describing what it does and its parameters/return
(e.g., "NewStoreGetters returns a new storeGetters backed by the given
*store.Store"); ensure the comment starts with "NewStoreGetters" (godoc style)
and briefly documents the returned value (storeGetters) and the input store
parameter so the exported function is properly documented.
In `@apps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.go`:
- Around line 143-145: The EnqueueJobEligibility stub on mockReconcileSetter
currently returns nil and records nothing; change it to record calls so tests
can assert the handoff happened: add observable state on mockReconcileSetter
(e.g., a slice or last-enqueued record and optionally a sync.Mutex/counter) and
have EnqueueJobEligibility(ctx context.Context, workspace string, rt
*ReleaseTarget) append or set that state (and return nil); update tests to
inspect that recorded workspace and ReleaseTarget to verify the enqueue
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e094b380-e905-46f4-be96-0f6d1ce85f81
📒 Files selected for processing (21)
apps/workspace-engine/pkg/db/jobs.sql.goapps/workspace-engine/pkg/db/queries/jobs.sqlapps/workspace-engine/pkg/db/queries/releases.sqlapps/workspace-engine/pkg/db/releases.sql.goapps/workspace-engine/pkg/workspace/jobs/factory.goapps/workspace-engine/pkg/workspace/jobs/factory_test.goapps/workspace-engine/pkg/workspace/jobs/getters_store.goapps/workspace-engine/pkg/workspace/workflowmanager/manager.goapps/workspace-engine/pkg/workspace/workflowmanager/manager_test.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile.goapps/workspace-engine/svc/controllers/desiredrelease/reconcile_test.goapps/workspace-engine/svc/controllers/desiredrelease/setters.goapps/workspace-engine/svc/controllers/desiredrelease/setters_postgres.goapps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.goapps/workspace-engine/svc/controllers/jobeligibility/postgres_test.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile.goapps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.goapps/workspace-engine/svc/controllers/jobeligibility/setters.goapps/workspace-engine/svc/controllers/jobeligibility/setters_postgres.goapps/workspace-engine/test/controllers/harness/mocks.goapps/workspace-engine/test/e2e/engine_workflow_lifecycle_test.go
✅ Files skipped from review due to trivial changes (1)
- apps/workspace-engine/test/e2e/engine_workflow_lifecycle_test.go
Comment on lines
+3
to
+435
| // func TestWorkflowManager_CreatesNewWorkflowRun(t *testing.T) { | ||
| // ctx := context.Background() | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // var stringInput oapi.WorkflowInput | ||
| // _ = stringInput.FromWorkflowStringInput(oapi.WorkflowStringInput{ | ||
| // Key: "test-input", | ||
| // Type: oapi.String, | ||
| // Default: &[]string{"test-default"}[0], | ||
| // }) | ||
|
|
||
| // jobAgent1ID := uuid.New().String() | ||
| // jobAgent1 := &oapi.JobAgent{ | ||
| // Id: jobAgent1ID, | ||
| // Name: "test-job-agent-1", | ||
| // Type: "test-runner", | ||
| // Config: map[string]any{ | ||
| // "test-config": "test-value-1", | ||
| // }, | ||
| // } | ||
| // store.JobAgents.Upsert(ctx, jobAgent1) | ||
|
|
||
| // workflowID := uuid.New().String() | ||
| // workflowJobTemplateID := uuid.New().String() | ||
| // workflow := &oapi.Workflow{ | ||
| // Id: workflowID, | ||
| // Name: "test-workflow", | ||
| // Inputs: []oapi.WorkflowInput{stringInput}, | ||
| // Jobs: []oapi.WorkflowJobTemplate{ | ||
| // { | ||
| // Id: workflowJobTemplateID, | ||
| // Name: "test-job", | ||
| // Ref: jobAgent1ID, | ||
| // Config: map[string]any{ | ||
| // "delaySeconds": 10, | ||
| // }, | ||
| // }, | ||
| // }, | ||
| // } | ||
| // store.Workflows.Upsert(ctx, workflow) | ||
|
|
||
| // wfRun, err := manager.CreateWorkflowRun(ctx, workflowID, map[string]any{ | ||
| // "test-input": "test-value", | ||
| // }) | ||
|
|
||
| // workflowRun, ok := store.WorkflowRuns.Get(wfRun.Id) | ||
| // assert.True(t, ok) | ||
| // assert.NoError(t, err) | ||
| // assert.NotNil(t, workflowRun) | ||
| // assert.Equal(t, workflowID, workflowRun.WorkflowId) | ||
| // assert.Equal(t, map[string]any{ | ||
| // "test-input": "test-value", | ||
| // }, workflowRun.Inputs) | ||
|
|
||
| // wfJobs := store.WorkflowJobs.GetByWorkflowRunId(workflowRun.Id) | ||
| // assert.Len(t, wfJobs, 1) | ||
| // assert.Equal(t, 0, wfJobs[0].Index) | ||
|
|
||
| // jobs := store.Jobs.GetByWorkflowJobId(wfJobs[0].Id) | ||
| // assert.Len(t, jobs, 1) | ||
| // job := jobs[0] | ||
| // assert.NotNil(t, job) | ||
| // assert.Equal(t, oapi.JobStatusPending, job.Status) | ||
| // assert.Equal(t, wfJobs[0].Id, job.WorkflowJobId) | ||
| // assert.Equal(t, jobAgent1.Id, job.JobAgentId) | ||
| // assert.Equal(t, oapi.JobAgentConfig{ | ||
| // "test-config": "test-value-1", | ||
| // "delaySeconds": 10, | ||
| // }, job.JobAgentConfig) | ||
| // } | ||
|
|
||
| // func TestWorkflowManager_DispatchesAllJobsConcurrently(t *testing.T) { | ||
| // ctx := context.Background() | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // var stringInput oapi.WorkflowInput | ||
| // _ = stringInput.FromWorkflowStringInput(oapi.WorkflowStringInput{ | ||
| // Key: "test-input", | ||
| // Type: oapi.String, | ||
| // Default: &[]string{"test-default"}[0], | ||
| // }) | ||
|
|
||
| // jobAgent1 := &oapi.JobAgent{ | ||
| // Id: "test-job-agent-1", | ||
| // Name: "test-job-agent-1", | ||
| // Type: "test-runner", | ||
| // Config: map[string]any{ | ||
| // "test-config": "test-value-1", | ||
| // }, | ||
| // } | ||
| // store.JobAgents.Upsert(ctx, jobAgent1) | ||
|
|
||
| // jobAgent2 := &oapi.JobAgent{ | ||
| // Id: "test-job-agent-2", | ||
| // Name: "test-job-agent-2", | ||
| // Type: "test-runner", | ||
| // Config: map[string]any{ | ||
| // "test-config": "test-value-2", | ||
| // }, | ||
| // } | ||
| // store.JobAgents.Upsert(ctx, jobAgent2) | ||
|
|
||
| // workflow := &oapi.Workflow{ | ||
| // Id: "test-workflow", | ||
| // Name: "test-workflow", | ||
| // Inputs: []oapi.WorkflowInput{stringInput}, | ||
| // Jobs: []oapi.WorkflowJobTemplate{ | ||
| // { | ||
| // Id: "test-job-1", | ||
| // Name: "test-job-1", | ||
| // Ref: "test-job-agent-1", | ||
| // Config: map[string]any{ | ||
| // "delaySeconds": 10, | ||
| // }, | ||
| // }, | ||
| // { | ||
| // Id: "test-job-2", | ||
| // Name: "test-job-2", | ||
| // Ref: "test-job-agent-2", | ||
| // Config: map[string]any{ | ||
| // "delaySeconds": 20, | ||
| // }, | ||
| // }, | ||
| // }, | ||
| // } | ||
| // store.Workflows.Upsert(ctx, workflow) | ||
|
|
||
| // wfRun, err := manager.CreateWorkflowRun(ctx, "test-workflow", map[string]any{ | ||
| // "test-input": "test-value", | ||
| // }) | ||
| // assert.NoError(t, err) | ||
| // assert.NotNil(t, wfRun) | ||
| // assert.Equal(t, "test-workflow", wfRun.WorkflowId) | ||
|
|
||
| // wfJobs := store.WorkflowJobs.GetByWorkflowRunId(wfRun.Id) | ||
| // assert.Len(t, wfJobs, 2) | ||
| // assert.Equal(t, 0, wfJobs[0].Index) | ||
| // assert.Equal(t, 1, wfJobs[1].Index) | ||
|
|
||
| // jobs1 := store.Jobs.GetByWorkflowJobId(wfJobs[0].Id) | ||
| // assert.Len(t, jobs1, 1) | ||
| // assert.Equal(t, oapi.JobStatusPending, jobs1[0].Status) | ||
| // assert.Equal(t, jobAgent1.Id, jobs1[0].JobAgentId) | ||
| // assert.Equal(t, oapi.JobAgentConfig{ | ||
| // "test-config": "test-value-1", | ||
| // "delaySeconds": 10, | ||
| // }, jobs1[0].JobAgentConfig) | ||
|
|
||
| // jobs2 := store.Jobs.GetByWorkflowJobId(wfJobs[1].Id) | ||
| // assert.Len(t, jobs2, 1) | ||
| // assert.Equal(t, oapi.JobStatusPending, jobs2[0].Status) | ||
| // assert.Equal(t, jobAgent2.Id, jobs2[0].JobAgentId) | ||
| // assert.Equal(t, oapi.JobAgentConfig{ | ||
| // "test-config": "test-value-2", | ||
| // "delaySeconds": 20, | ||
| // }, jobs2[0].JobAgentConfig) | ||
| // } | ||
|
|
||
| // func TestWorkflowView_IsComplete(t *testing.T) { | ||
| // ctx := context.Background() | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // jobAgent1 := &oapi.JobAgent{ | ||
| // Id: "test-job-agent-1", | ||
| // Name: "test-job-agent-1", | ||
| // Type: "test-runner", | ||
| // } | ||
| // store.JobAgents.Upsert(ctx, jobAgent1) | ||
|
|
||
| // jobAgent2 := &oapi.JobAgent{ | ||
| // Id: "test-job-agent-2", | ||
| // Name: "test-job-agent-2", | ||
| // Type: "test-runner", | ||
| // } | ||
| // store.JobAgents.Upsert(ctx, jobAgent2) | ||
|
|
||
| // workflow := &oapi.Workflow{ | ||
| // Id: "test-workflow", | ||
| // Name: "test-workflow", | ||
| // Jobs: []oapi.WorkflowJobTemplate{ | ||
| // {Id: "test-job-1", Name: "test-job-1", Ref: "test-job-agent-1"}, | ||
| // {Id: "test-job-2", Name: "test-job-2", Ref: "test-job-agent-2"}, | ||
| // }, | ||
| // } | ||
| // store.Workflows.Upsert(ctx, workflow) | ||
|
|
||
| // wfRun, _ := manager.CreateWorkflowRun(ctx, "test-workflow", nil) | ||
|
|
||
| // wfv, err := NewWorkflowRunView(store, wfRun.Id) | ||
| // assert.NoError(t, err) | ||
| // assert.False(t, wfv.IsComplete()) | ||
|
|
||
| // wfJobs := store.WorkflowJobs.GetByWorkflowRunId(wfRun.Id) | ||
| // now := time.Now().UTC() | ||
|
|
||
| // job1 := store.Jobs.GetByWorkflowJobId(wfJobs[0].Id)[0] | ||
| // job1.CompletedAt = &now | ||
| // job1.Status = oapi.JobStatusSuccessful | ||
| // store.Jobs.Upsert(ctx, job1) | ||
|
|
||
| // wfv, _ = NewWorkflowRunView(store, wfRun.Id) | ||
| // assert.False(t, wfv.IsComplete()) | ||
|
|
||
| // job2 := store.Jobs.GetByWorkflowJobId(wfJobs[1].Id)[0] | ||
| // job2.CompletedAt = &now | ||
| // job2.Status = oapi.JobStatusSuccessful | ||
| // store.Jobs.Upsert(ctx, job2) | ||
|
|
||
| // wfv, _ = NewWorkflowRunView(store, wfRun.Id) | ||
| // assert.True(t, wfv.IsComplete()) | ||
| // } | ||
|
|
||
| // func TestCreateWorkflow_SkipsJobWhenIfEvaluatesToFalse(t *testing.T) { | ||
| // ctx := context.Background() | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // jobAgent := &oapi.JobAgent{ | ||
| // Id: "test-job-agent", | ||
| // Name: "test-job-agent", | ||
| // Type: "test-runner", | ||
| // } | ||
| // store.JobAgents.Upsert(ctx, jobAgent) | ||
|
|
||
| // ifTrue := "inputs.run_job == true" | ||
| // ifFalse := "inputs.run_job == false" | ||
|
|
||
| // workflow := &oapi.Workflow{ | ||
| // Id: "test-workflow", | ||
| // Name: "test-workflow", | ||
| // Jobs: []oapi.WorkflowJobTemplate{ | ||
| // {Id: "always-job", Name: "always-job", Ref: "test-job-agent", Config: map[string]any{}}, | ||
| // {Id: "true-job", Name: "true-job", Ref: "test-job-agent", Config: map[string]any{}, If: &ifTrue}, | ||
| // {Id: "false-job", Name: "false-job", Ref: "test-job-agent", Config: map[string]any{}, If: &ifFalse}, | ||
| // }, | ||
| // } | ||
| // store.Workflows.Upsert(ctx, workflow) | ||
|
|
||
| // wfRun, err := manager.CreateWorkflowRun(ctx, "test-workflow", map[string]any{ | ||
| // "run_job": true, | ||
| // }) | ||
| // assert.NoError(t, err) | ||
| // assert.NotNil(t, wfRun) | ||
|
|
||
| // wfJobs := store.WorkflowJobs.GetByWorkflowRunId(wfRun.Id) | ||
| // assert.Len(t, wfJobs, 2, "should have 2 jobs: always-job and true-job, but not false-job") | ||
| // } | ||
|
|
||
| // func TestMaybeSetDefaultInputValues_SetsStringDefault(t *testing.T) { | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // var stringInput oapi.WorkflowInput | ||
| // _ = stringInput.FromWorkflowStringInput(oapi.WorkflowStringInput{ | ||
| // Key: "string-input", | ||
| // Type: oapi.String, | ||
| // Default: &[]string{"default-value"}[0], | ||
| // }) | ||
|
|
||
| // workflow := &oapi.Workflow{ | ||
| // Id: "test-workflow", | ||
| // Inputs: []oapi.WorkflowInput{stringInput}, | ||
| // } | ||
|
|
||
| // inputs := map[string]any{} | ||
| // manager.maybeSetDefaultInputValues(inputs, workflow) | ||
|
|
||
| // assert.Equal(t, "default-value", inputs["string-input"]) | ||
| // } | ||
|
|
||
| // func TestMaybeSetDefaultInputValues_SetsNumberDefault(t *testing.T) { | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // var numberInput oapi.WorkflowInput | ||
| // _ = numberInput.FromWorkflowNumberInput(oapi.WorkflowNumberInput{ | ||
| // Key: "number-input", | ||
| // Type: oapi.Number, | ||
| // Default: &[]float32{42.0}[0], | ||
| // }) | ||
|
|
||
| // workflow := &oapi.Workflow{ | ||
| // Id: "test-workflow", | ||
| // Inputs: []oapi.WorkflowInput{numberInput}, | ||
| // } | ||
|
|
||
| // inputs := map[string]any{} | ||
| // manager.maybeSetDefaultInputValues(inputs, workflow) | ||
|
|
||
| // assert.Equal(t, float32(42.0), inputs["number-input"]) | ||
| // } | ||
|
|
||
| // func TestMaybeSetDefaultInputValues_SetsBooleanDefault(t *testing.T) { | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // var booleanInput oapi.WorkflowInput | ||
| // _ = booleanInput.FromWorkflowBooleanInput(oapi.WorkflowBooleanInput{ | ||
| // Key: "boolean-input", | ||
| // Type: oapi.Boolean, | ||
| // Default: &[]bool{true}[0], | ||
| // }) | ||
|
|
||
| // workflow := &oapi.Workflow{ | ||
| // Id: "test-workflow", | ||
| // Inputs: []oapi.WorkflowInput{booleanInput}, | ||
| // } | ||
|
|
||
| // inputs := map[string]any{} | ||
| // manager.maybeSetDefaultInputValues(inputs, workflow) | ||
|
|
||
| // assert.Equal(t, true, inputs["boolean-input"]) | ||
| // } | ||
|
|
||
| // func TestMaybeSetDefaultInputValues_SetsObjectDefault(t *testing.T) { | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // var objectInput oapi.WorkflowInput | ||
| // _ = objectInput.FromWorkflowObjectInput(oapi.WorkflowObjectInput{ | ||
| // Key: "object-input", | ||
| // Type: oapi.Object, | ||
| // Default: &map[string]any{"key": "value"}, | ||
| // }) | ||
|
|
||
| // workflow := &oapi.Workflow{ | ||
| // Id: "test-workflow", | ||
| // Inputs: []oapi.WorkflowInput{objectInput}, | ||
| // } | ||
|
|
||
| // inputs := map[string]any{} | ||
| // manager.maybeSetDefaultInputValues(inputs, workflow) | ||
| // assert.Equal(t, map[string]any{"key": "value"}, inputs["object-input"]) | ||
| // } | ||
|
|
||
| // func TestMaybeSetDefaultInputValues_DoesNotOverwriteExistingValue(t *testing.T) { | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // var stringInput oapi.WorkflowInput | ||
| // _ = stringInput.FromWorkflowStringInput(oapi.WorkflowStringInput{ | ||
| // Key: "string-input", | ||
| // Type: oapi.String, | ||
| // Default: &[]string{"default-value"}[0], | ||
| // }) | ||
|
|
||
| // workflow := &oapi.Workflow{ | ||
| // Id: "test-workflow", | ||
| // Inputs: []oapi.WorkflowInput{stringInput}, | ||
| // } | ||
|
|
||
| // inputs := map[string]any{ | ||
| // "string-input": "user-provided-value", | ||
| // } | ||
| // manager.maybeSetDefaultInputValues(inputs, workflow) | ||
|
|
||
| // assert.Equal(t, "user-provided-value", inputs["string-input"]) | ||
| // } | ||
|
|
||
| // func TestMaybeSetDefaultInputValues_HandlesMultipleInputTypes(t *testing.T) { | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // var stringInput oapi.WorkflowInput | ||
| // _ = stringInput.FromWorkflowStringInput(oapi.WorkflowStringInput{ | ||
| // Key: "string-input", | ||
| // Type: oapi.String, | ||
| // Default: &[]string{"default-string"}[0], | ||
| // }) | ||
|
|
||
| // var numberInput oapi.WorkflowInput | ||
| // _ = numberInput.FromWorkflowNumberInput(oapi.WorkflowNumberInput{ | ||
| // Key: "number-input", | ||
| // Type: oapi.Number, | ||
| // Default: &[]float32{123.0}[0], | ||
| // }) | ||
|
|
||
| // var booleanInput oapi.WorkflowInput | ||
| // _ = booleanInput.FromWorkflowBooleanInput(oapi.WorkflowBooleanInput{ | ||
| // Key: "boolean-input", | ||
| // Type: oapi.Boolean, | ||
| // Default: &[]bool{false}[0], | ||
| // }) | ||
|
|
||
| // workflow := &oapi.Workflow{ | ||
| // Id: "test-workflow-template", | ||
| // Inputs: []oapi.WorkflowInput{stringInput, numberInput, booleanInput}, | ||
| // } | ||
|
|
||
| // inputs := map[string]any{} | ||
| // manager.maybeSetDefaultInputValues(inputs, workflow) | ||
|
|
||
| // assert.Equal(t, "default-string", inputs["string-input"]) | ||
| // assert.Equal(t, float32(123.0), inputs["number-input"]) | ||
| // assert.Equal(t, false, inputs["boolean-input"]) | ||
| // } | ||
|
|
||
| // func TestMaybeSetDefaultInputValues_SkipsInputsWithoutDefault(t *testing.T) { | ||
| // store := store.New("test-workspace", statechange.NewChangeSet[any]()) | ||
| // jobAgentRegistry := jobagents.NewRegistry(store, verification.NewManager(store), nil) | ||
| // manager := NewWorkflowManager(store, jobAgentRegistry) | ||
|
|
||
| // var stringInput oapi.WorkflowInput | ||
| // _ = stringInput.FromWorkflowStringInput(oapi.WorkflowStringInput{ | ||
| // Key: "string-input", | ||
| // Type: oapi.String, | ||
| // // No default | ||
| // }) | ||
|
|
||
| // workflow := &oapi.Workflow{ | ||
| // Id: "test-workflow", | ||
| // Inputs: []oapi.WorkflowInput{stringInput}, | ||
| // } | ||
|
|
||
| // inputs := map[string]any{} | ||
| // manager.maybeSetDefaultInputValues(inputs, workflow) | ||
|
|
||
| // _, exists := inputs["string-input"] | ||
| // assert.False(t, exists) | ||
| // } |
Contributor
There was a problem hiding this comment.
Don't comment out the workflow manager coverage without a replacement.
This removes the only tests for workflow-run creation, default inputs, and CEL job gating while the production workflow event handler still calls CreateWorkflowRun, so the regression can ship unnoticed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/workflowmanager/manager_test.go` around
lines 3 - 435, The PR removed critical workflow manager tests by commenting out
TestWorkflowManager_CreatesNewWorkflowRun,
TestWorkflowManager_DispatchesAllJobsConcurrently, TestWorkflowView_IsComplete
and multiple TestMaybeSetDefaultInputValues cases; restore these tests (or
replace them with equivalent tests) so CreateWorkflowRun,
maybeSetDefaultInputValues, and CEL-based job gating are covered: re-enable the
commented test functions (TestWorkflowManager_CreatesNewWorkflowRun,
TestWorkflowManager_DispatchesAllJobsConcurrently, TestWorkflowView_IsComplete,
and the TestMaybeSetDefaultInputValues variants) or add new tests that exercise
WorkflowManager.CreateWorkflowRun, WorkflowRun view completeness via
NewWorkflowRunView, and manager.maybeSetDefaultInputValues, ensuring assertions
verify job creation, JobAgentConfig merging, job If-expression behavior, and
default input population.
Comment on lines
82
to
+135
| func (m *Manager) CreateWorkflowRun(ctx context.Context, workflowId string, inputs map[string]any) (*oapi.WorkflowRun, error) { | ||
| workflow, ok := m.store.Workflows.Get(workflowId) | ||
| if !ok { | ||
| return nil, fmt.Errorf("workflow %s not found", workflowId) | ||
| } | ||
|
|
||
| m.maybeSetDefaultInputValues(inputs, workflow) | ||
|
|
||
| workflowRun := &oapi.WorkflowRun{ | ||
| Id: uuid.New().String(), | ||
| WorkflowId: workflowId, | ||
| Inputs: maps.Clone(inputs), | ||
| } | ||
|
|
||
| m.store.WorkflowRuns.Upsert(ctx, workflowRun) | ||
|
|
||
| workflowJobs := make([]*oapi.WorkflowJob, 0, len(workflow.Jobs)) | ||
| for idx, jobTemplate := range workflow.Jobs { | ||
| if jobTemplate.If != nil { | ||
| shouldRun, err := m.evaluateJobTemplateIf(jobTemplate, inputs) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to evaluate CEL expression for job %q: %w", jobTemplate.Name, err) | ||
| } | ||
| if !shouldRun { | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| wfJob := &oapi.WorkflowJob{ | ||
| Id: uuid.New().String(), | ||
| WorkflowRunId: workflowRun.Id, | ||
| Index: idx, | ||
| Ref: jobTemplate.Ref, | ||
| Config: maps.Clone(jobTemplate.Config), | ||
| } | ||
| m.store.WorkflowJobs.Upsert(ctx, wfJob) | ||
| workflowJobs = append(workflowJobs, wfJob) | ||
| } | ||
|
|
||
| m.store.WorkflowRuns.Upsert(ctx, workflowRun) | ||
|
|
||
| for _, wfJob := range workflowJobs { | ||
| job, err := m.factory.CreateJobForWorkflowJob(ctx, wfJob) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create job for workflow job %q: %w", wfJob.Id, err) | ||
| } | ||
| m.store.Jobs.Upsert(ctx, job) | ||
| if err := m.jobAgentRegistry.Dispatch(ctx, job); err != nil { | ||
| return nil, fmt.Errorf("failed to dispatch job: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return workflowRun, nil | ||
| return nil, nil | ||
| // workflow, ok := m.store.Workflows.Get(workflowId) | ||
| // if !ok { | ||
| // return nil, fmt.Errorf("workflow %s not found", workflowId) | ||
| // } | ||
|
|
||
| // m.maybeSetDefaultInputValues(inputs, workflow) | ||
|
|
||
| // workflowRun := &oapi.WorkflowRun{ | ||
| // Id: uuid.New().String(), | ||
| // WorkflowId: workflowId, | ||
| // Inputs: maps.Clone(inputs), | ||
| // } | ||
|
|
||
| // m.store.WorkflowRuns.Upsert(ctx, workflowRun) | ||
|
|
||
| // workflowJobs := make([]*oapi.WorkflowJob, 0, len(workflow.Jobs)) | ||
| // for idx, jobTemplate := range workflow.Jobs { | ||
| // if jobTemplate.If != nil { | ||
| // shouldRun, err := m.evaluateJobTemplateIf(jobTemplate, inputs) | ||
| // if err != nil { | ||
| // return nil, fmt.Errorf("failed to evaluate CEL expression for job %q: %w", jobTemplate.Name, err) | ||
| // } | ||
| // if !shouldRun { | ||
| // continue | ||
| // } | ||
| // } | ||
|
|
||
| // wfJob := &oapi.WorkflowJob{ | ||
| // Id: uuid.New().String(), | ||
| // WorkflowRunId: workflowRun.Id, | ||
| // Index: idx, | ||
| // Ref: jobTemplate.Ref, | ||
| // Config: maps.Clone(jobTemplate.Config), | ||
| // } | ||
| // m.store.WorkflowJobs.Upsert(ctx, wfJob) | ||
| // workflowJobs = append(workflowJobs, wfJob) | ||
| // } | ||
|
|
||
| // m.store.WorkflowRuns.Upsert(ctx, workflowRun) | ||
|
|
||
| // for _, wfJob := range workflowJobs { | ||
| // job, err := m.factory.CreateJobForWorkflowJob(ctx, wfJob) | ||
| // if err != nil { | ||
| // return nil, fmt.Errorf("failed to create job for workflow job %q: %w", wfJob.Id, err) | ||
| // } | ||
| // m.store.Jobs.Upsert(ctx, job) | ||
| // if err := m.jobAgentRegistry.Dispatch(ctx, job); err != nil { | ||
| // return nil, fmt.Errorf("failed to dispatch job: %w", err) | ||
| // } | ||
| // } | ||
|
|
||
| // return workflowRun, nil |
Contributor
There was a problem hiding this comment.
Don't silently no-op CreateWorkflowRun.
The caller in apps/workspace-engine/pkg/events/handler/workflows/workflows.go only checks err, so returning (nil, nil) here will acknowledge workflow-run events without creating a run or dispatching any jobs.
⚠️ Minimal safe fallback
func (m *Manager) CreateWorkflowRun(ctx context.Context, workflowId string, inputs map[string]any) (*oapi.WorkflowRun, error) {
- return nil, nil
+ return nil, fmt.Errorf("CreateWorkflowRun is temporarily disabled")
// workflow, ok := m.store.Workflows.Get(workflowId)
// if !ok {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/pkg/workspace/workflowmanager/manager.go` around lines
82 - 135, The CreateWorkflowRun method currently returns (nil, nil) which
silently no-ops; restore its implementation in Manager.CreateWorkflowRun so it
actually looks up the workflow via m.store.Workflows.Get, sets default inputs
via m.maybeSetDefaultInputValues, creates and upserts a WorkflowRun, iterates
workflow.Jobs applying m.evaluateJobTemplateIf, creates WorkflowJob records and
upserts them (m.store.WorkflowJobs.Upsert), uses
m.factory.CreateJobForWorkflowJob to create concrete jobs and upserts them
(m.store.Jobs.Upsert), and dispatches each job with m.jobAgentRegistry.Dispatch;
preserve and propagate errors (return non-nil err) instead of swallowing them so
callers relying on err can detect failures.
Comment on lines
+19
to
20
| Queue reconcile.Queue | ||
| } |
Contributor
There was a problem hiding this comment.
Initialize Queue before using the new enqueue path.
NewPostgresSetter() still produces a setter with Queue == nil, and events.EnqueueJobEligibility immediately dereferences that queue. Any caller using the current constructor or a zero-value PostgresSetter will panic on the first enqueue instead of returning an error. Wire the enqueue queue into this setter and fail fast here if it is unset.
Also applies to: 85-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/desiredrelease/setters_postgres.go`
around lines 19 - 20, The PostgresSetter's Queue field is left nil by
NewPostgresSetter causing a panic when events.EnqueueJobEligibility dereferences
it; update NewPostgresSetter to accept a reconcile.Queue parameter and assign it
to the PostgresSetter.Queue (and similarly ensure any code paths that construct
a PostgresSetter set Queue, including the zero-value case referenced around the
alternative constructor lines ~85-91), and make NewPostgresSetter return an
error if the provided queue is nil so the package fails fast rather than
panicking when events.EnqueueJobEligibility is called.
Comment on lines
+54
to
+85
| func (g *PostgresGetter) GetJobsForReleaseTarget(ctx context.Context, releaseTarget *oapi.ReleaseTarget) map[string]*oapi.Job { | ||
| deploymentID, err := uuid.Parse(releaseTarget.DeploymentId) | ||
| if err != nil { | ||
| log.Error("failed to parse deployment id", "deploymentID", releaseTarget.DeploymentId, "error", err) | ||
| return nil | ||
| } | ||
| environmentID, err := uuid.Parse(releaseTarget.EnvironmentId) | ||
| if err != nil { | ||
| log.Error("failed to parse environment id", "environmentID", releaseTarget.EnvironmentId, "error", err) | ||
| return nil | ||
| } | ||
| resourceID, err := uuid.Parse(releaseTarget.ResourceId) | ||
| if err != nil { | ||
| log.Error("failed to parse resource id", "resourceID", releaseTarget.ResourceId, "error", err) | ||
| return nil | ||
| } | ||
| rows, err := db.GetQueries(ctx).ListJobsByReleaseTarget(ctx, db.ListJobsByReleaseTargetParams{ | ||
| DeploymentID: deploymentID, | ||
| EnvironmentID: environmentID, | ||
| ResourceID: resourceID, | ||
| }) | ||
| if err != nil { | ||
| log.Error("failed to get jobs for release target", "releaseTarget", releaseTarget.Key(), "error", err) | ||
| return nil | ||
| } | ||
| jobs := make(map[string]*oapi.Job, len(rows)) | ||
| for _, row := range rows { | ||
| job := db.ToOapiJob(db.ListJobsByReleaseIDRow(row)) | ||
| jobs[job.Id] = job | ||
| } | ||
| return jobs | ||
| } |
Contributor
There was a problem hiding this comment.
Don't hide job lookup failures behind a nil map.
These methods feed the concurrency/retry eligibility checks, but UUID parse and query errors are only logged and converted to nil. That makes a failed read indistinguishable from "no matching jobs" and can produce permissive eligibility decisions from incomplete data.
Also applies to: 87-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/jobeligibility/getters_postgres.go`
around lines 54 - 85, The function GetJobsForReleaseTarget (and the sibling
method block at lines 87-119) must not swallow parse/query failures by returning
nil map; change their signatures to return (map[string]*oapi.Job, error),
replace the logged-only error paths (uuid.Parse failures and
ListJobsByReleaseTarget error) to return (nil, err) after logging, and on
success return (jobs, nil); update callers to handle the error. Ensure you
update the usages of GetJobsForReleaseTarget and the other getter so they
propagate and handle the error instead of treating nil as "no jobs."
Comment on lines
+66
to
+70
| func TestPostgresSetter_CreateJob_Stub(t *testing.T) { | ||
| s := &PostgresSetter{Queue: &mockQueue{}} | ||
| err := s.CreateJob(context.Background(), &oapi.Job{Id: uuid.New().String(), ReleaseId: uuid.New().String()}, &oapi.Release{Id: uuid.New()}) | ||
| require.Error(t, err) | ||
| } |
Contributor
There was a problem hiding this comment.
Don't codify the CreateJob stub.
Given this PR wires the eligibility controller into the main service, CI should be asserting successful job persistence here, not treating any error as expected behavior. As written, the core happy path can remain unimplemented and still go green.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/workspace-engine/svc/controllers/jobeligibility/postgres_test.go` around
lines 66 - 70, The test TestPostgresSetter_CreateJob_Stub currently treats
CreateJob as a stub by expecting an error; change it to assert the real happy
path: instantiate a PostgresSetter backed by a test DB (or proper test fixture)
and a mockQueue, call PostgresSetter.CreateJob, require.NoError(t, err), and
then verify persistence side-effects (e.g., query the jobs table or call a
repository method to assert the Job with the given Id/ReleaseId exists and/or
that mockQueue received the expected enqueue call). Ensure the test uses
PostgresSetter and CreateJob identifiers to locate the implementation and
replace require.Error with require.NoError plus an assertion that the job was
persisted/enqueued.
Comment on lines
+103
to
+134
| var earliestNextTime *time.Time | ||
| hasPending := false | ||
| hasBlocked := false | ||
| reason := "eligible" | ||
|
|
||
| for _, eval := range evals { | ||
| result := eval.Evaluate(ctx, r.release) | ||
|
|
||
| if result.ActionRequired { | ||
| hasPending = true | ||
| reason = result.Message | ||
| if result.NextEvaluationTime != nil { | ||
| if earliestNextTime == nil || result.NextEvaluationTime.Before(*earliestNextTime) { | ||
| earliestNextTime = result.NextEvaluationTime | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if !result.Allowed && !result.ActionRequired { | ||
| hasBlocked = true | ||
| reason = result.Message | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if hasPending { | ||
| return false, earliestNextTime, reason | ||
| } | ||
| if hasBlocked { | ||
| return false, nil, reason | ||
| } | ||
| return true, nil, reason |
Contributor
There was a problem hiding this comment.
A hard deny is currently downgraded to “requeue later.”
If one evaluator returns pending/backoff and a later evaluator returns a terminal deny, both flags become true and this method still returns NextReconcileAt because hasPending is checked first. That will requeue work that should be denied outright.
🛠️ Minimal fix
- if hasPending {
- return false, earliestNextTime, reason
- }
if hasBlocked {
return false, nil, reason
}
+ if hasPending {
+ return false, earliestNextTime, reason
+ }📝 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
| var earliestNextTime *time.Time | |
| hasPending := false | |
| hasBlocked := false | |
| reason := "eligible" | |
| for _, eval := range evals { | |
| result := eval.Evaluate(ctx, r.release) | |
| if result.ActionRequired { | |
| hasPending = true | |
| reason = result.Message | |
| if result.NextEvaluationTime != nil { | |
| if earliestNextTime == nil || result.NextEvaluationTime.Before(*earliestNextTime) { | |
| earliestNextTime = result.NextEvaluationTime | |
| } | |
| } | |
| } | |
| if !result.Allowed && !result.ActionRequired { | |
| hasBlocked = true | |
| reason = result.Message | |
| break | |
| } | |
| } | |
| if hasPending { | |
| return false, earliestNextTime, reason | |
| } | |
| if hasBlocked { | |
| return false, nil, reason | |
| } | |
| return true, nil, reason | |
| var earliestNextTime *time.Time | |
| hasPending := false | |
| hasBlocked := false | |
| reason := "eligible" | |
| for _, eval := range evals { | |
| result := eval.Evaluate(ctx, r.release) | |
| if result.ActionRequired { | |
| hasPending = true | |
| reason = result.Message | |
| if result.NextEvaluationTime != nil { | |
| if earliestNextTime == nil || result.NextEvaluationTime.Before(*earliestNextTime) { | |
| earliestNextTime = result.NextEvaluationTime | |
| } | |
| } | |
| } | |
| if !result.Allowed && !result.ActionRequired { | |
| hasBlocked = true | |
| reason = result.Message | |
| break | |
| } | |
| } | |
| if hasBlocked { | |
| return false, nil, reason | |
| } | |
| if hasPending { | |
| return false, earliestNextTime, reason | |
| } | |
| return true, nil, reason |
🤖 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.go` around
lines 103 - 134, The loop currently marks pending before checking for a terminal
deny, causing a later terminal deny to be ignored; change the order inside the
evals loop in reconcile.go so you immediately handle a terminal deny (if
!result.Allowed && !result.ActionRequired) by returning false, nil,
result.Message (or at minimum setting hasBlocked and breaking) before you set
hasPending/reason/earliestNextTime for result.ActionRequired; use the existing
symbols (evals, result.ActionRequired, result.Allowed,
result.NextEvaluationTime, earliestNextTime, reason) to locate and reorder the
checks so denies are not downgraded to “requeue later.”
apps/workspace-engine/svc/controllers/jobeligibility/setters_postgres.go
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/test/controllers/harness/mocks.go (1)
341-347:⚠️ Potential issue | 🟡 MinorDon’t silently accept
nilreleases in the harness.Returning
nilhere makes the mock more permissive than the real path and can hide a broken reconcile flow in tests. Failing fast is safer.Suggested change
func (s *DesiredReleaseSetter) SetDesiredRelease(ctx context.Context, _ *desiredrelease.ReleaseTarget, r *oapi.Release) error { s.mu.Lock() defer s.mu.Unlock() s.CallCount++ if r == nil { - return nil + return fmt.Errorf("nil release") } s.Releases = append(s.Releases, r)Based on learnings, the user prefers not to add null safety checks or defensive programming in test code, as they prioritize simplicity and focus on the main functionality being tested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/controllers/harness/mocks.go` around lines 341 - 347, The mock DesiredReleaseSetter.SetDesiredRelease currently ignores nil releases by returning nil; change it to fail fast: when r == nil return a non-nil error (e.g., errors.New or fmt.Errorf) so tests surface bogus reconcile flows instead of silently accepting nil. Update the SetDesiredRelease implementation in DesiredReleaseSetter to return an error on nil r (keeping mu.Lock(), defer, and CallCount behavior) and adjust any test expectations that relied on the permissive behavior.
♻️ Duplicate comments (1)
apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go (1)
492-493:⚠️ Potential issue | 🟡 MinorPopulate
processingJobshere to exercise the concurrency gate.
releasetargetconcurrencyreadsGetJobsInProcessingStateForReleaseTarget, so this case can still pass through retry/history behavior even if the active-job gate is broken. Seedgetter.processingJobsinstead, and ideally leavegetter.jobsempty so the test isolates the processing-state path.Suggested fix
getter, setter := setupHappyPath(rt, release) - getter.jobs = []*oapi.Job{activeJob} + getter.processingJobs = []*oapi.Job{activeJob}🤖 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 492 - 493, The test currently seeds getter.jobs with activeJob but should exercise the concurrency gate by populating getter.processingJobs instead; update the test (where setupHappyPath(rt, release) returns getter, setter) to set getter.processingJobs = []*oapi.Job{activeJob} and leave getter.jobs empty so the code path that calls GetJobsInProcessingStateForReleaseTarget is exercised; ensure you reference the same getter from setupHappyPath and keep the release variable as-is.
🧹 Nitpick comments (1)
apps/workspace-engine/test/controllers/harness/mocks.go (1)
383-388: Snapshot the release target before storing it.
desiredrelease/reconcile.gopasses the sharedr.rtpointer into this mock. Appending that pointer directly means later mutations can change what this recorded call appears to contain.Suggested change
func (s *DesiredReleaseSetter) EnqueueJobEligibility(ctx context.Context, workspaceID string, rt *desiredrelease.ReleaseTarget) error { s.mu.Lock() defer s.mu.Unlock() - s.eligibilityCalls = append(s.eligibilityCalls, eligibilityCall{WorkspaceID: workspaceID, RT: rt}) + rtCopy := *rt + s.eligibilityCalls = append(s.eligibilityCalls, eligibilityCall{ + WorkspaceID: workspaceID, + RT: &rtCopy, + }) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/workspace-engine/test/controllers/harness/mocks.go` around lines 383 - 388, The mock EnqueueJobEligibility in DesiredReleaseSetter is appending the shared pointer rt directly (in EnqueueJobEligibility), which lets later mutations change the recorded call; fix it by storing a deep copy/snapshot of the release target instead of the original pointer before appending to s.eligibilityCalls (e.g., create a local copy or use an available Clone/DeepCopy helper on desiredrelease.ReleaseTarget, handle nil rt safely, and append the copied value or pointer to eligibilityCall so the recorded RT is immutable).
🤖 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/svc/controllers/jobeligibility/reconcile_test.go`:
- Around line 669-706: The tests
TestReconcile_ExplicitRetryOnStatuses_OnlyCountsThose,
TestReconcile_DefaultRetryOnStatuses_MaxRetriesGT0 (and similarly
TestReconcile_DifferentReleaseBreaksRetryChain) are too permissive and won't
fail if the chain-break logic is wrong; update each test to make the retry
budget tighter so a chain-breaking entry changes the allow/deny outcome — e.g.,
reduce MaxRetries (in the testPolicy calls that set RetryRule.MaxRetries) or add
one more counted failure job in getter.jobs so that when a
cancelled/successful/different-release entry is incorrectly counted the
Reconcile result flips; ensure you adjust the setup in these tests (references:
Reconcile, testPolicy, TestReconcile_ExplicitRetryOnStatuses_OnlyCountsThose,
TestReconcile_DefaultRetryOnStatuses_MaxRetriesGT0,
TestReconcile_DifferentReleaseBreaksRetryChain) so the assertion on
setter.createdJobs and result.NextReconcileAt will fail if chain-breaking logic
regresses.
- Around line 1128-1142: The test's current setup has two failure jobs so order
doesn't affect behavior; update TestReconcile_JobsSortedByCreatedAt to make the
jobs' statuses different so ordering matters — e.g., use testJobForRelease to
create old as oapi.JobStatusSuccess and newer as oapi.JobStatusFailure (keep the
created times and keep getter.jobs = []*oapi.Job{old, newer}), so the reconciler
must sort newest-first to pick the retryable failure (observe Reconcile
producing one created job via setter.createdJobs).
- Around line 1102-1110: TestReconcile_CreateJobFails_Error currently only
asserts an error from Reconcile; you must also assert that no enqueue/dispatch
was attempted after a failed CreateJob. Update the test
(TestReconcile_CreateJobFails_Error) to set up the fake setter via
setupHappyPath (where setter.createJobErr = fmt.Errorf("create failed")) and
then assert the setter's enqueue/dispatch flag or call count (the field used by
your fake in buildAndDispatchJob) remains zero/false after calling Reconcile,
ensuring buildAndDispatchJob did not enqueue when CreateJob failed.
---
Outside diff comments:
In `@apps/workspace-engine/test/controllers/harness/mocks.go`:
- Around line 341-347: The mock DesiredReleaseSetter.SetDesiredRelease currently
ignores nil releases by returning nil; change it to fail fast: when r == nil
return a non-nil error (e.g., errors.New or fmt.Errorf) so tests surface bogus
reconcile flows instead of silently accepting nil. Update the SetDesiredRelease
implementation in DesiredReleaseSetter to return an error on nil r (keeping
mu.Lock(), defer, and CallCount behavior) and adjust any test expectations that
relied on the permissive behavior.
---
Duplicate comments:
In `@apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.go`:
- Around line 492-493: The test currently seeds getter.jobs with activeJob but
should exercise the concurrency gate by populating getter.processingJobs
instead; update the test (where setupHappyPath(rt, release) returns getter,
setter) to set getter.processingJobs = []*oapi.Job{activeJob} and leave
getter.jobs empty so the code path that calls
GetJobsInProcessingStateForReleaseTarget is exercised; ensure you reference the
same getter from setupHappyPath and keep the release variable as-is.
---
Nitpick comments:
In `@apps/workspace-engine/test/controllers/harness/mocks.go`:
- Around line 383-388: The mock EnqueueJobEligibility in DesiredReleaseSetter is
appending the shared pointer rt directly (in EnqueueJobEligibility), which lets
later mutations change the recorded call; fix it by storing a deep copy/snapshot
of the release target instead of the original pointer before appending to
s.eligibilityCalls (e.g., create a local copy or use an available Clone/DeepCopy
helper on desiredrelease.ReleaseTarget, handle nil rt safely, and append the
copied value or pointer to eligibilityCall so the recorded RT is immutable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cbffb61a-58bc-46a3-973e-3812fbc8ace0
📒 Files selected for processing (2)
apps/workspace-engine/svc/controllers/jobeligibility/reconcile_test.goapps/workspace-engine/test/controllers/harness/mocks.go
This was referenced Mar 19, 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
2 participants
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.
Summary by CodeRabbit
New Features
Improvements
Tests
Note
Medium Risk
Adds a new reconcile worker that can drive job creation/dispatch and changes shared evaluator interfaces, so wiring/behavior issues could affect deployment automation once the Postgres stubs are implemented.
Overview
Introduces a new
job-eligibilityreconcile controller/service, wired intomain.go, plusevents.EnqueueJobEligibilityhelpers to enqueue work scoped by release target.The controller runs eligibility checks (release-target concurrency + policy-driven retry/backoff) and, when allowed, builds jobs with full dispatch context and enqueues
job-dispatch; Postgres getter/setter plumbing is added but key DB operations (GetDesiredRelease, job lookup,CreateJob) are currently stubbed.Policy evaluator plumbing is updated to be context-aware (
GetJobsForReleaseTarget(ctx, ...)) andreleasetargetconcurrencyis refactored to accept aGettersinterface, with downstream mocks/tests updated accordingly; extensive new unit tests cover eligibility decisions, retry/backoff, concurrency blocking, and dispatch enqueueing.Written by Cursor Bugbot for commit 35b99cc. This will update automatically on new commits. Configure here.