* refactor(backend): LLD maintainability fixes in controllers/service layers Addresses the high + medium severity findings from the LLD review of backend/internal/httpd and backend/internal/service (#95): 1. Controllers no longer import internal/session_manager. Session sentinel errors are now *domain.ServiceError values carrying their own HTTP mapping, so the controller translates them with one generic errors.As — no cross-package sentinel imports. 2. One error pattern across services: project.Error is now an alias of the shared domain.ServiceError, and session_manager sentinels use it too. A single writeServiceError replaces the per-resource error switches. 3. Clean-orchestrator business logic moved out of the controller into session.Service.SpawnOrchestrator(ctx, projectID, clean). 4. isGitRepo no longer treats case-different paths as equal on case-sensitive filesystems; case-insensitive compare is gated to darwin/windows via samePath. 5. Project repo check sits behind an injectable GitChecker, so the service is testable without a real git binary. 6. httpd exports only the production constructors (NewWithDeps, NewRouterWithControl); the 3 test-only wrappers are removed and the "router with empty deps" convenience moved to an unexported test helper. Closes #95 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(backend): standardize service errors on internal/httpd/errors Replace the domain.ServiceError approach with a REST-API-scoped error package and a single envelope renderer, per review feedback: - Add internal/httpd/errors (package errors, aliased apierr): one structured Error type with semantic Kinds (Internal/Invalid/NotFound/Conflict) and constructors. Imports nothing, so any layer can depend on it. - envelope.WriteError is now the single path from a service error to the wire APIError, and the only place a Kind becomes an HTTP status/word. The per-resource writeProjectError/writeSessionError translators are gone. - Delete domain/errors.go (keeps domain pure of HTTP-flavored kinds) and service/project/errors.go (no per-service error files); services build errors inline via apierr constructors. - session_manager sentinels are apierr.Error values (pointer identity still works with errors.Is). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * revert(backend): drop GitChecker seam and isGitRepo case-sensitivity change Defer findings #4 (isGitRepo case-sensitivity) and #5 (GitChecker seam) out of this PR. Restores the original exec-based isGitRepo and the New(store) constructor; removes git.go, git_test.go, and the test-only export shims. The error-standardization and other findings are unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(session): translate engine errors to API errors at the facade The session_manager is the internal command engine and must not depend on the REST API error vocabulary. Revert its sentinels to plain errors.New values and move the engine→API translation into the service/session facade (toAPIError), which is the correct boundary. Controllers still see apierr.Error and never import the engine; the engine no longer imports internal/httpd/errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(session): tighten error comments to state what the code does Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style(envelope): make KindInternal an explicit case in httpStatus Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(apierr): rename package, test SpawnOrchestrator, parity fixes Address review feedback on PR #96: - Rename internal/httpd/errors → internal/httpd/apierr (package apierr) so importers no longer alias around the stdlib errors package. - Add a commander seam to session.Service and unit-test the relocated clean-orchestrator rule: clean=true kills all active orchestrators before spawning; clean=false spawns without kills. - project.Add: wrap the UpsertProject store error in apierr.Internal for parity with its sibling paths (was a raw 500). - Document that KindInternal is iota's zero value, so a zero-value Error defaults to 500. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
neversettle
GitHub
parent
e25b2ad4de
commit
7880f59cf5
|
|
@ -62,6 +62,10 @@ func (f *fakeSessionService) Spawn(_ context.Context, cfg ports.SpawnConfig) (do
|
|||
}, nil
|
||||
}
|
||||
|
||||
func (f *fakeSessionService) SpawnOrchestrator(ctx context.Context, projectID domain.ProjectID, _ bool) (domain.Session, error) {
|
||||
return f.Spawn(ctx, ports.SpawnConfig{ProjectID: projectID, Kind: domain.KindOrchestrator})
|
||||
}
|
||||
|
||||
func (f *fakeSessionService) Get(context.Context, domain.SessionID) (domain.Session, error) {
|
||||
return domain.Session{}, nil
|
||||
}
|
||||
|
|
@ -124,10 +128,10 @@ func startDriftTestDaemon(t *testing.T, sessions controllers.SessionService, pro
|
|||
t.Helper()
|
||||
|
||||
log := slog.New(slog.NewTextHandler(io.Discard, nil))
|
||||
router := httpd.NewRouterWithAPI(config.Config{}, log, nil, httpd.APIDeps{
|
||||
router := httpd.NewRouterWithControl(config.Config{}, log, nil, httpd.APIDeps{
|
||||
Sessions: sessions,
|
||||
Projects: projects,
|
||||
})
|
||||
}, httpd.ControlDeps{})
|
||||
srv := httptest.NewServer(router)
|
||||
t.Cleanup(srv.Close)
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,66 @@
|
|||
// Package apierr defines the REST API's error vocabulary: a single structured
|
||||
// error type every service returns and the controllers render into the locked
|
||||
// APIError envelope with one errors.As. It is deliberately scoped to the HTTP
|
||||
// API tree — these services exist to serve the daemon's REST surface — and
|
||||
// imports nothing, so any layer may depend on it without an import cycle.
|
||||
package apierr
|
||||
|
||||
// Kind is a semantic failure category. It is not an HTTP status or word: the
|
||||
// envelope layer is the only place a Kind is translated into one.
|
||||
type Kind int
|
||||
|
||||
const (
|
||||
// KindInternal is an unexpected failure; it maps to 500. As iota's zero
|
||||
// value it is also the Kind of a zero-value Error, so an Error built without
|
||||
// a Kind safely defaults to a 500.
|
||||
KindInternal Kind = iota
|
||||
// KindInvalid is malformed or rejected input; it maps to 400.
|
||||
KindInvalid
|
||||
// KindNotFound is a missing resource; it maps to 404.
|
||||
KindNotFound
|
||||
// KindConflict is a state/uniqueness clash; it maps to 409.
|
||||
KindConflict
|
||||
)
|
||||
|
||||
// Error is the structured error every service returns. Code is a stable machine
|
||||
// identifier (e.g. "SESSION_NOT_FOUND"); Message is the human-facing text. It
|
||||
// reaches the controller through fmt.Errorf("...: %w", err) wrapping and is
|
||||
// matched there with errors.As.
|
||||
type Error struct {
|
||||
Kind Kind
|
||||
Code string
|
||||
Message string
|
||||
Details map[string]any
|
||||
}
|
||||
|
||||
func (e *Error) Error() string {
|
||||
if e == nil {
|
||||
return ""
|
||||
}
|
||||
return e.Message
|
||||
}
|
||||
|
||||
// New builds an Error from its parts.
|
||||
func New(kind Kind, code, message string, details map[string]any) *Error {
|
||||
return &Error{Kind: kind, Code: code, Message: message, Details: details}
|
||||
}
|
||||
|
||||
// Invalid is a 400-class error.
|
||||
func Invalid(code, message string, details map[string]any) *Error {
|
||||
return New(KindInvalid, code, message, details)
|
||||
}
|
||||
|
||||
// NotFound is a 404-class error.
|
||||
func NotFound(code, message string) *Error {
|
||||
return New(KindNotFound, code, message, nil)
|
||||
}
|
||||
|
||||
// Conflict is a 409-class error.
|
||||
func Conflict(code, message string, details map[string]any) *Error {
|
||||
return New(KindConflict, code, message, details)
|
||||
}
|
||||
|
||||
// Internal is a 500-class error.
|
||||
func Internal(code, message string) *Error {
|
||||
return New(KindInternal, code, message, nil)
|
||||
}
|
||||
|
|
@ -20,7 +20,7 @@ import (
|
|||
// spec coverage, and the spec can't describe a route that isn't served.
|
||||
func TestRouteSpecParity(t *testing.T) {
|
||||
log := slog.New(slog.NewTextHandler(io.Discard, nil))
|
||||
router := httpd.NewRouter(config.Config{}, log, nil)
|
||||
router := httpd.NewRouterWithControl(config.Config{}, log, nil, httpd.APIDeps{}, httpd.ControlDeps{})
|
||||
|
||||
mounted := map[string]bool{}
|
||||
err := chi.Walk(router, func(method, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error {
|
||||
|
|
|
|||
|
|
@ -6,7 +6,6 @@ package controllers
|
|||
|
||||
import (
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"net/http"
|
||||
|
||||
"github.com/go-chi/chi/v5"
|
||||
|
|
@ -38,7 +37,7 @@ func (c *ProjectsController) list(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
projects, err := c.Mgr.List(r.Context())
|
||||
if err != nil {
|
||||
writeProjectError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
if projects == nil {
|
||||
|
|
@ -59,7 +58,7 @@ func (c *ProjectsController) add(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
p, err := c.Mgr.Add(r.Context(), in)
|
||||
if err != nil {
|
||||
writeProjectError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusCreated, ProjectResponse{Project: p})
|
||||
|
|
@ -72,7 +71,7 @@ func (c *ProjectsController) get(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
got, err := c.Mgr.Get(r.Context(), projectID(r))
|
||||
if err != nil {
|
||||
writeProjectError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
resp, err := newGetProjectResponse(got)
|
||||
|
|
@ -90,7 +89,7 @@ func (c *ProjectsController) remove(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
result, err := c.Mgr.Remove(r.Context(), projectID(r))
|
||||
if err != nil {
|
||||
writeProjectError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusOK, result)
|
||||
|
|
@ -103,27 +102,3 @@ func projectID(r *http.Request) domain.ProjectID {
|
|||
func decodeJSON(r *http.Request, out any) error {
|
||||
return json.NewDecoder(r.Body).Decode(out)
|
||||
}
|
||||
|
||||
// writeProjectError maps a projectsvc.Error to its HTTP status, falling back to
|
||||
// 500 for an unrecognized kind or a non-projectsvc.Error.
|
||||
func writeProjectError(w http.ResponseWriter, r *http.Request, err error) {
|
||||
var pe *projectsvc.Error
|
||||
if errors.As(err, &pe) {
|
||||
status := http.StatusInternalServerError
|
||||
switch pe.Kind {
|
||||
case "bad_request":
|
||||
status = http.StatusBadRequest
|
||||
case "not_found":
|
||||
status = http.StatusNotFound
|
||||
case "conflict":
|
||||
status = http.StatusConflict
|
||||
case "not_implemented":
|
||||
status = http.StatusNotImplemented
|
||||
case "internal":
|
||||
status = http.StatusInternalServerError
|
||||
}
|
||||
envelope.WriteAPIError(w, r, status, pe.Kind, pe.Code, pe.Message, pe.Details)
|
||||
return
|
||||
}
|
||||
envelope.WriteAPIError(w, r, http.StatusInternalServerError, "internal", "INTERNAL_ERROR", "Internal server error", nil)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -58,10 +58,10 @@ func TestProjectsAPI_GetEmptyResultIs500(t *testing.T) {
|
|||
|
||||
log := slog.New(slog.NewTextHandler(io.Discard, nil))
|
||||
|
||||
srv := httptest.NewServer(httpd.NewRouterWithAPI(config.Config{}, log, nil, httpd.APIDeps{
|
||||
srv := httptest.NewServer(httpd.NewRouterWithControl(config.Config{}, log, nil, httpd.APIDeps{
|
||||
|
||||
Projects: emptyGetManager{},
|
||||
}))
|
||||
}, httpd.ControlDeps{}))
|
||||
|
||||
t.Cleanup(srv.Close)
|
||||
|
||||
|
|
@ -89,10 +89,10 @@ func newTestServer(t *testing.T) *httptest.Server {
|
|||
|
||||
t.Cleanup(func() { _ = store.Close() })
|
||||
|
||||
srv := httptest.NewServer(httpd.NewRouterWithAPI(config.Config{}, log, nil, httpd.APIDeps{
|
||||
srv := httptest.NewServer(httpd.NewRouterWithControl(config.Config{}, log, nil, httpd.APIDeps{
|
||||
|
||||
Projects: projectsvc.New(store),
|
||||
}))
|
||||
}, httpd.ControlDeps{}))
|
||||
|
||||
t.Cleanup(srv.Close)
|
||||
|
||||
|
|
@ -104,7 +104,7 @@ func TestProjectsRoutes_DefaultToStubsWithoutManager(t *testing.T) {
|
|||
|
||||
log := slog.New(slog.NewTextHandler(io.Discard, nil))
|
||||
|
||||
srv := httptest.NewServer(httpd.NewRouter(config.Config{}, log, nil))
|
||||
srv := httptest.NewServer(httpd.NewRouterWithControl(config.Config{}, log, nil, httpd.APIDeps{}, httpd.ControlDeps{}))
|
||||
|
||||
t.Cleanup(srv.Close)
|
||||
|
||||
|
|
|
|||
|
|
@ -31,7 +31,7 @@ func (f *fakePRService) ResolveComments(_ context.Context, _ string, _ []string)
|
|||
func newPRTestServer(t *testing.T, svc prsvc.ActionManager) *httptest.Server {
|
||||
t.Helper()
|
||||
log := slog.New(slog.NewTextHandler(io.Discard, nil))
|
||||
srv := httptest.NewServer(httpd.NewRouterWithAPI(config.Config{}, log, nil, httpd.APIDeps{PRs: svc}))
|
||||
srv := httptest.NewServer(httpd.NewRouterWithControl(config.Config{}, log, nil, httpd.APIDeps{PRs: svc}, httpd.ControlDeps{}))
|
||||
t.Cleanup(srv.Close)
|
||||
return srv
|
||||
}
|
||||
|
|
|
|||
|
|
@ -15,7 +15,6 @@ import (
|
|||
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/envelope"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/ports"
|
||||
sessionsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/session"
|
||||
sessionmanager "github.com/aoagents/agent-orchestrator/backend/internal/session_manager"
|
||||
)
|
||||
|
||||
const (
|
||||
|
|
@ -27,6 +26,7 @@ const (
|
|||
type SessionService interface {
|
||||
List(ctx context.Context, filter sessionsvc.ListFilter) ([]domain.Session, error)
|
||||
Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.Session, error)
|
||||
SpawnOrchestrator(ctx context.Context, projectID domain.ProjectID, clean bool) (domain.Session, error)
|
||||
Get(ctx context.Context, id domain.SessionID) (domain.Session, error)
|
||||
Restore(ctx context.Context, id domain.SessionID) (domain.Session, error)
|
||||
Kill(ctx context.Context, id domain.SessionID) (bool, error)
|
||||
|
|
@ -68,7 +68,7 @@ func (c *SessionsController) list(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
sessions, err := c.Svc.List(r.Context(), filter)
|
||||
if err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusOK, ListSessionsResponse{Sessions: sessions})
|
||||
|
|
@ -97,7 +97,7 @@ func (c *SessionsController) spawn(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
sess, err := c.Svc.Spawn(r.Context(), ports.SpawnConfig{ProjectID: in.ProjectID, IssueID: in.IssueID, Kind: in.Kind, Harness: in.Harness, Branch: in.Branch, Prompt: in.Prompt, AgentRules: in.AgentRules})
|
||||
if err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusCreated, SessionResponse{Session: sess})
|
||||
|
|
@ -110,7 +110,7 @@ func (c *SessionsController) get(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
sess, err := c.Svc.Get(r.Context(), sessionID(r))
|
||||
if err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusOK, SessionResponse{Session: sess})
|
||||
|
|
@ -132,7 +132,7 @@ func (c *SessionsController) rename(w http.ResponseWriter, r *http.Request) {
|
|||
return
|
||||
}
|
||||
if err := c.Svc.Rename(r.Context(), sessionID(r), displayName); err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusOK, RenameSessionResponse{OK: true, SessionID: sessionID(r), DisplayName: displayName})
|
||||
|
|
@ -145,7 +145,7 @@ func (c *SessionsController) restore(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
sess, err := c.Svc.Restore(r.Context(), sessionID(r))
|
||||
if err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusOK, RestoreSessionResponse{OK: true, SessionID: sessionID(r), Session: sess})
|
||||
|
|
@ -158,7 +158,7 @@ func (c *SessionsController) kill(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
freed, err := c.Svc.Kill(r.Context(), sessionID(r))
|
||||
if err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusOK, KillSessionResponse{OK: true, SessionID: sessionID(r), Freed: freed})
|
||||
|
|
@ -171,7 +171,7 @@ func (c *SessionsController) cleanup(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
cleaned, err := c.Svc.Cleanup(r.Context(), domain.ProjectID(r.URL.Query().Get("project")))
|
||||
if err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusOK, CleanupSessionsResponse{OK: true, Cleaned: cleaned})
|
||||
|
|
@ -197,7 +197,7 @@ func (c *SessionsController) send(w http.ResponseWriter, r *http.Request) {
|
|||
}
|
||||
message := stripUnsafeControlChars(in.Message)
|
||||
if err := c.Svc.Send(r.Context(), sessionID(r), message); err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusOK, SendSessionMessageResponse{OK: true, SessionID: sessionID(r), Message: message})
|
||||
|
|
@ -217,23 +217,9 @@ func (c *SessionsController) spawnOrchestrator(w http.ResponseWriter, r *http.Re
|
|||
envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "PROJECT_ID_REQUIRED", "projectId is required", nil)
|
||||
return
|
||||
}
|
||||
if in.Clean {
|
||||
active := true
|
||||
orchestrators, err := c.Svc.List(r.Context(), sessionsvc.ListFilter{ProjectID: in.ProjectID, Active: &active, OrchestratorOnly: true})
|
||||
if err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
return
|
||||
}
|
||||
for _, existing := range orchestrators {
|
||||
if _, err := c.Svc.Kill(r.Context(), existing.ID); err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
return
|
||||
}
|
||||
}
|
||||
}
|
||||
sess, err := c.Svc.Spawn(r.Context(), ports.SpawnConfig{ProjectID: in.ProjectID, Kind: domain.KindOrchestrator})
|
||||
sess, err := c.Svc.SpawnOrchestrator(r.Context(), in.ProjectID, in.Clean)
|
||||
if err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusCreated, SpawnOrchestratorResponse{
|
||||
|
|
@ -248,7 +234,7 @@ func (c *SessionsController) listOrchestrators(w http.ResponseWriter, r *http.Re
|
|||
}
|
||||
sessions, err := c.Svc.List(r.Context(), sessionsvc.ListFilter{OrchestratorOnly: true})
|
||||
if err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusOK, ListSessionsResponse{Sessions: sessions})
|
||||
|
|
@ -261,11 +247,11 @@ func (c *SessionsController) getOrchestrator(w http.ResponseWriter, r *http.Requ
|
|||
}
|
||||
sess, err := c.Svc.Get(r.Context(), orchestratorID(r))
|
||||
if err != nil {
|
||||
writeSessionError(w, r, err)
|
||||
envelope.WriteError(w, r, err)
|
||||
return
|
||||
}
|
||||
if sess.Kind != domain.KindOrchestrator {
|
||||
writeSessionError(w, r, sessionmanager.ErrNotFound)
|
||||
envelope.WriteAPIError(w, r, http.StatusNotFound, "not_found", "SESSION_NOT_FOUND", "Unknown session", nil)
|
||||
return
|
||||
}
|
||||
envelope.WriteJSON(w, http.StatusOK, SessionResponse{Session: sess})
|
||||
|
|
@ -314,20 +300,3 @@ func stripUnsafeControlChars(message string) string {
|
|||
return r
|
||||
}, message)
|
||||
}
|
||||
|
||||
func writeSessionError(w http.ResponseWriter, r *http.Request, err error) {
|
||||
switch {
|
||||
case errors.Is(err, sessionmanager.ErrNotFound):
|
||||
envelope.WriteAPIError(w, r, http.StatusNotFound, "not_found", "SESSION_NOT_FOUND", "Unknown session", nil)
|
||||
case errors.Is(err, sessionmanager.ErrNotRestorable):
|
||||
envelope.WriteAPIError(w, r, http.StatusConflict, "conflict", "SESSION_NOT_RESTORABLE", "Session is not restorable", nil)
|
||||
case errors.Is(err, sessionmanager.ErrTerminated):
|
||||
envelope.WriteAPIError(w, r, http.StatusConflict, "conflict", "SESSION_TERMINATED", "Session is terminated", nil)
|
||||
case errors.Is(err, sessionmanager.ErrIncompleteHandle):
|
||||
envelope.WriteAPIError(w, r, http.StatusConflict, "conflict", "SESSION_INCOMPLETE_HANDLE", "Session is missing runtime or workspace handles", nil)
|
||||
case errors.Is(err, sessionmanager.ErrProjectNotResolvable):
|
||||
envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "PROJECT_NOT_RESOLVABLE", "Project is not registered or has no repo — register it with `ao project add`", nil)
|
||||
default:
|
||||
envelope.WriteAPIError(w, r, http.StatusInternalServerError, "internal", "SESSION_OPERATION_FAILED", "Session operation failed", nil)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -12,9 +12,9 @@ import (
|
|||
"github.com/aoagents/agent-orchestrator/backend/internal/config"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/httpd"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/apierr"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/ports"
|
||||
sessionsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/session"
|
||||
sessionmanager "github.com/aoagents/agent-orchestrator/backend/internal/session_manager"
|
||||
)
|
||||
|
||||
type fakeSessionService struct {
|
||||
|
|
@ -54,6 +54,22 @@ func (f *fakeSessionService) Spawn(_ context.Context, cfg ports.SpawnConfig) (do
|
|||
return s, nil
|
||||
}
|
||||
|
||||
func (f *fakeSessionService) SpawnOrchestrator(ctx context.Context, projectID domain.ProjectID, clean bool) (domain.Session, error) {
|
||||
if clean {
|
||||
active := true
|
||||
existing, err := f.List(ctx, sessionsvc.ListFilter{ProjectID: projectID, Active: &active, OrchestratorOnly: true})
|
||||
if err != nil {
|
||||
return domain.Session{}, err
|
||||
}
|
||||
for _, o := range existing {
|
||||
if _, err := f.Kill(ctx, o.ID); err != nil {
|
||||
return domain.Session{}, err
|
||||
}
|
||||
}
|
||||
}
|
||||
return f.Spawn(ctx, ports.SpawnConfig{ProjectID: projectID, Kind: domain.KindOrchestrator})
|
||||
}
|
||||
|
||||
func (f *fakeSessionService) Get(_ context.Context, id domain.SessionID) (domain.Session, error) {
|
||||
return f.sessions[id], nil
|
||||
}
|
||||
|
|
@ -85,7 +101,7 @@ func (f *fakeSessionService) Cleanup(_ context.Context, project domain.ProjectID
|
|||
func (f *fakeSessionService) Rename(_ context.Context, id domain.SessionID, displayName string) error {
|
||||
s, ok := f.sessions[id]
|
||||
if !ok {
|
||||
return sessionmanager.ErrNotFound
|
||||
return apierr.NotFound("SESSION_NOT_FOUND", "Unknown session")
|
||||
}
|
||||
s.DisplayName = displayName
|
||||
f.sessions[id] = s
|
||||
|
|
@ -100,14 +116,14 @@ func (f *fakeSessionService) Send(_ context.Context, _ domain.SessionID, message
|
|||
func newSessionTestServer(t *testing.T, svc *fakeSessionService) *httptest.Server {
|
||||
t.Helper()
|
||||
log := slog.New(slog.NewTextHandler(io.Discard, nil))
|
||||
srv := httptest.NewServer(httpd.NewRouterWithAPI(config.Config{}, log, nil, httpd.APIDeps{Sessions: svc}))
|
||||
srv := httptest.NewServer(httpd.NewRouterWithControl(config.Config{}, log, nil, httpd.APIDeps{Sessions: svc}, httpd.ControlDeps{}))
|
||||
t.Cleanup(srv.Close)
|
||||
return srv
|
||||
}
|
||||
|
||||
func TestSessionsRoutes_DefaultToStubsWithoutService(t *testing.T) {
|
||||
log := slog.New(slog.NewTextHandler(io.Discard, nil))
|
||||
srv := httptest.NewServer(httpd.NewRouter(config.Config{}, log, nil))
|
||||
srv := httptest.NewServer(httpd.NewRouterWithControl(config.Config{}, log, nil, httpd.APIDeps{}, httpd.ControlDeps{}))
|
||||
t.Cleanup(srv.Close)
|
||||
|
||||
body, status, headers := doRequest(t, srv, "GET", "/api/v1/sessions", "")
|
||||
|
|
|
|||
|
|
@ -2,9 +2,12 @@ package envelope
|
|||
|
||||
import (
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"net/http"
|
||||
|
||||
"github.com/go-chi/chi/v5/middleware"
|
||||
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/apierr"
|
||||
)
|
||||
|
||||
// APIError is the locked wire shape for every non-2xx response.
|
||||
|
|
@ -33,3 +36,33 @@ func WriteAPIError(w http.ResponseWriter, r *http.Request, status int, kind, cod
|
|||
Details: details,
|
||||
})
|
||||
}
|
||||
|
||||
// WriteError is the single path from any service error to the wire envelope. It
|
||||
// renders an *apierr.Error (anywhere in the chain) using its Kind, and falls
|
||||
// back to a 500 for any other error so internal details never leak. This is the
|
||||
// only place an apierr.Kind is translated into an HTTP status and wire word.
|
||||
func WriteError(w http.ResponseWriter, r *http.Request, err error) {
|
||||
var e *apierr.Error
|
||||
if errors.As(err, &e) {
|
||||
status, kind := httpStatus(e.Kind)
|
||||
WriteAPIError(w, r, status, kind, e.Code, e.Message, e.Details)
|
||||
return
|
||||
}
|
||||
WriteAPIError(w, r, http.StatusInternalServerError, "internal", "INTERNAL_ERROR", "Internal server error", nil)
|
||||
}
|
||||
|
||||
// httpStatus maps a semantic failure Kind to its HTTP status and wire word.
|
||||
func httpStatus(k apierr.Kind) (int, string) {
|
||||
switch k {
|
||||
case apierr.KindInvalid:
|
||||
return http.StatusBadRequest, "bad_request"
|
||||
case apierr.KindNotFound:
|
||||
return http.StatusNotFound, "not_found"
|
||||
case apierr.KindConflict:
|
||||
return http.StatusConflict, "conflict"
|
||||
case apierr.KindInternal:
|
||||
return http.StatusInternalServerError, "internal"
|
||||
default:
|
||||
return http.StatusInternalServerError, "internal"
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -9,7 +9,7 @@ import (
|
|||
)
|
||||
|
||||
func TestNewRouterAllowsNilLogger(t *testing.T) {
|
||||
router := NewRouter(config.Config{}, nil, nil)
|
||||
router := newTestRouter(config.Config{}, nil, nil)
|
||||
rec := httptest.NewRecorder()
|
||||
req := httptest.NewRequest(http.MethodGet, "/healthz", nil)
|
||||
router.ServeHTTP(rec, req)
|
||||
|
|
|
|||
|
|
@ -17,8 +17,16 @@ import (
|
|||
"github.com/aoagents/agent-orchestrator/backend/internal/terminal"
|
||||
)
|
||||
|
||||
// NewRouter builds the root router with the standard middleware stack and the
|
||||
// health probes mounted.
|
||||
// ControlDeps carries the daemon-control hooks the router exposes, such as the
|
||||
// callback that requests a graceful shutdown.
|
||||
type ControlDeps struct {
|
||||
RequestShutdown func()
|
||||
}
|
||||
|
||||
// NewRouterWithControl builds the root router with the standard middleware
|
||||
// stack, the API surface, and the daemon-control hooks wired from ControlDeps.
|
||||
// Missing Managers in deps keep routes registered but return OpenAPI-backed 501
|
||||
// responses.
|
||||
//
|
||||
// Middleware order (outermost first):
|
||||
//
|
||||
|
|
@ -29,24 +37,6 @@ import (
|
|||
//
|
||||
// The per-request timeout is deliberately not global: it wraps only bounded
|
||||
// REST routes, never long-lived terminal streams or health probes.
|
||||
func NewRouter(cfg config.Config, log *slog.Logger, termMgr *terminal.Manager) chi.Router {
|
||||
return NewRouterWithAPI(cfg, log, termMgr, APIDeps{})
|
||||
}
|
||||
|
||||
// ControlDeps carries the daemon-control hooks the router exposes, such as the
|
||||
// callback that requests a graceful shutdown.
|
||||
type ControlDeps struct {
|
||||
RequestShutdown func()
|
||||
}
|
||||
|
||||
// NewRouterWithAPI is the dependency-injected variant. Missing Managers keep
|
||||
// routes registered but return OpenAPI-backed 501 responses.
|
||||
func NewRouterWithAPI(cfg config.Config, log *slog.Logger, termMgr *terminal.Manager, deps APIDeps) chi.Router {
|
||||
return NewRouterWithControl(cfg, log, termMgr, deps, ControlDeps{})
|
||||
}
|
||||
|
||||
// NewRouterWithControl is NewRouterWithAPI plus daemon-control hooks: it mounts
|
||||
// the same API surface and additionally wires the ControlDeps callbacks.
|
||||
func NewRouterWithControl(cfg config.Config, log *slog.Logger, termMgr *terminal.Manager, deps APIDeps, control ControlDeps) chi.Router {
|
||||
log = loggerOrDefault(log)
|
||||
r := chi.NewRouter()
|
||||
|
|
|
|||
|
|
@ -29,15 +29,10 @@ type Server struct {
|
|||
shutdownOnce sync.Once
|
||||
}
|
||||
|
||||
// New constructs a Server and binds the listener immediately so a port
|
||||
// conflict fails fast — before any running.json is written. The caller owns
|
||||
// the returned Server's lifecycle via Run. termMgr may be nil, in which case
|
||||
// the /mux terminal surface is not mounted.
|
||||
func New(cfg config.Config, log *slog.Logger, termMgr *terminal.Manager) (*Server, error) {
|
||||
return NewWithDeps(cfg, log, termMgr, APIDeps{})
|
||||
}
|
||||
|
||||
// NewWithDeps constructs a Server with API dependencies supplied by the daemon.
|
||||
// NewWithDeps constructs a Server with API dependencies supplied by the daemon
|
||||
// and binds the listener immediately so a port conflict fails fast — before any
|
||||
// running.json is written. The caller owns the returned Server's lifecycle via
|
||||
// Run. termMgr may be nil, in which case the /mux terminal surface is not mounted.
|
||||
func NewWithDeps(cfg config.Config, log *slog.Logger, termMgr *terminal.Manager, deps APIDeps) (*Server, error) {
|
||||
log = loggerOrDefault(log)
|
||||
ln, err := net.Listen("tcp", cfg.Addr())
|
||||
|
|
|
|||
|
|
@ -19,7 +19,7 @@ func discardLogger() *slog.Logger {
|
|||
}
|
||||
|
||||
func TestHealthProbes(t *testing.T) {
|
||||
router := NewRouter(config.Config{}, discardLogger(), nil)
|
||||
router := newTestRouter(config.Config{}, discardLogger(), nil)
|
||||
srv := httptest.NewServer(router)
|
||||
defer srv.Close()
|
||||
|
||||
|
|
@ -51,7 +51,7 @@ func TestServerLifecycle(t *testing.T) {
|
|||
RunFilePath: runPath,
|
||||
}
|
||||
|
||||
srv, err := New(cfg, discardLogger(), nil)
|
||||
srv, err := NewWithDeps(cfg, discardLogger(), nil, APIDeps{})
|
||||
if err != nil {
|
||||
t.Fatalf("New: %v", err)
|
||||
}
|
||||
|
|
@ -100,7 +100,7 @@ func TestServerShutdownEndpoint(t *testing.T) {
|
|||
RunFilePath: runPath,
|
||||
}
|
||||
|
||||
srv, err := New(cfg, discardLogger(), nil)
|
||||
srv, err := NewWithDeps(cfg, discardLogger(), nil, APIDeps{})
|
||||
if err != nil {
|
||||
t.Fatalf("New: %v", err)
|
||||
}
|
||||
|
|
@ -159,7 +159,7 @@ func waitForHealth(t *testing.T, base string) {
|
|||
func TestNewFailsOnPortConflict(t *testing.T) {
|
||||
cfg := config.Config{Host: "127.0.0.1", Port: 0, RunFilePath: filepath.Join(t.TempDir(), "r.json")}
|
||||
|
||||
first, err := New(cfg, discardLogger(), nil)
|
||||
first, err := NewWithDeps(cfg, discardLogger(), nil, APIDeps{})
|
||||
if err != nil {
|
||||
t.Fatalf("first New: %v", err)
|
||||
}
|
||||
|
|
@ -167,7 +167,7 @@ func TestNewFailsOnPortConflict(t *testing.T) {
|
|||
|
||||
// Re-bind the exact port the first server took.
|
||||
conflict := config.Config{Host: "127.0.0.1", Port: first.boundPort(), RunFilePath: cfg.RunFilePath}
|
||||
if _, err := New(conflict, discardLogger(), nil); err == nil {
|
||||
if _, err := NewWithDeps(conflict, discardLogger(), nil, APIDeps{}); err == nil {
|
||||
t.Fatal("New on an already-bound port = nil error, want bind failure")
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -37,7 +37,7 @@ type terminalMuxFrame struct {
|
|||
|
||||
func dialMux(t *testing.T, mgr *terminal.Manager) (*websocket.Conn, func()) {
|
||||
t.Helper()
|
||||
router := NewRouter(config.Config{}, discardLogger(), mgr)
|
||||
router := newTestRouter(config.Config{}, discardLogger(), mgr)
|
||||
ts := httptest.NewServer(router)
|
||||
url := "ws" + strings.TrimPrefix(ts.URL, "http") + "/mux"
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,17 @@
|
|||
package httpd
|
||||
|
||||
import (
|
||||
"log/slog"
|
||||
|
||||
"github.com/go-chi/chi/v5"
|
||||
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/config"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/terminal"
|
||||
)
|
||||
|
||||
// newTestRouter builds a router with empty API and control deps. It is the
|
||||
// test-only convenience that used to be the exported NewRouter wrapper; keeping
|
||||
// it here leaves the package's exported surface to the production constructors.
|
||||
func newTestRouter(cfg config.Config, log *slog.Logger, termMgr *terminal.Manager) chi.Router {
|
||||
return NewRouterWithControl(cfg, log, termMgr, APIDeps{}, ControlDeps{})
|
||||
}
|
||||
|
|
@ -1,37 +0,0 @@
|
|||
package project
|
||||
|
||||
// Error is the service-level error shape controllers translate into the
|
||||
// locked HTTP APIError envelope without knowing store internals.
|
||||
type Error struct {
|
||||
Kind string
|
||||
Code string
|
||||
Message string
|
||||
Details map[string]any
|
||||
}
|
||||
|
||||
func (e *Error) Error() string {
|
||||
if e == nil {
|
||||
return ""
|
||||
}
|
||||
return e.Message
|
||||
}
|
||||
|
||||
func newError(kind, code, message string, details map[string]any) *Error {
|
||||
return &Error{Kind: kind, Code: code, Message: message, Details: details}
|
||||
}
|
||||
|
||||
func badRequest(code, message string, details map[string]any) *Error {
|
||||
return newError("bad_request", code, message, details)
|
||||
}
|
||||
|
||||
func notFound(code, message string) *Error {
|
||||
return newError("not_found", code, message, nil)
|
||||
}
|
||||
|
||||
func conflict(code, message string, details map[string]any) *Error {
|
||||
return newError("conflict", code, message, details)
|
||||
}
|
||||
|
||||
func internal(code, message string) *Error {
|
||||
return newError("internal", code, message, nil)
|
||||
}
|
||||
|
|
@ -11,6 +11,7 @@ import (
|
|||
"time"
|
||||
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/apierr"
|
||||
)
|
||||
|
||||
// Manager is the controller-facing contract for the /api/v1/projects surface.
|
||||
|
|
@ -46,7 +47,7 @@ func New(store Store) *Service {
|
|||
func (m *Service) List(ctx context.Context) ([]Summary, error) {
|
||||
projects, err := m.store.ListProjects(ctx)
|
||||
if err != nil {
|
||||
return nil, internal("PROJECTS_LIST_FAILED", "Failed to load projects")
|
||||
return nil, apierr.Internal("PROJECTS_LIST_FAILED", "Failed to load projects")
|
||||
}
|
||||
out := make([]Summary, 0, len(projects))
|
||||
for _, row := range projects {
|
||||
|
|
@ -66,10 +67,10 @@ func (m *Service) Get(ctx context.Context, id domain.ProjectID) (GetResult, erro
|
|||
}
|
||||
row, ok, err := m.store.GetProject(ctx, string(id))
|
||||
if err != nil {
|
||||
return GetResult{}, internal("PROJECT_LOAD_FAILED", "Failed to load project")
|
||||
return GetResult{}, apierr.Internal("PROJECT_LOAD_FAILED", "Failed to load project")
|
||||
}
|
||||
if !ok || !row.ArchivedAt.IsZero() {
|
||||
return GetResult{}, notFound("PROJECT_NOT_FOUND", "Unknown project")
|
||||
return GetResult{}, apierr.NotFound("PROJECT_NOT_FOUND", "Unknown project")
|
||||
}
|
||||
p := projectFromRow(row)
|
||||
return GetResult{Status: "ok", Project: &p}, nil
|
||||
|
|
@ -82,7 +83,7 @@ func (m *Service) Add(ctx context.Context, in AddInput) (Project, error) {
|
|||
return Project{}, err
|
||||
}
|
||||
if !isGitRepo(path) {
|
||||
return Project{}, badRequest("NOT_A_GIT_REPO", "Repository path must point to a git repository", nil)
|
||||
return Project{}, apierr.Invalid("NOT_A_GIT_REPO", "Repository path must point to a git repository", nil)
|
||||
}
|
||||
|
||||
id := defaultProjectID(path)
|
||||
|
|
@ -102,17 +103,17 @@ func (m *Service) Add(ctx context.Context, in AddInput) (Project, error) {
|
|||
}
|
||||
|
||||
if existing, ok, err := m.store.FindProjectByPath(ctx, path); err != nil {
|
||||
return Project{}, internal("PROJECT_LOAD_FAILED", "Failed to load project")
|
||||
return Project{}, apierr.Internal("PROJECT_LOAD_FAILED", "Failed to load project")
|
||||
} else if ok {
|
||||
return Project{}, conflict("PATH_ALREADY_REGISTERED", "A project at this path is already registered", map[string]any{
|
||||
return Project{}, apierr.Conflict("PATH_ALREADY_REGISTERED", "A project at this path is already registered", map[string]any{
|
||||
"existingProjectId": existing.ID,
|
||||
"suggestedProjectId": string(m.suggestID(ctx, id)),
|
||||
})
|
||||
}
|
||||
if existing, ok, err := m.store.GetProject(ctx, string(id)); err != nil {
|
||||
return Project{}, internal("PROJECT_LOAD_FAILED", "Failed to load project")
|
||||
return Project{}, apierr.Internal("PROJECT_LOAD_FAILED", "Failed to load project")
|
||||
} else if ok && existing.ArchivedAt.IsZero() && existing.Path != path {
|
||||
return Project{}, conflict("ID_ALREADY_REGISTERED", "A project with this id is already registered for a different path", map[string]any{
|
||||
return Project{}, apierr.Conflict("ID_ALREADY_REGISTERED", "A project with this id is already registered for a different path", map[string]any{
|
||||
"existingProjectId": existing.ID,
|
||||
"suggestedProjectId": string(m.suggestID(ctx, id)),
|
||||
})
|
||||
|
|
@ -125,7 +126,7 @@ func (m *Service) Add(ctx context.Context, in AddInput) (Project, error) {
|
|||
RegisteredAt: time.Now(),
|
||||
}
|
||||
if err := m.store.UpsertProject(ctx, row); err != nil {
|
||||
return Project{}, err
|
||||
return Project{}, apierr.Internal("PROJECT_ADD_FAILED", "Failed to register project")
|
||||
}
|
||||
return projectFromRow(row), nil
|
||||
}
|
||||
|
|
@ -137,10 +138,10 @@ func (m *Service) Remove(ctx context.Context, id domain.ProjectID) (RemoveResult
|
|||
}
|
||||
ok, err := m.store.ArchiveProject(ctx, string(id), time.Now())
|
||||
if err != nil {
|
||||
return RemoveResult{}, internal("PROJECT_REMOVE_FAILED", "Failed to remove project")
|
||||
return RemoveResult{}, apierr.Internal("PROJECT_REMOVE_FAILED", "Failed to remove project")
|
||||
}
|
||||
if !ok {
|
||||
return RemoveResult{}, notFound("PROJECT_NOT_FOUND", "Unknown project")
|
||||
return RemoveResult{}, apierr.NotFound("PROJECT_NOT_FOUND", "Unknown project")
|
||||
}
|
||||
return RemoveResult{ProjectID: id, RemovedStorageDir: false}, nil
|
||||
}
|
||||
|
|
@ -174,12 +175,12 @@ func displayName(row domain.ProjectRecord) string {
|
|||
func normalizePath(raw string) (string, error) {
|
||||
raw = strings.TrimSpace(raw)
|
||||
if raw == "" {
|
||||
return "", badRequest("PATH_REQUIRED", "Repository path is required", nil)
|
||||
return "", apierr.Invalid("PATH_REQUIRED", "Repository path is required", nil)
|
||||
}
|
||||
if strings.HasPrefix(raw, "~") {
|
||||
home, err := os.UserHomeDir()
|
||||
if err != nil {
|
||||
return "", badRequest("INVALID_PATH", "Repository path could not be expanded", nil)
|
||||
return "", apierr.Invalid("INVALID_PATH", "Repository path could not be expanded", nil)
|
||||
}
|
||||
if raw == "~" {
|
||||
raw = home
|
||||
|
|
@ -189,7 +190,7 @@ func normalizePath(raw string) (string, error) {
|
|||
}
|
||||
abs, err := filepath.Abs(raw)
|
||||
if err != nil {
|
||||
return "", badRequest("INVALID_PATH", "Repository path is invalid", nil)
|
||||
return "", apierr.Invalid("INVALID_PATH", "Repository path is invalid", nil)
|
||||
}
|
||||
return filepath.Clean(abs), nil
|
||||
}
|
||||
|
|
@ -232,7 +233,7 @@ func validateProjectID(id domain.ProjectID) error {
|
|||
// (e.g. "a..b") passes it yet yields a branch like "ao/a..b-1" that git's
|
||||
// check-ref-format rejects — surfacing as an opaque 500 at spawn time.
|
||||
if raw == "" || raw == "." || strings.Contains(raw, "..") || strings.ContainsAny(raw, `/\`) || !projectIDPattern.MatchString(raw) {
|
||||
return badRequest("INVALID_PROJECT_ID", "Project id failed storage-path validation", nil)
|
||||
return apierr.Invalid("INVALID_PROJECT_ID", "Project id failed storage-path validation", nil)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import (
|
|||
"testing"
|
||||
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/apierr"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/service/project"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/storage/sqlite"
|
||||
)
|
||||
|
|
@ -35,12 +36,12 @@ func gitRepo(t *testing.T) string {
|
|||
|
||||
func ptr(s string) *string { return &s }
|
||||
|
||||
// wantCode asserts err is a *project.Error carrying the given machine code.
|
||||
// wantCode asserts err is an *apierr.Error carrying the given machine code.
|
||||
func wantCode(t *testing.T, err error, code string) {
|
||||
t.Helper()
|
||||
var e *project.Error
|
||||
var e *apierr.Error
|
||||
if !errors.As(err, &e) {
|
||||
t.Fatalf("error = %v, want *project.Error", err)
|
||||
t.Fatalf("error = %v, want *apierr.Error", err)
|
||||
}
|
||||
if e.Code != code {
|
||||
t.Fatalf("code = %q, want %q", e.Code, code)
|
||||
|
|
|
|||
|
|
@ -2,11 +2,13 @@ package session
|
|||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/apierr"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/ports"
|
||||
sessionmanager "github.com/aoagents/agent-orchestrator/backend/internal/session_manager"
|
||||
)
|
||||
|
|
@ -28,11 +30,21 @@ type ListFilter struct {
|
|||
Fresh bool
|
||||
}
|
||||
|
||||
// commander is the command-side surface Service delegates to: the
|
||||
// *sessionmanager.Manager in production, a fake in tests.
|
||||
type commander interface {
|
||||
Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.SessionRecord, error)
|
||||
Restore(ctx context.Context, id domain.SessionID) (domain.SessionRecord, error)
|
||||
Kill(ctx context.Context, id domain.SessionID) (bool, error)
|
||||
Send(ctx context.Context, id domain.SessionID, message string) error
|
||||
Cleanup(ctx context.Context, project domain.ProjectID) ([]domain.SessionID, error)
|
||||
}
|
||||
|
||||
// Service is the controller-facing session service. It delegates command-side
|
||||
// session operations to the internal sessionmanager.Manager and owns read-model
|
||||
// assembly, including user-facing display status derivation.
|
||||
type Service struct {
|
||||
manager *sessionmanager.Manager
|
||||
manager commander
|
||||
store Store
|
||||
}
|
||||
|
||||
|
|
@ -45,42 +57,63 @@ func New(manager *sessionmanager.Manager, store Store) *Service {
|
|||
func (s *Service) Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.Session, error) {
|
||||
rec, err := s.manager.Spawn(ctx, cfg)
|
||||
if err != nil {
|
||||
return domain.Session{}, err
|
||||
return domain.Session{}, toAPIError(err)
|
||||
}
|
||||
return s.toSession(ctx, rec)
|
||||
}
|
||||
|
||||
// SpawnOrchestrator spawns an orchestrator session for a project. When clean is
|
||||
// true it first tears down any active orchestrator(s) for that project so the new
|
||||
// one is the only live coordinator — a business rule that belongs here, not in the
|
||||
// HTTP controller.
|
||||
func (s *Service) SpawnOrchestrator(ctx context.Context, projectID domain.ProjectID, clean bool) (domain.Session, error) {
|
||||
if clean {
|
||||
active := true
|
||||
existing, err := s.List(ctx, ListFilter{ProjectID: projectID, Active: &active, OrchestratorOnly: true})
|
||||
if err != nil {
|
||||
return domain.Session{}, err
|
||||
}
|
||||
for _, orch := range existing {
|
||||
if _, err := s.Kill(ctx, orch.ID); err != nil {
|
||||
return domain.Session{}, err
|
||||
}
|
||||
}
|
||||
}
|
||||
return s.Spawn(ctx, ports.SpawnConfig{ProjectID: projectID, Kind: domain.KindOrchestrator})
|
||||
}
|
||||
|
||||
// Restore relaunches a terminated session and returns the API-facing read model.
|
||||
func (s *Service) Restore(ctx context.Context, id domain.SessionID) (domain.Session, error) {
|
||||
rec, err := s.manager.Restore(ctx, id)
|
||||
if err != nil {
|
||||
return domain.Session{}, err
|
||||
return domain.Session{}, toAPIError(err)
|
||||
}
|
||||
return s.toSession(ctx, rec)
|
||||
}
|
||||
|
||||
// Kill delegates terminal intent and teardown to the internal manager.
|
||||
func (s *Service) Kill(ctx context.Context, id domain.SessionID) (bool, error) {
|
||||
return s.manager.Kill(ctx, id)
|
||||
freed, err := s.manager.Kill(ctx, id)
|
||||
return freed, toAPIError(err)
|
||||
}
|
||||
|
||||
// Send delegates agent messaging to the internal manager.
|
||||
func (s *Service) Send(ctx context.Context, id domain.SessionID, message string) error {
|
||||
return s.manager.Send(ctx, id, message)
|
||||
return toAPIError(s.manager.Send(ctx, id, message))
|
||||
}
|
||||
|
||||
// Rename updates the user-facing session display name.
|
||||
func (s *Service) Rename(ctx context.Context, id domain.SessionID, displayName string) error {
|
||||
displayName = strings.TrimSpace(displayName)
|
||||
if displayName == "" {
|
||||
return fmt.Errorf("rename %s: display name is required", id)
|
||||
return apierr.Invalid("DISPLAY_NAME_REQUIRED", "Display name is required", nil)
|
||||
}
|
||||
renamed, err := s.store.RenameSession(ctx, id, displayName, time.Now().UTC())
|
||||
if err != nil {
|
||||
return fmt.Errorf("rename %s: %w", id, err)
|
||||
}
|
||||
if !renamed {
|
||||
return fmt.Errorf("rename %s: %w", id, sessionmanager.ErrNotFound)
|
||||
return apierr.NotFound("SESSION_NOT_FOUND", "Unknown session")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
|
@ -138,18 +171,40 @@ func matchesSessionFilter(rec domain.SessionRecord, filter ListFilter) bool {
|
|||
return true
|
||||
}
|
||||
|
||||
// Get returns one session as an enriched display model, or sessionmanager.ErrNotFound if it is absent.
|
||||
// Get returns one session as an enriched display model, or an apierr.NotFound
|
||||
// (SESSION_NOT_FOUND) if it is absent.
|
||||
func (s *Service) Get(ctx context.Context, id domain.SessionID) (domain.Session, error) {
|
||||
rec, ok, err := s.store.GetSession(ctx, id)
|
||||
if err != nil {
|
||||
return domain.Session{}, fmt.Errorf("get %s: %w", id, err)
|
||||
}
|
||||
if !ok {
|
||||
return domain.Session{}, fmt.Errorf("get %s: %w", id, sessionmanager.ErrNotFound)
|
||||
return domain.Session{}, apierr.NotFound("SESSION_NOT_FOUND", "Unknown session")
|
||||
}
|
||||
return s.toSession(ctx, rec)
|
||||
}
|
||||
|
||||
// toAPIError maps the session engine's sentinel errors to their REST API
|
||||
// equivalents; an unrecognized error passes through and surfaces as a 500.
|
||||
func toAPIError(err error) error {
|
||||
switch {
|
||||
case err == nil:
|
||||
return nil
|
||||
case errors.Is(err, sessionmanager.ErrNotFound):
|
||||
return apierr.NotFound("SESSION_NOT_FOUND", "Unknown session")
|
||||
case errors.Is(err, sessionmanager.ErrNotRestorable):
|
||||
return apierr.Conflict("SESSION_NOT_RESTORABLE", "Session is not restorable", nil)
|
||||
case errors.Is(err, sessionmanager.ErrTerminated):
|
||||
return apierr.Conflict("SESSION_TERMINATED", "Session is terminated", nil)
|
||||
case errors.Is(err, sessionmanager.ErrIncompleteHandle):
|
||||
return apierr.Conflict("SESSION_INCOMPLETE_HANDLE", "Session is missing runtime or workspace handles", nil)
|
||||
case errors.Is(err, sessionmanager.ErrProjectNotResolvable):
|
||||
return apierr.Invalid("PROJECT_NOT_RESOLVABLE", "Project is not registered or has no repo — register it with `ao project add`", nil)
|
||||
default:
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
func (s *Service) toSession(ctx context.Context, rec domain.SessionRecord) (domain.Session, error) {
|
||||
pr, ok, err := s.store.GetDisplayPRFactsForSession(ctx, rec.ID)
|
||||
if err != nil {
|
||||
|
|
|
|||
|
|
@ -8,7 +8,8 @@ import (
|
|||
"time"
|
||||
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
|
||||
sessionmanager "github.com/aoagents/agent-orchestrator/backend/internal/session_manager"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/apierr"
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/ports"
|
||||
)
|
||||
|
||||
type fakeStore struct {
|
||||
|
|
@ -98,7 +99,72 @@ func TestSessionRenameMissingSessionReturnsNotFound(t *testing.T) {
|
|||
st := newFakeStore()
|
||||
|
||||
err := (&Service{store: st}).Rename(context.Background(), "mer-404", "Missing")
|
||||
if !errors.Is(err, sessionmanager.ErrNotFound) {
|
||||
t.Fatalf("err = %v, want ErrNotFound", err)
|
||||
var e *apierr.Error
|
||||
if !errors.As(err, &e) || e.Kind != apierr.KindNotFound || e.Code != "SESSION_NOT_FOUND" {
|
||||
t.Fatalf("err = %v, want apierr NotFound SESSION_NOT_FOUND", err)
|
||||
}
|
||||
}
|
||||
|
||||
// fakeCommander records Kill/Spawn calls so a test can assert the
|
||||
// clean-orchestrator ordering without wiring a real session engine.
|
||||
type fakeCommander struct {
|
||||
killed []domain.SessionID
|
||||
spawned bool
|
||||
killsAtSpawn int
|
||||
}
|
||||
|
||||
func (f *fakeCommander) Spawn(_ context.Context, cfg ports.SpawnConfig) (domain.SessionRecord, error) {
|
||||
f.spawned = true
|
||||
f.killsAtSpawn = len(f.killed)
|
||||
return domain.SessionRecord{ID: "mer-9", ProjectID: cfg.ProjectID, Kind: cfg.Kind}, nil
|
||||
}
|
||||
func (f *fakeCommander) Restore(context.Context, domain.SessionID) (domain.SessionRecord, error) {
|
||||
return domain.SessionRecord{}, nil
|
||||
}
|
||||
func (f *fakeCommander) Kill(_ context.Context, id domain.SessionID) (bool, error) {
|
||||
f.killed = append(f.killed, id)
|
||||
return true, nil
|
||||
}
|
||||
func (f *fakeCommander) Send(context.Context, domain.SessionID, string) error { return nil }
|
||||
func (f *fakeCommander) Cleanup(context.Context, domain.ProjectID) ([]domain.SessionID, error) {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func TestSpawnOrchestratorCleanKillsActiveOrchestratorsBeforeSpawn(t *testing.T) {
|
||||
st := newFakeStore()
|
||||
// Two active orchestrators plus an unrelated worker and a terminated
|
||||
// orchestrator that must be left alone.
|
||||
st.sessions["mer-1"] = domain.SessionRecord{ID: "mer-1", ProjectID: "mer", Kind: domain.KindOrchestrator}
|
||||
st.sessions["mer-2"] = domain.SessionRecord{ID: "mer-2", ProjectID: "mer", Kind: domain.KindOrchestrator}
|
||||
st.sessions["mer-3"] = domain.SessionRecord{ID: "mer-3", ProjectID: "mer", Kind: domain.KindWorker}
|
||||
st.sessions["mer-4"] = domain.SessionRecord{ID: "mer-4", ProjectID: "mer", Kind: domain.KindOrchestrator, IsTerminated: true}
|
||||
|
||||
fc := &fakeCommander{}
|
||||
svc := &Service{manager: fc, store: st}
|
||||
|
||||
if _, err := svc.SpawnOrchestrator(context.Background(), "mer", true); err != nil {
|
||||
t.Fatalf("SpawnOrchestrator: %v", err)
|
||||
}
|
||||
|
||||
if len(fc.killed) != 2 {
|
||||
t.Fatalf("killed = %v, want the two active orchestrators", fc.killed)
|
||||
}
|
||||
if !fc.spawned || fc.killsAtSpawn != 2 {
|
||||
t.Fatalf("spawn must run after both kills: spawned=%v killsAtSpawn=%d", fc.spawned, fc.killsAtSpawn)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSpawnOrchestratorNoCleanSkipsKills(t *testing.T) {
|
||||
st := newFakeStore()
|
||||
st.sessions["mer-1"] = domain.SessionRecord{ID: "mer-1", ProjectID: "mer", Kind: domain.KindOrchestrator}
|
||||
|
||||
fc := &fakeCommander{}
|
||||
svc := &Service{manager: fc, store: st}
|
||||
|
||||
if _, err := svc.SpawnOrchestrator(context.Background(), "mer", false); err != nil {
|
||||
t.Fatalf("SpawnOrchestrator: %v", err)
|
||||
}
|
||||
if len(fc.killed) != 0 || !fc.spawned {
|
||||
t.Fatalf("clean=false must spawn without kills: killed=%v spawned=%v", fc.killed, fc.spawned)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -12,7 +12,8 @@ import (
|
|||
"github.com/aoagents/agent-orchestrator/backend/internal/ports"
|
||||
)
|
||||
|
||||
// Sentinel errors returned by the Session Manager.
|
||||
// Sentinel errors returned by the Session Manager; callers match them with
|
||||
// errors.Is.
|
||||
var (
|
||||
ErrNotFound = errors.New("session: not found")
|
||||
ErrNotRestorable = errors.New("session: not restorable (not terminal)")
|
||||
|
|
|
|||
Loading…
Reference in New Issue