refactor: consolidate job creation into factory package#805
refactor: consolidate job creation into factory package#805adityachoudhari26 merged 4 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughRefactors dispatch-context construction: logic for merging JobAgentConfig and populating DispatchContext is moved from the jobagents registry into the jobs Factory. Registry.Dispatch is simplified to delegate directly to dispatcher; Factory now builds jobs (release and workflow) with merged configs and complete DispatchContext before upsert and dispatch. Changes
Sequence Diagram(s)sequenceDiagram
participant WF as WorkflowManager
participant F as jobs.Factory
participant S as Store
participant R as jobagents.Registry
participant D as Dispatcher
WF->>F: CreateJobForWorkflowJob(workflowRunID, workflowJob)
F->>S: read environment/resource/release/version/agent
F->>F: mergeJobAgentConfig + buildDispatchContext
F-->>WF: Job (with DispatchContext)
WF->>S: UpsertJob(Job)
WF->>R: Dispatch(Job)
R->>D: Dispatch(job, jobAgentType, DispatchContext)
D-->>R: result
R-->>WF: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go`:
- Around line 78-91: The local variable mergedConfig is pre-allocated with
make(oapi.JobAgentConfig) but immediately overwritten by mergeJobAgentConfig;
remove the wasted allocation by deleting the make call and either declare
mergedConfig via short assignment from mergeJobAgentConfig (e.g. mergedConfig,
err := mergeJobAgentConfig(...)) or declare var mergedConfig oapi.JobAgentConfig
before calling mergeJobAgentConfig; update the call site in
Factory.buildJobAgentConfig that uses jobAgent.Config, deployment.JobAgentConfig
and release.Version.JobAgentConfig to use the returned mergedConfig and handle
err as already done.
🧹 Nitpick comments (6)
apps/workspace-engine/pkg/workspace/jobs/helpers.go (2)
19-29:deepMergeassigns nested source maps by reference whendst[k]is not already a map.When
srccontains a nestedmap[string]anyanddst[k]doesn't exist (or isn't a map), line 27 assigns the source's map reference directly intodst. This means the merged config shares mutable state with the original config maps. If any source config is later mutated in-place (e.g., via a store update), the merged config silently changes too.Given that configs are likely treated as immutable in practice, this may be acceptable, but it's worth being aware of.
🔧 Defensive copy of nested maps
func deepMerge(dst, src map[string]any) { for k, v := range src { if sm, ok := v.(map[string]any); ok { if dm, ok := dst[k].(map[string]any); ok { deepMerge(dm, sm) continue } + // Clone the source map to avoid sharing references + cloned := make(map[string]any, len(sm)) + deepMerge(cloned, sm) + dst[k] = cloned + continue } dst[k] = v } }
11-17:mergeJobAgentConfignever returns an error.The function signature returns
error, but the current implementation always returnsnil. This is fine for forward-compatibility if error handling may be added later, but callers currently check for an error that can never occur.apps/workspace-engine/pkg/workspace/jobs/factory_test.go (1)
67-76: Minor inconsistency:createTestResourceomits fields that inline construction includes.The helper doesn't set
Metadata,CreatedAt, orWorkspaceId, while inlineoapi.Resourceconstruction insetupFullStore(line 340) and other tests does. This could cause subtle differences in test behavior if the factory code ever accesses those fields from the dispatch context.Not blocking, but consider aligning the helper with the inline patterns for consistency.
apps/workspace-engine/pkg/workspace/jobs/factory.go (3)
32-76: Significant duplication betweennoAgentConfiguredJobandjobAgentNotFoundJob.These two methods differ only in the message string and trace step name. The job construction (UUID, status, timestamps, metadata) is identical. Consider extracting a shared helper to reduce duplication.
Also, the trace step name in
jobAgentNotFoundJob(line 58) says"Create NoAgentFoundJob job"which doesn't match any status name — consider aligning it with"Create InvalidJobAgent job"for consistency.Example consolidation
+func (f *Factory) invalidJobAgentJob(releaseID, jobAgentID, message, stepName string, action *trace.Action) *oapi.Job { + if action != nil { + action.AddStep(stepName, trace.StepResultPass, + fmt.Sprintf("Created InvalidJobAgent job for release %s with job agent %s", releaseID, jobAgentID)). + AddMetadata("release_id", releaseID). + AddMetadata("job_agent_id", jobAgentID). + AddMetadata("message", message) + } + return &oapi.Job{ + Id: uuid.New().String(), + ReleaseId: releaseID, + JobAgentId: jobAgentID, + JobAgentConfig: oapi.JobAgentConfig{}, + Status: oapi.JobStatusInvalidJobAgent, + Message: &message, + CreatedAt: time.Now(), + UpdatedAt: time.Now(), + Metadata: make(map[string]string), + } +} + func (f *Factory) noAgentConfiguredJob(releaseID, jobAgentID, deploymentName string, action *trace.Action) *oapi.Job { - ... + msg := fmt.Sprintf("No job agent configured for deployment '%s'", deploymentName) + return f.invalidJobAgentJob(releaseID, jobAgentID, msg, "Create InvalidJobAgent job", action) } func (f *Factory) jobAgentNotFoundJob(releaseID, jobAgentID, deploymentName string, action *trace.Action) *oapi.Job { - ... + msg := fmt.Sprintf("Job agent '%s' not found for deployment '%s'", jobAgentID, deploymentName) + return f.invalidJobAgentJob(releaseID, jobAgentID, msg, "Create InvalidJobAgent job", action) }
275-302: Missing doc comment on exported methodCreateJobForWorkflowJob.
CreateJobForRelease(line 139) has a doc comment, butCreateJobForWorkflowJobdoes not. As per coding guidelines, exported functions/methods should be documented.Proposed fix
+// CreateJobForWorkflowJob creates a job for a given workflow job. +// The job is configured with merged settings from JobAgent + WorkflowJob config. func (f *Factory) CreateJobForWorkflowJob(ctx context.Context, wfJob *oapi.WorkflowJob) (*oapi.Job, error) {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."
186-190: Trace step message is misleading post-refactor.
"Applied default job agent configuration"no longer accurately describes what happens — configs are now merged from agent, deployment, and version sources. If this is pre-existing text, consider updating it to reflect the new merge behavior.
| func (f *Factory) buildJobAgentConfig(release *oapi.Release, deployment *oapi.Deployment, jobAgent *oapi.JobAgent) (oapi.JobAgentConfig, error) { | ||
| mergedConfig := make(oapi.JobAgentConfig) | ||
|
|
||
| agentConfig := jobAgent.Config | ||
| deploymentConfig := deployment.JobAgentConfig | ||
| versionConfig := release.Version.JobAgentConfig | ||
|
|
||
| mergedConfig, err := mergeJobAgentConfig(agentConfig, deploymentConfig, versionConfig) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to merge job agent configs: %w", err) | ||
| } | ||
|
|
||
| return mergedConfig, nil | ||
| } |
There was a problem hiding this comment.
Wasted allocation on line 79.
mergedConfig is initialized with make(oapi.JobAgentConfig) but immediately reassigned by the return of mergeJobAgentConfig on line 85. The make call allocates a map that is never used.
Proposed fix
func (f *Factory) buildJobAgentConfig(release *oapi.Release, deployment *oapi.Deployment, jobAgent *oapi.JobAgent) (oapi.JobAgentConfig, error) {
- mergedConfig := make(oapi.JobAgentConfig)
-
agentConfig := jobAgent.Config
deploymentConfig := deployment.JobAgentConfig
versionConfig := release.Version.JobAgentConfig
- mergedConfig, err := mergeJobAgentConfig(agentConfig, deploymentConfig, versionConfig)
+ mergedConfig, err := mergeJobAgentConfig(agentConfig, deploymentConfig, versionConfig)
if err != nil {
return nil, fmt.Errorf("failed to merge job agent configs: %w", err)
}
return mergedConfig, 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.
| func (f *Factory) buildJobAgentConfig(release *oapi.Release, deployment *oapi.Deployment, jobAgent *oapi.JobAgent) (oapi.JobAgentConfig, error) { | |
| mergedConfig := make(oapi.JobAgentConfig) | |
| agentConfig := jobAgent.Config | |
| deploymentConfig := deployment.JobAgentConfig | |
| versionConfig := release.Version.JobAgentConfig | |
| mergedConfig, err := mergeJobAgentConfig(agentConfig, deploymentConfig, versionConfig) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to merge job agent configs: %w", err) | |
| } | |
| return mergedConfig, nil | |
| } | |
| func (f *Factory) buildJobAgentConfig(release *oapi.Release, deployment *oapi.Deployment, jobAgent *oapi.JobAgent) (oapi.JobAgentConfig, error) { | |
| agentConfig := jobAgent.Config | |
| deploymentConfig := deployment.JobAgentConfig | |
| versionConfig := release.Version.JobAgentConfig | |
| mergedConfig, err := mergeJobAgentConfig(agentConfig, deploymentConfig, versionConfig) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to merge job agent configs: %w", err) | |
| } | |
| return mergedConfig, nil | |
| } |
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go` around lines 78 - 91,
The local variable mergedConfig is pre-allocated with make(oapi.JobAgentConfig)
but immediately overwritten by mergeJobAgentConfig; remove the wasted allocation
by deleting the make call and either declare mergedConfig via short assignment
from mergeJobAgentConfig (e.g. mergedConfig, err := mergeJobAgentConfig(...)) or
declare var mergedConfig oapi.JobAgentConfig before calling mergeJobAgentConfig;
update the call site in Factory.buildJobAgentConfig that uses jobAgent.Config,
deployment.JobAgentConfig and release.Version.JobAgentConfig to use the returned
mergedConfig and handle err as already done.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go`:
- Around line 125-134: DispatchContext currently sets Version and Variables to
&releaseCopy.Version and &releaseCopy.Variables which aliases those fields to
Release and risks silent mutation; change this to create independent copies
(e.g., make local copies like versionCopy := releaseCopy.Version and
variablesCopy := releaseCopy.Variables) and assign &versionCopy and
&variablesCopy to DispatchContext.Version and DispatchContext.Variables
respectively (or, if aliasing was intentional, add a clear inline comment in the
factory function explaining the deliberate shared-state decision); reference
DispatchContext, Release, Version, Variables, and releaseCopy to locate the
change.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/jobs/factory.go (1)
32-76: Consider extracting a shared helper for invalid-agent jobs.
noAgentConfiguredJobandjobAgentNotFoundJobbuild identicaloapi.Jobstructs — only themessageand trace step text differ. A single private helper (e.g.,invalidJobAgentJob(releaseID, jobAgentID, message string, action …)) called from both sites would remove the duplication.
| return &oapi.DispatchContext{ | ||
| Release: releaseCopy, | ||
| Deployment: deploymentCopy, | ||
| Environment: envCopy, | ||
| Resource: resourceCopy, | ||
| JobAgent: *agentCopy, | ||
| JobAgentConfig: mergedConfig, | ||
| Version: &releaseCopy.Version, | ||
| Variables: &releaseCopy.Variables, | ||
| }, nil |
There was a problem hiding this comment.
Version and Variables alias fields inside Release — intentional?
&releaseCopy.Version and &releaseCopy.Variables point into the same struct held by Release. Mutating dispatchContext.Version silently mutates dispatchContext.Release.Version (and likewise for Variables). Given the care taken to deep-copy everything else, this shared-state shortcut is worth a brief comment confirming it's intentional, or separate copies if it isn't.
🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/jobs/factory.go` around lines 125 - 134,
DispatchContext currently sets Version and Variables to &releaseCopy.Version and
&releaseCopy.Variables which aliases those fields to Release and risks silent
mutation; change this to create independent copies (e.g., make local copies like
versionCopy := releaseCopy.Version and variablesCopy := releaseCopy.Variables)
and assign &versionCopy and &variablesCopy to DispatchContext.Version and
DispatchContext.Variables respectively (or, if aliasing was intentional, add a
clear inline comment in the factory function explaining the deliberate
shared-state decision); reference DispatchContext, Release, Version, Variables,
and releaseCopy to locate the change.
Summary by CodeRabbit
Refactor
Tests