refactor(server): hexagonal architecture — folder reorg + ForgePort + ClaudeAgentPort #274
No reviewers
Labels
No labels
area:agents
area:dashboard
area:database
area:design
area:design-review
area:flows
area:infra
area:meta
area:security
area:sessions
area:webhook
area:workdir
security
type:bug
type:chore
type:meta
type:user-story
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
charles/claude-hooks!274
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "refactor/hexagonal-phase1-4"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Depends on #273 (contains the janitor fix this branch is based on). Rebase on main once #273 merges.
Scope
Six-commit stack that reorganises
apps/server/src/and introduces two hexagonal ports for the service's external dependencies. Groundwork for migrating Forgejo and Claude SDK callers off direct library imports.Commits
4632630http/domain/infrastructure/background/shared/da7043dForgePort(22 methods, 7 domain shapes) +ClaudeAgentPort(runTask→AsyncIterable<TaskEvent>)e2e518fForgejoAdapterimplementingForgePort, wrappingforgejo-api.tsb4acea2review-loop,slash-commands,maintoForgePortf4b5230foreman; extend port withForgeDirEntry+ForgeIssue.htmlUrle311000SdkClaudeAgentimplementingClaudeAgentPort, wrapping@anthropic-ai/claude-agent-sdk'squery()Folder layout after Phase 1
Consumer migrations
Forgejo (4/10):
review-loop.ts,slash-commands.ts,main.ts,foreman.tsForgejo (6/10 pending — recommend per-consumer follow-up PRs):
deps.ts,webhook-handlers.ts,pipeline.ts,board.ts,webhook-ci.ts,janitor.tsClaude SDK (0/2 pending):
agent-runner.tsandforeman.tsstill callquery()directly. Migration requires Phase 5 (split agent-runner's multi-responsibility body).Port highlights
ForgePort
Forges domain shapes with no Forgejo field names leaking:
Token is bound at adapter construction — one adapter per agent-type.
ClaudeAgentPort
Single
runTask(req): AsyncIterable<TaskEvent>. Seven event variants (assistant, user, result, system, tool_use, tool_progress, tool_summary), each carrying araw: unknownescape hatch for callers that still need SDKMessage internals.Checks
bunx tsc --noEmit -p apps/server/tsconfig.json— EXIT=0bunx biome check apps/server/src— cleanbun --cwd apps/server test— 990 pass, 4 pre-existing fails (3 sweeper JSONL pruning + 1 foreman flaky test; all present onmain)What's explicitly NOT in this PR
forgejo-api.ts— still the implementationForgejoAdapterwraps; will deprecate once all consumers migrateRisk
ForgejoAdapteris new; nothing calls it except the 4 migrated consumersSdkClaudeAgentis new, no consumers.Follow-up backlog
foreman.tsinto agent-logic + HTTP-handler halves (Phase 5a)agent-runner.tstoSdkClaudeAgent(Phase 5b — highest-value SDK decoupling)forgejo-api.tsexports once zero consumers remainNew module apps/server/src/janitor.ts with 7 rules: - closes_keyword_drift: close issues referenced by Closes/Fixes/Resolves in a recently merged PR that Forgejo didn't auto-close - design_approved_not_closed: close issues where design-reviewer posted '**Verdict**: APPROVED' but the skill missed the close step - label_dispatch_miss: bounce area:design / area:design-review / area:dashboard routing labels that have no dispatch in the last 2h - dependent_unblocked: re-fire assignee bounce for issues whose blockers are all closed but the propagator never ran - zero_output_success: flag success tasks with 0 turns / null cost - stale_idle_assigned: comment + bounce assignee on issues idle >30 min - ready_to_merge: flag PRs that are mergeable + CI green + APPROVED Config block janitor: { enabled, interval_ms, mode, rules } in config/agents.json. Default mode is dry-run. GET /janitor/history returns last 100 actions. Each action emits a janitor_action SSE envelope. Per-rule per-target 6h idempotency guard. Supporting changes: - forgejo-api: patchIssue, listRepoLabels, addIssueLabels, removeIssueLabel, listClosedPullRequests, extend PullRequestDetail with merged/merged_at, make IssueListEntry labels id optional - task-store: hasRecentDispatch, listZeroOutputSuccesses - webhook-config: parse janitor: block - shared: JanitorAction, JanitorReport, JanitorHistoryResponse types 42 unit tests, all green.The s11 refactor moved dispatchByType / getWorker / broadcastSSE out of main.ts into registry.ts / sse.ts. Several test files updated the module specifier in their mock.module() calls, but three related leaks made 20 sibling-file tests fail in the full bun test run: 1. janitor.test.ts used mock.module("./forgejo-api") and ("./task-analytics") to stub its deps. Bun's mock.module is process-global — those stubs persisted through every later test file and broke deps.test.ts, webhook-handlers.test.ts pivot tests, propagateDependencyClosure, etc. Fix: add `impl` test seam to janitor.ts (same pattern as cleanup.ts) so the test mutates impl.* instead of touching module registry. 2. webhook-post-merge.test.ts was still mocking "./main" for dispatchByType (refactor missed this one) and also mocked ./main's broadcastSSE, both of which now live in ./registry / ./sse. Fix: point mock.module at the correct modules; drop the unused getWorker override (not exercised by handlePostMergeRebase). 3. webhook-assign-dedup / webhook-ci / force-merge.integration all mocked ./registry with both dispatchByType AND getWorker. getWorker is only actually exercised by webhook-assign-dedup's assign-dedup tests; the other two carried stale overrides that corrupted getWorker for every later test file (main.test.ts POST /cancel + POST /reset). Fix: drop the getWorker override where unused; in webhook-assign-dedup, keep the stub but fall back to the real function for names the test doesn't track, so main.test.ts's registered workers still resolve. Plus: fix the stale mock.module("./main") → ("./registry") in webhook-handlers.test.ts and refresh an outdated comment in main.test.ts. Server test suite now: 990 pass / 4 fail — and the 4 remaining failures (session JSONL pruning + foreman session CRUD) are pre-existing on main, out of scope for this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Port additions: - ForgeDirEntry shape for listDir (name, path, type, size, sha) - htmlUrl field on ForgeIssue (needed by createIssue → breakdown summary link) - readFile returns { content, sha } so callers can chain writeFile - writeFile takes optional sha for update-vs-create discrimination foreman.ts migration: - /foreman/repos, /foreman/files, /foreman/specs/:name save, /foreman/breakdown-preview remote fetch, /foreman/create-issues all go through ForgejoAdapter - spec save now surfaces a 502 on adapter failure instead of 500ing out of createOrUpdateRepoFile's throw (adapter swallows to bool) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Review flagged five correctness issues that would bite the first Phase 5 consumer migration. None change runtime behavior today (no consumer reads the affected fields yet), but the port was advertising data the adapter couldn't actually deliver from the narrow forgejo-api types. ### Shape mapper faithfulness Widened forgejo-api.ts raw types so every field ForgePort promises is actually declared on the underlying response: - PullRequestSummary / PullRequestDetail / ClosedPullRequest — added state, base.ref, body, labels, assignees, requested_reviewers, mergeable, merged, draft, created_at, updated_at, merged_at - PullReview — added submitted_at (was hardcoded to "" in adapter) - WorkflowRunSummary — added name, head_sha, run_started_at, created_at (all were hardcoded to "" in toForgeWorkflowRun) - IssueSummary / IssueListEntry — added user, html_url, created_at, closed_at, milestone, assignees so getIssue / listIssues return the full shape ForgeIssue advertises ForgejoAdapter's shape mappers no longer need `as unknown as` casts — types line up directly with the widened API. ### Dead label-id cache ForgejoAdapter is instantiated per-call by every migrated consumer (different tokens per agent type), so the instance-scoped cache was always a miss. Dropped it; added a comment documenting that a TTL cache is the right answer if we ever need one. ### parseRepo hardens on malformed input parseRepo("foo") used to return `{owner: "foo", name: ""}` and silently cause 404s downstream. Now throws with a clear error. Every current caller pre-validates (regex or trusted source), so no caller is affected. ### Stale path in force-merge PR comment review-loop.ts posted `src/review-loop.ts` in its user-visible force-merge comment; after the reorg the file is at `src/domain/workflow/review-loop.ts`. Fixed. ### ToolUseEvent dropped Declared by ClaudeAgentPort but never emitted — the SDK doesn't have a top-level tool_use message type (tool uses are content blocks on the assistant message, not separate SDKMessage entries). Removed from the TaskEvent union so the port advertises only what the adapter actually produces. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>bf70609576989f1cb673