feat: fan out workflow jobs with resource selector input#1161
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds resource-aware workflow-run fan-out: new OpenAPI schemas for run results with per-job dispatch info, dispatch planning that matches job agents to resources, data-access helpers to load resources and runners, refactored persistence for precomputed dispatches, handler orchestration, unit and e2e tests, and updated generated types. ChangesWorkflow Run Fan-Out
Sequence DiagramsequenceDiagram
participant Client
participant Handler as CreateWorkflowRun
participant Getter as Getter
participant Planner as planDispatches
participant Setter as PersistWorkflowRun
Client->>Handler: POST /v1/workspaces/{wsId}/workflows/{id}/runs
Handler->>Getter: GetWorkflow(workflowId)
Handler->>Handler: resolve + extract inputs
Handler->>Getter: GetResourcesMatching(selector)
Handler->>Getter: GetJobAgentsByRef(jobAgents)
Handler->>Planner: planDispatches(resources, jobAgents, runners)
Planner-->>Handler: plannedDispatch[]
Handler->>Setter: PersistWorkflowRun(workflowID, inputs, dispatches)
Setter->>DB: insert run & jobs
Setter->>Queue: enqueue job-dispatch items
Setter-->>Handler: WorkflowRunResult
Handler-->>Client: 201 WorkflowRunResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Pull request overview
This PR adds workflow-run fan-out: a single workflow run can now dispatch multiple jobs (one per matched resource), with server-side routing of each resource to an appropriate job agent via selectors. It also updates the public OpenAPI contract to return the dispatched job IDs/agent IDs for a run, and adds coverage via unit + E2E tests.
Changes:
- Introduce
WorkflowRunResult/WorkflowRunJobresponse types and update workflow-run creation endpoints to return dispatched jobs. - Add workspace-engine server logic to (1) resolve a reserved
resourceSelectorinput, (2) match resources, (3) plan dispatches per resource, and (4) persist/enqueue resulting jobs. - Add unit tests for resource matching/dispatch planning and an end-to-end test covering fan-out + routing behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/workspace-engine-sdk/src/schema.ts | Updates workspace-engine SDK OpenAPI types to return WorkflowRunResult. |
| e2e/tests/api/workflows.spec.ts | Adds E2E coverage for fan-out and server-side routing of resources to job agents. |
| e2e/api/schema.ts | Regenerates E2E OpenAPI types to include WorkflowRunResult / WorkflowRunJob. |
| apps/workspace-engine/svc/http/server/openapi/workflows/workflows.go | Implements fan-out run creation flow (resource selection → dispatch planning → persistence). |
| apps/workspace-engine/svc/http/server/openapi/workflows/workflows_test.go | Adds unit tests for resource matching and dispatch routing behavior. |
| apps/workspace-engine/svc/http/server/openapi/workflows/setters.go | Refactors persistence to accept pre-planned dispatches and return WorkflowRunResult. |
| apps/workspace-engine/svc/http/server/openapi/workflows/getters.go | Adds resource matching and job-agent lookup helpers for run creation. |
| apps/workspace-engine/svc/http/server/openapi/workflows/dispatch.go | New dispatch planning logic to route resources to job agents via selectors. |
| apps/api/src/types/openapi.ts | Regenerates API OpenAPI TS types for the updated workflow-run response. |
| apps/api/openapi/schemas/workflows.jsonnet | Adds WorkflowRunResult and WorkflowRunJob schemas to API OpenAPI. |
| apps/api/openapi/paths/workflows.jsonnet | Updates workflow-run creation responses to reference WorkflowRunResult. |
| apps/api/openapi/openapi.json | Regenerates the consolidated OpenAPI JSON output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+80
to
+82
| if sel == "" { | ||
| return []*oapi.Resource{nil}, nil | ||
| } |
Comment on lines
+89
to
+92
| rows, err := db.GetQueries(ctx).ListResourcesByWorkspaceID(ctx, workspaceUUID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("list resources: %w", err) | ||
| } |
Comment on lines
+105
to
+106
| resourceSelector, _ := inputs[resourceSelectorInputKey].(string) | ||
| resources, err := w.getter.GetResourcesMatching(ctx, workspaceId, resourceSelector) |
Comment on lines
+77
to
+86
| for _, job := range jobs { | ||
| if err := s.queue.Enqueue(ctx, reconcile.EnqueueParams{ | ||
| WorkspaceID: workspaceID, | ||
| Kind: "job-dispatch", | ||
| ScopeType: "job", | ||
| ScopeID: job.JobId, | ||
| }); err != nil { | ||
| return nil, fmt.Errorf("enqueue job dispatch: %w", err) | ||
| } | ||
| } |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/openapi/openapi.json (1)
10409-10417:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument the reserved resource-selector input in the create-run contract.
These endpoints now expose the new fan-out result shape, but the request schema still presents
inputsas an opaque object. The server behavior introduced in this PR depends on a reserved selector input key, so consumers generated from this spec still have no discoverable contract for how to trigger resource-based fan-out. Please add that key to the source OpenAPI/jsonnet request schema or at least document it oninputs, then regenerate the generated artifacts.As per coding guidelines, "Do not hand-edit generated OpenAPI output (
src/types/openapi.tsandopenapi/openapi.json). Regenerate using the generate command instead."Also applies to: 10693-10701
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/openapi/openapi.json` around lines 10409 - 10417, The OpenAPI contract for the create-run endpoint exposes the new fan-out result shape but does not document the reserved resource-selector input key on the request `inputs` schema; update the source request schema (not the generated artifacts) to include or document the reserved selector key for triggering resource-based fan-out (the request body `inputs` property used by the create-run endpoints that return `WorkflowRunResult`), then run the OpenAPI generation command to regenerate `openapi/openapi.json` and `src/types/openapi.ts`; ensure the `inputs` schema includes a clearly named reserved field and description so generated clients can discover the selector contract.
🧹 Nitpick comments (3)
apps/workspace-engine/svc/http/server/openapi/workflows/workflows_test.go (1)
50-115: ⚡ Quick winUse a table-driven shape for selector-routing cases.
The three
TestPlanDispatches_*cases exercise the same selector-routing axis and are better expressed as one table-driven test with per-case inputs/expected dispatch counts. That keeps future selector edge cases easy to add and aligned with this package’s test style.As per coding guidelines:
apps/workspace-engine/**/*_test.go: “Use table-driven tests for condition, selector, policy, and conversion logic.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/workspace-engine/svc/http/server/openapi/workflows/workflows_test.go` around lines 50 - 115, Combine the three tests (TestPlanDispatches_RoutesEachResourceToItsServer, TestPlanDispatches_NoMatchingServerYieldsNoDispatches, TestPlanDispatches_NilResourceRunsGateOnce) into a single table-driven test that iterates cases exercising selector-routing for planDispatches; for each case provide name, resources (use resourceOnServer/ nil where needed), agents (use argoAgent or constructed oapi.WorkflowJobAgent), runners map, and expected dispatch count and any extra assertions (e.g., expect nil dispatchCtx.Resource). In the combined test call planDispatches(ctx, base, tc.resources, tc.agents, tc.runners), assert no error, assert len(dispatches)==tc.expectedCount, and for routing cases verify the server routing by comparing dispatchCtx.Resource.Config["argo"].(map[string]any)["server"] against runner.Config["serverUrl"] as before; include the nil-resource case to assert dispatches[0].dispatchCtx.Resource == nil. Ensure test name indicates selector-routing table-driven style and reuse helper functions resourceOnServer, argoAgent and planDispatches to locate relevant code.apps/workspace-engine/svc/http/server/openapi/workflows/workflows.go (1)
136-136: 💤 Low valueConsider returning HTTP 201 Created for workflow run creation.
Creating a new workflow run is semantically a resource creation operation. HTTP 201 is more appropriate than 200.
- c.JSON(http.StatusOK, result) + c.JSON(http.StatusCreated, result)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/workspace-engine/svc/http/server/openapi/workflows/workflows.go` at line 136, The response for creating a workflow run currently returns http.StatusOK via c.JSON(http.StatusOK, result); update the handler (the function that calls c.JSON in workflows.go — e.g., the CreateWorkflowRun HTTP handler) to return http.StatusCreated instead and set a Location header pointing to the newly created resource (use the run ID from result to build the URL, e.g., c.Header("Location", fmt.Sprintf("/workflows/%s/runs/%s", workflowID, result.ID))) before sending the JSON body.apps/workspace-engine/svc/http/server/openapi/workflows/setters.go (1)
77-86: ⚖️ Poor tradeoffPartial enqueue failure leaves committed jobs without dispatch work items.
Jobs are persisted and committed (line 73) before enqueueing dispatch work items. If
Enqueuefails mid-loop (e.g., on the 3rd of 5 jobs), the first 2 jobs have work items but the remaining jobs do not—they're committed to the DB but won't be dispatched.Options to consider:
- Continue on error: Log failures and continue enqueueing remaining jobs (best-effort)
- Batch enqueue: Enqueue all items in a single call if the queue supports it
- Accept current behavior: Rely on eventual consistency/retry mechanisms elsewhere
If jobs without dispatch work items can be recovered through other means (e.g., periodic reconciliation), the current approach may be acceptable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/workspace-engine/svc/http/server/openapi/workflows/setters.go` around lines 77 - 86, The loop that calls s.queue.Enqueue for each job currently returns on the first Enqueue error, leaving later committed jobs without dispatch work items; change it to a best-effort approach: inside the for _, job := range jobs loop, catch Enqueue errors, log them (e.g., s.logger.Warnf or s.logger.Errorf) with context including workspaceID and job.JobId, and continue to the next job instead of returning immediately; optionally collect errors into a slice and after the loop return an aggregated error (or nil) so the caller knows some enqueues failed while still ensuring all jobs are attempted (references: s.queue.Enqueue, reconcile.EnqueueParams, jobs, workspaceID, job.JobId).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/workspace-engine/svc/http/server/openapi/workflows/workflows.go`:
- Line 105: The code currently does a silent type assertion for
inputs[resourceSelectorInputKey] into resourceSelector which masks non-string
inputs; update the handling in the function that reads inputs (look for
resourceSelectorInputKey and the inputs map usage) to first check if the key
exists, then perform a safe type assertion (val, ok :=
inputs[resourceSelectorInputKey].(string)); if the key exists but ok is false,
return a 400 bad request (or the function's equivalent validation error)
explaining that resourceSelector must be a string; if ok is true assign
resourceSelector=val and continue.
In `@e2e/tests/api/workflows.spec.ts`:
- Around line 407-543: This test provisions entities inline; refactor it to use
YAML-backed fixtures and the existing
importEntitiesFromYaml/cleanupImportedEntities helpers. Create a corresponding
.spec.yaml fixture declaring the two job-agents (prod/staging with serverUrl),
the resource-provider, the three resources, and the workflow (with jobAgents
entries using refs and the routing selector), then in the test replace the
inline PUT/POST/DELETE setup/teardown with a call to importEntitiesFromYaml to
load the fixture and capture returned ids (providerId, prodAgentId,
stagingAgentId, workflowId) and call cleanupImportedEntities in finally/after to
remove them; remove direct calls to api.PUT/POST/DELETE that create agents,
provider, resources, and workflow, and leave the run/assert logic using the
imported workflowId and agent ids. Ensure you reference importEntitiesFromYaml
and cleanupImportedEntities and the test’s routingSelector/workflow run logic
when making the move.
---
Outside diff comments:
In `@apps/api/openapi/openapi.json`:
- Around line 10409-10417: The OpenAPI contract for the create-run endpoint
exposes the new fan-out result shape but does not document the reserved
resource-selector input key on the request `inputs` schema; update the source
request schema (not the generated artifacts) to include or document the reserved
selector key for triggering resource-based fan-out (the request body `inputs`
property used by the create-run endpoints that return `WorkflowRunResult`), then
run the OpenAPI generation command to regenerate `openapi/openapi.json` and
`src/types/openapi.ts`; ensure the `inputs` schema includes a clearly named
reserved field and description so generated clients can discover the selector
contract.
---
Nitpick comments:
In `@apps/workspace-engine/svc/http/server/openapi/workflows/setters.go`:
- Around line 77-86: The loop that calls s.queue.Enqueue for each job currently
returns on the first Enqueue error, leaving later committed jobs without
dispatch work items; change it to a best-effort approach: inside the for _, job
:= range jobs loop, catch Enqueue errors, log them (e.g., s.logger.Warnf or
s.logger.Errorf) with context including workspaceID and job.JobId, and continue
to the next job instead of returning immediately; optionally collect errors into
a slice and after the loop return an aggregated error (or nil) so the caller
knows some enqueues failed while still ensuring all jobs are attempted
(references: s.queue.Enqueue, reconcile.EnqueueParams, jobs, workspaceID,
job.JobId).
In `@apps/workspace-engine/svc/http/server/openapi/workflows/workflows_test.go`:
- Around line 50-115: Combine the three tests
(TestPlanDispatches_RoutesEachResourceToItsServer,
TestPlanDispatches_NoMatchingServerYieldsNoDispatches,
TestPlanDispatches_NilResourceRunsGateOnce) into a single table-driven test that
iterates cases exercising selector-routing for planDispatches; for each case
provide name, resources (use resourceOnServer/ nil where needed), agents (use
argoAgent or constructed oapi.WorkflowJobAgent), runners map, and expected
dispatch count and any extra assertions (e.g., expect nil dispatchCtx.Resource).
In the combined test call planDispatches(ctx, base, tc.resources, tc.agents,
tc.runners), assert no error, assert len(dispatches)==tc.expectedCount, and for
routing cases verify the server routing by comparing
dispatchCtx.Resource.Config["argo"].(map[string]any)["server"] against
runner.Config["serverUrl"] as before; include the nil-resource case to assert
dispatches[0].dispatchCtx.Resource == nil. Ensure test name indicates
selector-routing table-driven style and reuse helper functions resourceOnServer,
argoAgent and planDispatches to locate relevant code.
In `@apps/workspace-engine/svc/http/server/openapi/workflows/workflows.go`:
- Line 136: The response for creating a workflow run currently returns
http.StatusOK via c.JSON(http.StatusOK, result); update the handler (the
function that calls c.JSON in workflows.go — e.g., the CreateWorkflowRun HTTP
handler) to return http.StatusCreated instead and set a Location header pointing
to the newly created resource (use the run ID from result to build the URL,
e.g., c.Header("Location", fmt.Sprintf("/workflows/%s/runs/%s", workflowID,
result.ID))) before sending the JSON body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad02eb77-0aa8-4a57-9bcd-7926edb1254c
📒 Files selected for processing (12)
apps/api/openapi/openapi.jsonapps/api/openapi/paths/workflows.jsonnetapps/api/openapi/schemas/workflows.jsonnetapps/api/src/types/openapi.tsapps/workspace-engine/svc/http/server/openapi/workflows/dispatch.goapps/workspace-engine/svc/http/server/openapi/workflows/getters.goapps/workspace-engine/svc/http/server/openapi/workflows/setters.goapps/workspace-engine/svc/http/server/openapi/workflows/workflows.goapps/workspace-engine/svc/http/server/openapi/workflows/workflows_test.goe2e/api/schema.tse2e/tests/api/workflows.spec.tspackages/workspace-engine-sdk/src/schema.ts
Comment on lines
+407
to
+543
| test("should fan a run out to one job per matched resource, routed by server", async ({ | ||
| api, | ||
| workspace, | ||
| }) => { | ||
| const suffix = faker.string.alphanumeric(8); | ||
| const prodServer = `argocd-prod-${suffix}.example.com`; | ||
| const stagingServer = `argocd-staging-${suffix}.example.com`; | ||
| const kind = `ArgoApp${suffix}`; | ||
| const routingSelector = | ||
| "resource.config.argo.server.contains(jobAgent.config.serverUrl)"; | ||
|
|
||
| // Two registered job agents, each holding one server's URL in its config — | ||
| // the selector routes each resource to the agent whose serverUrl it matches. | ||
| const prodAgentId = uuidv4(); | ||
| const stagingAgentId = uuidv4(); | ||
| for (const [jobAgentId, serverUrl] of [ | ||
| [prodAgentId, prodServer], | ||
| [stagingAgentId, stagingServer], | ||
| ] as const) { | ||
| const agentRes = await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/job-agents/{jobAgentId}", | ||
| { | ||
| params: { path: { workspaceId: workspace.id, jobAgentId } }, | ||
| body: { | ||
| name: `argo-${jobAgentId.slice(0, 8)}`, | ||
| type: "argo-cd", | ||
| config: { serverUrl }, | ||
| }, | ||
| }, | ||
| ); | ||
| expect(agentRes.response.status).toBe(202); | ||
| } | ||
|
|
||
| const providerName = `wf-fanout-${suffix}`; | ||
| const providerRes = await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/resource-providers", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { id: uuidv4(), name: providerName }, | ||
| }, | ||
| ); | ||
| expect(providerRes.response.status).toBe(202); | ||
| const providerId = providerRes.data!.id; | ||
|
|
||
| let workflowId: string | undefined; | ||
| try { | ||
| const resource = (label: string, server: string) => ({ | ||
| createdAt: new Date().toISOString(), | ||
| identifier: `${kind}-${label}-${suffix}`, | ||
| name: `${kind}-${label}`, | ||
| kind, | ||
| version: "1.0.0", | ||
| config: { argo: { server: `https://${server}` } }, | ||
| metadata: {}, | ||
| }); | ||
| const setRes = await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/resource-providers/{providerId}/set", | ||
| { | ||
| params: { path: { workspaceId: workspace.id, providerId } }, | ||
| body: { | ||
| resources: [ | ||
| resource("a", prodServer), | ||
| resource("b", prodServer), | ||
| resource("c", stagingServer), | ||
| ], | ||
| }, | ||
| }, | ||
| ); | ||
| expect(setRes.response.status).toBe(202); | ||
|
|
||
| // Barrier: ensure the three resources are queryable before the run. | ||
| const listRes = await api.GET( | ||
| "/v1/workspaces/{workspaceId}/resource-providers/name/{name}/resources", | ||
| { params: { path: { workspaceId: workspace.id, name: providerName } } }, | ||
| ); | ||
| expect(listRes.data!.items).toHaveLength(3); | ||
|
|
||
| const createRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/workflows", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { | ||
| name: `Delete Argo ${suffix}`, | ||
| inputs: [ | ||
| { | ||
| key: "resourceSelector", | ||
| type: "string", | ||
| default: `resource.kind == '${kind}'`, | ||
| }, | ||
| ], | ||
| jobAgents: [ | ||
| { name: "prod", ref: prodAgentId, config: {}, selector: routingSelector }, | ||
| { name: "staging", ref: stagingAgentId, config: {}, selector: routingSelector }, | ||
| ], | ||
| }, | ||
| }, | ||
| ); | ||
| expect(createRes.response.status).toBe(201); | ||
| workflowId = createRes.data!.id; | ||
|
|
||
| const runRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/workflows/{workflowId}/runs", | ||
| { | ||
| params: { path: { workspaceId: workspace.id, workflowId } }, | ||
| body: { inputs: {} }, | ||
| }, | ||
| ); | ||
| expect(runRes.response.status).toBe(201); | ||
|
|
||
| const jobs = runRes.data!.jobs; | ||
| expect(jobs).toHaveLength(3); // one per matched resource | ||
|
|
||
| const jobsPerAgent = jobs.reduce<Record<string, number>>((acc, job) => { | ||
| acc[job.jobAgentId] = (acc[job.jobAgentId] ?? 0) + 1; | ||
| return acc; | ||
| }, {}); | ||
| expect(jobsPerAgent[prodAgentId]).toBe(2); // two prod resources | ||
| expect(jobsPerAgent[stagingAgentId]).toBe(1); // one staging resource | ||
| } finally { | ||
| if (workflowId != null) | ||
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/workflows/{workflowId}", | ||
| { params: { path: { workspaceId: workspace.id, workflowId } } }, | ||
| ); | ||
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/resource-providers/{providerId}/set", | ||
| { | ||
| params: { path: { workspaceId: workspace.id, providerId } }, | ||
| body: { resources: [] }, | ||
| }, | ||
| ); | ||
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/resource-providers/name/{name}", | ||
| { params: { path: { workspaceId: workspace.id, name: providerName } } }, | ||
| ); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move this fan-out scenario to YAML-backed E2E fixtures.
This new E2E case provisions entities inline instead of using the required .spec.yaml + importEntitiesFromYaml / cleanupImportedEntities flow. Please migrate this scenario to fixture-backed setup/teardown so it matches the suite’s standard and remains easier to maintain.
As per coding guidelines: “E2E tests use YAML fixture files (.spec.yaml alongside .spec.ts) to declare test entities; use importEntitiesFromYaml to load them and cleanupImportedEntities to tear them down.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/tests/api/workflows.spec.ts` around lines 407 - 543, This test provisions
entities inline; refactor it to use YAML-backed fixtures and the existing
importEntitiesFromYaml/cleanupImportedEntities helpers. Create a corresponding
.spec.yaml fixture declaring the two job-agents (prod/staging with serverUrl),
the resource-provider, the three resources, and the workflow (with jobAgents
entries using refs and the routing selector), then in the test replace the
inline PUT/POST/DELETE setup/teardown with a call to importEntitiesFromYaml to
load the fixture and capture returned ids (providerId, prodAgentId,
stagingAgentId, workflowId) and call cleanupImportedEntities in finally/after to
remove them; remove direct calls to api.PUT/POST/DELETE that create agents,
provider, resources, and workflow, and leave the run/assert logic using the
imported workflowId and agent ids. Ensure you reference importEntitiesFromYaml
and cleanupImportedEntities and the test’s routingSelector/workflow run logic
when making the move.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/tests/api/workflows.spec.ts (1)
522-658:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up job agents in the finally block.
The test creates two job agents (lines 537-553) but the cleanup logic (lines 640-657) only removes the workflow and resource provider. Add DELETE calls for both
prodAgentIdandstagingAgentIdto prevent test data accumulation.🧹 Proposed cleanup addition
} finally { if (workflowId != null) await api.DELETE( "/v1/workspaces/{workspaceId}/workflows/{workflowId}", { params: { path: { workspaceId: workspace.id, workflowId } } }, ); + await Promise.all([ + api.DELETE( + "/v1/workspaces/{workspaceId}/job-agents/{jobAgentId}", + { params: { path: { workspaceId: workspace.id, jobAgentId: prodAgentId } } }, + ), + api.DELETE( + "/v1/workspaces/{workspaceId}/job-agents/{jobAgentId}", + { params: { path: { workspaceId: workspace.id, jobAgentId: stagingAgentId } } }, + ), + ]); await api.PUT(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/tests/api/workflows.spec.ts` around lines 522 - 658, The finally block is not deleting the two job agents created earlier (prodAgentId and stagingAgentId); add API DELETE calls to remove both agents there by calling the same endpoint pattern used for other cleanup and passing workspace.id and each jobAgentId (prodAgentId and stagingAgentId) as path params so the job agents are removed after the test; ensure these DELETEs run before or alongside the existing resource-provider cleanup to avoid leaking test data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@e2e/tests/api/workflows.spec.ts`:
- Around line 522-658: The finally block is not deleting the two job agents
created earlier (prodAgentId and stagingAgentId); add API DELETE calls to remove
both agents there by calling the same endpoint pattern used for other cleanup
and passing workspace.id and each jobAgentId (prodAgentId and stagingAgentId) as
path params so the job agents are removed after the test; ensure these DELETEs
run before or alongside the existing resource-provider cleanup to avoid leaking
test data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43d5f128-ec1a-47b6-9eab-fc57ff34d9ca
📒 Files selected for processing (4)
apps/api/src/routes/v1/workspaces/workflows.tsapps/workspace-engine/svc/http/server/openapi/workflows/getters.goapps/workspace-engine/svc/http/server/openapi/workflows/workflows.goe2e/tests/api/workflows.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/workspace-engine/svc/http/server/openapi/workflows/workflows.go
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
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
API Changes
Validation
Tests