Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe pull request introduces matrix resolution and job expansion logic to the workflow engine. It adds a Changes
Sequence DiagramsequenceDiagram
participant Client
participant WorkflowManager
participant SelectorResolver
participant MatrixResolver
participant JobDispatcher
participant Store
Client->>WorkflowManager: CreateWorkflow(template, inputs)
WorkflowManager->>SelectorResolver: getInputWithResolvedSelectors()
SelectorResolver->>Store: getResources/getEnvironments/getDeployments(selector)
Store-->>SelectorResolver: matched entities
SelectorResolver-->>WorkflowManager: resolved inputs
loop For each jobTemplate
alt Matrix present
WorkflowManager->>MatrixResolver: NewMatrixResolver(matrix, inputs)
MatrixResolver-->>MatrixResolver: getMatrixRows()
MatrixResolver-->>MatrixResolver: computeCartesianProduct()
MatrixResolver-->>WorkflowManager: Resolve()
WorkflowManager->>WorkflowManager: store ResolvedMatrix on WorkflowJob
else No matrix
WorkflowManager->>WorkflowManager: ResolvedMatrix = nil
end
end
WorkflowManager->>JobDispatcher: dispatch workflow
loop For each workflowJob
alt ResolvedMatrix exists
loop For each matrix item
JobDispatcher->>JobDispatcher: mergeJobAgentConfig + matrix item
JobDispatcher->>Store: create Job with augmented config
end
else No matrix
JobDispatcher->>JobDispatcher: mergeJobAgentConfig
JobDispatcher->>Store: create single Job
end
end
JobDispatcher-->>Client: workflow with jobs created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/pkg/workspace/jobagents/registry.go (1)
82-90:⚠️ Potential issue | 🟠 MajorGuard the matrix type assertion to avoid panics.
Line 88 will panic if
matrixisn’t amap[string]interface{}(e.g., array or string). Use a safe cast and fail fast with a clear error.✅ Suggested fix
- if job.JobAgentConfig["matrix"] != nil { - renderContext.Matrix = job.JobAgentConfig["matrix"].(map[string]interface{}) - } + if raw, ok := job.JobAgentConfig["matrix"]; ok && raw != nil { + matrix, ok := raw.(map[string]interface{}) + if !ok { + return fmt.Errorf("job agent config matrix must be an object") + } + renderContext.Matrix = matrix + }
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/manager.go`:
- Around line 54-60: The current loop upserts WorkflowJobs early and then calls
NewMatrixResolver(...).Resolve(), which can fail and leave orphaned jobs; change
the logic in the manager that iterates job templates (the code that constructs
wfJob and calls WorkflowJobs.Upsert) to first resolve all job templates
including calling NewMatrixResolver(jobTemplate.Matrix,
inputsWithResolvedSelectors).Resolve() and setting WorkflowJob.ResolvedMatrix
(or collect resolved wfJob objects in-memory), and only after all templates
resolve successfully perform WorkflowJobs.Upsert for each wfJob; alternatively,
implement a rollback path that calls WorkflowJobs.Delete (or the repository's
delete/cleanup method) for any jobs already Upserted if Resolve() returns an
error—apply this change around the loop that creates wfJob and where
WorkflowJobs.Upsert is invoked.
In `@apps/workspace-engine/pkg/workspace/workflowmanager/matrix_resolver.go`:
- Around line 25-49: In getMatrixRows, the branch handling AsWorkflowJobMatrix1
assumes r.inputs[asString] is []map[string]interface{} which fails for
JSON-decoded arrays (they are []interface{}); update the logic in getMatrixRows
(and the matrixRow creation) to first attempt the existing
[]map[string]interface{} assertion, and if that fails check for []interface{}
and iterate over its elements converting each element to map[string]interface{}
(returning an error if any element is not a map); keep the same error message
("input %s is not an array") or refine to indicate element type issues and
ensure the converted slice is used as Values when appending the matrixRow.
In `@apps/workspace-engine/pkg/workspace/workflowmanager/resolve_selectors.go`:
- Around line 62-105: The code in getInputWithResolvedSelectors currently
unsafely asserts selectorInputEntry.(string) which panics for non-string types;
change the handling of selectorInputEntry (from asSelectorArray.Name) to perform
a safe type check (e.g., type switch or ok, str := selectorInputEntry.(string))
and if the value is not a string return a clear validation error rather than
panicking, then pass the validated string into sel.FromCelSelector (or handle
the absence/invalid type by falling back to the default selector) so sel is only
constructed when a valid string is provided.
Comment on lines
+54
to
+60
| if jobTemplate.Matrix != nil { | ||
| resolvedMatrix, err := NewMatrixResolver(jobTemplate.Matrix, inputsWithResolvedSelectors).Resolve() | ||
| if err != nil { | ||
| return workflow, fmt.Errorf("failed to resolve matrix: %w", err) | ||
| } | ||
| wfJob.ResolvedMatrix = &resolvedMatrix | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid partial WorkflowJob persistence on matrix resolution failure.
If matrix resolution fails (Line 55–57), earlier jobs have already been Upserted (Line 61), but the workflow isn’t stored yet—this leaves orphaned jobs. Consider deferring WorkflowJobs.Upsert until all templates resolve, or rolling back on error.
💡 Suggested change to defer job persistence
for idx, jobTemplate := range workflowTemplate.Jobs {
wfJob := &oapi.WorkflowJob{
Id: uuid.New().String(),
WorkflowId: workflow.Id,
Index: idx,
Ref: jobTemplate.Ref,
Config: maps.Clone(jobTemplate.Config),
}
if jobTemplate.Matrix != nil {
resolvedMatrix, err := NewMatrixResolver(jobTemplate.Matrix, inputsWithResolvedSelectors).Resolve()
if err != nil {
return workflow, fmt.Errorf("failed to resolve matrix: %w", err)
}
wfJob.ResolvedMatrix = &resolvedMatrix
}
- m.store.WorkflowJobs.Upsert(ctx, wfJob)
workflowJobs = append(workflowJobs, wfJob)
}
+ for _, wfJob := range workflowJobs {
+ m.store.WorkflowJobs.Upsert(ctx, wfJob)
+ }🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/manager.go` around lines
54 - 60, The current loop upserts WorkflowJobs early and then calls
NewMatrixResolver(...).Resolve(), which can fail and leave orphaned jobs; change
the logic in the manager that iterates job templates (the code that constructs
wfJob and calls WorkflowJobs.Upsert) to first resolve all job templates
including calling NewMatrixResolver(jobTemplate.Matrix,
inputsWithResolvedSelectors).Resolve() and setting WorkflowJob.ResolvedMatrix
(or collect resolved wfJob objects in-memory), and only after all templates
resolve successfully perform WorkflowJobs.Upsert for each wfJob; alternatively,
implement a rollback path that calls WorkflowJobs.Delete (or the repository's
delete/cleanup method) for any jobs already Upserted if Resolve() returns an
error—apply this change around the loop that creates wfJob and where
WorkflowJobs.Upsert is invoked.
Comment on lines
+25
to
+49
| func (r *MatrixResolver) getMatrixRows() ([]matrixRow, error) { | ||
| matrixRows := make([]matrixRow, 0, len(*r.matrix)) | ||
| for key, value := range *r.matrix { | ||
| asArray, err := value.AsWorkflowJobMatrix0() | ||
| if err == nil { | ||
| matrixRows = append(matrixRows, matrixRow{ | ||
| Key: key, | ||
| Values: asArray, | ||
| }) | ||
| continue | ||
| } | ||
| asString, err := value.AsWorkflowJobMatrix1() | ||
| if err == nil { | ||
| arrayFromInput, ok := r.inputs[asString].([]map[string]interface{}) | ||
| if !ok { | ||
| return nil, fmt.Errorf("input %s is not an array", asString) | ||
| } | ||
| matrixRows = append(matrixRows, matrixRow{ | ||
| Key: key, | ||
| Values: arrayFromInput, | ||
| }) | ||
| continue | ||
| } | ||
| } | ||
| return matrixRows, nil |
Contributor
There was a problem hiding this comment.
Handle JSON-decoded input arrays when matrix references inputs.
Line 38 assumes []map[string]interface{}. Inputs coming from JSON are typically []interface{} (with map[string]interface{} elements), so this path will error even for valid arrays. Consider accepting []interface{} and converting items.
🔧 Suggested conversion to support JSON-decoded arrays
asString, err := value.AsWorkflowJobMatrix1()
if err == nil {
- arrayFromInput, ok := r.inputs[asString].([]map[string]interface{})
- if !ok {
- return nil, fmt.Errorf("input %s is not an array", asString)
- }
+ raw, ok := r.inputs[asString]
+ if !ok {
+ return nil, fmt.Errorf("input %s not found", asString)
+ }
+ var arrayFromInput []map[string]interface{}
+ switch v := raw.(type) {
+ case []map[string]interface{}:
+ arrayFromInput = v
+ case []interface{}:
+ arrayFromInput = make([]map[string]interface{}, 0, len(v))
+ for _, item := range v {
+ m, ok := item.(map[string]interface{})
+ if !ok {
+ return nil, fmt.Errorf("input %s contains non-object item", asString)
+ }
+ arrayFromInput = append(arrayFromInput, m)
+ }
+ default:
+ return nil, fmt.Errorf("input %s is not an array", asString)
+ }
matrixRows = append(matrixRows, matrixRow{
Key: key,
Values: arrayFromInput,
})
continue
}📝 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
| func (r *MatrixResolver) getMatrixRows() ([]matrixRow, error) { | |
| matrixRows := make([]matrixRow, 0, len(*r.matrix)) | |
| for key, value := range *r.matrix { | |
| asArray, err := value.AsWorkflowJobMatrix0() | |
| if err == nil { | |
| matrixRows = append(matrixRows, matrixRow{ | |
| Key: key, | |
| Values: asArray, | |
| }) | |
| continue | |
| } | |
| asString, err := value.AsWorkflowJobMatrix1() | |
| if err == nil { | |
| arrayFromInput, ok := r.inputs[asString].([]map[string]interface{}) | |
| if !ok { | |
| return nil, fmt.Errorf("input %s is not an array", asString) | |
| } | |
| matrixRows = append(matrixRows, matrixRow{ | |
| Key: key, | |
| Values: arrayFromInput, | |
| }) | |
| continue | |
| } | |
| } | |
| return matrixRows, nil | |
| func (r *MatrixResolver) getMatrixRows() ([]matrixRow, error) { | |
| matrixRows := make([]matrixRow, 0, len(*r.matrix)) | |
| for key, value := range *r.matrix { | |
| asArray, err := value.AsWorkflowJobMatrix0() | |
| if err == nil { | |
| matrixRows = append(matrixRows, matrixRow{ | |
| Key: key, | |
| Values: asArray, | |
| }) | |
| continue | |
| } | |
| asString, err := value.AsWorkflowJobMatrix1() | |
| if err == nil { | |
| raw, ok := r.inputs[asString] | |
| if !ok { | |
| return nil, fmt.Errorf("input %s not found", asString) | |
| } | |
| var arrayFromInput []map[string]interface{} | |
| switch v := raw.(type) { | |
| case []map[string]interface{}: | |
| arrayFromInput = v | |
| case []interface{}: | |
| arrayFromInput = make([]map[string]interface{}, 0, len(v)) | |
| for _, item := range v { | |
| m, ok := item.(map[string]interface{}) | |
| if !ok { | |
| return nil, fmt.Errorf("input %s contains non-object item", asString) | |
| } | |
| arrayFromInput = append(arrayFromInput, m) | |
| } | |
| default: | |
| return nil, fmt.Errorf("input %s is not an array", asString) | |
| } | |
| matrixRows = append(matrixRows, matrixRow{ | |
| Key: key, | |
| Values: arrayFromInput, | |
| }) | |
| continue | |
| } | |
| } | |
| return matrixRows, nil | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/matrix_resolver.go`
around lines 25 - 49, In getMatrixRows, the branch handling AsWorkflowJobMatrix1
assumes r.inputs[asString] is []map[string]interface{} which fails for
JSON-decoded arrays (they are []interface{}); update the logic in getMatrixRows
(and the matrixRow creation) to first attempt the existing
[]map[string]interface{} assertion, and if that fails check for []interface{}
and iterate over its elements converting each element to map[string]interface{}
(returning an error if any element is not a map); keep the same error message
("input %s is not an array") or refine to indicate element type issues and
ensure the converted slice is used as Values when appending the matrixRow.
Comment on lines
+62
to
+105
| func (m *Manager) getInputWithResolvedSelectors(ctx context.Context, workflowTemplate *oapi.WorkflowTemplate, inputs map[string]any) (map[string]any, error) { | ||
| inputsClone := maps.Clone(inputs) | ||
|
|
||
| for _, input := range workflowTemplate.Inputs { | ||
| asArray, err := input.AsWorkflowArrayInput() | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| asSelectorArray, err := asArray.AsWorkflowSelectorArrayInput() | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| sel := asSelectorArray.Selector.Default | ||
|
|
||
| selectorInputEntry, ok := inputsClone[asSelectorArray.Name] | ||
| if ok { | ||
| selectorInputEntryString := selectorInputEntry.(string) | ||
| sel = &oapi.Selector{} | ||
| if err := sel.FromCelSelector(oapi.CelSelector{Cel: selectorInputEntryString}); err != nil { | ||
| return nil, fmt.Errorf("failed to parse selector: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if sel == nil { | ||
| return nil, fmt.Errorf("selector is nil") | ||
| } | ||
|
|
||
| var matchedEntities []map[string]interface{} | ||
|
|
||
| switch asSelectorArray.Selector.EntityType { | ||
| case oapi.WorkflowSelectorArrayInputSelectorEntityTypeResource: | ||
| matchedEntities = m.getResources(ctx, sel) | ||
| case oapi.WorkflowSelectorArrayInputSelectorEntityTypeEnvironment: | ||
| matchedEntities = m.getEnvironments(ctx, sel) | ||
| case oapi.WorkflowSelectorArrayInputSelectorEntityTypeDeployment: | ||
| matchedEntities = m.getDeployments(ctx, sel) | ||
| } | ||
|
|
||
| inputsClone[asSelectorArray.Name] = matchedEntities | ||
| } | ||
|
|
||
| return inputsClone, nil |
Contributor
There was a problem hiding this comment.
Guard selector input type to avoid runtime panic.
Pipeline failure shows a panic at Line 80: interface {} is float64, not string. The direct selectorInputEntry.(string) assertion will panic for non-string inputs (e.g., JSON numbers). Use a safe type assertion and return a validation error instead.
🛠️ Suggested fix for safe selector parsing
selectorInputEntry, ok := inputsClone[asSelectorArray.Name]
if ok {
- selectorInputEntryString := selectorInputEntry.(string)
+ selectorInputEntryString, ok := selectorInputEntry.(string)
+ if !ok {
+ return nil, fmt.Errorf("selector input %s must be a string", asSelectorArray.Name)
+ }
sel = &oapi.Selector{}
if err := sel.FromCelSelector(oapi.CelSelector{Cel: selectorInputEntryString}); err != nil {
return nil, fmt.Errorf("failed to parse selector: %w", err)
}
}🧰 Tools
🪛 GitHub Actions: Apps / Workspace Engine
[error] 80-80: Runtime panic: interface conversion: interface {} is float64, not string at resolve_selectors.go:80
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/resolve_selectors.go`
around lines 62 - 105, The code in getInputWithResolvedSelectors currently
unsafely asserts selectorInputEntry.(string) which panics for non-string types;
change the handling of selectorInputEntry (from asSelectorArray.Name) to perform
a safe type check (e.g., type switch or ok, str := selectorInputEntry.(string))
and if the value is not a string return a clear validation error rather than
panicking, then pass the validated string into sel.FromCelSelector (or handle
the absence/invalid type by falling back to the default selector) so sel is only
constructed when a valid string is provided.
|
|
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
Release Notes