refactor(backend): LLD maintainability fixes in controllers/service layers (#95)

[neversettle17-101]

What & why

Implements the High + Medium severity maintainability findings from the LLD review of backend/internal/httpd (controllers/router/server) and backend/internal/service. Partially addresses #95.

Scope note: the two isGitRepo findings (case-sensitivity fix and the GitChecker testability seam) are deferred to #97 and not part of this PR.

Changes

1. [High] Controller no longer imports internal/session_manager. Controllers render structured errors generically — the cross-package sentinel imports are gone.
2. [Med] One error pattern across all services (see "Error standardization" below).
3. [Med] Clean-orchestrator logic moved out of the controller into session.Service.SpawnOrchestrator(ctx, projectID, clean). The controller just validates input and delegates; the rule is unit-tested at the service.
4. [Med] httpd constructor ladder collapsed to the two production constructors (NewWithDeps, NewRouterWithControl). The 3 test-only wrappers are removed; the "router with empty deps" convenience is an unexported newTestRouter helper.

Error standardization (finding #2)

The error story collapses to one type + one renderer, scoped to the REST API:

• internal/httpd/apierr (package apierr) — a single structured Error{Kind, Code, Message, Details} with semantic Kinds (KindInternal/KindInvalid/KindNotFound/KindConflict), not HTTP words. Constructors: Invalid/NotFound/Conflict/Internal. The package imports nothing.
• envelope.WriteError(w, r, err) is the single path from any service error to the locked APIError wire shape, and the only place a Kind is mapped to an HTTP status/word. The per-resource writeProjectError/writeSessionError translators are deleted.
• No per-service errors.go and nothing in domain. Services build errors inline via apierr.*. domain stays free of HTTP-flavored kinds.
• session_manager keeps plain errors.New engine sentinels — the internal command engine does not depend on the API error vocabulary. The service/session facade (toAPIError) is the single boundary that translates those engine sentinels into apierr errors before they reach a controller.

Layering note (deliberate)

The error vocabulary lives under internal/httpd/, so the service facades (service/project, service/session) import the httpd tree — the conscious "these services exist to serve the REST API" coupling, fine given the CLI is a thin HTTP client. The internal engine (session_manager) does not import it. No import cycle: httpd/apierr is a leaf (imports nothing internal), verified with go list -deps.

Acceptance criteria

• Controllers no longer import internal/session_manager
• One error pattern used consistently across services
• Clean-orchestrator logic moved into the session service (and unit-tested)
• httpd exports only production-used constructors

Deferred to #97: isGitRepo case-sensitivity fix; project service testable without a real git binary.

Testing

• go build ./..., go vet ./..., gofmt -l clean
• golangci-lint run ./... (v2.12.2, the CI linter): 0 issues
• go test ./... passes except one pre-existing, environment-specific failure in internal/terminal (zellij IPC socket path too long under macOS $TMPDIR) — unrelated to these changes
• Branch is merged up to date with main (PRs feat(agent): opencode adapter + activity plugin hooks #80/feat: add issue #90 light backend CLI commands #98/feat(cli): enrich ao doctor (#90) #99), with main's new session code (rename, cleanup, orchestrator list/get) reconciled to the apierr pattern

🤖 Generated with Claude Code

[harshitsinghbhandari]

@greptileai Review

[greptile-apps]

Greptile Summary

This PR consolidates the backend's error handling story into a single apierr package and a single renderer (envelope.WriteError), removes cross-layer sentinel imports from controllers, and moves the clean-orchestrator business rule into session.Service.SpawnOrchestrator. It also trims the exported httpd constructor surface to the two production constructors.

• Error unification: apierr.Error{Kind, Code, Message, Details} replaces the per-service Error types; envelope.WriteError is now the only place a Kind maps to an HTTP status, with status code behaviour preserved exactly for every former sentinel and project-error kind.
• Service extraction: SpawnOrchestrator encapsulates the kill-then-spawn orchestration that previously lived in the HTTP controller; service_test.go adds direct unit tests for the ordering invariant.
• API surface reduction: New, NewRouter, and NewRouterWithAPI are removed; test files that called them are updated to NewRouterWithControl, and an unexported newTestRouter helper handles the zero-dep test cases inside the package.

Confidence Score: 5/5

Safe to merge — the refactoring is a direct mechanical substitution with no behavioral changes to HTTP status codes or error codes on any path.

All five session_manager sentinel-to-status mappings in toAPIError match the removed writeSessionError switch case for case. All project service error kind strings map to the same HTTP statuses via the new apierr.Kind enum. The UpsertProject path that previously passed raw store errors through is now consistently wrapped as apierr.Internal. The commander interface and new service-layer unit tests tighten coverage without introducing new risks.

No files require special attention.

Important Files Changed

Filename	Overview
backend/internal/httpd/apierr/apierr.go	New leaf package defining the single structured error type; uses iota so KindInternal is the zero value, meaning a zero-value Error safely defaults to 500. Clean with no internal imports.
backend/internal/httpd/envelope/envelope.go	Adds WriteError as the single apierr to HTTP translation point; httpStatus now has explicit KindInternal case plus default fallback. All four Kinds are covered correctly.
backend/internal/service/session/service.go	Introduces commander interface, SpawnOrchestrator, and toAPIError which correctly maps all five session_manager sentinels to apierr types via errors.Is. Status code mappings match the removed writeSessionError exactly.
backend/internal/httpd/controllers/sessions.go	Removes writeSessionError and session_manager import; replaces all per-handler translators with envelope.WriteError. getOrchestrator inline 404 is behaviorally identical to the removed writeSessionError(ErrNotFound).
backend/internal/service/project/service.go	Replaces all package-local error constructors with apierr equivalents; the UpsertProject error path is upgraded from a raw passthrough to an explicit apierr.Internal, fixing a latent inconsistency in the old code.
backend/internal/httpd/router.go	Collapses three exported wrappers into one production constructor NewRouterWithControl; unexported newTestRouter in testhelpers_test.go keeps the package test surface clean.
backend/internal/service/project/errors.go	Deleted — all functionality migrated to apierr package, removing the per-service error vocabulary in favour of the shared type.

Sequence Diagram

sequenceDiagram
    participant C as Controller
    participant S as Service
    participant SM as session_manager.Manager
    participant EW as envelope.WriteError

    C->>S: SpawnOrchestrator / Spawn / Kill / etc.
    S->>SM: Spawn / Kill / Restore / Send
    SM-->>S: error sentinel or raw
    S->>S: toAPIError via errors.Is
    S-->>C: apierr.Error or raw error
    C->>EW: WriteError(w, r, err)
    EW->>EW: errors.As for apierr.Error
    alt apierr.Error found
        EW-->>C: httpStatus Kind maps to 400 404 409 500
    else fallback
        EW-->>C: 500 INTERNAL_ERROR
    end

Loading

_{Reviews (4): Last reviewed commit: "refactor(apierr): rename package, test S..." | Re-trigger Greptile}

[harshitsinghbhandari]

Review (local; verdict: approve-with-nits)

Cleanly addresses #95 High + Medium. Build/vet/race-tests green locally on httpd/..., service/..., session_manager/... (80/80).

Coverage of #95

Finding	Done	Where
1. Controller drops session_manager import	✅	controllers/sessions.go; engine sentinels translated at the facade in service/session/service.go:160-177
2. One error pattern	✅	*apierr.Error everywhere; envelope.WriteError is the sole renderer
3. Clean-orchestrator moved to service	✅	service/session/service.go:56-70
4. httpd constructor ladder collapsed	✅	only NewWithDeps + NewRouterWithControl exported; test convenience moved to testhelpers_test.go

Concerns

• Testing gap. The relocated business rule in Service.SpawnOrchestrator(clean=true) (service/session/service.go:56-70) has no direct unit test. The controller test only exercises clean=false; the controller fake reproduces the loop but the real Service path is uncovered. Worth a small test in service/session/service_test.go seeding two active orchestrators and asserting both are killed before spawn.
• PR description nit. "session_manager sentinels are apierr.Error values" — they're not; session_manager/manager.go:17-25 still defines plain errors.New sentinels and the translation happens in service/session/toAPIError. The implementation choice is the better one (keeps session_manager decoupled), but the description misdescribes it.
• Package name. internal/httpd/errors is package errors, forcing every importer to alias (you've used apierr consistently). Renaming the package to apierr would eliminate the foot-gun.
• Zero-value Kind. KindInternal = iota means a zero-value apierr.Error{} advertises as 500 — safe default, worth a one-line comment. (Greptile flagged the same shape on httpStatus's default branch — agree with the spirit; an explicit case KindInternal makes future Kind additions a compile-time conversation.)
• envelope.WriteError default branch silently swallows the underlying error. Correct on the wire; consider a log.Error at the boundary so unexpected non-apierr errors aren't invisible. Out of LLD: maintainability fixes in controllers/service layers (high + medium severity) #95 scope.
• Parity nit. service/project/service.go:128 — UpsertProject is the one project path that still returns a raw store error (renders as generic INTERNAL_ERROR), whereas its siblings build apierr.Internal("PROJECT_*_FAILED", …). Pre-existing, but it now stands out.

Recommended actions (priority order)

1. Add the Service.SpawnOrchestrator(clean=true) unit test.
2. Fix the PR description's session_manager bullet.
3. Add the partial-failure comment on SpawnOrchestrator and the zero-value-Kind comment on the const block.
4. Optional follow-ups (not blockers): rename package errors → apierr; wrap the UpsertProject error for parity; log on the WriteError fallthrough.

Scope discipline is good — the explicit revert(backend): drop GitChecker seam keeps the diff aligned with #95. None of the above is blocking.

[neversettle17-101]

Thanks for the thorough review — addressed in fead016:

• Testing gap (SpawnOrchestrator clean=true): added a commander seam on session.Service and two unit tests — clean=true kills both active orchestrators before spawning (asserts ordering, and leaves the worker + terminated orchestrator alone); clean=false spawns with no kills.
• PR description nit: fixed. The description now states session_manager keeps plain errors.New sentinels and service/session.toAPIError is the translation boundary.
• Package name: renamed internal/httpd/errors → internal/httpd/apierr (package apierr), so importers no longer alias around stdlib errors.
• Zero-value Kind: added a doc line noting KindInternal is iota's zero value, so a zero-value Error safely defaults to 500. (The explicit case KindInternal in httpStatus is already in from the earlier Greptile note.)
• Parity nit: project.Add now wraps the UpsertProject store error in apierr.Internal("PROJECT_ADD_FAILED", …) like its siblings.
• envelope.WriteError default swallows the error: agreed in spirit; leaving it for a follow-up since it's out of LLD: maintainability fixes in controllers/service layers (high + medium severity) #95 scope and envelope has no logger today — a boundary log.Error (or returning the original for the caller to log) is the right fix and I'll track it separately.

[harshitsinghbhandari]

Approving — final pass on `fead016`.

All 5 actionable items from my earlier comment are in:

• ✅ `Service.SpawnOrchestrator(clean=true)` covered by TestSpawnOrchestratorCleanKillsActiveOrchestratorsBeforeSpawn (asserts kill-before-spawn ordering, leaves the worker and terminated orchestrator alone) + TestSpawnOrchestratorNoCleanSkipsKills. The unexported commander seam (service.go:31-39) is the right shape for this — internal, no production surface change.
• ✅ Package renamed internal/httpd/errors → internal/httpd/apierr (package apierr); no more alias dance for callers.
• ✅ Const block on Kind now documents that KindInternal = iota means a zero-value Error defaults to 500.
• ✅ project.Add wraps the UpsertProject store error with apierr.Internal("PROJECT_ADD_FAILED", …) for parity with sibling paths.
• ✅ PR description's session_manager bullet corrected to reflect what the code actually does (sentinels stay as errors.New; service/session/toAPIError is the translation boundary).

The log.Error on envelope.WriteError's fallthrough was reasonably deferred — envelope carries no logger today and it's out of #95's scope.

Local: go build ./... clean, race-tests green across httpd/..., service/..., session_manager/.... CI is 7/7 SUCCESS on fead016. Nothing blocking.