charles/agent-hooks

Fork 0

refactor(architect): make docker-exec relay mandatory in runArchitectTurn (AR-3) #1204

Merged

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

2026-05-15 13:16:21 +00:00

code-lead commented

2026-05-15 13:08:43 +00:00

Collaborator

Closes #1198.

Summary

AR-3 closes the in-process SDK code path that pre-#1132 served /architect/chat and /architect/spec. The relay was wired in #1132 by having ArchitectAgentDispatch pass a DockerExecClaudeAgent into runArchitectTurn explicitly, but the runner still defaulted to SdkClaudeAgent when called without an explicit claudeAgent — an in-process escape hatch that no production caller used but that any future caller could trip into.

This PR moves the construction into runArchitectTurn's default so the relay is enforced at the runner boundary itself:

runArchitectTurn now defaults claudeAgent to new DockerExecClaudeAgent({ agentName }) (was new SdkClaudeAgent()). The in-process SDK import is gone from the architect runner.
ArchitectAgentDispatch drops the redundant explicit claudeAgent construction — the runner is now the single source of truth for the relay.
Module + dispatch-port JSDoc updated to reflect "relay is mandatory" instead of "inline SDK".

Acceptance criteria

/architect/chat is rewired to docker exec into claude-hooks-architect-default. Handler input schema + SSE stream shape unchanged — the change is the runner's default ClaudeAgentPort.
/architect/spec (file write / breakdown-preview) is unchanged — those handlers route through forge.writeFile / forge.readFile, never through the SDK; the relay was already complete on that side since #1133.
The in-process SDK code path that served these endpoints is deleted entirely — no if (host_mode) branch in the chat/spec handler, no SdkClaudeAgent fallback in runArchitectTurn.
Reconcile contract is unchanged: reconcileOne("architect-default") still works exactly like every other agent.
No role === "host" read remains in any chat / spec handler (verified by grep against apps/server/src/http/handlers/architect.ts + apps/server/src/domain/agent/architect.ts).

Tests

Three new structural guards in apps/server/src/http/handlers/architect.test.ts under AR-3 (#1198) — in-process SDK path is deleted:

domain/agent/architect.ts must not import SdkClaudeAgent.
domain/agent/architect.ts must import DockerExecClaudeAgent (the new default).
infrastructure/agents/architect-agent-dispatch.ts must not construct DockerExecClaudeAgent itself — the runner owns that.

Existing coverage for the relay shape (docker-exec-adapter.test.ts) and dispatch routing (agent-dispatch-port.test.ts) is unchanged.

Test plan

bun test apps/server/src — 3357 → 3360 pass, 0 fail (3 new structural tests + all existing pass).
just qa — typecheck + lint + format + sql-layer + paraglide + i18n-string + flow-schema all green.

Out of scope

Renaming role: "host" → "planner" (#1199).
Deleting HOST_MODE_TYPES / HostContainerLifecycle / getArchitectWorker host filter (#1200).

🤖 Generated with Claude Code

Closes #1198. ## Summary AR-3 closes the in-process SDK code path that pre-#1132 served `/architect/chat` and `/architect/spec`. The relay was wired in #1132 by having `ArchitectAgentDispatch` pass a `DockerExecClaudeAgent` into `runArchitectTurn` explicitly, but the runner still defaulted to `SdkClaudeAgent` when called without an explicit `claudeAgent` — an in-process escape hatch that no production caller used but that any future caller could trip into. This PR moves the construction into `runArchitectTurn`'s default so the relay is enforced at the runner boundary itself: - `runArchitectTurn` now defaults `claudeAgent` to `new DockerExecClaudeAgent({ agentName })` (was `new SdkClaudeAgent()`). The in-process SDK import is gone from the architect runner. - `ArchitectAgentDispatch` drops the redundant explicit `claudeAgent` construction — the runner is now the single source of truth for the relay. - Module + dispatch-port JSDoc updated to reflect "relay is mandatory" instead of "inline SDK". ## Acceptance criteria - [x] `/architect/chat` is rewired to `docker exec` into `claude-hooks-architect-default`. Handler input schema + SSE stream shape unchanged — the change is the runner's default `ClaudeAgentPort`. - [x] `/architect/spec` (file write / breakdown-preview) is unchanged — those handlers route through `forge.writeFile` / `forge.readFile`, never through the SDK; the relay was already complete on that side since #1133. - [x] The in-process SDK code path that served these endpoints is deleted entirely — no `if (host_mode)` branch in the chat/spec handler, no `SdkClaudeAgent` fallback in `runArchitectTurn`. - [x] Reconcile contract is unchanged: `reconcileOne("architect-default")` still works exactly like every other agent. - [x] No `role === "host"` read remains in any chat / spec handler (verified by grep against `apps/server/src/http/handlers/architect.ts` + `apps/server/src/domain/agent/architect.ts`). ## Tests Three new structural guards in `apps/server/src/http/handlers/architect.test.ts` under `AR-3 (#1198) — in-process SDK path is deleted`: 1. `domain/agent/architect.ts` must not import `SdkClaudeAgent`. 2. `domain/agent/architect.ts` must import `DockerExecClaudeAgent` (the new default). 3. `infrastructure/agents/architect-agent-dispatch.ts` must not construct `DockerExecClaudeAgent` itself — the runner owns that. Existing coverage for the relay shape (`docker-exec-adapter.test.ts`) and dispatch routing (`agent-dispatch-port.test.ts`) is unchanged. ## Test plan - [x] `bun test apps/server/src` — 3357 → 3360 pass, 0 fail (3 new structural tests + all existing pass). - [x] `just qa` — typecheck + lint + format + sql-layer + paraglide + i18n-string + flow-schema all green. ## Out of scope - Renaming `role: "host"` → `"planner"` (#1199). - Deleting `HOST_MODE_TYPES` / `HostContainerLifecycle` / `getArchitectWorker` host filter (#1200). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

code-lead added 1 commit

2026-05-15 13:08:43 +00:00

refactor(architect): make docker-exec relay mandatory in runArchitectTurn (#1198 )

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

Details

qa / dockerfile (pull_request) Successful in 11s

Details

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

Details

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

Details

qa / qa-1 (pull_request) Successful in 3m41s

Details

qa / qa (pull_request) Successful in 0s

Details

93a807abc0

AR-3 closes the in-process SDK code path that pre-#1132 served
/architect/chat and /architect/spec. The relay was wired in #1132 by
having ArchitectAgentDispatch pass a DockerExecClaudeAgent into
runArchitectTurn explicitly, but the runner still defaulted to
SdkClaudeAgent when called without an explicit `claudeAgent` — an
in-process escape hatch that no production caller used but that future
callers could trip into.

Move the construction into runArchitectTurn's default so the relay is
enforced at the runner boundary, drop the now-redundant explicit
construction from ArchitectAgentDispatch, and add structural guards in
architect.test.ts that fail if either half of the invariant regresses
(SdkClaudeAgent import re-introduced into the architect runner, or
DockerExecClaudeAgent construction reappearing in the dispatch adapter).

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

code-lead requested review from reviewer

2026-05-15 13:14:45 +00:00

reviewer approved these changes

2026-05-15 13:16:09 +00:00

reviewer left a comment

Review — AR-3: make docker-exec relay mandatory in `runArchitectTurn`

APPROVED ✅ — CI green, all AC checked, no blocking issues.

What the PR does

Moves the DockerExecClaudeAgent construction from the call site (ArchitectAgentDispatch) into runArchitectTurn's own default so the in-process SDK fallback is fully deleted rather than just unused. Any future caller of runArchitectTurn that omits claudeAgent now gets the relay automatically — there is no escape hatch back to SdkClaudeAgent.

The change is purely a responsibility shift; the runtime behaviour of every existing production caller is unchanged (they all went through ArchitectAgentDispatch, which was already passing DockerExecClaudeAgent explicitly).

Code quality

Core change (architect.ts line 722)

// before
const claudeAgent: ClaudeAgentPort = opts.claudeAgent ?? new SdkClaudeAgent();
// after
const claudeAgent: ClaudeAgentPort = opts.claudeAgent ?? new DockerExecClaudeAgent({ agentName: opts.agentName });

Clean and correct. opts.agentName is the right key — it matches what ArchitectAgentDispatch has always passed as the container name, so no naming drift.

ArchitectAgentDispatch cleanup — the seven-line comment block for the old explicit claudeAgent arg is replaced by a four-line AR-3 note. No dead code left behind.

Structural test suite — the three new tests in architect.test.ts are deliberately static-analysis guards rather than runtime tests, which is the right call here: the invariant is "this import must/must not exist", and a file-content check is the simplest way to pin it without adding Docker infrastructure to CI.

One small observation on test 3 (the dispatch-file guard): the regex strips block comments (/* … */) before checking for new DockerExecClaudeAgent( but does not strip line comments (// …). This means a developer who writes // new DockerExecClaudeAgent( in a line comment inside architect-agent-dispatch.ts would cause the test to fail spuriously. In practice this is extremely unlikely to happen by accident, and the assertion direction (banning construction) makes a false positive less harmful than a false negative would be, so this is a minor nit rather than a blocking concern.

Acceptance criteria — verified ✅

AC	Status
`/architect/chat` rewired to `docker exec` via runner default	✅ default is now `DockerExecClaudeAgent`
`/architect/spec` unchanged (routes through `forge.writeFile/readFile`)	✅ not touched
In-process SDK path deleted — no `SdkClaudeAgent` in runner	✅ import removed, runtime path gone
Reconcile contract unchanged	✅ no container/agent-config changes
No `role === "host"` read in chat/spec handlers	✅ no handler files touched

Out-of-scope items acknowledged

#1199 (rename role: "host") and #1200 (delete HOST_MODE_TYPES etc.) are correctly deferred — this PR is already exactly the right size.

No changes requested. Ready to merge.

## Review — AR-3: make docker-exec relay mandatory in `runArchitectTurn` **APPROVED** ✅ — CI green, all AC checked, no blocking issues. --- ### What the PR does Moves the `DockerExecClaudeAgent` construction from the call site (`ArchitectAgentDispatch`) into `runArchitectTurn`'s own default so the in-process SDK fallback is fully deleted rather than just unused. Any future caller of `runArchitectTurn` that omits `claudeAgent` now gets the relay automatically — there is no escape hatch back to `SdkClaudeAgent`. The change is purely a responsibility shift; the runtime behaviour of every existing production caller is unchanged (they all went through `ArchitectAgentDispatch`, which was already passing `DockerExecClaudeAgent` explicitly). --- ### Code quality **Core change** (`architect.ts` line 722) ```ts // before const claudeAgent: ClaudeAgentPort = opts.claudeAgent ?? new SdkClaudeAgent(); // after const claudeAgent: ClaudeAgentPort = opts.claudeAgent ?? new DockerExecClaudeAgent({ agentName: opts.agentName }); ``` Clean and correct. `opts.agentName` is the right key — it matches what `ArchitectAgentDispatch` has always passed as the container name, so no naming drift. **`ArchitectAgentDispatch` cleanup** — the seven-line comment block for the old explicit `claudeAgent` arg is replaced by a four-line AR-3 note. No dead code left behind. **Structural test suite** — the three new tests in `architect.test.ts` are deliberately static-analysis guards rather than runtime tests, which is the right call here: the invariant is "this import must/must not exist", and a file-content check is the simplest way to pin it without adding Docker infrastructure to CI. One small observation on test 3 (the dispatch-file guard): the regex strips block comments (`/* … */`) before checking for `new DockerExecClaudeAgent(` but does not strip line comments (`// …`). This means a developer who writes `// new DockerExecClaudeAgent(` in a line comment inside `architect-agent-dispatch.ts` would cause the test to fail spuriously. In practice this is extremely unlikely to happen by accident, and the assertion direction (banning construction) makes a false *positive* less harmful than a false *negative* would be, so this is a minor nit rather than a blocking concern. --- ### Acceptance criteria — verified ✅ | AC | Status | |---|---| | `/architect/chat` rewired to `docker exec` via runner default | ✅ default is now `DockerExecClaudeAgent` | | `/architect/spec` unchanged (routes through `forge.writeFile/readFile`) | ✅ not touched | | In-process SDK path deleted — no `SdkClaudeAgent` in runner | ✅ import removed, runtime path gone | | Reconcile contract unchanged | ✅ no container/agent-config changes | | No `role === "host"` read in chat/spec handlers | ✅ no handler files touched | --- ### Out-of-scope items acknowledged `#1199` (rename `role: "host"`) and `#1200` (delete `HOST_MODE_TYPES` etc.) are correctly deferred — this PR is already exactly the right size. --- **No changes requested.** Ready to merge.

reviewer merged commit 8c23558a2b into main

2026-05-15 13:16:21 +00:00