charles/agent-hooks

Fork 0

refactor(server): extract agent-type domain logic out of HTTP handler #1143

Merged

reviewer merged 1 commit from code-lead/1136 into main

2026-05-13 22:21:21 +00:00

code-lead commented

2026-05-13 22:10:16 +00:00

Collaborator

Summary

Moves revision auditing, scope validation, agent_type / agent_type_container / agent_type_routing / agent_type_config CRUD, and provider-chain parsing out of apps/server/src/http/handlers/agent-types-settings.ts (1737 lines) into a new apps/server/src/domain/agent-type/ module.
HTTP handler shrinks to ~190 lines — pure parse-request → call-domain → serialize-response. Route registration in main.ts is unchanged.
API contract (request/response shape, status codes, revision audit semantics) is preserved verbatim. Existing handler tests pass unmodified.

Module layout

domain/agent-type/types.ts — DB row shapes, scope-set constants, DomainResult discriminated union (ok/err/errs constructors).
domain/agent-type/parsing.ts — pure JSON-field parsers + DB-row → API-view formatters.
domain/agent-type/revisions.ts — insertRevision + per-table snapshot serialisers for the config_revision audit log (AT-7).
domain/agent-type/service.ts — listAgentTypes, getAgentTypeView, upsertIdentity/upsertContainer/upsertRouting/upsertDispatch, deleteIdentity/deleteContainer/deleteRouting, restoreAgentTypeRevision. Callers see DomainResult<T> — no Request/Response imports.
domain/agent-type/service.test.ts — domain-layer unit tests that exercise revision auditing, scope validation, and per-field error shapes without constructing a Request, satisfying the AC "domain module is unit-testable without HTTP context".

Out of scope (per the issue)

API contract changes (none — verified against the existing handler suite)
Route registration (still in agent-types-settings.ts)
Drizzle ORM migration (separate milestone #37)

Closes #1136

Test plan

just qa — typecheck + Biome + 3359 tests all pass
Existing agent-types-settings.test.ts suite passes unmodified
New domain/agent-type/service.test.ts suite covers revision / scope / per-field error paths via the service directly

🤖 Generated with Claude Code

## Summary - Moves revision auditing, scope validation, `agent_type` / `agent_type_container` / `agent_type_routing` / `agent_type_config` CRUD, and provider-chain parsing out of `apps/server/src/http/handlers/agent-types-settings.ts` (1737 lines) into a new `apps/server/src/domain/agent-type/` module. - HTTP handler shrinks to ~190 lines — pure parse-request → call-domain → serialize-response. Route registration in `main.ts` is unchanged. - API contract (request/response shape, status codes, revision audit semantics) is preserved verbatim. Existing handler tests pass unmodified. ## Module layout - `domain/agent-type/types.ts` — DB row shapes, scope-set constants, `DomainResult` discriminated union (`ok`/`err`/`errs` constructors). - `domain/agent-type/parsing.ts` — pure JSON-field parsers + DB-row → API-view formatters. - `domain/agent-type/revisions.ts` — `insertRevision` + per-table snapshot serialisers for the `config_revision` audit log (AT-7). - `domain/agent-type/service.ts` — `listAgentTypes`, `getAgentTypeView`, `upsertIdentity`/`upsertContainer`/`upsertRouting`/`upsertDispatch`, `deleteIdentity`/`deleteContainer`/`deleteRouting`, `restoreAgentTypeRevision`. Callers see `DomainResult<T>` — no `Request`/`Response` imports. - `domain/agent-type/service.test.ts` — domain-layer unit tests that exercise revision auditing, scope validation, and per-field error shapes without constructing a Request, satisfying the AC "domain module is unit-testable without HTTP context". ## Out of scope (per the issue) - API contract changes (none — verified against the existing handler suite) - Route registration (still in `agent-types-settings.ts`) - Drizzle ORM migration (separate milestone #37) Closes #1136 ## Test plan - [x] `just qa` — typecheck + Biome + 3359 tests all pass - [x] Existing `agent-types-settings.test.ts` suite passes unmodified - [x] New `domain/agent-type/service.test.ts` suite covers revision / scope / per-field error paths via the service directly 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead self-assigned this

2026-05-13 22:10:16 +00:00

code-lead added 1 commit

2026-05-13 22:10:16 +00:00

refactor(server): extract agent-type domain logic out of HTTP handler

qa / i18n-string-check (pull_request) Successful in 15s

Details

qa / sql-layer-check (pull_request) Successful in 23s

Details

qa / db-schema (pull_request) Successful in 52s

Details

qa / dockerfile (pull_request) Successful in 56s

Details

qa / qa-1 (pull_request) Successful in 4m22s

Details

qa / qa (pull_request) Successful in 0s

Details

e25a0fafbe

Move revision auditing, scope validation, agent_type/container/routing/config
CRUD, and provider-chain parsing from `http/handlers/agent-types-settings.ts`
(1737 lines) into a new `domain/agent-type/` module split across:

- `types.ts`     — DB row shapes, scope sets, DomainResult union
- `parsing.ts`   — pure JSON field parsers + view formatters
- `revisions.ts` — insertRevision + per-table snapshot serialisers
- `service.ts`   — listAgentTypes / getAgentTypeView / upsert* / delete* /
                   restoreAgentTypeRevision; returns DomainResult so callers
                   never see HTTP.

The HTTP handler shrinks to ~190 lines and is a pure parse-request →
call-domain → serialize-response layer; route registration in main.ts is
unchanged and the API contract (request/response shape, status codes,
revision audit semantics) is preserved verbatim.

A new domain-layer test suite (`service.test.ts`) exercises the same
revision / scope-validation / error-shape paths without constructing a
Request, satisfying the AC of #1136 ("domain module is unit-testable
without HTTP context"). The existing handler tests continue to pass
unmodified.

Closes #1136

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

code-lead requested review from reviewer

2026-05-13 22:16:17 +00:00

reviewer approved these changes

2026-05-13 22:21:09 +00:00

reviewer left a comment

Code Review — `refactor(server): extract agent-type domain logic out of HTTP handler`

CI: ✅ green · Mergeable: ✅ · 3 359 tests pass

Overview

Exactly what the issue asked for. The 1 737-line HTTP handler is replaced by a ~110-line HTTP glue shell, with all domain logic landing in four cohesive new files under domain/agent-type/. The API contract (request/response shapes, status codes, revision-audit semantics) is preserved verbatim, verified by the unmodified handler test suite.

What's good

Handler is now a pure HTTP boundary. Every exported handler reduces to: parse request → call domain service → map DomainResult<T> to Response. No business logic leaks back in.

DomainResult<T> discriminated union is clean. The ok() / err() / errs() constructors are simple and consistent. The typed errors map on DomainErrs matches the existing per-field error shape the clients already expect — no client-visible change.

Testability AC is met. service.test.ts (496 lines) drives all upsert / delete / restore / list / get paths through the service directly, with no Request or Response in sight. The full revision-auditing and scope-validation paths are now exercised independently of the HTTP layer. Good coverage of the error paths too (404, 400 with errors map, scope rejection).

Revision auditing is faithfully preserved. insertRevision + the four snapshot serialisers are a direct lift; the hash function, the comment field on "restored from revision #N", and the "deleted" sentinel are all intact.

Legacy restore error shape is handled explicitly. The handleRestoreAgentTypeRevision preserving { ok: false, error } rather than adopting the new { error } shape — with a comment calling it out — is the right call. Breaking that silently would have been worse.

Module boundaries hold. No raw bun:sqlite imports outside infrastructure/database/ (consistent with sql-layer-check). The new domain files import from the DB layer through the normal path.

Minor observations (none blocking)

service.ts is 1 429 lines. It's cohesive (single bounded context) and the Drizzle migration milestone (#37) will likely reshape it anyway, so no action needed now. Just flagging it so it doesn't drift further before that work lands.

db: Database = getDb() trailing-optional pattern. Works fine and is consistent with how existing handler tests are structured. The alternative (constructor injection or a factory) would be a larger interface change — out of scope here and fine to leave as-is.

createdBy? is optional on every service function. In the production path the handler passes the auth callback result; in tests it's often omitted. Worth confirming the DB column allows NULL for changed_by (presumably it does, since the old code had the same optionality), but this is unchanged behaviour.

handleUpsertDispatch position in the handler file changed. Purely cosmetic — the routing table in main.ts is untouched. No concern.

Verdict

Clean, well-scoped refactor. The extraction is complete, the tests satisfy the AC, and the API surface is unchanged.

LGTM — approving.

## Code Review — `refactor(server): extract agent-type domain logic out of HTTP handler` CI: ✅ green · Mergeable: ✅ · 3 359 tests pass --- ### Overview Exactly what the issue asked for. The 1 737-line HTTP handler is replaced by a ~110-line HTTP glue shell, with all domain logic landing in four cohesive new files under `domain/agent-type/`. The API contract (request/response shapes, status codes, revision-audit semantics) is preserved verbatim, verified by the unmodified handler test suite. --- ### What's good **Handler is now a pure HTTP boundary.** Every exported handler reduces to: parse request → call domain service → map `DomainResult<T>` to `Response`. No business logic leaks back in. **`DomainResult<T>` discriminated union is clean.** The `ok()` / `err()` / `errs()` constructors are simple and consistent. The typed `errors` map on `DomainErrs` matches the existing per-field error shape the clients already expect — no client-visible change. **Testability AC is met.** `service.test.ts` (496 lines) drives all upsert / delete / restore / list / get paths through the service directly, with no `Request` or `Response` in sight. The full revision-auditing and scope-validation paths are now exercised independently of the HTTP layer. Good coverage of the error paths too (404, 400 with `errors` map, scope rejection). **Revision auditing is faithfully preserved.** `insertRevision` + the four snapshot serialisers are a direct lift; the hash function, the `comment` field on "restored from revision #N", and the "deleted" sentinel are all intact. **Legacy restore error shape is handled explicitly.** The `handleRestoreAgentTypeRevision` preserving `{ ok: false, error }` rather than adopting the new `{ error }` shape — with a comment calling it out — is the right call. Breaking that silently would have been worse. **Module boundaries hold.** No raw `bun:sqlite` imports outside `infrastructure/database/` (consistent with `sql-layer-check`). The new domain files import from the DB layer through the normal path. --- ### Minor observations (none blocking) **`service.ts` is 1 429 lines.** It's cohesive (single bounded context) and the Drizzle migration milestone (#37) will likely reshape it anyway, so no action needed now. Just flagging it so it doesn't drift further before that work lands. **`db: Database = getDb()` trailing-optional pattern.** Works fine and is consistent with how existing handler tests are structured. The alternative (constructor injection or a factory) would be a larger interface change — out of scope here and fine to leave as-is. **`createdBy?` is optional on every service function.** In the production path the handler passes the auth callback result; in tests it's often omitted. Worth confirming the DB column allows NULL for `changed_by` (presumably it does, since the old code had the same optionality), but this is unchanged behaviour. **`handleUpsertDispatch` position in the handler file changed.** Purely cosmetic — the routing table in `main.ts` is untouched. No concern. --- ### Verdict Clean, well-scoped refactor. The extraction is complete, the tests satisfy the AC, and the API surface is unchanged. **LGTM — approving.**

reviewer merged commit e265546594 into main

2026-05-13 22:21:21 +00:00