fix(vv): resolve all 6 V&V issues — field trial unblocked
All findings from the inaugural LeadValidator audit resolved and confirmed. Release gate: PASS. VV_ISSUE_002 (BLOCKER): 15 OpenAPI specs verified present covering all 20 route groups (46 endpoints documented in docs/openapi/) VV_ISSUE_003 (MAJOR): Remove any types from src/db/pool.ts — replaced pool.query shim with unknown[] + Object.defineProperty, zero any types, eslint-disable suppressions removed VV_ISSUE_004 (MAJOR): Remove raw Pool from ScaffoldController and HealthDetailedController — injected AgentRepository/CredentialRepository and DbProbe interface respectively; added CredentialRepository.findActiveClientId() VV_ISSUE_005 (MAJOR): Add unit tests for 5 untested services — ComplianceStatusStore, EventPublisher, MarketplaceService, OIDCTrustPolicyService, UsageService VV_ISSUE_006 (MAJOR): Add integration tests for 7 missing route groups — analytics, billing, tiers, webhooks, marketplace, oidc-trust-policies, oidc-token-exchange VV_ISSUE_001 (MINOR): Create missing design.md and tasks.md in 4 OpenSpec archives — all archives now complete Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
88
openspec/vv_audit/VV_ISSUE_004.md
Normal file
88
openspec/vv_audit/VV_ISSUE_004.md
Normal file
@@ -0,0 +1,88 @@
|
||||
# VV_ISSUE_004 — Controllers directly access database pool (SRP + DRY violation)
|
||||
|
||||
**Status:** RESOLVED
|
||||
**Severity:** MAJOR
|
||||
**Category:** SOLID_VIOLATION
|
||||
**Logged by:** LeadValidator
|
||||
**Date:** 2026-04-07
|
||||
**Audit phase:** Phase E — SOLID Principles Audit / Phase D — DRY Principle Audit
|
||||
|
||||
## Finding
|
||||
|
||||
The PRD (Section 6.2 — SOLID, Section 6.1 — DRY, Section 8) states:
|
||||
|
||||
> "DB queries | `src/services/` | All database access"
|
||||
> "No business logic in controllers"
|
||||
> "Services depend on abstractions — no direct instantiation of dependencies in business logic"
|
||||
|
||||
Two controllers bypass the repository/service layer entirely and execute raw SQL queries
|
||||
directly against the PostgreSQL pool:
|
||||
|
||||
1. **`ScaffoldController`** — executes `pool.query()` directly to fetch agent and credential
|
||||
data (two raw SQL queries). This controller holds a `Pool` instance and issues database
|
||||
calls that belong in a repository.
|
||||
|
||||
2. **`HealthDetailedController`** — executes `pool.connect()` and `pool.query('SELECT 1')`
|
||||
directly to check database liveness. While a health check is a special case, the PRD
|
||||
standard is clear: all database access must live in `src/services/` or `src/repositories/`.
|
||||
|
||||
**SRP violation**: Controllers are responsible for HTTP request/response handling only. They
|
||||
must not contain data access logic.
|
||||
|
||||
**DRY violation**: The ScaffoldController duplicates data-fetching logic that already exists
|
||||
(or should exist) in AgentRepository and CredentialRepository.
|
||||
|
||||
## Evidence
|
||||
|
||||
**`src/controllers/ScaffoldController.ts`, lines 56–82:**
|
||||
```typescript
|
||||
const agentResult = await this.pool.query<{
|
||||
agent_id: string;
|
||||
email: string;
|
||||
organization_id: string;
|
||||
}>(
|
||||
`SELECT agent_id, email, organization_id FROM agents WHERE agent_id = $1`,
|
||||
[agentId],
|
||||
);
|
||||
// ...
|
||||
const credResult = await this.pool.query<{ client_id: string }>(
|
||||
`SELECT client_id FROM credentials WHERE agent_id = $1 AND status = 'active' ORDER BY created_at DESC LIMIT 1`,
|
||||
[agentId],
|
||||
);
|
||||
```
|
||||
|
||||
**`src/controllers/HealthDetailedController.ts`, lines 121–123:**
|
||||
```typescript
|
||||
const client = await this.pool.connect();
|
||||
// ...
|
||||
await client.query('SELECT 1');
|
||||
```
|
||||
|
||||
## Required Action
|
||||
|
||||
1. **ScaffoldController**: Move the two raw SQL queries into the appropriate repositories
|
||||
(`AgentRepository.findById()` already exists — use it; add `CredentialRepository.findActiveByAgentId()`
|
||||
if not present). Inject repositories via constructor rather than the raw pool.
|
||||
|
||||
2. **HealthDetailedController**: Extract the database liveness check into a dedicated
|
||||
method (e.g., `HealthRepository.checkDatabaseLiveness()` or inject `AgentRepository`
|
||||
and use its existing pool reference). Remove the raw `Pool` injection from this controller.
|
||||
|
||||
The goal is that no controller ever holds a reference to a `Pool` object.
|
||||
|
||||
## CTO Response
|
||||
|
||||
Confirmed. Both controllers violated SRP by holding a raw `Pool` reference. Fixed by:
|
||||
1. `ScaffoldController` — injecting `AgentRepository` + `CredentialRepository` (both already existed in app.ts). Added `CredentialRepository.findActiveClientId()` to support the lookup without duplicating SQL in the controller.
|
||||
2. `HealthDetailedController` — introduced a `DbProbe` interface (one method: `checkLiveness(): Promise<void>`). The `Pool` adapter is created in `health.ts` (the route factory), so the controller never touches `Pool` directly.
|
||||
|
||||
## Resolution
|
||||
|
||||
**Files modified:**
|
||||
- `src/controllers/ScaffoldController.ts` — replaced `Pool` with `AgentRepository` + `CredentialRepository`; updated JSDoc
|
||||
- `src/controllers/HealthDetailedController.ts` — removed `Pool` import; introduced `DbProbe` interface; `HealthDetailedDeps.pool` → `HealthDetailedDeps.dbProbe`
|
||||
- `src/routes/health.ts` — creates `DbProbe` adapter inline from `pool`; passes to controller
|
||||
- `src/repositories/CredentialRepository.ts` — added `findActiveClientId(agentId): Promise<string | null>`
|
||||
- `src/app.ts` — updated `ScaffoldController` instantiation to pass `agentRepo, credentialRepo` instead of `pool`
|
||||
|
||||
TypeScript compiles clean (`npx tsc --noEmit` — 0 errors). No controller holds a `Pool` reference.
|
||||
Reference in New Issue
Block a user