-
Notifications
You must be signed in to change notification settings - Fork 14
chore: init workflow manager #759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughRename Job.taskId → Job.workflowStepId; add WorkflowJobAgentConfig and TestRunnerJobAgentConfig; replace workflowStepTemplateId with index and jobAgent refs; add workflow manager and view, job-agent registry and types, multiple dispatchable agent implementations (full TestRunner, stubs for Argo/GitHub/TFE), and store helper filters. Changes
Sequence Diagram(s)sequenceDiagram
participant WFM as WorkflowManager
participant WFV as WorkflowView
participant Store as Store
participant Registry as JobAgentRegistry
participant Disp as Dispatcher
WFM->>WFV: NewWorkflowView(store, workflowId)
WFV->>Store: GetWorkflow / GetSteps / GetJobsByStep
Store-->>WFV: workflow, steps, jobs
WFM->>WFV: IsComplete() / HasPendingJobs()
WFV-->>WFM: statuses
WFM->>WFV: GetNextStep()
WFV-->>WFM: next step
WFM->>Store: Get jobs for step
Store-->>WFM: job list
WFM->>Registry: Dispatch(job)
Registry->>Store: GetJobAgent(job.JobAgentId)
Store-->>Registry: jobAgent
Registry->>Store: Enrich (release/deployment/workflow/step)
Store-->>Registry: enriched data
Registry->>Registry: build RenderContext & validate Capabilities
Registry->>Disp: Dispatch(ctx, RenderContext)
Disp-->>Registry: result
Registry-->>WFM: dispatch result
sequenceDiagram
participant TR as TestRunner
participant Store as Store
participant Kafka as Kafka
participant Trace as OpenTelemetry
TR->>Trace: start span
TR->>TR: parse config → delay, final status
TR->>TR: spawn goroutine resolveJobAfterDelay (non-blocking)
par Async resolution
TR->>TR: sleep delay
TR->>Store: Get job current state
Store-->>TR: job
TR->>Kafka: publish JobUpdateEvent
Kafka-->>TR: ack
TR->>Trace: record outcome
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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/workflow_manager.go`:
- Around line 19-38: The four stub methods createWorkflow, getNextStepTemplate,
createWorkflowStep, and dispatchJobForStep currently return nil values causing
silent no-ops; replace those nil returns with explicit "not implemented" errors
(e.g., errors.New("createWorkflow: not implemented") or fmt.Errorf) in each
function so callers receive a clear error until real logic is implemented—update
each function (createWorkflow, getNextStepTemplate, createWorkflowStep,
dispatchJobForStep) to return the appropriate zero-value result plus the
not-implemented error.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/workflowmanager/workflow_manager.go (1)
9-16: Add Go doc comments for exported identifiers.
WorkflowManagerandNewWorkflowManagerare exported but undocumented; please add Go doc comments to satisfy lint and the project guidelines.
As per coding guidelines, exported functions/types should be documented.♻️ Suggested patch
type WorkflowManager struct { store *store.Store } +// NewWorkflowManager constructs a WorkflowManager backed by the given store. func NewWorkflowManager(store *store.Store) *WorkflowManager { return &WorkflowManager{ store: store, } }
| // createWorkflow creates and stores a new workflow with inputs | ||
| func (m *WorkflowManager) createWorkflow(ctx context.Context, workflowTemplate *oapi.WorkflowTemplate, inputs map[string]interface{}) (*oapi.Workflow, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| // getNextStepTemplate gets the next step template to execute based on the current workflow step | ||
| // this function will take into account current step status and dependencies | ||
| func (m *WorkflowManager) getNextStepTemplate(ctx context.Context, workflow *oapi.Workflow) (*oapi.WorkflowStepTemplate, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| // createWorkflowStep creates and stores a new workflow step | ||
| // it will template the config of the step template with the inputs of the workflow | ||
| func (m *WorkflowManager) createWorkflowStep(ctx context.Context, workflow *oapi.Workflow, stepTemplate *oapi.WorkflowStepTemplate) (*oapi.WorkflowStep, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| // dispatchJobForStep dispatches a job for the given step | ||
| func (m *WorkflowManager) dispatchJobForStep(ctx context.Context, workflow *oapi.Workflow, step *oapi.WorkflowStep) error { | ||
| return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stub methods currently return “success,” causing silent no-ops.
With all internal steps returning nil, CreateWorkflow/ContinueWorkflow will report success even though nothing happened. Return explicit “not implemented” errors until real logic is wired in.
🐛 Suggested patch
import (
"context"
+ "errors"
"workspace-engine/pkg/oapi"
"workspace-engine/pkg/workspace/store"
)
+var errWorkflowManagerNotImplemented = errors.New("workflow manager: not implemented")
+
// createWorkflow creates and stores a new workflow with inputs
func (m *WorkflowManager) createWorkflow(ctx context.Context, workflowTemplate *oapi.WorkflowTemplate, inputs map[string]interface{}) (*oapi.Workflow, error) {
- return nil, nil
+ return nil, errWorkflowManagerNotImplemented
}
// getNextStepTemplate gets the next step template to execute based on the current workflow step
// this function will take into account current step status and dependencies
func (m *WorkflowManager) getNextStepTemplate(ctx context.Context, workflow *oapi.Workflow) (*oapi.WorkflowStepTemplate, error) {
- return nil, nil
+ return nil, errWorkflowManagerNotImplemented
}
// createWorkflowStep creates and stores a new workflow step
// it will template the config of the step template with the inputs of the workflow
func (m *WorkflowManager) createWorkflowStep(ctx context.Context, workflow *oapi.Workflow, stepTemplate *oapi.WorkflowStepTemplate) (*oapi.WorkflowStep, error) {
- return nil, nil
+ return nil, errWorkflowManagerNotImplemented
}
// dispatchJobForStep dispatches a job for the given step
func (m *WorkflowManager) dispatchJobForStep(ctx context.Context, workflow *oapi.Workflow, step *oapi.WorkflowStep) error {
- return nil
+ return errWorkflowManagerNotImplemented
}🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/workflow_manager.go`
around lines 19 - 38, The four stub methods createWorkflow, getNextStepTemplate,
createWorkflowStep, and dispatchJobForStep currently return nil values causing
silent no-ops; replace those nil returns with explicit "not implemented" errors
(e.g., errors.New("createWorkflow: not implemented") or fmt.Errorf) in each
function so callers receive a clear error until real logic is implemented—update
each function (createWorkflow, getNextStepTemplate, createWorkflowStep,
dispatchJobForStep) to return the appropriate zero-value result plus the
not-implemented error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/workflow_manager.go`:
- Around line 131-143: ContinueWorkflow currently assumes getNextStepTemplate
always returns a non-nil template and will panic if nextStepTemplate is nil; add
a nil guard after calling getNextStepTemplate and handle the no-next-step case
(e.g., return nil or a specific sentinel error) instead of proceeding to
createWorkflowStep and dispatchJobForStep; update ContinueWorkflow to check if
nextStepTemplate == nil and return early, referencing the ContinueWorkflow,
getNextStepTemplate, nextStepTemplate, createWorkflowStep, and
dispatchJobForStep symbols.
- Around line 23-25: The isJobCompleted function currently treats jobs with
actionRequired set as completed; update it to treat actionRequired as incomplete
by adding an explicit check for the ActionRequired field. Change the predicate
in isJobCompleted (function isJobCompleted(job *oapi.Job)) to also require that
job.ActionRequired is false (e.g., return job.Status != oapi.JobStatusPending &&
job.Status != oapi.JobStatusInProgress && !job.ActionRequired) so workflows do
not advance while manual action is required.
- Around line 42-66: getNextStepTemplate currently calls
WorkflowSteps.Get(stepTemplate.Id) which assumes template IDs equal step IDs and
isn't scoped to the workflow, causing missed matches or cross-workflow matches
and creating duplicate steps when an existing step has no jobs; update this
logic to look up the WorkflowStep by both workflow and template (e.g., use or
add a store method like WorkflowSteps.GetByWorkflowAndTemplate(workflow.Id,
stepTemplate.Id) or filter existing steps to match step.WorkflowId ==
workflow.Id && step.TemplateId == stepTemplate.Id), then if that step exists
reuse it (inspect its jobs via Jobs.GetByWorkflowStepId(step.Id)) and only
return the template for creation when no existing step exists or all jobs are
completed; keep references to getNextStepTemplate,
WorkflowSteps.Get/GetByWorkflowAndTemplate, WorkflowTemplates.Get,
Jobs.GetByWorkflowStepId, and isJobCompleted.
| func isJobCompleted(job *oapi.Job) bool { | ||
| return job.Status != oapi.JobStatusPending && job.Status != oapi.JobStatusInProgress | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Treat actionRequired as incomplete to avoid premature progression.
Right now actionRequired is considered “completed,” which likely advances workflows before manual action is resolved.
🔧 Suggested fix
func isJobCompleted(job *oapi.Job) bool {
- return job.Status != oapi.JobStatusPending && job.Status != oapi.JobStatusInProgress
+ switch job.Status {
+ case oapi.JobStatusPending, oapi.JobStatusInProgress, oapi.JobStatusActionRequired:
+ return false
+ default:
+ return true
+ }
}📝 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.
| func isJobCompleted(job *oapi.Job) bool { | |
| return job.Status != oapi.JobStatusPending && job.Status != oapi.JobStatusInProgress | |
| } | |
| func isJobCompleted(job *oapi.Job) bool { | |
| switch job.Status { | |
| case oapi.JobStatusPending, oapi.JobStatusInProgress, oapi.JobStatusActionRequired: | |
| return false | |
| default: | |
| return true | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/workflow_manager.go`
around lines 23 - 25, The isJobCompleted function currently treats jobs with
actionRequired set as completed; update it to treat actionRequired as incomplete
by adding an explicit check for the ActionRequired field. Change the predicate
in isJobCompleted (function isJobCompleted(job *oapi.Job)) to also require that
job.ActionRequired is false (e.g., return job.Status != oapi.JobStatusPending &&
job.Status != oapi.JobStatusInProgress && !job.ActionRequired) so workflows do
not advance while manual action is required.
| func (m *WorkflowManager) getNextStepTemplate(workflow *oapi.Workflow) (*oapi.WorkflowStepTemplate, error) { | ||
| workflowTemplate, ok := m.store.WorkflowTemplates.Get(workflow.WorkflowTemplateId) | ||
| if !ok { | ||
| return nil, fmt.Errorf("workflow template %s not found", workflow.WorkflowTemplateId) | ||
| } | ||
|
|
||
| for _, stepTemplate := range workflowTemplate.Steps { | ||
| step, ok := m.store.WorkflowSteps.Get(stepTemplate.Id) | ||
| if !ok { | ||
| return &stepTemplate, nil | ||
| } | ||
|
|
||
| jobs := m.store.Jobs.GetByWorkflowStepId(step.Id) | ||
| if len(jobs) == 0 { | ||
| return &stepTemplate, nil | ||
| } | ||
|
|
||
| // if any job is not completed, return nil as this step is still in progress | ||
| // the hooks will retrigger this engine once those jobs complete | ||
| for _, job := range jobs { | ||
| if !isJobCompleted(job) { | ||
| return nil, nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Workflow step lookup is likely incorrect and can duplicate steps.
WorkflowSteps.Get(stepTemplate.Id) assumes step IDs equal template IDs and doesn’t scope by workflow, which can (a) never find existing steps or (b) accidentally match another workflow. Also, if a step exists but has no jobs, this path will create a duplicate step instead of reusing/dispatching the existing one.
🔧 Directional fix (adapt to store APIs)
- for _, stepTemplate := range workflowTemplate.Steps {
- step, ok := m.store.WorkflowSteps.Get(stepTemplate.Id)
+ steps := m.store.WorkflowSteps.GetByWorkflowId(workflow.Id)
+ stepByTemplate := map[string]*oapi.WorkflowStep{}
+ for _, s := range steps {
+ stepByTemplate[s.WorkflowStepTemplateId] = s
+ }
+
+ for _, stepTemplate := range workflowTemplate.Steps {
+ step, ok := stepByTemplate[stepTemplate.Id]
if !ok {
return &stepTemplate, nil
}
jobs := m.store.Jobs.GetByWorkflowStepId(step.Id)
if len(jobs) == 0 {
- return &stepTemplate, nil
+ // consider reusing existing step rather than creating a new one
+ return &stepTemplate, nil
}🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/workflow_manager.go`
around lines 42 - 66, getNextStepTemplate currently calls
WorkflowSteps.Get(stepTemplate.Id) which assumes template IDs equal step IDs and
isn't scoped to the workflow, causing missed matches or cross-workflow matches
and creating duplicate steps when an existing step has no jobs; update this
logic to look up the WorkflowStep by both workflow and template (e.g., use or
add a store method like WorkflowSteps.GetByWorkflowAndTemplate(workflow.Id,
stepTemplate.Id) or filter existing steps to match step.WorkflowId ==
workflow.Id && step.TemplateId == stepTemplate.Id), then if that step exists
reuse it (inspect its jobs via Jobs.GetByWorkflowStepId(step.Id)) and only
return the template for creation when no existing step exists or all jobs are
completed; keep references to getNextStepTemplate,
WorkflowSteps.Get/GetByWorkflowAndTemplate, WorkflowTemplates.Get,
Jobs.GetByWorkflowStepId, and isJobCompleted.
| // ContinueWorkflow continues a workflow from the given step | ||
| func (m *WorkflowManager) ContinueWorkflow(ctx context.Context, workflow *oapi.Workflow) error { | ||
| nextStepTemplate, err := m.getNextStepTemplate(workflow) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| step, err := m.createWorkflowStep(ctx, workflow, nextStepTemplate) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return m.dispatchJobForStep(ctx, workflow, step) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard against nil nextStepTemplate in ContinueWorkflow.
If there are no remaining steps or a step is in progress, getNextStepTemplate can return nil and this will panic.
🔧 Suggested fix
func (m *WorkflowManager) ContinueWorkflow(ctx context.Context, workflow *oapi.Workflow) error {
nextStepTemplate, err := m.getNextStepTemplate(workflow)
if err != nil {
return err
}
+ if nextStepTemplate == nil {
+ return nil
+ }
step, err := m.createWorkflowStep(ctx, workflow, nextStepTemplate)
if err != nil {
return err
}🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/workflow_manager.go`
around lines 131 - 143, ContinueWorkflow currently assumes getNextStepTemplate
always returns a non-nil template and will panic if nextStepTemplate is nil; add
a nil guard after calling getNextStepTemplate and handle the no-next-step case
(e.g., return nil or a specific sentinel error) instead of proceeding to
createWorkflowStep and dispatchJobForStep; update ContinueWorkflow to check if
nextStepTemplate == nil and return early, referencing the ContinueWorkflow,
getNextStepTemplate, nextStepTemplate, createWorkflowStep, and
dispatchJobForStep symbols.
| w.store.changeset.RecordDelete(workflowStep) | ||
| } | ||
|
|
||
| func (w *WorkflowSteps) GetByWorkflowId(workflowId string) []*oapi.WorkflowStep { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func (w *WorkflowSteps) GetByWorkflowId(workflowId string) []*oapi.WorkflowStep { | |
| func (w *WorkflowSteps) GetByWorkflowID(workflowID string) []*oapi.WorkflowStep { |
I think this is the go way
| } | ||
| } | ||
|
|
||
| func isJobCompleted(job *oapi.Job) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should check for specific state, and not checking what it isn't
| type WorkflowManager struct { | ||
| store *store.Store | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| type WorkflowManager struct { | |
| store *store.Store | |
| } | |
| type Manager struct { | |
| store *store.Store | |
| } |
lets follow the other package patterns
- Introduced `WorkflowJobAgentConfig` schema to encapsulate job agent configuration. - Updated `WorkflowStepTemplate` and `WorkflowStep` to reference the new job agent schema. - Removed the deprecated `workflowmanager` package to streamline workflow management. - Enhanced the `oapi` generation to reflect the new schema structure. - Minor adjustments to comments for clarity in the `Manager` methods.
- Removed unused imports and commented-out code in `funcs.go` and `funcs_test.go`. - Deleted the `TestRenderMap` function and its associated test cases to streamline testing. - Simplified the `RenderMap` function by removing unnecessary comments and improving readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/manager.go`:
- Around line 53-61: dispatchStep currently returns a hardcoded "not
implemented" error; replace it with real dispatch logic: construct an oapi.Job
(use uuid.New().String() for Id, set WorkflowStepId to step.Id, JobAgentId to
step.JobAgent.Id and JobAgentConfig to step.JobAgent.Config), persist/enqueue it
via the manager's job persistence/queue API (e.g., call the existing job
store/creator method on the Manager such as m.jobStore.Create, m.jobs.CreateJob,
or m.enqueueJob — use whichever method exists on Manager), handle and return any
persistence/enqueue error, and return nil on success; if dispatching isn't ready
yet, instead gate by logging the attempt and return nil (or a specific non-fatal
sentinel) so ReconcileWorkflow doesn't block.
In `@apps/workspace-engine/pkg/workspace/workflowmanager/workflow_view.go`:
- Around line 28-37: The map stepJobs is built but not assigned to the returned
WorkflowView, leaving w.stepJobs nil and causing IsComplete/HasPendingJobs logic
to be incorrect; update the WorkflowView construction in New/wherever this
function returns to include the computed stepJobs (assign stepJobs to the
WorkflowView field named stepJobs) so the view stores the jobs map used by
IsComplete and HasPendingJobs.
- Around line 49-57: The isStepComplete function currently checks
len(w.stepJobs[stepId]) inside the loop and therefore treats empty job slices as
complete and ignores the ActionRequired state; move the empty-slice check to the
top of isStepComplete (return false if len(w.stepJobs[stepId]) == 0) before
iterating, and in the loop treat oapi.JobStatusActionRequired as not complete
alongside oapi.JobStatusPending and oapi.JobStatusInProgress so that any
ActionRequired job causes isStepComplete to return false; update references to
w.stepJobs[stepId] and job.Status in the isStepComplete method accordingly.
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/workflowmanager/manager.go (1)
14-24: Add GoDoc for exported workflow manager APIs.
Manager,NewWorkflowManager, andCreateWorkfloware exported but lack doc comments. Please add brief GoDoc to clarify intent and satisfy repo conventions.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.
apps/workspace-engine/pkg/workspace/workflowmanager/workflow_view.go (1)
90-91: Guard GetStep against out-of-range indices.Returning
nilfor invalid indices avoids panics in callers.♻️ Optional guard
func (w *WorkflowView) GetStep(index int) *oapi.WorkflowStep { + if index < 0 || index >= len(w.steps) { + return nil + } return w.steps[index] }
| // dispatchJobForStep dispatches a job for the given step | ||
| func (m *Manager) dispatchStep(ctx context.Context, workflow *oapi.Workflow, step *oapi.WorkflowStep) error { | ||
| // job := &oapi.Job{ | ||
| // Id: uuid.New().String(), | ||
| // WorkflowStepId: step.Id, | ||
| // JobAgentId: step.JobAgent.Id, | ||
| // JobAgentConfig: step.JobAgent.Config, | ||
| // } | ||
| return errors.New("not implemented") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dispatchStep always returns “not implemented,” so workflows can’t progress.
ReconcileWorkflow will return this error whenever it needs to dispatch the next step, which blocks execution if this path is active. Please implement dispatching or gate usage until it’s ready.
If you want, I can draft an initial dispatch implementation aligned with your job/agent models.
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/manager.go` around lines
53 - 61, dispatchStep currently returns a hardcoded "not implemented" error;
replace it with real dispatch logic: construct an oapi.Job (use
uuid.New().String() for Id, set WorkflowStepId to step.Id, JobAgentId to
step.JobAgent.Id and JobAgentConfig to step.JobAgent.Config), persist/enqueue it
via the manager's job persistence/queue API (e.g., call the existing job
store/creator method on the Manager such as m.jobStore.Create, m.jobs.CreateJob,
or m.enqueueJob — use whichever method exists on Manager), handle and return any
persistence/enqueue error, and return nil on success; if dispatching isn't ready
yet, instead gate by logging the attempt and return nil (or a specific non-fatal
sentinel) so ReconcileWorkflow doesn't block.
| stepJobs := make(map[string][]*oapi.Job) | ||
| for _, step := range steps { | ||
| stepJobs[step.Id] = store.Jobs.GetByWorkflowStepId(step.Id) | ||
| } | ||
|
|
||
| return &WorkflowView{ | ||
| store: store, | ||
| workflow: workflow, | ||
| steps: steps, | ||
| }, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stepJobs is computed but never stored on WorkflowView.
This leaves w.stepJobs nil, so IsComplete/HasPendingJobs treat workflows as complete and never dispatch. Persist the map.
🔧 Suggested fix
return &WorkflowView{
store: store,
workflow: workflow,
steps: steps,
+ stepJobs: stepJobs,
}, nil📝 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.
| stepJobs := make(map[string][]*oapi.Job) | |
| for _, step := range steps { | |
| stepJobs[step.Id] = store.Jobs.GetByWorkflowStepId(step.Id) | |
| } | |
| return &WorkflowView{ | |
| store: store, | |
| workflow: workflow, | |
| steps: steps, | |
| }, nil | |
| stepJobs := make(map[string][]*oapi.Job) | |
| for _, step := range steps { | |
| stepJobs[step.Id] = store.Jobs.GetByWorkflowStepId(step.Id) | |
| } | |
| return &WorkflowView{ | |
| store: store, | |
| workflow: workflow, | |
| steps: steps, | |
| stepJobs: stepJobs, | |
| }, nil |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/workflow_view.go` around
lines 28 - 37, The map stepJobs is built but not assigned to the returned
WorkflowView, leaving w.stepJobs nil and causing IsComplete/HasPendingJobs logic
to be incorrect; update the WorkflowView construction in New/wherever this
function returns to include the computed stepJobs (assign stepJobs to the
WorkflowView field named stepJobs) so the view stores the jobs map used by
IsComplete and HasPendingJobs.
| func (w *WorkflowView) isStepComplete(stepId string) bool { | ||
| for _, job := range w.stepJobs[stepId] { | ||
| if len(w.stepJobs[stepId]) == 0 { | ||
| return false | ||
| } | ||
| if job.Status == oapi.JobStatusPending || job.Status == oapi.JobStatusInProgress { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isStepComplete marks steps complete when there are zero jobs.
The length check is inside the loop, so empty slices return true; actionRequired is also treated as complete. Move the len check before the loop and include actionRequired.
🔧 Suggested fix
func (w *WorkflowView) isStepComplete(stepId string) bool {
- for _, job := range w.stepJobs[stepId] {
- if len(w.stepJobs[stepId]) == 0 {
- return false
- }
- if job.Status == oapi.JobStatusPending || job.Status == oapi.JobStatusInProgress {
- return false
- }
- }
+ jobs := w.stepJobs[stepId]
+ if len(jobs) == 0 {
+ return false
+ }
+ for _, job := range jobs {
+ if job.Status == oapi.JobStatusPending ||
+ job.Status == oapi.JobStatusInProgress ||
+ job.Status == oapi.JobStatusActionRequired {
+ return false
+ }
+ }
return true
}📝 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.
| func (w *WorkflowView) isStepComplete(stepId string) bool { | |
| for _, job := range w.stepJobs[stepId] { | |
| if len(w.stepJobs[stepId]) == 0 { | |
| return false | |
| } | |
| if job.Status == oapi.JobStatusPending || job.Status == oapi.JobStatusInProgress { | |
| return false | |
| } | |
| } | |
| func (w *WorkflowView) isStepComplete(stepId string) bool { | |
| jobs := w.stepJobs[stepId] | |
| if len(jobs) == 0 { | |
| return false | |
| } | |
| for _, job := range jobs { | |
| if job.Status == oapi.JobStatusPending || | |
| job.Status == oapi.JobStatusInProgress || | |
| job.Status == oapi.JobStatusActionRequired { | |
| return false | |
| } | |
| } | |
| return true | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/workflow_view.go` around
lines 49 - 57, The isStepComplete function currently checks
len(w.stepJobs[stepId]) inside the loop and therefore treats empty job slices as
complete and ignores the ActionRequired state; move the empty-slice check to the
top of isStepComplete (return false if len(w.stepJobs[stepId]) == 0) before
iterating, and in the loop treat oapi.JobStatusActionRequired as not complete
alongside oapi.JobStatusPending and oapi.JobStatusInProgress so that any
ActionRequired job causes isStepComplete to return false; update references to
w.stepJobs[stepId] and job.Status in the isStepComplete method accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/jobagents/registry.go`:
- Around line 39-44: The call to r.store.Jobs.GetWithRelease currently discards
its error; change it to capture the returned error (e.g., jobWithRelease, err :=
r.store.Jobs.GetWithRelease(job.Id)) and handle it rather than ignoring it: if
the release data is optional, log err at debug level (using the component's
logger) and proceed with the nil-check for jobWithRelease before setting
renderContext.Release/Deployment/Environment/Resource; if the release context is
required, propagate or return the error instead of continuing. Ensure you
reference r.store.Jobs.GetWithRelease, jobWithRelease, err and renderContext in
the fix so callers or logs reveal underlying store/connectivity issues.
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/jobagents/types.go (1)
8-11: Parameter namecontextshadows the imported package.The second parameter in
Dispatchis namedcontext, which shadows thecontextpackage imported on line 4. Consider renaming it torenderCtxorrcfor clarity.Suggested fix
type Dispatchable interface { Type() string - Dispatch(ctx context.Context, context RenderContext) error + Dispatch(ctx context.Context, renderCtx RenderContext) error }apps/workspace-engine/pkg/workspace/jobagents/registry.go (1)
10-22: Registry implementation is incomplete and unused.The
Registryinitializes an emptydispatchersmap with no mechanism to populate it. TheDispatchmethod will always fail with "job agent type not found" because no dispatchers are ever registered. Additionally,NewRegistryis never called anywhere in the codebase, and the Registry type appears unused throughout the application.Either complete the implementation by adding a registration mechanism (e.g.,
Registermethod), or remove this unused code.
| if jobWithRelease, _ := r.store.Jobs.GetWithRelease(job.Id); jobWithRelease != nil { | ||
| renderContext.Release = &jobWithRelease.Release | ||
| renderContext.Deployment = jobWithRelease.Deployment | ||
| renderContext.Environment = jobWithRelease.Environment | ||
| renderContext.Resource = jobWithRelease.Resource | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discarded error from GetWithRelease may mask store issues.
The error returned by GetWithRelease is silently discarded. While the nil check provides a safe fallback, this could hide underlying store or connectivity issues. Consider logging the error at debug level if the release data is truly optional, or propagating it if the release context is required for dispatch.
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobagents/registry.go` around lines 39 -
44, The call to r.store.Jobs.GetWithRelease currently discards its error; change
it to capture the returned error (e.g., jobWithRelease, err :=
r.store.Jobs.GetWithRelease(job.Id)) and handle it rather than ignoring it: if
the release data is optional, log err at debug level (using the component's
logger) and proceed with the nil-check for jobWithRelease before setting
renderContext.Release/Deployment/Environment/Resource; if the release context is
required, propagate or return the error instead of continuing. Ensure you
reference r.store.Jobs.GetWithRelease, jobWithRelease, err and renderContext in
the fix so callers or logs reveal underlying store/connectivity issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/jobagents/registry.go`:
- Around line 10-22: The Registry currently initializes an empty dispatchers map
in NewRegistry but provides no way to add dispatchers, so Dispatch will always
fail; add an exported method on Registry named Register that accepts a
Dispatchable and stores it into the registry's dispatchers map keyed by
dispatcher.Type() (i.e., r.dispatchers[dispatcher.Type()] = dispatcher) so
callers can register implementations before calling Dispatch.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/jobagents/registry.go (1)
57-62: Guard workflow step lookup withisWorkflowcheck.When
job.WorkflowStepIdis empty, this still callsWorkflowSteps.Get("")which is wasteful. Guard with the existingisWorkflowflag.♻️ Suggested fix
- if workflowStep, ok := r.store.WorkflowSteps.Get(job.WorkflowStepId); ok { - renderContext.WorkflowStep = workflowStep - if workflow, ok := r.store.Workflows.Get(workflowStep.WorkflowId); ok { - renderContext.Workflow = workflow + if isWorkflow { + if workflowStep, ok := r.store.WorkflowSteps.Get(job.WorkflowStepId); ok { + renderContext.WorkflowStep = workflowStep + if workflow, ok := r.store.Workflows.Get(workflowStep.WorkflowId); ok { + renderContext.Workflow = workflow + } } }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go`:
- Around line 30-31: Replace the panic in ArgoApplication.Dispatch with a
returned error so the workspace engine doesn't crash when the Argo agent is
invoked; in the method ArgoApplication.Dispatch(ctx context.Context, context
types.RenderContext) return a descriptive, typed error (e.g., errors.New or
fmt.Errorf) such as "ArgoApplication.Dispatch not implemented" or a
package-level sentinel error, ensuring callers can handle it without process
termination.
In `@apps/workspace-engine/pkg/workspace/jobagents/github/githubaction.go`:
- Around line 30-32: The Dispatch method on GithubAction currently panics;
replace the panic with returning a descriptive error instead (e.g., using
fmt.Errorf or errors.New) so callers receive a clear failure rather than
crashing; update the GithubAction.Dispatch(ctx context.Context, context
types.RenderContext) implementation to return an error such as "github action
dispatch not implemented" and ensure the method signature and imports reflect
the chosen error constructor.
In `@apps/workspace-engine/pkg/workspace/jobagents/registry.go`:
- Around line 72-77: The current block populates renderContext from r.store
using job.WorkflowStepId but silently skips or continues when the WorkflowStep
or its Workflow is missing; change this to fail fast by returning an error when
job.WorkflowStepId is non-empty and
r.store.WorkflowSteps.Get(job.WorkflowStepId) does not return ok, and likewise
return an error if the Workflow referenced by workflowStep.WorkflowId is not
found; update the surrounding function (the method that contains this code) to
propagate that error instead of silently proceeding, referencing
r.store.WorkflowSteps.Get, r.store.Workflows.Get, renderContext, and
job.WorkflowStepId to locate the checks to modify.
In `@apps/workspace-engine/pkg/workspace/jobagents/terraformcloud/tfe.go`:
- Around line 30-31: The TFE.Dispatch method currently panics which can crash
the engine; replace the panic("unimplemented") with a proper error return (e.g.,
return fmt.Errorf("TFE.Dispatch: unimplemented") or errors.New("TFE.Dispatch:
unimplemented")) so callers receive an error instead of crashing, and add the
corresponding import (fmt or errors) if missing; keep the function signature as
func (t *TFE) Dispatch(ctx context.Context, context types.RenderContext) error
and ensure any linter unused-variable issues are addressed if needed.
🧹 Nitpick comments (7)
apps/workspace-engine/pkg/workspace/jobagents/types/types.go (1)
8-30: Add doc comments for exported job-agent types.
Dispatchable,Capabilities, andRenderContextare exported but undocumented. Please add package-level doc comments that describe their purpose/usage (not just restating fields) to keep godoc/lint clean.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.
apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go (1)
11-29: Add doc comments for exported identifiers.
ArgoApplication,NewArgoApplication, and the exported methods should have doc comments describing their role in the dispatch registry.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.
apps/workspace-engine/pkg/workspace/jobagents/github/githubaction.go (1)
11-31: Replace the redundant inline comment with proper doc comments.
// Dispatch implements types.Dispatchable.just restates the signature. Prefer doc comments onGithubAction,NewGithubAction, andDispatchthat explain intent/usage.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.
apps/workspace-engine/pkg/workspace/jobagents/testrunner/testrunner.go (2)
27-101: Add doc comments for exported identifiers.
TestRunner,New,Type,Supports, andDispatchare exported without package docs. Please add brief purpose/usage comments.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.
154-180: Pass context fromresolveJobAfterDelayintosendJobUpdateEventto maintain trace linkage.Starting spans with
context.Background()breaks the trace chain. Pass the parent context so child spans are properly connected to the parent operation.♻️ Refactor
- if err := t.sendJobUpdateEvent(job, status, finalMessage); err != nil { + if err := t.sendJobUpdateEvent(ctx, job, status, finalMessage); err != nil { span.RecordError(err) span.SetStatus(codes.Error, "failed to send job update event") return } @@ -func (t *TestRunner) sendJobUpdateEvent(job *oapi.Job, status oapi.JobStatus, message string) error { - _, span := tracer.Start(context.Background(), "TestRunner.sendJobUpdateEvent") +func (t *TestRunner) sendJobUpdateEvent(ctx context.Context, job *oapi.Job, status oapi.JobStatus, message string) error { + _, span := tracer.Start(ctx, "TestRunner.sendJobUpdateEvent") defer span.End()apps/workspace-engine/pkg/workspace/jobagents/terraformcloud/tfe.go (1)
11-29: Add doc comments for exported identifiers.
TFE,NewTFE, and the exported methods should be documented with purpose/usage notes.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.
apps/workspace-engine/pkg/workspace/jobagents/registry.go (1)
16-39: Add meaningful doc comments for exported API.
Registry,NewRegistry, andDispatchare exported but undocumented; theRegistercomment just restates the name. Please add concise purpose/usage notes.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.
| func (a *ArgoApplication) Dispatch(ctx context.Context, context types.RenderContext) error { | ||
| panic("unimplemented") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid panic in Dispatch; return a typed error instead.
This dispatcher is registered by default; a panic will crash the workspace engine if the Argo agent is invoked. Return a descriptive error until implemented.
🐛 Proposed fix
import (
"context"
+ "errors"
"workspace-engine/pkg/workspace/jobagents/types"
"workspace-engine/pkg/workspace/store"
)
@@
func (a *ArgoApplication) Dispatch(ctx context.Context, context types.RenderContext) error {
- panic("unimplemented")
+ return errors.New("argo-application dispatch not implemented")
}📝 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.
| func (a *ArgoApplication) Dispatch(ctx context.Context, context types.RenderContext) error { | |
| panic("unimplemented") | |
| func (a *ArgoApplication) Dispatch(ctx context.Context, context types.RenderContext) error { | |
| return errors.New("argo-application dispatch not implemented") | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobagents/argo/argoapp.go` around lines
30 - 31, Replace the panic in ArgoApplication.Dispatch with a returned error so
the workspace engine doesn't crash when the Argo agent is invoked; in the method
ArgoApplication.Dispatch(ctx context.Context, context types.RenderContext)
return a descriptive, typed error (e.g., errors.New or fmt.Errorf) such as
"ArgoApplication.Dispatch not implemented" or a package-level sentinel error,
ensuring callers can handle it without process termination.
| // Dispatch implements types.Dispatchable. | ||
| func (a *GithubAction) Dispatch(ctx context.Context, context types.RenderContext) error { | ||
| panic("unimplemented") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid panic in Dispatch; return an error until implemented.
A panic here will crash the process whenever the GitHub agent is selected. Prefer a descriptive error.
🐛 Proposed fix
import (
"context"
+ "errors"
"workspace-engine/pkg/workspace/jobagents/types"
"workspace-engine/pkg/workspace/store"
)
@@
func (a *GithubAction) Dispatch(ctx context.Context, context types.RenderContext) error {
- panic("unimplemented")
+ return errors.New("github-action dispatch not implemented")
}📝 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.
| // Dispatch implements types.Dispatchable. | |
| func (a *GithubAction) Dispatch(ctx context.Context, context types.RenderContext) error { | |
| panic("unimplemented") | |
| // Dispatch implements types.Dispatchable. | |
| func (a *GithubAction) Dispatch(ctx context.Context, context types.RenderContext) error { | |
| return errors.New("github-action dispatch not implemented") | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobagents/github/githubaction.go` around
lines 30 - 32, The Dispatch method on GithubAction currently panics; replace the
panic with returning a descriptive error instead (e.g., using fmt.Errorf or
errors.New) so callers receive a clear failure rather than crashing; update the
GithubAction.Dispatch(ctx context.Context, context types.RenderContext)
implementation to return an error such as "github action dispatch not
implemented" and ensure the method signature and imports reflect the chosen
error constructor.
| if workflowStep, ok := r.store.WorkflowSteps.Get(job.WorkflowStepId); ok { | ||
| renderContext.WorkflowStep = workflowStep | ||
| if workflow, ok := r.store.Workflows.Get(workflowStep.WorkflowId); ok { | ||
| renderContext.Workflow = workflow | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fail fast when a workflow step or workflow is missing.
If job.WorkflowStepId is set, dispatching without a valid step/workflow will likely break downstream renderers or silently lose context. Return an error when those records are missing.
🐛 Proposed fix
- if workflowStep, ok := r.store.WorkflowSteps.Get(job.WorkflowStepId); ok {
- renderContext.WorkflowStep = workflowStep
- if workflow, ok := r.store.Workflows.Get(workflowStep.WorkflowId); ok {
- renderContext.Workflow = workflow
- }
- }
+ if isWorkflow {
+ workflowStep, ok := r.store.WorkflowSteps.Get(job.WorkflowStepId)
+ if !ok {
+ return fmt.Errorf("workflow step %s not found", job.WorkflowStepId)
+ }
+ renderContext.WorkflowStep = workflowStep
+ workflow, ok := r.store.Workflows.Get(workflowStep.WorkflowId)
+ if !ok {
+ return fmt.Errorf("workflow %s not found", workflowStep.WorkflowId)
+ }
+ renderContext.Workflow = workflow
+ }🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobagents/registry.go` around lines 72 -
77, The current block populates renderContext from r.store using
job.WorkflowStepId but silently skips or continues when the WorkflowStep or its
Workflow is missing; change this to fail fast by returning an error when
job.WorkflowStepId is non-empty and
r.store.WorkflowSteps.Get(job.WorkflowStepId) does not return ok, and likewise
return an error if the Workflow referenced by workflowStep.WorkflowId is not
found; update the surrounding function (the method that contains this code) to
propagate that error instead of silently proceeding, referencing
r.store.WorkflowSteps.Get, r.store.Workflows.Get, renderContext, and
job.WorkflowStepId to locate the checks to modify.
| func (t *TFE) Dispatch(ctx context.Context, context types.RenderContext) error { | ||
| panic("unimplemented") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid panic in Dispatch; return an error until implemented.
A panic here can crash the engine whenever a TFE job agent is used.
🐛 Proposed fix
import (
"context"
+ "errors"
"workspace-engine/pkg/workspace/jobagents/types"
"workspace-engine/pkg/workspace/store"
)
@@
func (t *TFE) Dispatch(ctx context.Context, context types.RenderContext) error {
- panic("unimplemented")
+ return errors.New("tfe dispatch not implemented")
}📝 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.
| func (t *TFE) Dispatch(ctx context.Context, context types.RenderContext) error { | |
| panic("unimplemented") | |
| func (t *TFE) Dispatch(ctx context.Context, context types.RenderContext) error { | |
| return errors.New("tfe dispatch not implemented") | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobagents/terraformcloud/tfe.go` around
lines 30 - 31, The TFE.Dispatch method currently panics which can crash the
engine; replace the panic("unimplemented") with a proper error return (e.g.,
return fmt.Errorf("TFE.Dispatch: unimplemented") or errors.New("TFE.Dispatch:
unimplemented")) so callers receive an error instead of crashing, and add the
corresponding import (fmt or errors) if missing; keep the function signature as
func (t *TFE) Dispatch(ctx context.Context, context types.RenderContext) error
and ensure any linter unused-variable issues are addressed if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/jobagents/testrunner/testrunner.go`:
- Around line 178-180: sendJobUpdateEvent currently starts a span with
context.Background(), which breaks trace propagation from resolveJobAfterDelay;
change sendJobUpdateEvent to accept a context.Context (e.g., func (t
*TestRunner) sendJobUpdateEvent(ctx context.Context, job *oapi.Job, status
oapi.JobStatus, message string) error) and update all callers (notably
resolveJobAfterDelay) to pass their existing ctx instead of creating a new
background context, then use that ctx when calling tracer.Start to ensure the
span is linked to the parent trace.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/jobagents/testrunner/testrunner.go (1)
110-115: Consider validating the configured status value.The function only recognizes
"failure"and defaults everything else to"successful". If a user misconfigures the status (e.g., typo like"sucess"or an invalid value like"pending"), it will silently resolve as successful without warning.♻️ Suggested improvement
func (t *TestRunner) getFinalStatus(cfg *oapi.TestRunnerJobAgentConfig) oapi.JobStatus { - if cfg.Status != nil && *cfg.Status == string(oapi.JobStatusFailure) { - return oapi.JobStatusFailure + if cfg.Status != nil { + switch *cfg.Status { + case string(oapi.JobStatusFailure): + return oapi.JobStatusFailure + case string(oapi.JobStatusSuccessful): + return oapi.JobStatusSuccessful + } } return oapi.JobStatusSuccessful }
| func (t *TestRunner) sendJobUpdateEvent(job *oapi.Job, status oapi.JobStatus, message string) error { | ||
| _, span := tracer.Start(context.Background(), "TestRunner.sendJobUpdateEvent") | ||
| defer span.End() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trace context lost due to context.Background().
Using context.Background() creates an orphaned span that won't be linked to the parent trace from resolveJobAfterDelay. This breaks distributed tracing continuity.
🔧 Proposed fix
-func (t *TestRunner) sendJobUpdateEvent(job *oapi.Job, status oapi.JobStatus, message string) error {
- _, span := tracer.Start(context.Background(), "TestRunner.sendJobUpdateEvent")
+func (t *TestRunner) sendJobUpdateEvent(ctx context.Context, job *oapi.Job, status oapi.JobStatus, message string) error {
+ _, span := tracer.Start(ctx, "TestRunner.sendJobUpdateEvent")
defer span.End()Update the call site in resolveJobAfterDelay:
- if err := t.sendJobUpdateEvent(job, status, finalMessage); err != nil {
+ if err := t.sendJobUpdateEvent(ctx, job, status, finalMessage); err != nil {🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobagents/testrunner/testrunner.go`
around lines 178 - 180, sendJobUpdateEvent currently starts a span with
context.Background(), which breaks trace propagation from resolveJobAfterDelay;
change sendJobUpdateEvent to accept a context.Context (e.g., func (t
*TestRunner) sendJobUpdateEvent(ctx context.Context, job *oapi.Job, status
oapi.JobStatus, message string) error) and update all callers (notably
resolveJobAfterDelay) to pass their existing ctx instead of creating a new
background context, then use that ctx when calling tracer.Start to ensure the
span is linked to the parent trace.
Summary by CodeRabbit
New Features
API/Schema Updates
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Medium Risk
Touches core orchestration paths (workflow/step sequencing and job dispatch) and changes API models, which can break integrations or alter execution order if edge cases aren’t covered.
Overview
Adds an initial workflow manager layer that can create/reconcile workflows, enumerate ordered workflow steps, detect pending jobs, and compute the next step to run.
Introduces a job-agent registry/dispatch mechanism so jobs can be routed to pluggable agents with enriched execution context (workflow, release, deployment, environment, resource).
Updates public API/schema to align with the new workflow model:
Jobnow referencesworkflowStepId, and workflow steps/templates gain explicit ordering (index) plus optional per-stepjobAgentconfiguration (reflected in generated OpenAPI types).Written by Cursor Bugbot for commit 474d960. This will update automatically on new commits. Configure here.