7441c9f298
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>
89 lines
4.0 KiB
Markdown
89 lines
4.0 KiB
Markdown
# 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.
|