From c9ad3a661a238e0bfd76677f2fa23c3502daa0e7 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Fri, 6 Feb 2026 14:09:06 -0600 Subject: [PATCH] fix(CQ-ORCH-9): Deduplicate spawn validation logic 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 --- .../src/api/agents/agents.controller.spec.ts | 75 ------------------- .../src/api/agents/agents.controller.ts | 34 +-------- 2 files changed, 3 insertions(+), 106 deletions(-) diff --git a/apps/orchestrator/src/api/agents/agents.controller.spec.ts b/apps/orchestrator/src/api/agents/agents.controller.spec.ts index bd4d7ad..c63f8b6 100644 --- a/apps/orchestrator/src/api/agents/agents.controller.spec.ts +++ b/apps/orchestrator/src/api/agents/agents.controller.spec.ts @@ -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"); diff --git a/apps/orchestrator/src/api/agents/agents.controller.ts b/apps/orchestrator/src/api/agents/agents.controller.ts index fb46d7b..1d54ea9 100644 --- a/apps/orchestrator/src/api/agents/agents.controller.ts +++ b/apps/orchestrator/src/api/agents/agents.controller.ts @@ -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"); - } - } }