LLD: maintainability fixes in controllers/service layers (high + medium severity)

[neversettle17-101]

Summary

LLD review of backend/internal/httpd/ (controllers, router, server) and backend/internal/service/ surfaced several maintainability issues. This issue tracks the High and Medium severity findings. (Low-severity polish items are tracked separately.)

Note: the two isGitRepo findings (case-sensitivity fix and the GitChecker testability seam) were removed from this issue's scope and deferred for now.

---

1. [High] Controller imports session_manager directly — layering violation

controllers/sessions.go imports the internal command engine and matches its sentinels directly:

import sessionmanager "...internal/session_manager"

case errors.Is(err, sessionmanager.ErrNotFound):
case errors.Is(err, sessionmanager.ErrNotRestorable):

The controller layer should only know about the service/session facade, never the internal engine. There is a chain: session_manager defines the error → service/session re-raises it (service.go:129) → controllers catches it by importing session_manager again. If the engine is renamed or split, the controller breaks.

Fix: Move the session sentinel errors into domain (or have service/session re-export them), and have writeSessionError match against that single home. Removes two cross-package imports.

• backend/internal/httpd/controllers/sessions.go:18-19, 248-262
• backend/internal/service/session/service.go:129

---

2. [Medium] Inconsistent error patterns across services

project.Service uses a struct-typed *projectsvc.Error checked with errors.As. session_manager uses plain sentinel errors.New(...) checked with errors.Is. Controllers must use a different idiom per resource, and adding a new resource means picking one arbitrarily (or inventing a third).

Fix: Standardize on one error pattern across all services.

• backend/internal/service/project/errors.go
• backend/internal/session_manager/manager.go:15-24

---

3. [Medium] Business logic (clean-orchestrator) lives in the controller

spawnOrchestrator runs a list+kill loop before spawning:

if in.Clean {
    orchestrators, _ := c.Svc.List(...)
    for _, existing := range orchestrators {
        c.Svc.Kill(ctx, existing.ID)
    }
}

This is a business rule, not HTTP glue. It forces the controller's unit tests to exercise multi-step orchestration.

Fix: Push into a service.SpawnOrchestrator(ctx, cfg, clean bool) method.

• backend/internal/httpd/controllers/sessions.go:184-196

---

4. [Medium] httpd: collapse the constructor ladder to production-only constructors

internal/httpd exposes 5 exported constructors, but only 2 are used in production:

• Production: NewWithDeps (server) → NewRouterWithControl (router) — the daemon's only path.
• Test-only: New, NewRouter, NewRouterWithAPI — each just defaults an empty APIDeps{}/ControlDeps{} and forwards down the chain. No production caller.

Fix: delete the 3 test-only wrappers; keep NewWithDeps + NewRouterWithControl. Move the "router with empty deps" convenience into an unexported test helper. Point the one cross-package caller (internal/cli/dto_drift_e2e_test.go) at NewRouterWithControl.

Why not keep them: they exist solely to shorten test call sites — not a production need, and httpd is an internal/ package so there are no external consumers either. Keeping them exported means a permanent public surface that must be documented and maintained as if production depends on it, and it misleads readers about which entry points are actually load-bearing. Test convenience belongs in a test helper, not the package's exported API.

• backend/internal/httpd/server.go:36-63, backend/internal/httpd/router.go:32-46

---

Acceptance criteria

• Controllers no longer import internal/session_manager
• One error pattern used consistently across service/ packages
• Clean-orchestrator logic moved into the session service
• httpd exports only production-used constructors (NewWithDeps, NewRouterWithControl)

Filed from an LLD review of the controllers/service/httpd layers.

[neversettle17-101]

The two isGitRepo findings (case-sensitivity fix + GitChecker testability seam) were split out of this issue and are now tracked in #97. PR #96 covers the remaining four findings.