mosaic/stack
1
0
Fork 0
Code Issues 10 Pull Requests Actions Packages Projects Releases 2 Wiki Activity

fix(CQ-ORCH-9): Deduplicate spawn validation logic
Some checks failed
ci/woodpecker/push/woodpecker Pipeline failed
Details

Browse Source 
Remove duplicate validateSpawnRequest from AgentsController. Validation
is now handled exclusively by:
1. ValidationPipe + DTO decorators (HTTP layer, class-validator)
2. AgentSpawnerService.validateSpawnRequest (business logic layer)

This eliminates the maintenance burden and divergence risk of having
identical validation in two places. Controller tests for the removed
duplicate validation are also removed since they are fully covered by
the service tests and DTO validation decorators.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Jason Woltje
2026-02-06 14:09:06 -06:00
parent a0062494b7
commit c9ad3a661a
2 changed files with 3 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files

75
apps/orchestrator/src/api/agents/agents.controller.spec.ts
View File

@@ -3,7 +3,6 @@ import { QueueService } from "../../queue/queue.service";
import { AgentSpawnerService } from "../../spawner/agent-spawner.service";
import { AgentLifecycleService } from "../../spawner/agent-lifecycle.service";
import { KillswitchService } from "../../killswitch/killswitch.service";
import { BadRequestException } from "@nestjs/common";
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
describe("AgentsController", () => {
@@ -289,80 +288,6 @@ describe("AgentsController", () => {
expect(result.agentId).toBe(agentId);
});
it("should throw BadRequestException when taskId is missing", async () => {
// Arrange
const invalidRequest = {
agentType: "worker" as const,
context: validRequest.context,
} as unknown as typeof validRequest;
// Act & Assert
await expect(controller.spawn(invalidRequest)).rejects.toThrow(BadRequestException);
expect(spawnerService.spawnAgent).not.toHaveBeenCalled();
expect(queueService.addTask).not.toHaveBeenCalled();
});
it("should throw BadRequestException when agentType is invalid", async () => {
// Arrange
const invalidRequest = {
...validRequest,
agentType: "invalid" as unknown as "worker",
};
// Act & Assert
await expect(controller.spawn(invalidRequest)).rejects.toThrow(BadRequestException);
expect(spawnerService.spawnAgent).not.toHaveBeenCalled();
expect(queueService.addTask).not.toHaveBeenCalled();
});
it("should throw BadRequestException when repository is missing", async () => {
// Arrange
const invalidRequest = {
...validRequest,
context: {
...validRequest.context,
repository: "",
},
};
// Act & Assert
await expect(controller.spawn(invalidRequest)).rejects.toThrow(BadRequestException);
expect(spawnerService.spawnAgent).not.toHaveBeenCalled();
expect(queueService.addTask).not.toHaveBeenCalled();
});
it("should throw BadRequestException when branch is missing", async () => {
// Arrange
const invalidRequest = {
...validRequest,
context: {
...validRequest.context,
branch: "",
},
};
// Act & Assert
await expect(controller.spawn(invalidRequest)).rejects.toThrow(BadRequestException);
expect(spawnerService.spawnAgent).not.toHaveBeenCalled();
expect(queueService.addTask).not.toHaveBeenCalled();
});
it("should throw BadRequestException when workItems is empty", async () => {
// Arrange
const invalidRequest = {
...validRequest,
context: {
...validRequest.context,
workItems: [],
},
};
// Act & Assert
await expect(controller.spawn(invalidRequest)).rejects.toThrow(BadRequestException);
expect(spawnerService.spawnAgent).not.toHaveBeenCalled();
expect(queueService.addTask).not.toHaveBeenCalled();
});
it("should propagate errors from spawner service", async () => {
// Arrange
const error = new Error("Spawner failed");

34
apps/orchestrator/src/api/agents/agents.controller.ts
View File

@@ -4,7 +4,6 @@ import {
Get,
Body,
Param,
BadRequestException,
NotFoundException,
Logger,
UsePipes,
@@ -57,8 +56,9 @@ export class AgentsController {
this.logger.log(`Received spawn request for task: ${dto.taskId}`);
try {
// Validate request manually (in addition to ValidationPipe)
this.validateSpawnRequest(dto);
// Validation is handled by:
// 1. ValidationPipe + DTO decorators at the HTTP layer
// 2. AgentSpawnerService.validateSpawnRequest for business logic
// Spawn agent using spawner service
const spawnResponse = this.spawnerService.spawnAgent({
@@ -243,32 +243,4 @@ export class AgentsController {
throw error;
}
}
/**
* Validate spawn request
* @param dto Spawn request to validate
* @throws BadRequestException if validation fails
*/
private validateSpawnRequest(dto: SpawnAgentDto): void {
if (!dto.taskId || dto.taskId.trim() === "") {
throw new BadRequestException("taskId is required");
}
const validAgentTypes = ["worker", "reviewer", "tester"];
if (!validAgentTypes.includes(dto.agentType)) {
throw new BadRequestException(`agentType must be one of: ${validAgentTypes.join(", ")}`);
}
if (!dto.context.repository || dto.context.repository.trim() === "") {
throw new BadRequestException("context.repository is required");
}
if (!dto.context.branch || dto.context.branch.trim() === "") {
throw new BadRequestException("context.branch is required");
}
if (dto.context.workItems.length === 0) {
throw new BadRequestException("context.workItems must not be empty");
}
}
}