refactor: merge job agent config in dispatcher #803
Conversation
📝 WalkthroughWalkthroughRemoves the Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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}
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 (2)
apps/workspace-engine/pkg/workspace/jobs/factory.go (2)
32-33:⚠️ Potential issue | 🟡 MinorStale doc comment — merging no longer happens here.
The comment says "configured with merged settings from JobAgent + Deployment" but this function now sets an empty
JobAgentConfig{}. The merging was moved to the registry'sDispatchflow.Proposed fix
-// CreateJobForRelease creates a job for a given release (PURE FUNCTION, NO WRITES). -// The job is configured with merged settings from JobAgent + Deployment. +// CreateJobForRelease creates a job for a given release (PURE FUNCTION, NO WRITES). +// The job is created with an empty JobAgentConfig; config merging is handled during dispatch.
119-123:⚠️ Potential issue | 🟡 MinorTrace step message is misleading.
"Applied default job agent configuration"implies some configuration was applied, but the job is created with an empty config. Consider updating the message to reflect that config merging is deferred to dispatch.
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/jobagents/registry_test.go`:
- Around line 369-396: The test
TestGetMergedJobAgentConfig_AllSourcesMergeInOrder mocks ctx.Version directly
which masks the bug where fillReleaseContext never populates
DispatchContext.Version; add an end-to-end integration test that exercises the
real Dispatch flow (invoke Dispatch or the higher-level method that calls
fillReleaseContext) and assert that the DeploymentVersion.JobAgentConfig is
merged into the final config; ensure the new test does not set ctx.Version
manually but triggers the code paths that populate Version via
fillReleaseContext so getMergedJobAgentConfig is validated in a realistic
scenario.
In `@apps/workspace-engine/pkg/workspace/jobagents/registry.go`:
- Line 82: Rename the misspelled variable `wofklowJobConfig` to
`workflowJobConfig` (type oapi.JobAgentConfig) and update every reference to it
(including the usage around the later code noted at line ~97) so the identifier
is consistent; ensure you only change the identifier name and keep the type and
initialization intact.
- Around line 40-57: In fillReleaseContext, after assigning ctx.Release,
ctx.Deployment, ctx.Environment, and ctx.Resource, also set ctx.Version from the
release (e.g. ctx.Version = jobWithRelease.Release.Version) so version-level
config merging in getMergedJobAgentConfig will run; additionally correct the
typo `wofklowJobConfig` to `workflowJobConfig` wherever that identifier appears.
🧹 Nitpick comments (2)
apps/workspace-engine/pkg/workspace/jobagents/registry.go (1)
139-145:mergeJobAgentConfignever returns an error — signature is misleading.The function always returns
nilfor the error. Either simplify the signature to return onlyoapi.JobAgentConfig, or document why the error return is kept (e.g., future-proofing for JSON-based merging).apps/workspace-engine/pkg/workspace/jobs/factory_test.go (1)
28-66: Remove unused typed config helper functions.
getGithubJobAgentConfig,getArgoCDJobAgentConfig,getTerraformCloudJobAgentConfig, andgetTestRunnerJobAgentConfigare not called anywhere in the codebase and appear to be remnants of removed tests. Remove them.
|
@cursoragent could you review the pull and explain why capability introspection from job agents is no longer needed |
|
Here’s the short version: capability introspection was removed in PR #803 (“refactor: merge job agent config in dispatcher”) to simplify the job-agent interface and centralize config/dispatch logic.
Code references: type Dispatchable interface {
Type() string
Dispatch(ctx context.Context, context DispatchContext) error
}func (r *Registry) Dispatch(ctx context.Context, job *oapi.Job) error {
jobAgent, ok := r.store.JobAgents.Get(job.JobAgentId)
...
mergedConfig, err := r.getMergedJobAgentConfig(jobAgent, &dispatchContext)
...
job.JobAgentConfig = mergedConfig
r.store.Jobs.Upsert(ctx, job)
return dispatcher.Dispatch(ctx, dispatchContext)
}job := &oapi.Job{
...
JobAgentConfig: oapi.JobAgentConfig{}, // now defaulted; merged later in registry
...
}Status: I reviewed PR #803 and its diff to confirm capability introspection removal and the centralization of config merging in the registry; no blockers found. |
2 participants
Summary by CodeRabbit