# Internal API Clean Code 整理设计方案

> Canonical path: `specs/api/internal-api-clean-code-design.md`
> Status: living architecture spec
> Related spec: [`specs/api/handler-adapter.md`](./handler-adapter.md)
> Scope: `internal/api` and `internal/api/{domain}` package organization, naming, dependencies, handler patterns, DTO/error boundaries, tests, and guardrails.
> Non-goal: changing route/wire/status/message contracts, redesigning product semantics, or rewriting all raw Gin handlers in one pass.

## 1. Executive decision

`internal/api` should converge on a **thin HTTP adapter + domain-owned handler packages** architecture:

1. Root `internal/api` remains the shared HTTP adapter toolkit: middleware, typed requests, response helpers, JSON binding, and generic error plumbing only.
2. Each `internal/api/{domain}` package owns its route registration, feature-focused handlers, narrow ports, domain-specific error classification, DTO conversion, and route tests.
3. New browser/workspace/auth routes use typed handlers by default: `AuthenticatedRequest`, `WorkspaceScopedRequest`, `AuthenticatedJSON[T]`, `WorkspaceScopedJSON[T]`.
4. Raw Gin remains allowed only for exceptional protocol surfaces: auth/public sessions and callbacks, agent/daemon protocol flows, multipart/media, webhooks/OAuth callbacks, optional-body endpoints, and streaming/realtime.
5. Clean-code work should be delivered as **small, ratcheted cleanup slices** with route parity and guardrail grep, not as a single broad rewrite.

This design builds on the typed adapter direction while adding stricter package organization and Go style rules so `internal/api` stays maintainable as routes grow.

## 2. Current evidence and inventory policy

`internal/api` is large enough that cleanup decisions must be driven by repeatable inventory rather than thread history or one-off grep output. The durable inventory sources are:

- `guardrails/api/internal-api-clean-code-design.sh` for raw Gin counts, JSON bind counts, route snapshot inputs, adapter invariant checks, and oversized/cohesion candidates.
- `specs/api/raw-gin-surface-classification.md` for durable keep-raw / migrate-now / migrate-later classification policy and the live-inventory procedure.

Interpretation: the typed handler adapter already removed the highest-value repeated handler plumbing. Maintainability now depends on consistent package/file organization, explicit raw handler classification, generated inventory output, and dependency boundaries that keep domain packages understandable.

When a cleanup slice starts, refresh the guardrail output. Do not treat frozen counts in an old task handoff or spec as canonical; update the classification spec only when categories, allowed reasons, or procedure changes.

## 3. Target package shape

Each domain package should use the same conceptual sections. Small packages can keep fewer files, but names and responsibilities should remain consistent.

```text
internal/api/<domain>/
  routes.go                 # RegisterRoutes, route groups, domain adapter wrappers only
  handlers.go               # handler struct declarations / constructors when many handlers exist
  ports.go                  # shared narrow consumer-side interfaces used by multiple handlers
  api_<feature>.go          # endpoint methods for one feature/resource cluster
  <feature>_params.go       # request-to-service parameter mapping, if non-trivial
  <feature>_responses.go    # response assembly, if non-trivial
  errors.go                 # domain error classifier / HTTP error mapping
  *_test.go                 # unit tests near behavior; route parity tests per domain
  *_integration_test.go     # integration tests, separated by scenario and dependencies
```

### Rules

- `routes.go` is the composition root for that domain package. It may contain small `handleDomainWorkspaceJSON` wrappers because wrappers are route-adapter policy, not business logic.
- Do not create helper-only `adapter_helpers.go` / `http_helpers.go` files just to hold adapter wrappers.
- Avoid generic catch-all files named `helpers.go`, `types.go`, or `http_helpers.go` unless the package is very small and the file has one clear theme. Prefer semantic names like `email_normalization.go`, `oauth_callback.go`, `store_errors.go`, or `route_parity.go`.
- `api_<feature>.go` owns endpoint methods and tiny private helpers for that feature only. If helpers become reusable across features, move them to a semantic support file.
- Keep package-local code organized by domain language, not by technical layer. Example: `task/api_checkpoints.go`, `task/checkpoint_params.go`, `task/checkpoint_responses.go` is preferable to one giant `helpers.go`.

## 4. Handler naming and method rules

### Handler type names

Use explicit feature names:

- Good: `CheckpointHandler`, `BoardOptionHandler`, `WorkspaceAppHandler`, `GiteaHandler`, `EmailVerificationHandler`.
- Avoid: a bare `Handler` in packages with more than one feature cluster.

Allowed exceptions: very small packages with exactly one public route cluster may use `Handler`, but the preferred style is still `<Feature>Handler` when future growth is likely.

### Method names

Use REST/tool verbs and domain verbs consistently:

- CRUD/listing: `Create`, `Get`, `List`, `Update`, `Patch`, `Delete`.
- State transitions: `Pause`, `Resume`, `Claim`, `Unclaim`, `Approve`, `RequestChanges`.
- Domain operations: choose a precise verb such as `Deliver`, `Publish`, `Promote`, `Export`, `Validate`.

Do not encode HTTP details into method names unless needed to disambiguate. Prefer `Create` over `PostCreate`, `List` over `GetList`.

### Handler signatures

Default for new/migrated browser/workspace/auth routes:

```go
func (h *FeatureHandler) Create(r api.WorkspaceScopedJSON[dto.FeatureCreateRequest]) (api.Response, error)
func (h *FeatureHandler) Get(r api.WorkspaceScopedRequest) (api.Response, error)
```

Rules:

- Request parameter name is always `r`.
- Strict JSON body comes from `r.Body`. Legacy routes with an explicit empty-or-JSON body contract keep that exception in a domain-local wrapper; handlers run route/domain preconditions first, then call the local `DecodeBody()` helper and use `hasBody` only when they need to distinguish absent/empty body from a present zero-value object.
- Gin escape hatch is explicit: `r.Gin.Param`, `r.Gin.Query`, `r.Gin.GetHeader`; use `c := r.Gin` locally if repeated.
- Return `api.OK`, `api.Created`, `api.NoContent`, or `api.Response{...}` only when the helper does not fit.
- Do not return by writing `ctx.JSON` in typed handlers.
- Do not call `ShouldBindJSON` / `Bind*` in typed handlers.

## 5. Raw Gin classification

Raw Gin should not be treated as automatically bad. It should be classified and documented.

### Keep raw by design

Raw Gin is acceptable for:

1. Public/auth flows with protocol-specific cookies, redirects, refresh-token bodies, or unauthenticated state (`auth`, `public`).
2. Machine/API-key/internal daemon protocol flows (`agent`, `daemon`, selected app internal APIs).
3. Multipart upload/download/proxy flows (`media`, avatar, stickers, attachment download).
4. Webhook/OAuth callback entry points (`provider`, app webhooks).
5. Optional-body endpoints whose auth, redirect, cookie, or pre-body validation behavior requires domain-local empty-or-JSON handling to avoid wire drift.
6. Realtime/streaming/socket flows.

### Migrate raw opportunistically

Raw Gin should be migrated when all are true:

- The route is authenticated or workspace-scoped browser API.
- The body is ordinary JSON, or a legacy empty-or-JSON contract whose absent/empty/invalid-body semantics can be represented by a domain-local wrapper using `api.BindEmptyOrJSON` while keeping route/domain preconditions inside the handler before decode.
- Error mapping can be represented by domain classifier / `api.HTTPError` without status/message drift.
- Tests can lock route parity and wire behavior.

### Required documentation

For packages retaining raw handlers, maintain a short package-local comment or inventory section in the relevant spec/checklist explaining why raw is intentional. This prevents reviewers from repeatedly rediscovering the same exceptions.

## 6. Dependency and port management

### Consumer-side interface ownership

- If an interface is used by exactly one handler file, define it next to that handler.
- If an interface is shared by multiple handlers, put it in `ports.go` and keep it narrow.
- Interface methods must describe consumer needs, not mirror an entire store/service API.
- Do not name ports after implementation packages (`Store`, `Service`) unless the semantic role is genuinely generic. Prefer `TaskBoardService`, `WorkspaceResolver`, `MessageNotifier`, `ReviewStore`.

### Dependency struct rules

`Dependencies` in `routes.go` is allowed because route registration is a composition root. Keep it as a wiring shape, not a god object:

- Group fields by feature area when the domain has many collaborators.
- Do not pass `Dependencies` into handler methods.
- Build focused handler structs in `RegisterRoutes` or a small constructor.
- Avoid package-level mutable setters.

### Store/service imports

`internal/api` should usually depend on service/core/store ports through narrow interfaces. Direct store imports are acceptable in adapter packages during transition, but should not leak root store-style god dependencies back into handlers.

Target rule: handler methods call narrow domain ports; they do not know which store package, facade, or transaction implementation backs the port.

## 7. DTO, converter, and response assembly rules

Current single-package `internal/api/dto` should be retained for now to avoid import churn, but it needs stricter naming:

- Every request/response type name starts with the owning domain or route concept: `TaskReviewRequest`, `ChannelMemberRequest`, `ContextShareRequest`.
- No anonymous inline request/response structs in handlers.
- JSON tags are wire contracts; moving code must not silently rename fields.
- Keep `dto/converter` for cross-domain view conversion; do not add presenter/converter logic to handlers unless it is trivial and endpoint-specific.

Longer-term option: split `internal/api/dto` by domain only if the single package becomes a real bottleneck. This is a separate migration because it touches imports across the tree and can create churn without improving behavior.

## 8. Error handling and response rules

Target shape:

- Typed handlers return domain errors or `api.HTTPError` values; adapter writes the HTTP response.
- Domain route wrappers attach domain-specific classifiers via `api.WithErrorClassifier` where needed.
- Runtime-control errors from daemon/websocket control-plane ports are classified by the API domain that exposes the user-facing route, not by root `internal/api`. Shared sentinel errors and runtime-control request/response contracts live outside route packages so browser API domains do not import `internal/api/daemon`.
- Invalid JSON text remains domain-compatible via `api.WithInvalidJSONMessage` or domain wrappers, including domain-local empty-or-JSON adapters for legacy optional-body routes.
- `api.StoreError` and direct `ctx.JSON` should be confined to raw Gin flows or legacy code scheduled for migration.

Avoid:

- Scattering identical `switch` statements across handlers.
- Returning `api.Response{Status: ..., Body: ...}` when `api.OK` / `api.Created` / `api.NoContent` fits.
- Introducing root `internal/api` knowledge of domain-specific errors.

## 9. File-size and decomposition rules

Suggested thresholds:

- >500 lines in a non-test API file: review for split.
- >20 top-level funcs in a file: review cohesion.
- >8 interfaces in a file: check if ports are truly shared or just dumped together.
- >1 feature cluster in one `api_*.go`: split by route/resource.

Priority candidates based on current snapshot:

1. `internal/api/daemon/protocol.go` — split by request parsing, validation, response writing, and command execution protocol helpers.
2. Provider OAuth/Gitea support files — split OAuth flow state, Gitea request client, and provider response mapping when they grow beyond one concern.
3. `internal/api/message/provider_watch_commands.go` — split command parsing, watch mutation, and response rendering.
4. `internal/api/task/task_contract.go` and `internal/api/agent/task_contract.go` — reduce duplicate contract definitions or move shared contract types to a clearer package if they are true cross-surface contracts.
5. `internal/api/channel/api_channels.go` — split high-density channel operations into membership, notification, read/view preferences, and mutation files.
6. Large integration tests — split by behavior scenario and reduce setup duplication through testkit fixtures.

## 10. Testing and guardrails

Every cleanup slice should include evidence in its handoff.

### Required checks for API cleanup PRs

```bash
git diff --check
rg -n '<<<<<<<|=======|>>>>>>>' internal/api
GOWORK=off go test ./internal/api -run 'TestHandle.*JSON' -count=1
GOWORK=off go test -run '^$' ./internal/api/...
GOWORK=off go build ./...
GOWORK=off go vet ./...
```

Add targeted tests for touched domains, for example:

```bash
GOWORK=off go test ./internal/api/task ./internal/api/channel
```

### Guardrail grep

Maintain the documented grep set for adapter invariants in `guardrails/api/internal-api-clean-code-design.sh`:

- `api.JSON(` should stay 0; use `api.OK`, `api.Created`, `api.NoContent`.
- Old JSON wrapper names should stay 0: `AuthenticatedJSONRequest`, `WorkspaceScopedJSONRequest`.
- Anonymous Gin embed should stay 0 in typed request structs.
- Custom typed request facade methods named `Param`, `Query`, `Header`, `Bind` should stay 0.
- Root `internal/api/*.go` must not import `internal/api/{domain}` route packages. Shared contracts belong in non-route packages; domain-specific error mapping belongs in domain classifiers.
- Old two-parameter JSON handler shape should stay 0.
- Typed handler parameter `apiRequest` should stay 0; use `r`.
- Typed manual bind (`r.ShouldBindJSON`, `r.Bind*`, `r.Gin.ShouldBindJSON`, `r.Gin.Bind*`) should stay 0; legacy empty-or-JSON routes use a domain-local decode helper backed by `api.BindEmptyOrJSON` instead of Gin bind calls in handlers.
- Old server/current-server API symbols should stay 0.
- Helper-only domain adapter wrapper files should stay 0.
- Route method/path snapshots must not drift unless the task explicitly changes route contracts.

## 11. Proposed cleanup roadmap

### Phase 0 — Guardrail baseline and inventory

Goal: make future cleanup safe.

- Add or update the reusable root guardrail script `guardrails/api/internal-api-clean-code-design.sh` that prints:
  - raw Gin handler inventory by package;
  - `ShouldBindJSON` inventory by package;
  - route method/path snapshot;
  - adapter guardrail grep counts;
  - files over size/cohesion thresholds.
- Include the inventory output in the implementation handoff or review notes for any cleanup slice that relies on it.

### Phase 1 — Document intentional raw surfaces

Goal: stop ambiguous review debates.

- For `agent`, `auth`, `media`, `app`, `provider`, `workspace`, `public`, and `role`, classify remaining raw handlers into: keep raw, migrate now, migrate later, or remove/dead code.
- Add a small spec section or package-level comments for intentionally raw protocol flows.

### Phase 2 — Migrate safe leftover browser JSON/auth/workspace handlers

Goal: finish low-risk typed adapter consistency.

- Use the classification from Phase 1.
- Migrate only endpoints with ordinary JSON or a domain-local empty-or-JSON auth/workspace contract and stable error mapping.
- Preserve route/wire/status/message behavior.
- Keep each domain as a separate task/slice.

### Phase 3 — Normalize file names and handler names

Goal: improve navigation without semantic changes.

- Rename generic `helpers.go`, `types.go`, and oversized `http_helpers.go` files to semantic names.
- Rename bare `Handler` structs in multi-feature packages to `<Feature>Handler`.
- Split high-density files along feature boundaries.
- Avoid package churn that only changes names without improving discoverability.

### Phase 4 — Dependency boundary cleanup

Goal: reduce hidden coupling.

- Move single-use interfaces next to handlers; keep shared interfaces in `ports.go`.
- Split `ports.go` only when it becomes a dumping ground with unrelated concerns.
- Replace broad store/facade-like dependencies with narrow service/store ports.

### Phase 5 — Test and testkit cleanup

Goal: keep tests fast and understandable.

- Split huge integration tests by behavior area.
- Centralize common setup in testkit fixtures.
- Keep compile-only guardrails cheap and reliable.
- Document known environment fallout separately from regressions.

## 12. Review checklist for future implementation tasks

For every cleanup slice, reviewers should ask:

1. Did route method/path contracts stay unchanged?
2. Is every changed handler using the correct request scope?
3. Are raw Gin handlers either unchanged or explicitly justified?
4. Did dependency interfaces become narrower or at least not broader?
5. Did file names become more semantic and easier to navigate?
6. Did DTO names/tags and response wire shape stay stable?
7. Are tests targeted to the domain touched, plus adapter guardrails?
8. Is the diff small enough to review mechanically?

## 13. Operating rule for future cleanup

Every future `internal/api` cleanup should start from the current guardrail output and the raw-surface classification spec, then make the smallest behavior-preserving change that advances one roadmap item. Architecture decisions belong in these specs and guardrails, not only in chat history or PR descriptions.