From a86ae085b385e21f7db7f9b20793d74a46b22679 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Wed, 27 May 2026 19:23:20 +0530 Subject: [PATCH 01/23] =?UTF-8?q?feat(backend):=20HTTP=20daemon=20skeleton?= =?UTF-8?q?=20=E2=80=94=20config,=20health,=20runfile,=20graceful=20shutdo?= =?UTF-8?q?wn=20(#10)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1a of the Go HTTP daemon lane (#10). Stands up the loopback-only sidecar skeleton the later REST/SSE/WS/static surfaces build on: - config: env-driven (AO_HOST/PORT/ENV/timeouts/run-file) with zero-config defaults; binds 127.0.0.1:3001; validates and fails fast on bad input. - httpd: chi router with the recoverer → request-id → logger → real-ip middleware stack and /healthz + /readyz probes. Per-request timeout is carried in config but intentionally not global — it scopes to /api/v1 in Phase 1b so it never throttles SSE/WS/health. - runfile: atomic PID + port handshake (running.json) for the Electron supervisor, with a dead-PID stale check so a crashed predecessor doesn't block startup while a live one fails fast. - server: bind-before-publish (port conflict fails fast), graceful shutdown on SIGINT/SIGTERM via signal.NotifyContext with a 10s hard timeout, and run-file cleanup on exit. Why: the daemon must be safely supervisable as a child process — the supervisor needs a discoverable PID/port and the daemon must not leave a half-started process or stale handshake behind. Locking the lifecycle down now keeps the future port split a small change rather than a rewrite. Tests cover config defaults/overrides/validation, run-file round-trip and live/dead PID detection, health probes, full Run lifecycle, and port-conflict fail-fast. Co-Authored-By: Claude Opus 4.7 --- backend/go.mod | 2 + backend/go.sum | 2 + backend/internal/config/config.go | 133 +++++++++++++++++++++++ backend/internal/config/config_test.go | 91 ++++++++++++++++ backend/internal/httpd/json.go | 17 +++ backend/internal/httpd/router.go | 62 +++++++++++ backend/internal/httpd/server.go | 113 +++++++++++++++++++ backend/internal/httpd/server_test.go | 126 +++++++++++++++++++++ backend/internal/runfile/runfile.go | 124 +++++++++++++++++++++ backend/internal/runfile/runfile_test.go | 96 ++++++++++++++++ backend/main.go | 62 ++++++++++- 11 files changed, 826 insertions(+), 2 deletions(-) create mode 100644 backend/go.sum create mode 100644 backend/internal/config/config.go create mode 100644 backend/internal/config/config_test.go create mode 100644 backend/internal/httpd/json.go create mode 100644 backend/internal/httpd/router.go create mode 100644 backend/internal/httpd/server.go create mode 100644 backend/internal/httpd/server_test.go create mode 100644 backend/internal/runfile/runfile.go create mode 100644 backend/internal/runfile/runfile_test.go diff --git a/backend/go.mod b/backend/go.mod index 22a555cd..311cea28 100644 --- a/backend/go.mod +++ b/backend/go.mod @@ -1,3 +1,5 @@ module github.com/aoagents/agent-orchestrator/backend go 1.22 + +require github.com/go-chi/chi/v5 v5.1.0 diff --git a/backend/go.sum b/backend/go.sum new file mode 100644 index 00000000..823cdbb1 --- /dev/null +++ b/backend/go.sum @@ -0,0 +1,2 @@ +github.com/go-chi/chi/v5 v5.1.0 h1:acVI1TYaD+hhedDJ3r54HyA6sExp3HfXq7QWEEY/xMw= +github.com/go-chi/chi/v5 v5.1.0/go.mod h1:DslCQbL2OYiznFReuXYUmQ2hGd1aDpCnlMNITLSKoi8= diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go new file mode 100644 index 00000000..9d4b33b7 --- /dev/null +++ b/backend/internal/config/config.go @@ -0,0 +1,133 @@ +// Package config loads the daemon's runtime configuration. The HTTP daemon is +// a loopback-only sidecar: it binds 127.0.0.1, takes no public traffic, and +// reads everything it needs from the environment with sane defaults so it can +// boot with zero configuration in development. +package config + +import ( + "fmt" + "os" + "path/filepath" + "strconv" + "time" +) + +const ( + // DefaultHost is loopback only. The daemon must never bind a public + // interface — it speaks to the Electron main process over 127.0.0.1. + DefaultHost = "127.0.0.1" + // DefaultPort is the single port the whole surface (REST, SSE, WS, static) + // is served from. Single-port keeps it same-origin: no CORS, one lifecycle. + DefaultPort = 3001 + // DefaultRequestTimeout bounds a single request. Long-lived surfaces (SSE, + // WS) are mounted outside this timeout; it guards the REST surface only. + DefaultRequestTimeout = 60 * time.Second + // DefaultShutdownTimeout is the hard cap on graceful shutdown. After this + // the process exits even if connections are still draining. + DefaultShutdownTimeout = 10 * time.Second +) + +// Config is the fully-resolved daemon configuration. It is immutable once +// built by Load. +type Config struct { + // Host is the bind address. Always loopback in normal operation. + Host string + // Port is the TCP port to bind. The daemon fails fast if it is taken. + Port int + // Env is the deployment environment label ("development" | "production"). + // It only affects log verbosity / formatting, never bind behaviour. + Env string + // RequestTimeout bounds REST request handling. + RequestTimeout time.Duration + // ShutdownTimeout is the hard graceful-shutdown deadline. + ShutdownTimeout time.Duration + // RunFilePath is where the PID + port handshake file (running.json) is + // written so the Electron supervisor can discover and reap the daemon. + RunFilePath string +} + +// Addr returns the host:port the HTTP server binds. +func (c Config) Addr() string { + return fmt.Sprintf("%s:%d", c.Host, c.Port) +} + +// IsProduction reports whether the daemon is running in production mode. +func (c Config) IsProduction() bool { return c.Env == "production" } + +// Load resolves configuration from the environment, applying defaults. It +// returns an error only for values that are present but malformed (e.g. a +// non-numeric AO_PORT); missing values fall back to defaults. +// +// Recognised variables: +// +// AO_HOST bind host (default 127.0.0.1) +// AO_PORT bind port (default 3001) +// AO_ENV environment label (default development) +// AO_REQUEST_TIMEOUT per-request timeout (Go duration, default 60s) +// AO_SHUTDOWN_TIMEOUT shutdown deadline (Go duration, default 10s) +// AO_RUN_FILE running.json path (default /running.json) +func Load() (Config, error) { + cfg := Config{ + Host: getEnv("AO_HOST", DefaultHost), + Port: DefaultPort, + Env: getEnv("AO_ENV", "development"), + RequestTimeout: DefaultRequestTimeout, + ShutdownTimeout: DefaultShutdownTimeout, + } + + if raw := os.Getenv("AO_PORT"); raw != "" { + port, err := strconv.Atoi(raw) + if err != nil { + return Config{}, fmt.Errorf("invalid AO_PORT %q: %w", raw, err) + } + if port < 1 || port > 65535 { + return Config{}, fmt.Errorf("invalid AO_PORT %d: out of range 1-65535", port) + } + cfg.Port = port + } + + if raw := os.Getenv("AO_REQUEST_TIMEOUT"); raw != "" { + d, err := time.ParseDuration(raw) + if err != nil { + return Config{}, fmt.Errorf("invalid AO_REQUEST_TIMEOUT %q: %w", raw, err) + } + cfg.RequestTimeout = d + } + + if raw := os.Getenv("AO_SHUTDOWN_TIMEOUT"); raw != "" { + d, err := time.ParseDuration(raw) + if err != nil { + return Config{}, fmt.Errorf("invalid AO_SHUTDOWN_TIMEOUT %q: %w", raw, err) + } + cfg.ShutdownTimeout = d + } + + runFile, err := resolveRunFilePath() + if err != nil { + return Config{}, err + } + cfg.RunFilePath = runFile + + return cfg, nil +} + +// resolveRunFilePath picks where running.json lives. An explicit AO_RUN_FILE +// wins; otherwise it sits under the per-user state directory so multiple repos +// share one supervisor handshake location. +func resolveRunFilePath() (string, error) { + if p, ok := os.LookupEnv("AO_RUN_FILE"); ok && p != "" { + return p, nil + } + dir, err := os.UserConfigDir() + if err != nil { + return "", fmt.Errorf("resolve state dir: %w", err) + } + return filepath.Join(dir, "agent-orchestrator", "running.json"), nil +} + +func getEnv(key, fallback string) string { + if v, ok := os.LookupEnv(key); ok && v != "" { + return v + } + return fallback +} diff --git a/backend/internal/config/config_test.go b/backend/internal/config/config_test.go new file mode 100644 index 00000000..3eef47c6 --- /dev/null +++ b/backend/internal/config/config_test.go @@ -0,0 +1,91 @@ +package config + +import ( + "testing" + "time" +) + +func TestLoadDefaults(t *testing.T) { + // Clear every recognised var so we observe pure defaults regardless of the + // surrounding environment. + for _, k := range []string{"AO_HOST", "AO_PORT", "AO_ENV", "AO_REQUEST_TIMEOUT", "AO_SHUTDOWN_TIMEOUT", "AO_RUN_FILE"} { + t.Setenv(k, "") + } + + cfg, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.Host != DefaultHost { + t.Errorf("Host = %q, want %q", cfg.Host, DefaultHost) + } + if cfg.Port != DefaultPort { + t.Errorf("Port = %d, want %d", cfg.Port, DefaultPort) + } + if cfg.Env != "development" { + t.Errorf("Env = %q, want development", cfg.Env) + } + if cfg.RequestTimeout != DefaultRequestTimeout { + t.Errorf("RequestTimeout = %s, want %s", cfg.RequestTimeout, DefaultRequestTimeout) + } + if cfg.ShutdownTimeout != DefaultShutdownTimeout { + t.Errorf("ShutdownTimeout = %s, want %s", cfg.ShutdownTimeout, DefaultShutdownTimeout) + } + if cfg.RunFilePath == "" { + t.Error("RunFilePath is empty, want a resolved default path") + } + if cfg.IsProduction() { + t.Error("IsProduction() = true for development env") + } +} + +func TestLoadOverrides(t *testing.T) { + t.Setenv("AO_HOST", "127.0.0.2") + t.Setenv("AO_PORT", "4002") + t.Setenv("AO_ENV", "production") + t.Setenv("AO_REQUEST_TIMEOUT", "5s") + t.Setenv("AO_SHUTDOWN_TIMEOUT", "3s") + t.Setenv("AO_RUN_FILE", "/tmp/ao-test-running.json") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load: %v", err) + } + if cfg.Addr() != "127.0.0.2:4002" { + t.Errorf("Addr() = %q, want 127.0.0.2:4002", cfg.Addr()) + } + if !cfg.IsProduction() { + t.Error("IsProduction() = false, want true") + } + if cfg.RequestTimeout != 5*time.Second { + t.Errorf("RequestTimeout = %s, want 5s", cfg.RequestTimeout) + } + if cfg.ShutdownTimeout != 3*time.Second { + t.Errorf("ShutdownTimeout = %s, want 3s", cfg.ShutdownTimeout) + } + if cfg.RunFilePath != "/tmp/ao-test-running.json" { + t.Errorf("RunFilePath = %q, want /tmp/ao-test-running.json", cfg.RunFilePath) + } +} + +func TestLoadInvalid(t *testing.T) { + tests := []struct { + name string + env map[string]string + }{ + {"non-numeric port", map[string]string{"AO_PORT": "abc"}}, + {"port out of range", map[string]string{"AO_PORT": "70000"}}, + {"bad request timeout", map[string]string{"AO_REQUEST_TIMEOUT": "soon"}}, + {"bad shutdown timeout", map[string]string{"AO_SHUTDOWN_TIMEOUT": "later"}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + for k, v := range tc.env { + t.Setenv(k, v) + } + if _, err := Load(); err == nil { + t.Fatal("Load() = nil error, want error") + } + }) + } +} diff --git a/backend/internal/httpd/json.go b/backend/internal/httpd/json.go new file mode 100644 index 00000000..9b87461f --- /dev/null +++ b/backend/internal/httpd/json.go @@ -0,0 +1,17 @@ +package httpd + +import ( + "encoding/json" + "net/http" +) + +// writeJSON serialises v as JSON with the given status. It is the single JSON +// writer for the skeleton; the typed error envelope (open item Q1.3) will build +// on this in a later phase. +func writeJSON(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(status) + // A write error here means the client went away mid-response; there is + // nothing useful to do but stop. + _ = json.NewEncoder(w).Encode(v) +} diff --git a/backend/internal/httpd/router.go b/backend/internal/httpd/router.go new file mode 100644 index 00000000..1d366b69 --- /dev/null +++ b/backend/internal/httpd/router.go @@ -0,0 +1,62 @@ +// Package httpd builds and runs the daemon's HTTP surface. Phase 1a is the +// skeleton: the middleware stack, liveness/readiness probes, and a graceful +// run loop. Route registration (/api/v1, /events, /mux, /) lands in later +// phases on top of the router this package builds. +package httpd + +import ( + "net/http" + + "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" + + "github.com/aoagents/agent-orchestrator/backend/internal/config" +) + +// NewRouter builds the root router with the standard middleware stack and the +// health probes mounted. +// +// Middleware order (outermost first): +// +// Recoverer → turn a handler panic into 500 instead of crashing the daemon +// RequestID → attach a request id for correlation +// Logger → structured access log, carrying the request id +// RealIP → normalise client IP (loopback proxy from the dev server) +// +// The per-request Timeout from the decision table is deliberately NOT applied +// globally: it must wrap only the /api/v1 REST surface, never the long-lived +// SSE (/events) or WebSocket (/mux) surfaces, nor the always-must-answer health +// probes. It is therefore applied per-surface when those subrouters are mounted +// in Phase 1b; cfg.RequestTimeout carries the value through to that point. +func NewRouter(cfg config.Config) chi.Router { + r := chi.NewRouter() + + r.Use(middleware.Recoverer) + r.Use(middleware.RequestID) + r.Use(middleware.Logger) + r.Use(middleware.RealIP) + + mountHealth(r) + + return r +} + +// mountHealth registers the liveness and readiness probes the Electron +// supervisor polls before letting the renderer connect. +func mountHealth(r chi.Router) { + r.Get("/healthz", handleHealthz) + r.Get("/readyz", handleReadyz) +} + +// handleHealthz is the liveness probe: it answers 200 as long as the process is +// up and serving. It does no dependency checks by design. +func handleHealthz(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) +} + +// handleReadyz is the readiness probe. In the 1a skeleton the daemon is ready +// as soon as it is listening; later phases will gate this on dependency +// initialisation (e.g. store/event-bus warm-up). +func handleReadyz(w http.ResponseWriter, _ *http.Request) { + writeJSON(w, http.StatusOK, map[string]string{"status": "ready"}) +} diff --git a/backend/internal/httpd/server.go b/backend/internal/httpd/server.go new file mode 100644 index 00000000..cb97b810 --- /dev/null +++ b/backend/internal/httpd/server.go @@ -0,0 +1,113 @@ +package httpd + +import ( + "context" + "errors" + "fmt" + "log/slog" + "net" + "net/http" + "os" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/config" + "github.com/aoagents/agent-orchestrator/backend/internal/runfile" +) + +// Server is the daemon's HTTP server together with its lifecycle: bind the +// loopback port, publish the running.json handshake, serve until the context +// is cancelled, then shut down gracefully and clean up the handshake file. +type Server struct { + cfg config.Config + log *slog.Logger + http *http.Server + listen net.Listener +} + +// 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. +func New(cfg config.Config, log *slog.Logger) (*Server, error) { + ln, err := net.Listen("tcp", cfg.Addr()) + if err != nil { + return nil, fmt.Errorf("bind %s (is a daemon already running?): %w", cfg.Addr(), err) + } + + srv := &Server{ + cfg: cfg, + log: log, + listen: ln, + http: &http.Server{ + Handler: NewRouter(cfg), + // ReadHeaderTimeout guards against slow-loris even on loopback; + // per-request body/handler timeouts are applied per-surface. + ReadHeaderTimeout: 10 * time.Second, + }, + } + return srv, nil +} + +// Addr returns the actual bound address (useful when the configured port was 0 +// and the OS chose one — primarily in tests). +func (s *Server) Addr() net.Addr { return s.listen.Addr() } + +// Run serves until ctx is cancelled (SIGINT/SIGTERM via signal.NotifyContext), +// then performs a graceful shutdown bounded by cfg.ShutdownTimeout. It writes +// running.json before serving and removes it on the way out. Run blocks until +// shutdown is complete. +func (s *Server) Run(ctx context.Context) error { + info := runfile.Info{ + PID: os.Getpid(), + Port: s.boundPort(), + StartedAt: time.Now().UTC(), + } + if err := runfile.Write(s.cfg.RunFilePath, info); err != nil { + s.listen.Close() + return fmt.Errorf("write run-file: %w", err) + } + defer func() { + if err := runfile.Remove(s.cfg.RunFilePath); err != nil { + s.log.Warn("failed to remove run-file", "path", s.cfg.RunFilePath, "err", err) + } + }() + + serveErr := make(chan error, 1) + go func() { + s.log.Info("daemon listening", "addr", s.Addr().String(), "pid", info.PID, "env", s.cfg.Env) + // Serve returns ErrServerClosed on a clean Shutdown; that is success. + if err := s.http.Serve(s.listen); err != nil && !errors.Is(err, http.ErrServerClosed) { + serveErr <- err + return + } + serveErr <- nil + }() + + select { + case err := <-serveErr: + // Serve died on its own (bind already happened, so this is a real + // runtime failure) before any shutdown signal. + return err + case <-ctx.Done(): + s.log.Info("shutdown signal received, draining connections", "timeout", s.cfg.ShutdownTimeout) + } + + shutdownCtx, cancel := context.WithTimeout(context.Background(), s.cfg.ShutdownTimeout) + defer cancel() + + if err := s.http.Shutdown(shutdownCtx); err != nil { + // The deadline elapsed with connections still open; force them closed. + s.log.Warn("graceful shutdown timed out, forcing close", "err", err) + _ = s.http.Close() + return fmt.Errorf("graceful shutdown exceeded %s: %w", s.cfg.ShutdownTimeout, err) + } + + s.log.Info("daemon stopped cleanly") + return <-serveErr +} + +func (s *Server) boundPort() int { + if tcp, ok := s.listen.Addr().(*net.TCPAddr); ok { + return tcp.Port + } + return s.cfg.Port +} diff --git a/backend/internal/httpd/server_test.go b/backend/internal/httpd/server_test.go new file mode 100644 index 00000000..6f7f6527 --- /dev/null +++ b/backend/internal/httpd/server_test.go @@ -0,0 +1,126 @@ +package httpd + +import ( + "context" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/config" + "github.com/aoagents/agent-orchestrator/backend/internal/runfile" +) + +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +func TestHealthProbes(t *testing.T) { + router := NewRouter(config.Config{}) + srv := httptest.NewServer(router) + defer srv.Close() + + for _, path := range []string{"/healthz", "/readyz"} { + resp, err := http.Get(srv.URL + path) + if err != nil { + t.Fatalf("GET %s: %v", path, err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Errorf("GET %s = %d, want 200", path, resp.StatusCode) + } + if ct := resp.Header.Get("Content-Type"); ct != "application/json; charset=utf-8" { + t.Errorf("GET %s Content-Type = %q, want JSON", path, ct) + } + } +} + +// TestServerLifecycle exercises the full Run loop: bind an ephemeral port, +// publish running.json, serve a request, then cancel the context and confirm a +// clean shutdown that removes the handshake file. +func TestServerLifecycle(t *testing.T) { + runPath := filepath.Join(t.TempDir(), "running.json") + cfg := config.Config{ + Host: "127.0.0.1", + Port: 0, // let the OS pick a free port — no conflict with a real daemon + Env: "development", + ShutdownTimeout: 5 * time.Second, + RunFilePath: runPath, + } + + srv, err := New(cfg, discardLogger()) + if err != nil { + t.Fatalf("New: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + runErr := make(chan error, 1) + go func() { runErr <- srv.Run(ctx) }() + + // Wait for the handshake file to confirm the server is up. + base := "http://" + srv.Addr().String() + waitForHealth(t, base) + + info, err := runfile.Read(runPath) + if err != nil { + t.Fatalf("read run-file: %v", err) + } + if info == nil { + t.Fatal("run-file not written while server running") + } + if info.Port == 0 { + t.Error("run-file recorded port 0; want the actual bound port") + } + + cancel() + + select { + case err := <-runErr: + if err != nil { + t.Fatalf("Run returned error on graceful shutdown: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("Run did not return after context cancel") + } + + if after, _ := runfile.Read(runPath); after != nil { + t.Error("run-file still present after shutdown; want it removed") + } +} + +func waitForHealth(t *testing.T, base string) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + resp, err := http.Get(base + "/healthz") + if err == nil { + resp.Body.Close() + if resp.StatusCode == http.StatusOK { + return + } + } + time.Sleep(20 * time.Millisecond) + } + t.Fatal("server did not become healthy within timeout") +} + +// TestNewFailsOnPortConflict confirms a second bind of the same port fails +// fast rather than silently sharing it. +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()) + if err != nil { + t.Fatalf("first New: %v", err) + } + defer first.listen.Close() + + // 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()); err == nil { + t.Fatal("New on an already-bound port = nil error, want bind failure") + } +} diff --git a/backend/internal/runfile/runfile.go b/backend/internal/runfile/runfile.go new file mode 100644 index 00000000..26d5ac46 --- /dev/null +++ b/backend/internal/runfile/runfile.go @@ -0,0 +1,124 @@ +// Package runfile manages running.json — the PID + port handshake the Electron +// main process uses to discover, health-check, and reap the daemon. The daemon +// writes it on startup and removes it on graceful shutdown. On startup the +// daemon also checks for a stale entry left by a crashed predecessor so it can +// fail fast instead of fighting over the port. +package runfile + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "syscall" + "time" +) + +// Info is the on-disk handshake payload. +type Info struct { + // PID is the daemon process id. + PID int `json:"pid"` + // Port is the loopback port the daemon bound. + Port int `json:"port"` + // StartedAt is when the daemon came up (RFC 3339). + StartedAt time.Time `json:"startedAt"` +} + +// Write atomically writes running.json at path, creating parent directories as +// needed. It writes to a temp file and renames so a reader never observes a +// partial file. +func Write(path string, info Info) error { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return fmt.Errorf("create run-file dir: %w", err) + } + data, err := json.MarshalIndent(info, "", " ") + if err != nil { + return fmt.Errorf("marshal run-file: %w", err) + } + data = append(data, '\n') + + tmp, err := os.CreateTemp(filepath.Dir(path), ".running-*.json") + if err != nil { + return fmt.Errorf("create temp run-file: %w", err) + } + tmpName := tmp.Name() + defer os.Remove(tmpName) // no-op once the rename succeeds + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return fmt.Errorf("write temp run-file: %w", err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("close temp run-file: %w", err) + } + if err := os.Rename(tmpName, path); err != nil { + return fmt.Errorf("rename run-file into place: %w", err) + } + return nil +} + +// Read loads running.json. A missing file returns (nil, nil) — that is the +// normal "no daemon recorded" state, not an error. +func Read(path string) (*Info, error) { + data, err := os.ReadFile(path) + if errors.Is(err, os.ErrNotExist) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("read run-file: %w", err) + } + var info Info + if err := json.Unmarshal(data, &info); err != nil { + return nil, fmt.Errorf("parse run-file: %w", err) + } + return &info, nil +} + +// Remove deletes running.json. A missing file is not an error — graceful +// shutdown should be idempotent. +func Remove(path string) error { + if err := os.Remove(path); err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("remove run-file: %w", err) + } + return nil +} + +// CheckStale inspects an existing run-file before the new daemon binds. It +// returns: +// +// - (nil, nil) no run-file, or one left by a dead process (safe to +// proceed; the caller should overwrite it); +// - (*Info, nil) a run-file whose recorded PID is still alive — a live +// daemon already owns the port, so the caller should fail fast. +// +// A run-file pointing at a dead PID is treated as stale and reported safe; the +// fresh Write will overwrite it. +func CheckStale(path string) (*Info, error) { + info, err := Read(path) + if err != nil { + return nil, err + } + if info == nil || info.PID <= 0 { + return nil, nil + } + if processAlive(info.PID) { + return info, nil + } + return nil, nil +} + +// processAlive reports whether a process with the given PID exists. On Unix, +// signal 0 probes existence without delivering a signal: a nil error or +// EPERM (alive but not ours) both mean alive; ESRCH means gone. +func processAlive(pid int) bool { + proc, err := os.FindProcess(pid) + if err != nil { + return false + } + err = proc.Signal(syscall.Signal(0)) + if err == nil { + return true + } + return errors.Is(err, syscall.EPERM) +} diff --git a/backend/internal/runfile/runfile_test.go b/backend/internal/runfile/runfile_test.go new file mode 100644 index 00000000..877675f5 --- /dev/null +++ b/backend/internal/runfile/runfile_test.go @@ -0,0 +1,96 @@ +package runfile + +import ( + "os" + "path/filepath" + "testing" + "time" +) + +func TestWriteReadRoundTrip(t *testing.T) { + path := filepath.Join(t.TempDir(), "nested", "running.json") + want := Info{PID: 4242, Port: 3001, StartedAt: time.Now().UTC().Truncate(time.Second)} + + if err := Write(path, want); err != nil { + t.Fatalf("Write: %v", err) + } + got, err := Read(path) + if err != nil { + t.Fatalf("Read: %v", err) + } + if got == nil { + t.Fatal("Read returned nil for an existing file") + } + if got.PID != want.PID || got.Port != want.Port || !got.StartedAt.Equal(want.StartedAt) { + t.Errorf("round trip mismatch: got %+v, want %+v", *got, want) + } +} + +func TestReadMissingIsNotError(t *testing.T) { + got, err := Read(filepath.Join(t.TempDir(), "absent.json")) + if err != nil { + t.Fatalf("Read missing: %v", err) + } + if got != nil { + t.Errorf("Read missing = %+v, want nil", got) + } +} + +func TestRemoveIdempotent(t *testing.T) { + path := filepath.Join(t.TempDir(), "running.json") + if err := Remove(path); err != nil { + t.Errorf("Remove on missing file: %v", err) + } + if err := Write(path, Info{PID: 1, Port: 2}); err != nil { + t.Fatalf("Write: %v", err) + } + if err := Remove(path); err != nil { + t.Errorf("Remove existing: %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Errorf("file still present after Remove") + } +} + +func TestCheckStaleDeadPID(t *testing.T) { + path := filepath.Join(t.TempDir(), "running.json") + // PID 0x7FFFFFFF is effectively guaranteed not to exist. + if err := Write(path, Info{PID: 0x7FFFFFFF, Port: 3001}); err != nil { + t.Fatalf("Write: %v", err) + } + live, err := CheckStale(path) + if err != nil { + t.Fatalf("CheckStale: %v", err) + } + if live != nil { + t.Errorf("CheckStale on dead PID = %+v, want nil (stale, safe to overwrite)", live) + } +} + +func TestCheckStaleLivePID(t *testing.T) { + path := filepath.Join(t.TempDir(), "running.json") + // This test process is unquestionably alive. + if err := Write(path, Info{PID: os.Getpid(), Port: 3001}); err != nil { + t.Fatalf("Write: %v", err) + } + live, err := CheckStale(path) + if err != nil { + t.Fatalf("CheckStale: %v", err) + } + if live == nil { + t.Fatal("CheckStale on live PID = nil, want the live Info") + } + if live.PID != os.Getpid() { + t.Errorf("live.PID = %d, want %d", live.PID, os.Getpid()) + } +} + +func TestCheckStaleNoFile(t *testing.T) { + live, err := CheckStale(filepath.Join(t.TempDir(), "absent.json")) + if err != nil { + t.Fatalf("CheckStale: %v", err) + } + if live != nil { + t.Errorf("CheckStale with no file = %+v, want nil", live) + } +} diff --git a/backend/main.go b/backend/main.go index 30a6e84c..01d20390 100644 --- a/backend/main.go +++ b/backend/main.go @@ -1,7 +1,65 @@ +// Command backend is the Agent Orchestrator HTTP daemon: a loopback-only +// sidecar spawned and supervised by the Electron main process. Phase 1a brings +// up the server skeleton — config, 127.0.0.1 bind, middleware stack, health +// probes, the running.json handshake, and graceful shutdown. package main -import "fmt" +import ( + "context" + "fmt" + "log/slog" + "os" + "os/signal" + "syscall" + + "github.com/aoagents/agent-orchestrator/backend/internal/config" + "github.com/aoagents/agent-orchestrator/backend/internal/httpd" + "github.com/aoagents/agent-orchestrator/backend/internal/runfile" +) func main() { - fmt.Println("ao backend daemon starting") + if err := run(); err != nil { + fmt.Fprintln(os.Stderr, "ao backend daemon: "+err.Error()) + os.Exit(1) + } +} + +func run() error { + cfg, err := config.Load() + if err != nil { + return err + } + + log := newLogger(cfg) + + // Fail fast if a live daemon already owns the handshake file. A run-file + // left by a crashed predecessor (dead PID) is treated as stale and + // overwritten when the new server starts. + if live, err := runfile.CheckStale(cfg.RunFilePath); err != nil { + return fmt.Errorf("inspect run-file: %w", err) + } else if live != nil { + return fmt.Errorf("daemon already running (pid %d, port %d); refusing to start", live.PID, live.Port) + } + + srv, err := httpd.New(cfg, log) + if err != nil { + return err + } + + // signal.NotifyContext cancels ctx on SIGINT/SIGTERM, which drives the + // graceful shutdown inside Server.Run. + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) + defer stop() + + return srv.Run(ctx) +} + +// newLogger returns a text logger in development and JSON in production. The +// daemon logs to stderr so the Electron supervisor can capture it separately +// from any structured stdout protocol added later. +func newLogger(cfg config.Config) *slog.Logger { + if cfg.IsProduction() { + return slog.New(slog.NewJSONHandler(os.Stderr, nil)) + } + return slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelDebug})) } From 61c5b8a8a053b6044af9936a8a2be50114771520 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Thu, 28 May 2026 12:11:52 +0530 Subject: [PATCH 02/23] =?UTF-8?q?refactor(backend):=20drop=20Env=20config?= =?UTF-8?q?=20field=20=E2=80=94=20not=20needed=20yet=20(#10)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review on #14: AO_ENV / Config.Env / IsProduction() weren't load-bearing for Phase 1a — they only switched the slog handler. Removing them now keeps the surface minimal; the env knob can come back later when a real consumer needs it. - config: remove Env field, AO_ENV parsing, and IsProduction helper. - main: collapse newLogger to a single text-handler path. - httpd: drop the env field from the listening log line. - tests: drop the env assertions and AO_ENV fixture. Co-Authored-By: Claude Opus 4.7 --- backend/internal/config/config.go | 8 -------- backend/internal/config/config_test.go | 12 +----------- backend/internal/httpd/server.go | 2 +- backend/internal/httpd/server_test.go | 1 - backend/main.go | 13 +++++-------- 5 files changed, 7 insertions(+), 29 deletions(-) diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index 9d4b33b7..17026193 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -34,9 +34,6 @@ type Config struct { Host string // Port is the TCP port to bind. The daemon fails fast if it is taken. Port int - // Env is the deployment environment label ("development" | "production"). - // It only affects log verbosity / formatting, never bind behaviour. - Env string // RequestTimeout bounds REST request handling. RequestTimeout time.Duration // ShutdownTimeout is the hard graceful-shutdown deadline. @@ -51,9 +48,6 @@ func (c Config) Addr() string { return fmt.Sprintf("%s:%d", c.Host, c.Port) } -// IsProduction reports whether the daemon is running in production mode. -func (c Config) IsProduction() bool { return c.Env == "production" } - // Load resolves configuration from the environment, applying defaults. It // returns an error only for values that are present but malformed (e.g. a // non-numeric AO_PORT); missing values fall back to defaults. @@ -62,7 +56,6 @@ func (c Config) IsProduction() bool { return c.Env == "production" } // // AO_HOST bind host (default 127.0.0.1) // AO_PORT bind port (default 3001) -// AO_ENV environment label (default development) // AO_REQUEST_TIMEOUT per-request timeout (Go duration, default 60s) // AO_SHUTDOWN_TIMEOUT shutdown deadline (Go duration, default 10s) // AO_RUN_FILE running.json path (default /running.json) @@ -70,7 +63,6 @@ func Load() (Config, error) { cfg := Config{ Host: getEnv("AO_HOST", DefaultHost), Port: DefaultPort, - Env: getEnv("AO_ENV", "development"), RequestTimeout: DefaultRequestTimeout, ShutdownTimeout: DefaultShutdownTimeout, } diff --git a/backend/internal/config/config_test.go b/backend/internal/config/config_test.go index 3eef47c6..d17190c8 100644 --- a/backend/internal/config/config_test.go +++ b/backend/internal/config/config_test.go @@ -8,7 +8,7 @@ import ( func TestLoadDefaults(t *testing.T) { // Clear every recognised var so we observe pure defaults regardless of the // surrounding environment. - for _, k := range []string{"AO_HOST", "AO_PORT", "AO_ENV", "AO_REQUEST_TIMEOUT", "AO_SHUTDOWN_TIMEOUT", "AO_RUN_FILE"} { + for _, k := range []string{"AO_HOST", "AO_PORT", "AO_REQUEST_TIMEOUT", "AO_SHUTDOWN_TIMEOUT", "AO_RUN_FILE"} { t.Setenv(k, "") } @@ -22,9 +22,6 @@ func TestLoadDefaults(t *testing.T) { if cfg.Port != DefaultPort { t.Errorf("Port = %d, want %d", cfg.Port, DefaultPort) } - if cfg.Env != "development" { - t.Errorf("Env = %q, want development", cfg.Env) - } if cfg.RequestTimeout != DefaultRequestTimeout { t.Errorf("RequestTimeout = %s, want %s", cfg.RequestTimeout, DefaultRequestTimeout) } @@ -34,15 +31,11 @@ func TestLoadDefaults(t *testing.T) { if cfg.RunFilePath == "" { t.Error("RunFilePath is empty, want a resolved default path") } - if cfg.IsProduction() { - t.Error("IsProduction() = true for development env") - } } func TestLoadOverrides(t *testing.T) { t.Setenv("AO_HOST", "127.0.0.2") t.Setenv("AO_PORT", "4002") - t.Setenv("AO_ENV", "production") t.Setenv("AO_REQUEST_TIMEOUT", "5s") t.Setenv("AO_SHUTDOWN_TIMEOUT", "3s") t.Setenv("AO_RUN_FILE", "/tmp/ao-test-running.json") @@ -54,9 +47,6 @@ func TestLoadOverrides(t *testing.T) { if cfg.Addr() != "127.0.0.2:4002" { t.Errorf("Addr() = %q, want 127.0.0.2:4002", cfg.Addr()) } - if !cfg.IsProduction() { - t.Error("IsProduction() = false, want true") - } if cfg.RequestTimeout != 5*time.Second { t.Errorf("RequestTimeout = %s, want 5s", cfg.RequestTimeout) } diff --git a/backend/internal/httpd/server.go b/backend/internal/httpd/server.go index cb97b810..f3ace3f2 100644 --- a/backend/internal/httpd/server.go +++ b/backend/internal/httpd/server.go @@ -73,7 +73,7 @@ func (s *Server) Run(ctx context.Context) error { serveErr := make(chan error, 1) go func() { - s.log.Info("daemon listening", "addr", s.Addr().String(), "pid", info.PID, "env", s.cfg.Env) + s.log.Info("daemon listening", "addr", s.Addr().String(), "pid", info.PID) // Serve returns ErrServerClosed on a clean Shutdown; that is success. if err := s.http.Serve(s.listen); err != nil && !errors.Is(err, http.ErrServerClosed) { serveErr <- err diff --git a/backend/internal/httpd/server_test.go b/backend/internal/httpd/server_test.go index 6f7f6527..4fdb5496 100644 --- a/backend/internal/httpd/server_test.go +++ b/backend/internal/httpd/server_test.go @@ -46,7 +46,6 @@ func TestServerLifecycle(t *testing.T) { cfg := config.Config{ Host: "127.0.0.1", Port: 0, // let the OS pick a free port — no conflict with a real daemon - Env: "development", ShutdownTimeout: 5 * time.Second, RunFilePath: runPath, } diff --git a/backend/main.go b/backend/main.go index 01d20390..78a23292 100644 --- a/backend/main.go +++ b/backend/main.go @@ -30,7 +30,7 @@ func run() error { return err } - log := newLogger(cfg) + log := newLogger() // Fail fast if a live daemon already owns the handshake file. A run-file // left by a crashed predecessor (dead PID) is treated as stale and @@ -54,12 +54,9 @@ func run() error { return srv.Run(ctx) } -// newLogger returns a text logger in development and JSON in production. The -// daemon logs to stderr so the Electron supervisor can capture it separately -// from any structured stdout protocol added later. -func newLogger(cfg config.Config) *slog.Logger { - if cfg.IsProduction() { - return slog.New(slog.NewJSONHandler(os.Stderr, nil)) - } +// newLogger returns the daemon's slog logger. It writes to stderr so the +// Electron supervisor can capture it separately from any structured stdout +// protocol added later. +func newLogger() *slog.Logger { return slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelDebug})) } From 1d4d71b9010e6007573ab5d6291b0d6e5decfc42 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Thu, 28 May 2026 12:19:25 +0530 Subject: [PATCH 03/23] docs: add backend run + config quick-start to README (#10) Co-Authored-By: Claude Opus 4.7 --- README.md | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/README.md b/README.md index 353d1200..b85a3066 100644 --- a/README.md +++ b/README.md @@ -5,3 +5,44 @@ paired with an Electron + TypeScript frontend (`frontend/`). See [`docs/`](docs/README.md) for architecture and status — start with the Lifecycle Manager + Session Manager lane in [`docs/architecture.md`](docs/architecture.md). + +## Backend daemon + +The Go binary in [`backend/`](backend/) is the HTTP daemon — a loopback-only +sidecar the Electron supervisor will spawn (Phase 1c). Phase 1a landed the +skeleton: chi router, middleware stack (recoverer → request-id → logger → +real-ip), `/healthz` + `/readyz`, atomic `running.json` PID/port handshake, +graceful shutdown on SIGINT/SIGTERM. + +### Run + +```bash +cd backend +go run . # binds 127.0.0.1:3001 with all defaults +AO_PORT=3019 go run . # override per invocation +``` + +Health check: + +```bash +curl localhost:3001/healthz # {"status":"ok"} +curl localhost:3001/readyz # {"status":"ready"} +``` + +### Configuration (env only) + +| Var | Default | Purpose | +|---|---|---| +| `AO_HOST` | `127.0.0.1` | bind host — keep loopback | +| `AO_PORT` | `3001` | bind port; fails fast if taken | +| `AO_REQUEST_TIMEOUT` | `60s` | per-request timeout (Go duration) | +| `AO_SHUTDOWN_TIMEOUT` | `10s` | graceful-shutdown hard cap | +| `AO_RUN_FILE` | `/agent-orchestrator/running.json` | PID + port handshake path | + +### Test + +```bash +cd backend +gofmt -l . && go build ./... && go vet ./... && go test -race ./... +``` + From a1fda8c38fc5d2bbe3bdd7c46f6dc13902780350 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Thu, 28 May 2026 14:59:52 +0530 Subject: [PATCH 04/23] fix(backend): address Phase 1a review comments (#10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - config: drop AO_HOST entirely — the daemon is loopback-only by design, so making the bind host env-configurable was a security footgun - config: use net.JoinHostPort in Addr() so IPv6 literals stay valid - config: reject zero/negative AO_REQUEST_TIMEOUT and AO_SHUTDOWN_TIMEOUT (time.ParseDuration accepts both; either would silently break the daemon — instant request expiry / no graceful drain) - runfile: split processAlive into unix/windows build-tagged files so liveness detection is reliable on both platforms (Windows uses OpenProcess; POSIX keeps signal 0) - runfile: document os.Rename overwrite semantics (atomic on POSIX, REPLACE_EXISTING on Windows) so the temp-then-rename pattern's cross-platform behaviour is explicit - httpd tests: give probe/waitForHealth clients an explicit per-request timeout so a stalled connect can't hang the test on the outer deadline --- README.md | 5 +- backend/internal/config/config.go | 54 +++++++++++++-------- backend/internal/config/config_test.go | 15 +++--- backend/internal/httpd/server_test.go | 9 +++- backend/internal/runfile/process_unix.go | 24 +++++++++ backend/internal/runfile/process_windows.go | 21 ++++++++ backend/internal/runfile/runfile.go | 21 ++------ 7 files changed, 103 insertions(+), 46 deletions(-) create mode 100644 backend/internal/runfile/process_unix.go create mode 100644 backend/internal/runfile/process_windows.go diff --git a/README.md b/README.md index b85a3066..f5c17deb 100644 --- a/README.md +++ b/README.md @@ -31,9 +31,12 @@ curl localhost:3001/readyz # {"status":"ready"} ### Configuration (env only) +The bind host is always `127.0.0.1`: the daemon is a loopback-only sidecar +and binding any other interface would be a security regression, so the host +is intentionally not env-configurable. + | Var | Default | Purpose | |---|---|---| -| `AO_HOST` | `127.0.0.1` | bind host — keep loopback | | `AO_PORT` | `3001` | bind port; fails fast if taken | | `AO_REQUEST_TIMEOUT` | `60s` | per-request timeout (Go duration) | | `AO_SHUTDOWN_TIMEOUT` | `10s` | graceful-shutdown hard cap | diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index 17026193..c0fb8707 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -6,6 +6,7 @@ package config import ( "fmt" + "net" "os" "path/filepath" "strconv" @@ -13,9 +14,11 @@ import ( ) const ( - // DefaultHost is loopback only. The daemon must never bind a public - // interface — it speaks to the Electron main process over 127.0.0.1. - DefaultHost = "127.0.0.1" + // LoopbackHost is the only host the daemon ever binds. It is intentionally + // not env-configurable: this daemon is a loopback sidecar talking to the + // Electron supervisor over 127.0.0.1, and exposing it on any other + // interface would be a security regression, not a feature. + LoopbackHost = "127.0.0.1" // DefaultPort is the single port the whole surface (REST, SSE, WS, static) // is served from. Single-port keeps it same-origin: no CORS, one lifecycle. DefaultPort = 3001 @@ -30,7 +33,7 @@ const ( // Config is the fully-resolved daemon configuration. It is immutable once // built by Load. type Config struct { - // Host is the bind address. Always loopback in normal operation. + // Host is the bind address. Always loopback — see LoopbackHost. Host string // Port is the TCP port to bind. The daemon fails fast if it is taken. Port int @@ -43,9 +46,10 @@ type Config struct { RunFilePath string } -// Addr returns the host:port the HTTP server binds. +// Addr returns the host:port the HTTP server binds. It uses net.JoinHostPort so +// the result is correct for IPv6 literals as well as IPv4 / hostnames. func (c Config) Addr() string { - return fmt.Sprintf("%s:%d", c.Host, c.Port) + return net.JoinHostPort(c.Host, strconv.Itoa(c.Port)) } // Load resolves configuration from the environment, applying defaults. It @@ -54,14 +58,15 @@ func (c Config) Addr() string { // // Recognised variables: // -// AO_HOST bind host (default 127.0.0.1) // AO_PORT bind port (default 3001) -// AO_REQUEST_TIMEOUT per-request timeout (Go duration, default 60s) -// AO_SHUTDOWN_TIMEOUT shutdown deadline (Go duration, default 10s) +// AO_REQUEST_TIMEOUT per-request timeout (Go duration > 0, default 60s) +// AO_SHUTDOWN_TIMEOUT shutdown deadline (Go duration > 0, default 10s) // AO_RUN_FILE running.json path (default /running.json) +// +// The bind host is not configurable: the daemon is loopback-only by design. func Load() (Config, error) { cfg := Config{ - Host: getEnv("AO_HOST", DefaultHost), + Host: LoopbackHost, Port: DefaultPort, RequestTimeout: DefaultRequestTimeout, ShutdownTimeout: DefaultShutdownTimeout, @@ -79,17 +84,17 @@ func Load() (Config, error) { } if raw := os.Getenv("AO_REQUEST_TIMEOUT"); raw != "" { - d, err := time.ParseDuration(raw) + d, err := parsePositiveDuration("AO_REQUEST_TIMEOUT", raw) if err != nil { - return Config{}, fmt.Errorf("invalid AO_REQUEST_TIMEOUT %q: %w", raw, err) + return Config{}, err } cfg.RequestTimeout = d } if raw := os.Getenv("AO_SHUTDOWN_TIMEOUT"); raw != "" { - d, err := time.ParseDuration(raw) + d, err := parsePositiveDuration("AO_SHUTDOWN_TIMEOUT", raw) if err != nil { - return Config{}, fmt.Errorf("invalid AO_SHUTDOWN_TIMEOUT %q: %w", raw, err) + return Config{}, err } cfg.ShutdownTimeout = d } @@ -103,6 +108,20 @@ func Load() (Config, error) { return cfg, nil } +// parsePositiveDuration rejects zero and negative durations: a zero +// RequestTimeout would expire every request instantly, and a non-positive +// ShutdownTimeout would defeat graceful shutdown. +func parsePositiveDuration(name, raw string) (time.Duration, error) { + d, err := time.ParseDuration(raw) + if err != nil { + return 0, fmt.Errorf("invalid %s %q: %w", name, raw, err) + } + if d <= 0 { + return 0, fmt.Errorf("invalid %s %q: must be > 0", name, raw) + } + return d, nil +} + // resolveRunFilePath picks where running.json lives. An explicit AO_RUN_FILE // wins; otherwise it sits under the per-user state directory so multiple repos // share one supervisor handshake location. @@ -116,10 +135,3 @@ func resolveRunFilePath() (string, error) { } return filepath.Join(dir, "agent-orchestrator", "running.json"), nil } - -func getEnv(key, fallback string) string { - if v, ok := os.LookupEnv(key); ok && v != "" { - return v - } - return fallback -} diff --git a/backend/internal/config/config_test.go b/backend/internal/config/config_test.go index d17190c8..dfcb5b8a 100644 --- a/backend/internal/config/config_test.go +++ b/backend/internal/config/config_test.go @@ -8,7 +8,7 @@ import ( func TestLoadDefaults(t *testing.T) { // Clear every recognised var so we observe pure defaults regardless of the // surrounding environment. - for _, k := range []string{"AO_HOST", "AO_PORT", "AO_REQUEST_TIMEOUT", "AO_SHUTDOWN_TIMEOUT", "AO_RUN_FILE"} { + for _, k := range []string{"AO_PORT", "AO_REQUEST_TIMEOUT", "AO_SHUTDOWN_TIMEOUT", "AO_RUN_FILE"} { t.Setenv(k, "") } @@ -16,8 +16,8 @@ func TestLoadDefaults(t *testing.T) { if err != nil { t.Fatalf("Load: %v", err) } - if cfg.Host != DefaultHost { - t.Errorf("Host = %q, want %q", cfg.Host, DefaultHost) + if cfg.Host != LoopbackHost { + t.Errorf("Host = %q, want %q", cfg.Host, LoopbackHost) } if cfg.Port != DefaultPort { t.Errorf("Port = %d, want %d", cfg.Port, DefaultPort) @@ -34,7 +34,6 @@ func TestLoadDefaults(t *testing.T) { } func TestLoadOverrides(t *testing.T) { - t.Setenv("AO_HOST", "127.0.0.2") t.Setenv("AO_PORT", "4002") t.Setenv("AO_REQUEST_TIMEOUT", "5s") t.Setenv("AO_SHUTDOWN_TIMEOUT", "3s") @@ -44,8 +43,8 @@ func TestLoadOverrides(t *testing.T) { if err != nil { t.Fatalf("Load: %v", err) } - if cfg.Addr() != "127.0.0.2:4002" { - t.Errorf("Addr() = %q, want 127.0.0.2:4002", cfg.Addr()) + if cfg.Addr() != "127.0.0.1:4002" { + t.Errorf("Addr() = %q, want 127.0.0.1:4002", cfg.Addr()) } if cfg.RequestTimeout != 5*time.Second { t.Errorf("RequestTimeout = %s, want 5s", cfg.RequestTimeout) @@ -67,6 +66,10 @@ func TestLoadInvalid(t *testing.T) { {"port out of range", map[string]string{"AO_PORT": "70000"}}, {"bad request timeout", map[string]string{"AO_REQUEST_TIMEOUT": "soon"}}, {"bad shutdown timeout", map[string]string{"AO_SHUTDOWN_TIMEOUT": "later"}}, + {"zero request timeout", map[string]string{"AO_REQUEST_TIMEOUT": "0s"}}, + {"negative request timeout", map[string]string{"AO_REQUEST_TIMEOUT": "-1s"}}, + {"zero shutdown timeout", map[string]string{"AO_SHUTDOWN_TIMEOUT": "0s"}}, + {"negative shutdown timeout", map[string]string{"AO_SHUTDOWN_TIMEOUT": "-5s"}}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/backend/internal/httpd/server_test.go b/backend/internal/httpd/server_test.go index 4fdb5496..de930c49 100644 --- a/backend/internal/httpd/server_test.go +++ b/backend/internal/httpd/server_test.go @@ -23,8 +23,9 @@ func TestHealthProbes(t *testing.T) { srv := httptest.NewServer(router) defer srv.Close() + client := &http.Client{Timeout: 2 * time.Second} for _, path := range []string{"/healthz", "/readyz"} { - resp, err := http.Get(srv.URL + path) + resp, err := client.Get(srv.URL + path) if err != nil { t.Fatalf("GET %s: %v", path, err) } @@ -92,9 +93,13 @@ func TestServerLifecycle(t *testing.T) { func waitForHealth(t *testing.T, base string) { t.Helper() + // Per-request timeout so a stalled connect or hung handshake doesn't park + // the test for the full Go test timeout; the outer deadline only bounds + // the polling loop, not any single GET. + client := &http.Client{Timeout: 500 * time.Millisecond} deadline := time.Now().Add(5 * time.Second) for time.Now().Before(deadline) { - resp, err := http.Get(base + "/healthz") + resp, err := client.Get(base + "/healthz") if err == nil { resp.Body.Close() if resp.StatusCode == http.StatusOK { diff --git a/backend/internal/runfile/process_unix.go b/backend/internal/runfile/process_unix.go new file mode 100644 index 00000000..fcadda3c --- /dev/null +++ b/backend/internal/runfile/process_unix.go @@ -0,0 +1,24 @@ +//go:build !windows + +package runfile + +import ( + "errors" + "os" + "syscall" +) + +// processAlive probes existence with signal 0: kill(pid, 0) returns nil if the +// process exists and we can signal it, EPERM if it exists but is owned by +// another user, and ESRCH (or any other error from FindProcess) if it is gone. +func processAlive(pid int) bool { + proc, err := os.FindProcess(pid) + if err != nil { + return false + } + err = proc.Signal(syscall.Signal(0)) + if err == nil { + return true + } + return errors.Is(err, syscall.EPERM) +} diff --git a/backend/internal/runfile/process_windows.go b/backend/internal/runfile/process_windows.go new file mode 100644 index 00000000..1f8e78fe --- /dev/null +++ b/backend/internal/runfile/process_windows.go @@ -0,0 +1,21 @@ +//go:build windows + +package runfile + +import ( + "syscall" +) + +// processAlive opens the process with the minimum-rights query flag. On +// Windows, OpenProcess returns ERROR_INVALID_PARAMETER for a PID that no +// longer maps to a live process, and a usable handle when one is. We close +// the handle immediately; the only thing we needed was the open's outcome. +func processAlive(pid int) bool { + const PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 + h, err := syscall.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid)) + if err != nil { + return false + } + _ = syscall.CloseHandle(h) + return true +} diff --git a/backend/internal/runfile/runfile.go b/backend/internal/runfile/runfile.go index 26d5ac46..37032a26 100644 --- a/backend/internal/runfile/runfile.go +++ b/backend/internal/runfile/runfile.go @@ -11,7 +11,6 @@ import ( "fmt" "os" "path/filepath" - "syscall" "time" ) @@ -27,7 +26,11 @@ type Info struct { // Write atomically writes running.json at path, creating parent directories as // needed. It writes to a temp file and renames so a reader never observes a -// partial file. +// partial file. os.Rename overwrites an existing destination on both POSIX +// (rename(2) is atomic by default) and Windows (Go uses MoveFileExW with +// MOVEFILE_REPLACE_EXISTING) when source and destination are on the same +// volume, which is always true here because the temp file lives in the same +// directory as the target. func Write(path string, info Info) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return fmt.Errorf("create run-file dir: %w", err) @@ -108,17 +111,3 @@ func CheckStale(path string) (*Info, error) { return nil, nil } -// processAlive reports whether a process with the given PID exists. On Unix, -// signal 0 probes existence without delivering a signal: a nil error or -// EPERM (alive but not ours) both mean alive; ESRCH means gone. -func processAlive(pid int) bool { - proc, err := os.FindProcess(pid) - if err != nil { - return false - } - err = proc.Signal(syscall.Signal(0)) - if err == nil { - return true - } - return errors.Is(err, syscall.EPERM) -} From 1fc4efe5252b8df680c92f74f6562943226772f9 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Thu, 28 May 2026 16:59:51 +0530 Subject: [PATCH 05/23] fix(backend): strip trailing blank line from runfile.go (#10) gofmt CI was failing because removing the orphan processAlive doc comment left an extra newline at EOF. --- backend/internal/runfile/runfile.go | 1 - 1 file changed, 1 deletion(-) diff --git a/backend/internal/runfile/runfile.go b/backend/internal/runfile/runfile.go index 37032a26..a4d8bb69 100644 --- a/backend/internal/runfile/runfile.go +++ b/backend/internal/runfile/runfile.go @@ -110,4 +110,3 @@ func CheckStale(path string) (*Info, error) { } return nil, nil } - From 5bca1a2648a3cfd0c15be4c73d3b5f962ba2ed74 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Thu, 28 May 2026 17:13:36 +0530 Subject: [PATCH 06/23] fix(backend): cross-platform run-file replace + AO_HOST rationale (#10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - runfile: introduce build-tagged atomicReplace — POSIX rename(2) on Unix, MoveFileEx with MOVEFILE_REPLACE_EXISTING on Windows. The Go runtime happens to do the Windows call internally already, but invoking it directly makes the cross-platform contract explicit instead of a runtime implementation detail - runfile: tighten process_unix.go build tag from `!windows` to `unix` so plan9/js/wasm fail to build rather than silently using a broken signal-0 probe - runfile: add TestWriteOverwritesExisting covering the stale run-file replace path that none of the previous tests exercised - config: anchor the loopback-only decision in the LoopbackHost doc so the next contributor doesn't reintroduce AO_HOST without the security rationale --- backend/internal/config/config.go | 11 +++++---- backend/internal/runfile/process_unix.go | 2 +- backend/internal/runfile/rename_unix.go | 13 ++++++++++ backend/internal/runfile/rename_windows.go | 28 ++++++++++++++++++++++ backend/internal/runfile/runfile.go | 17 +++++++------ backend/internal/runfile/runfile_test.go | 23 ++++++++++++++++++ 6 files changed, 80 insertions(+), 14 deletions(-) create mode 100644 backend/internal/runfile/rename_unix.go create mode 100644 backend/internal/runfile/rename_windows.go diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index c0fb8707..d6765dba 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -14,10 +14,13 @@ import ( ) const ( - // LoopbackHost is the only host the daemon ever binds. It is intentionally - // not env-configurable: this daemon is a loopback sidecar talking to the - // Electron supervisor over 127.0.0.1, and exposing it on any other - // interface would be a security regression, not a feature. + // LoopbackHost is the only host the daemon ever binds. There is deliberately + // no AO_HOST env var: the daemon has no auth/CORS/TLS and a stray + // AO_HOST=0.0.0.0 would turn it into a public no-auth service. The legacy + // TS server bound all-interfaces by accident and docs/CROSS_PLATFORM.md + // already calls that out as a bug; the Go rewrite fixes it by removing the + // knob entirely. If a non-default loopback (e.g. ::1, 127.0.0.2) is ever + // needed, add it back with an IsLoopback() validator — not a raw env read. LoopbackHost = "127.0.0.1" // DefaultPort is the single port the whole surface (REST, SSE, WS, static) // is served from. Single-port keeps it same-origin: no CORS, one lifecycle. diff --git a/backend/internal/runfile/process_unix.go b/backend/internal/runfile/process_unix.go index fcadda3c..efe957e1 100644 --- a/backend/internal/runfile/process_unix.go +++ b/backend/internal/runfile/process_unix.go @@ -1,4 +1,4 @@ -//go:build !windows +//go:build unix package runfile diff --git a/backend/internal/runfile/rename_unix.go b/backend/internal/runfile/rename_unix.go new file mode 100644 index 00000000..dd9dbd50 --- /dev/null +++ b/backend/internal/runfile/rename_unix.go @@ -0,0 +1,13 @@ +//go:build unix + +package runfile + +import "os" + +// atomicReplace renames src to dst, replacing dst if it exists. POSIX +// rename(2) is atomic and overwrites an existing destination by default, +// provided src and dst live on the same filesystem — which is always true +// here because the temp file is created in the target directory. +func atomicReplace(src, dst string) error { + return os.Rename(src, dst) +} diff --git a/backend/internal/runfile/rename_windows.go b/backend/internal/runfile/rename_windows.go new file mode 100644 index 00000000..031411ee --- /dev/null +++ b/backend/internal/runfile/rename_windows.go @@ -0,0 +1,28 @@ +//go:build windows + +package runfile + +import "syscall" + +// movefileReplaceExisting tells MoveFileEx to overwrite dst if it already +// exists. Mirrors MOVEFILE_REPLACE_EXISTING from the Win32 API; declared +// locally so we don't pull in golang.org/x/sys for a single constant. +const movefileReplaceExisting = 0x1 + +// atomicReplace renames src to dst, replacing dst if it exists. Go's +// os.Rename on Windows happens to do the same MoveFileEx call internally, +// but calling it directly makes the cross-platform contract explicit instead +// of leaning on a runtime implementation detail. The replace is atomic +// against concurrent readers — readers see either the old or the new file, +// never an empty or partially-written one. +func atomicReplace(src, dst string) error { + srcPtr, err := syscall.UTF16PtrFromString(src) + if err != nil { + return err + } + dstPtr, err := syscall.UTF16PtrFromString(dst) + if err != nil { + return err + } + return syscall.MoveFileEx(srcPtr, dstPtr, movefileReplaceExisting) +} diff --git a/backend/internal/runfile/runfile.go b/backend/internal/runfile/runfile.go index a4d8bb69..7dafe1be 100644 --- a/backend/internal/runfile/runfile.go +++ b/backend/internal/runfile/runfile.go @@ -24,13 +24,12 @@ type Info struct { StartedAt time.Time `json:"startedAt"` } -// Write atomically writes running.json at path, creating parent directories as -// needed. It writes to a temp file and renames so a reader never observes a -// partial file. os.Rename overwrites an existing destination on both POSIX -// (rename(2) is atomic by default) and Windows (Go uses MoveFileExW with -// MOVEFILE_REPLACE_EXISTING) when source and destination are on the same -// volume, which is always true here because the temp file lives in the same -// directory as the target. +// Write atomically writes running.json at path, creating parent directories +// as needed. It writes to a temp file in the same directory and then calls +// atomicReplace — POSIX rename(2) on Unix, MoveFileEx with +// MOVEFILE_REPLACE_EXISTING on Windows — so a reader never observes a +// partial file and a stale running.json from a crashed predecessor is +// overwritten without an intermediate "no file" window. func Write(path string, info Info) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return fmt.Errorf("create run-file dir: %w", err) @@ -55,8 +54,8 @@ func Write(path string, info Info) error { if err := tmp.Close(); err != nil { return fmt.Errorf("close temp run-file: %w", err) } - if err := os.Rename(tmpName, path); err != nil { - return fmt.Errorf("rename run-file into place: %w", err) + if err := atomicReplace(tmpName, path); err != nil { + return fmt.Errorf("replace run-file: %w", err) } return nil } diff --git a/backend/internal/runfile/runfile_test.go b/backend/internal/runfile/runfile_test.go index 877675f5..fbdf74e0 100644 --- a/backend/internal/runfile/runfile_test.go +++ b/backend/internal/runfile/runfile_test.go @@ -26,6 +26,29 @@ func TestWriteReadRoundTrip(t *testing.T) { } } +// TestWriteOverwritesExisting is the cross-platform overwrite check: a stale +// running.json from a crashed predecessor must be replaced cleanly. POSIX +// rename(2) handles this natively; Windows needs MoveFileEx with +// MOVEFILE_REPLACE_EXISTING — atomicReplace gives us both. +func TestWriteOverwritesExisting(t *testing.T) { + path := filepath.Join(t.TempDir(), "running.json") + + if err := Write(path, Info{PID: 1, Port: 3001}); err != nil { + t.Fatalf("first Write: %v", err) + } + if err := Write(path, Info{PID: 2, Port: 3002}); err != nil { + t.Fatalf("second Write (overwrite): %v", err) + } + + got, err := Read(path) + if err != nil { + t.Fatalf("Read: %v", err) + } + if got == nil || got.PID != 2 || got.Port != 3002 { + t.Errorf("after overwrite: got %+v, want PID=2 Port=3002", got) + } +} + func TestReadMissingIsNotError(t *testing.T) { got, err := Read(filepath.Join(t.TempDir(), "absent.json")) if err != nil { From 24f57fddbc830431484a3b25383d5130b920a4b8 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Thu, 28 May 2026 22:20:05 +0530 Subject: [PATCH 07/23] fix(backend): route chi access logs through slog/stderr (#10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit chi's middleware.Logger writes via stdlib log to stdout, but the daemon's slog logger writes to stderr — so REST traffic and daemon logs landed on different streams in different formats. Replace it with a small slog-backed requestLogger that: - Wraps the response writer via middleware.NewWrapResponseWriter so status/bytes are accurate even when handlers return without an explicit WriteHeader. - Reads the request id off the context set by middleware.RequestID (kept mounted just before this middleware so the id is available). - Emits one structured Info line per request with method, path, status, bytes, duration, and remote — same key=value shape as the rest of the daemon, one stream for the Electron supervisor to capture. --- backend/internal/httpd/log.go | 40 +++++++++++++++++++++++++++ backend/internal/httpd/router.go | 13 +++++---- backend/internal/httpd/server.go | 2 +- backend/internal/httpd/server_test.go | 2 +- 4 files changed, 49 insertions(+), 8 deletions(-) create mode 100644 backend/internal/httpd/log.go diff --git a/backend/internal/httpd/log.go b/backend/internal/httpd/log.go new file mode 100644 index 00000000..abb9462e --- /dev/null +++ b/backend/internal/httpd/log.go @@ -0,0 +1,40 @@ +package httpd + +import ( + "log/slog" + "net/http" + "time" + + "github.com/go-chi/chi/v5/middleware" +) + +// requestLogger emits one structured access-log line per request via the +// daemon's slog logger. Chi's built-in middleware.Logger writes to stdout +// using stdlib log; reusing the daemon's slog keeps every line on stderr in +// the same key=value shape as the rest of the daemon (one stream for the +// Electron supervisor to capture, one format to grep). +// +// Status, bytes, and duration come from a wrapped ResponseWriter so the log +// is accurate even when the handler returns without calling WriteHeader. The +// request id is read off the context populated by middleware.RequestID, so +// this middleware must be mounted after it. +func requestLogger(log *slog.Logger) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor) + start := time.Now() + defer func() { + log.Info("http request", + "id", middleware.GetReqID(r.Context()), + "method", r.Method, + "path", r.URL.Path, + "status", ww.Status(), + "bytes", ww.BytesWritten(), + "duration", time.Since(start), + "remote", r.RemoteAddr, + ) + }() + next.ServeHTTP(ww, r) + }) + } +} diff --git a/backend/internal/httpd/router.go b/backend/internal/httpd/router.go index 1d366b69..6e078b8d 100644 --- a/backend/internal/httpd/router.go +++ b/backend/internal/httpd/router.go @@ -5,6 +5,7 @@ package httpd import ( + "log/slog" "net/http" "github.com/go-chi/chi/v5" @@ -18,22 +19,22 @@ import ( // // Middleware order (outermost first): // -// Recoverer → turn a handler panic into 500 instead of crashing the daemon -// RequestID → attach a request id for correlation -// Logger → structured access log, carrying the request id -// RealIP → normalise client IP (loopback proxy from the dev server) +// Recoverer → turn a handler panic into 500 instead of crashing the daemon +// RequestID → attach a request id for correlation +// requestLogger → slog-backed access log, stderr, carries the request id +// RealIP → normalise client IP (loopback proxy from the dev server) // // The per-request Timeout from the decision table is deliberately NOT applied // globally: it must wrap only the /api/v1 REST surface, never the long-lived // SSE (/events) or WebSocket (/mux) surfaces, nor the always-must-answer health // probes. It is therefore applied per-surface when those subrouters are mounted // in Phase 1b; cfg.RequestTimeout carries the value through to that point. -func NewRouter(cfg config.Config) chi.Router { +func NewRouter(cfg config.Config, log *slog.Logger) chi.Router { r := chi.NewRouter() r.Use(middleware.Recoverer) r.Use(middleware.RequestID) - r.Use(middleware.Logger) + r.Use(requestLogger(log)) r.Use(middleware.RealIP) mountHealth(r) diff --git a/backend/internal/httpd/server.go b/backend/internal/httpd/server.go index f3ace3f2..f42ed88a 100644 --- a/backend/internal/httpd/server.go +++ b/backend/internal/httpd/server.go @@ -38,7 +38,7 @@ func New(cfg config.Config, log *slog.Logger) (*Server, error) { log: log, listen: ln, http: &http.Server{ - Handler: NewRouter(cfg), + Handler: NewRouter(cfg, log), // ReadHeaderTimeout guards against slow-loris even on loopback; // per-request body/handler timeouts are applied per-surface. ReadHeaderTimeout: 10 * time.Second, diff --git a/backend/internal/httpd/server_test.go b/backend/internal/httpd/server_test.go index de930c49..2570397f 100644 --- a/backend/internal/httpd/server_test.go +++ b/backend/internal/httpd/server_test.go @@ -19,7 +19,7 @@ func discardLogger() *slog.Logger { } func TestHealthProbes(t *testing.T) { - router := NewRouter(config.Config{}) + router := NewRouter(config.Config{}, discardLogger()) srv := httptest.NewServer(router) defer srv.Close() From afee896d0ac8b9a50009c099f5c75169c768a388 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Thu, 28 May 2026 19:56:43 +0530 Subject: [PATCH 08/23] =?UTF-8?q?feat(api):=20projects=20route=20shell=20(?= =?UTF-8?q?7=20routes,=20REST-corrected)=20=E2=80=94=20#20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mounts the /api/v1 surface on the skeleton router (#10·1a) and registers the 7 canonical project routes as 501 stubs that emit a structured PlannedRoute body documenting the future contract. Shared scaffolding landed here (api.go, errors.go, stubs/, controllers/) so #21/#22 plug in without re-touching the wiring. WHY: opens the route-shell PRs in the Go HTTP daemon lane. Doing it interface-first lets the dashboard team build against the contract before any handler logic exists; the locked APIError envelope and PlannedRoute shape become #19's OpenAPI source-of-truth. REST audit corrections vs the legacy TS surface: R3 PUT /projects/:id alias of PATCH: PUT not registered → 405. R4 POST /projects/:id repair overload: canonical /repair; legacy 405. R5 degraded GET returns 200 with error field: discriminator status. R6 ok/success flag flips: drop on 2xx; return affected resource. R9 bare {error: msg}: locked {error,code,message,requestId,details?}. Legacy paths are deliberately NOT registered; each canonical handler carries PlannedRoute.Legacy so consumers can discover the migration. Zod schemas (TrackerConfig, SCMConfig, AgentConfig, ReactionConfig, LocalProjectConfig, RoleAgentConfig) ported to typed Go structs with an Extra map reserved for .passthrough() round-tripping in later PRs. Closes part of #18; targets feat/issue-10 until #14 merges. --- backend/internal/domain/config_types.go | 105 ++++++++ backend/internal/domain/project.go | 47 ++++ backend/internal/httpd/api.go | 82 +++++++ .../internal/httpd/controllers/projects.go | 209 ++++++++++++++++ .../httpd/controllers/projects_test.go | 232 ++++++++++++++++++ backend/internal/httpd/errors.go | 34 +++ backend/internal/httpd/router.go | 14 ++ backend/internal/httpd/stubs/notimpl.go | 66 +++++ backend/internal/ports/services.go | 77 ++++++ 9 files changed, 866 insertions(+) create mode 100644 backend/internal/domain/config_types.go create mode 100644 backend/internal/domain/project.go create mode 100644 backend/internal/httpd/api.go create mode 100644 backend/internal/httpd/controllers/projects.go create mode 100644 backend/internal/httpd/controllers/projects_test.go create mode 100644 backend/internal/httpd/errors.go create mode 100644 backend/internal/httpd/stubs/notimpl.go create mode 100644 backend/internal/ports/services.go diff --git a/backend/internal/domain/config_types.go b/backend/internal/domain/config_types.go new file mode 100644 index 00000000..6f06e340 --- /dev/null +++ b/backend/internal/domain/config_types.go @@ -0,0 +1,105 @@ +package domain + +// Typed configuration shapes ported from the TS Zod schemas in the old +// orchestrator (packages/core/src/config.ts + global-config.ts). They live in +// the domain package because both the project read-model (Project) and the +// future project-config service surface them on the wire. +// +// Zod .passthrough() preserves unknown keys; the Go equivalent will be a +// custom UnmarshalJSON that fills Extra. The route-shell PR (#20) only declares +// the shapes for documentation in planned bodies — the passthrough Marshal/ +// Unmarshal lands when a real handler first reads or writes config. + +// AgentPermission constrains the agent-permissions enum. Empty string means +// "not set" — distinct from the TS default of "permissionless" so the API can +// tell user-set values apart from defaults. +type AgentPermission string + +const ( + AgentPermissionPermissionless AgentPermission = "permissionless" + AgentPermissionDefault AgentPermission = "default" + AgentPermissionAutoEdit AgentPermission = "auto-edit" + AgentPermissionSuggest AgentPermission = "suggest" + AgentPermissionSkip AgentPermission = "skip" +) + +// TrackerConfig mirrors TrackerConfigSchema. .passthrough() preserves arbitrary +// plugin-specific keys; Extra is reserved for that round-trip. +type TrackerConfig struct { + Plugin string `json:"plugin,omitempty"` + Package string `json:"package,omitempty"` + Path string `json:"path,omitempty"` + Extra map[string]any `json:"-"` +} + +// SCMConfig mirrors SCMConfigSchema. Webhook nests its own optional block. +type SCMConfig struct { + Plugin string `json:"plugin,omitempty"` + Package string `json:"package,omitempty"` + Path string `json:"path,omitempty"` + Webhook *SCMWebhookConfig `json:"webhook,omitempty"` + Extra map[string]any `json:"-"` +} + +// SCMWebhookConfig — pointer Enabled distinguishes unset from explicit false. +type SCMWebhookConfig struct { + Enabled *bool `json:"enabled,omitempty"` + Path string `json:"path,omitempty"` + SecretEnvVar string `json:"secretEnvVar,omitempty"` + SignatureHeader string `json:"signatureHeader,omitempty"` + EventHeader string `json:"eventHeader,omitempty"` + DeliveryHeader string `json:"deliveryHeader,omitempty"` + MaxBodyBytes int `json:"maxBodyBytes,omitempty"` +} + +// AgentConfig mirrors AgentSpecificConfigSchema. .passthrough() preserves +// agent-plugin-specific keys. +type AgentConfig struct { + Permissions AgentPermission `json:"permissions,omitempty"` + Model string `json:"model,omitempty"` + OrchestratorModel string `json:"orchestratorModel,omitempty"` + OpenCodeSessionID string `json:"opencodeSessionId,omitempty"` + Extra map[string]any `json:"-"` +} + +// RoleAgentConfig mirrors RoleAgentConfigSchema for orchestrator/worker roles. +type RoleAgentConfig struct { + Agent string `json:"agent,omitempty"` + AgentConfig *AgentConfig `json:"agentConfig,omitempty"` +} + +// ReactionConfig mirrors ReactionConfigSchema. EscalateAfter accepts both +// numeric milliseconds and a duration string (e.g. "30m") in the TS schema, so +// the Go type stays open as `any` until handler validation lands. +type ReactionConfig struct { + Auto *bool `json:"auto,omitempty"` + Action string `json:"action,omitempty"` + Message string `json:"message,omitempty"` + Priority string `json:"priority,omitempty"` + Retries *int `json:"retries,omitempty"` + EscalateAfter any `json:"escalateAfter,omitempty"` + Threshold string `json:"threshold,omitempty"` + IncludeSummary *bool `json:"includeSummary,omitempty"` +} + +// LocalProjectConfig mirrors LocalProjectConfigSchema — the flat, behavior-only +// on-disk file at /agent-orchestrator.yaml. Identity fields +// (projectId, path, repo, defaultBranch, sessionPrefix) deliberately live in +// the global registry, not here. +type LocalProjectConfig struct { + Repo string `json:"repo,omitempty"` + DefaultBranch string `json:"defaultBranch,omitempty"` + Runtime string `json:"runtime,omitempty"` + Agent string `json:"agent,omitempty"` + Workspace string `json:"workspace,omitempty"` + Tracker *TrackerConfig `json:"tracker,omitempty"` + SCM *SCMConfig `json:"scm,omitempty"` + Symlinks []string `json:"symlinks,omitempty"` + PostCreate []string `json:"postCreate,omitempty"` + AgentConfig *AgentConfig `json:"agentConfig,omitempty"` + Orchestrator *RoleAgentConfig `json:"orchestrator,omitempty"` + Worker *RoleAgentConfig `json:"worker,omitempty"` + Reactions map[string]*ReactionConfig `json:"reactions,omitempty"` + AgentRules string `json:"agentRules,omitempty"` + Extra map[string]any `json:"-"` +} diff --git a/backend/internal/domain/project.go b/backend/internal/domain/project.go new file mode 100644 index 00000000..016450b9 --- /dev/null +++ b/backend/internal/domain/project.go @@ -0,0 +1,47 @@ +package domain + +// Project domain types for the HTTP API. ProjectID already exists in +// session.go; this file adds the list/read read-models the projects route +// surface returns. + +// ProjectSummary is the row shape returned by GET /api/v1/projects. It mirrors +// the TS ProjectInfo (packages/web/src/lib/project-name.ts) so the existing +// dashboard list view can read the Go daemon's response unchanged. +// +// ResolveError is populated only for degraded projects — entries whose config +// failed to load but whose registry entry still exists. The list view shows +// them with a warning instead of dropping them silently. +type ProjectSummary struct { + ID ProjectID `json:"id"` + Name string `json:"name"` + SessionPrefix string `json:"sessionPrefix"` + ResolveError string `json:"resolveError,omitempty"` +} + +// Project is the full read-model returned by GET /api/v1/projects/{id} when +// the project resolves cleanly. It joins the global-registry identity fields +// (id, name, path, repo, defaultBranch) with the local on-disk behaviour +// config (agent, runtime, tracker, scm, reactions). +type Project struct { + ID ProjectID `json:"id"` + Name string `json:"name"` + Path string `json:"path"` + Repo string `json:"repo"` + DefaultBranch string `json:"defaultBranch"` + Agent string `json:"agent,omitempty"` + Runtime string `json:"runtime,omitempty"` + Tracker *TrackerConfig `json:"tracker,omitempty"` + SCM *SCMConfig `json:"scm,omitempty"` + Reactions map[string]*ReactionConfig `json:"reactions,omitempty"` +} + +// DegradedProject is returned in place of Project when the project's config +// failed to load. The frontend uses ResolveError to render a recovery UI; the +// /projects/{id}/repair endpoint accepts the project id to fix a recoverable +// subset of degraded states (e.g. legacy wrapped-config format). +type DegradedProject struct { + ID ProjectID `json:"id"` + Name string `json:"name"` + Path string `json:"path"` + ResolveError string `json:"resolveError"` +} diff --git a/backend/internal/httpd/api.go b/backend/internal/httpd/api.go new file mode 100644 index 00000000..7af9a78b --- /dev/null +++ b/backend/internal/httpd/api.go @@ -0,0 +1,82 @@ +package httpd + +import ( + "net/http" + + "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" + + "github.com/aoagents/agent-orchestrator/backend/internal/config" + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/controllers" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +// APIDeps bundles every service the API layer's controllers depend on. The +// route-shell PR (#20) leaves all of them nil — handlers don't dereference +// them yet. Real handler PRs in the same lane fill the relevant field and +// flip stubs to real logic one route at a time. +type APIDeps struct { + Projects ports.ProjectService +} + +// API owns one controller per resource and is the single Register call the +// router invokes to mount the /api/v1 surface. Splitting per-resource means +// later PRs can land a controller's real handlers without touching the +// surrounding wiring. +type API struct { + cfg config.Config + projects *controllers.ProjectsController +} + +// NewAPI constructs the API surface from its dependencies. cfg carries the +// per-request timeout so the REST group can apply it without re-reading the +// environment. +func NewAPI(cfg config.Config, deps APIDeps) *API { + return &API{ + cfg: cfg, + projects: &controllers.ProjectsController{ + Svc: deps.Projects, + }, + } +} + +// Register mounts the API surface on root. /api/v1 hosts the REST group with +// the per-request Timeout that the skeleton router (router.go) deliberately +// kept off the global stack — REST routes are bounded, but long-lived surfaces +// (/events SSE, /mux WS) live outside this group when they land. +// +// /mux is mounted outside /api/v1 for parity with the legacy TS surface; it is +// a phase-4 placeholder and stays unregistered here until that lane starts. +func (a *API) Register(root chi.Router) { + timeout := a.cfg.RequestTimeout + if timeout <= 0 { + timeout = config.DefaultRequestTimeout + } + + root.Route("/api/v1", func(r chi.Router) { + r.Group(func(r chi.Router) { + r.Use(middleware.Timeout(timeout)) + a.projects.Register(r) + // Sibling controllers (sessions, issues, prs, ...) plug in here in + // follow-up PRs #21 / #22 without touching the timeout group. + }) + // Surfaces that intentionally bypass the REST timeout (SSE, future WS) + // register at this level — none exist in the route-shell PR. + }) +} + +// notFoundJSON returns the locked envelope for unmatched routes. Chi's default +// 404 is a text/plain body; the API surface must answer JSON so consumers can +// parse it uniformly. +func notFoundJSON(w http.ResponseWriter, r *http.Request) { + writeAPIError(w, r, http.StatusNotFound, "not_found", "ROUTE_NOT_FOUND", + r.Method+" "+r.URL.Path+" has no handler", nil) +} + +// methodNotAllowedJSON returns the locked envelope when a method probes a +// known path without a matching verb (e.g. PUT /projects/{id} after we drop +// the legacy PUT alias). +func methodNotAllowedJSON(w http.ResponseWriter, r *http.Request) { + writeAPIError(w, r, http.StatusMethodNotAllowed, "method_not_allowed", "METHOD_NOT_ALLOWED", + r.Method+" not allowed on "+r.URL.Path, nil) +} diff --git a/backend/internal/httpd/controllers/projects.go b/backend/internal/httpd/controllers/projects.go new file mode 100644 index 00000000..42032584 --- /dev/null +++ b/backend/internal/httpd/controllers/projects.go @@ -0,0 +1,209 @@ +// Package controllers holds the HTTP-facing controllers for the /api/v1 +// surface. Each controller groups one resource's routes, exposes a Register +// method that wires them on a chi.Router, and carries the service dependency +// it will eventually call. +// +// In the route-shell PR (#20) every handler returns 501 with a PlannedRoute +// body documenting the future contract. Handler-impl PRs in the same lane +// flip routes to real logic one at a time without touching the wiring. +package controllers + +import ( + "net/http" + + "github.com/go-chi/chi/v5" + + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/stubs" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +// ProjectsController owns the 7 canonical /projects routes. Svc is nil while +// handlers are stubs; impl PRs supply a real ProjectService. +type ProjectsController struct { + Svc ports.ProjectService +} + +// Register mounts the project routes on the supplied router. Route order +// matters: /projects/reload must register before /projects/{id} for the POST +// verb, otherwise chi would treat "reload" as an {id} match for repair. +// +// Legacy paths that the REST audit dropped are deliberately NOT registered +// here. They surface as 405 (sibling method exists, e.g. PUT /projects/{id}) +// or 404 (no sibling, e.g. POST /projects/{id} for repair → moved to +// /projects/{id}/repair). The mapping lives in each canonical handler's +// PlannedRoute.Legacy so consumers can discover the migration. +func (c *ProjectsController) Register(r chi.Router) { + r.Get("/projects", c.list) + r.Post("/projects", c.add) + r.Post("/projects/reload", c.reload) // BEFORE /projects/{id} + r.Get("/projects/{id}", c.get) + r.Patch("/projects/{id}", c.updateConfig) + r.Delete("/projects/{id}", c.remove) + r.Post("/projects/{id}/repair", c.repair) +} + +// list — GET /api/v1/projects +func (c *ProjectsController) list(w http.ResponseWriter, r *http.Request) { + stubs.NotImplemented(w, r, stubs.PlannedRoute{ + Route: "GET /api/v1/projects", + Response: map[string]any{ + "200": map[string]any{"projects": "[]domain.ProjectSummary"}, + }, + Errors: []stubs.PlannedError{ + {Status: 500, Code: "PROJECTS_LIST_FAILED", Message: "Failed to load projects"}, + }, + Notes: "Wraps the registry list. Includes degraded entries with a resolveError field.", + }) +} + +// add — POST /api/v1/projects +func (c *ProjectsController) add(w http.ResponseWriter, r *http.Request) { + stubs.NotImplemented(w, r, stubs.PlannedRoute{ + Route: "POST /api/v1/projects", + Request: map[string]any{ + "body": map[string]any{ + "path": "string (required; supports ~ home-expansion; must point to a git repo)", + "projectId": "string (optional; defaults to basename(path))", + "name": "string (optional; defaults to projectId)", + }, + }, + Response: map[string]any{ + "201": map[string]any{"project": "domain.Project"}, + }, + Errors: []stubs.PlannedError{ + {Status: 400, Code: "INVALID_JSON", Message: "Invalid JSON body"}, + {Status: 400, Code: "PATH_REQUIRED", Message: "Repository path is required"}, + {Status: 400, Code: "NOT_A_GIT_REPO", Message: "Repository path must point to a git repository"}, + { + Status: 409, Code: "PATH_ALREADY_REGISTERED", + Message: "A project at this path is already registered", + Details: map[string]any{ + "existingProjectId": "string", + "suggestedProjectId": "string", + }, + }, + { + Status: 409, Code: "ID_ALREADY_REGISTERED", + Message: "A project with this id is already registered for a different path", + Details: map[string]any{ + "existingProjectId": "string", + "suggestedProjectId": "string", + }, + }, + }, + }) +} + +// get — GET /api/v1/projects/{id} +func (c *ProjectsController) get(w http.ResponseWriter, r *http.Request) { + stubs.NotImplemented(w, r, stubs.PlannedRoute{ + Route: "GET /api/v1/projects/{id}", + Request: map[string]any{ + "path": map[string]any{"id": "ProjectID"}, + }, + Response: map[string]any{ + "200": map[string]any{ + "status": `"ok" | "degraded"`, + "project": "domain.Project (status=ok) | domain.DegradedProject (status=degraded)", + }, + }, + Errors: []stubs.PlannedError{ + {Status: 404, Code: "PROJECT_NOT_FOUND", Message: "Unknown project"}, + {Status: 500, Code: "PROJECT_LOAD_FAILED", Message: "Failed to load project"}, + }, + Notes: "REST-audit R5: degraded projects return 200 with a status discriminator, not 200 with an error field.", + }) +} + +// updateConfig — PATCH /api/v1/projects/{id} +func (c *ProjectsController) updateConfig(w http.ResponseWriter, r *http.Request) { + stubs.NotImplemented(w, r, stubs.PlannedRoute{ + Route: "PATCH /api/v1/projects/{id}", + Request: map[string]any{ + "path": map[string]any{"id": "ProjectID"}, + "body": map[string]any{ + "agent": "string (optional)", + "runtime": "string (optional)", + "tracker": "TrackerConfig (optional)", + "scm": "SCMConfig (optional)", + "reactions": "map[string]ReactionConfig (optional)", + }, + }, + Response: map[string]any{ + "200": map[string]any{"project": "domain.Project"}, + }, + Errors: []stubs.PlannedError{ + {Status: 400, Code: "INVALID_JSON", Message: "Invalid JSON body"}, + { + Status: 400, Code: "IDENTITY_FROZEN", + Message: "Identity fields cannot be patched", + Details: map[string]any{"fields": "[]string"}, + }, + {Status: 400, Code: "INVALID_LOCAL_CONFIG", Message: "Local project config failed validation"}, + {Status: 404, Code: "PROJECT_NOT_FOUND", Message: "Unknown project"}, + {Status: 409, Code: "PROJECT_DEGRADED", Message: "Project config is degraded; repair before patching"}, + {Status: 409, Code: "PROJECT_MISSING_PATH", Message: "Project registry entry is missing a path"}, + }, + Notes: "REST-audit R3: legacy PUT alias is NOT registered; PUT /projects/{id} returns 405. R6: returns {project}, not {ok:true}.", + }) +} + +// remove — DELETE /api/v1/projects/{id} +func (c *ProjectsController) remove(w http.ResponseWriter, r *http.Request) { + stubs.NotImplemented(w, r, stubs.PlannedRoute{ + Route: "DELETE /api/v1/projects/{id}", + Request: map[string]any{ + "path": map[string]any{"id": "ProjectID"}, + }, + Response: map[string]any{ + "200": map[string]any{ + "projectId": "ProjectID", + "removedStorageDir": "bool", + }, + }, + Errors: []stubs.PlannedError{ + {Status: 400, Code: "INVALID_PROJECT_ID", Message: "Project id failed storage-path validation"}, + {Status: 404, Code: "PROJECT_NOT_FOUND", Message: "Unknown project"}, + {Status: 500, Code: "PROJECT_REMOVE_FAILED", Message: "Failed to remove project"}, + }, + Notes: "Side effects (in handler-impl PR): stop project sessions, cleanup managed workspaces, unregister, remove storage dir.", + }) +} + +// repair — POST /api/v1/projects/{id}/repair +func (c *ProjectsController) repair(w http.ResponseWriter, r *http.Request) { + stubs.NotImplemented(w, r, stubs.PlannedRoute{ + Route: "POST /api/v1/projects/{id}/repair", + Legacy: []string{"POST /api/v1/projects/{id}"}, + Request: map[string]any{ + "path": map[string]any{"id": "ProjectID"}, + }, + Response: map[string]any{ + "200": map[string]any{"project": "domain.Project"}, + }, + Errors: []stubs.PlannedError{ + {Status: 400, Code: "PROJECT_NOT_DEGRADED", Message: "Project does not need repair"}, + {Status: 400, Code: "REPAIR_NOT_AVAILABLE", Message: "Automatic repair is not available for this degraded config"}, + {Status: 404, Code: "PROJECT_NOT_FOUND", Message: "Unknown project"}, + }, + Notes: "REST-audit R4: replaces the overloaded POST /projects/{id}. Legacy path is NOT registered; consumers must call /repair.", + }) +} + +// reload — POST /api/v1/projects/reload +func (c *ProjectsController) reload(w http.ResponseWriter, r *http.Request) { + stubs.NotImplemented(w, r, stubs.PlannedRoute{ + Route: "POST /api/v1/projects/reload", + Response: map[string]any{ + "200": map[string]any{ + "reloaded": "bool", + "projectCount": "int", + "degradedCount": "int", + }, + }, + Errors: []stubs.PlannedError{ + {Status: 500, Code: "RELOAD_FAILED", Message: "Failed to reload projects"}, + }, + Notes: "Invalidates the cached services registry and re-loads the global config.", + }) +} diff --git a/backend/internal/httpd/controllers/projects_test.go b/backend/internal/httpd/controllers/projects_test.go new file mode 100644 index 00000000..d27b82ab --- /dev/null +++ b/backend/internal/httpd/controllers/projects_test.go @@ -0,0 +1,232 @@ +package controllers_test + +// Route-shell tests for /api/v1/projects. Builds the full router (so the +// /api/v1 mount, middleware, NotFound, and MethodNotAllowed handlers are +// exercised together) and asserts every canonical route returns 501 with the +// locked envelope; legacy paths that the REST audit dropped return 404 or 405. + +import ( + "encoding/json" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/aoagents/agent-orchestrator/backend/internal/config" + "github.com/aoagents/agent-orchestrator/backend/internal/httpd" +) + +func newTestServer(t *testing.T) *httptest.Server { + t.Helper() + // Discard logger keeps test output clean — the access-log middleware + // added in base #10·1a wants a non-nil *slog.Logger. + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + srv := httptest.NewServer(httpd.NewRouter(config.Config{}, log)) + t.Cleanup(srv.Close) + return srv +} + +// TestProjectsRoutes_Canonical501 walks every canonical /projects route and +// asserts it returns 501 with the locked envelope. The "wantLegacy" cases +// double-check that the planned body advertises the TS path it replaced. +func TestProjectsRoutes_Canonical501(t *testing.T) { + srv := newTestServer(t) + + cases := []struct { + method string + path string + body string + wantRoute string + wantLegacy []string + }{ + {method: "GET", path: "/api/v1/projects", wantRoute: "GET /api/v1/projects"}, + {method: "POST", path: "/api/v1/projects", body: `{}`, wantRoute: "POST /api/v1/projects"}, + {method: "GET", path: "/api/v1/projects/p1", wantRoute: "GET /api/v1/projects/{id}"}, + {method: "PATCH", path: "/api/v1/projects/p1", body: `{}`, wantRoute: "PATCH /api/v1/projects/{id}"}, + {method: "DELETE", path: "/api/v1/projects/p1", wantRoute: "DELETE /api/v1/projects/{id}"}, + { + method: "POST", path: "/api/v1/projects/p1/repair", + wantRoute: "POST /api/v1/projects/{id}/repair", + wantLegacy: []string{"POST /api/v1/projects/{id}"}, + }, + {method: "POST", path: "/api/v1/projects/reload", wantRoute: "POST /api/v1/projects/reload"}, + } + + for _, tc := range cases { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + body, status, headers := doRequest(t, srv, tc.method, tc.path, tc.body) + + if status != http.StatusNotImplemented { + t.Fatalf("status = %d, want 501", status) + } + if ct := headers.Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { + t.Errorf("Content-Type = %q, want JSON", ct) + } + + var got envelope + if err := json.Unmarshal(body, &got); err != nil { + t.Fatalf("unmarshal: %v\nbody=%s", err, body) + } + if got.Error != "not_implemented" || got.Code != "NOT_IMPLEMENTED" { + t.Errorf("envelope error/code = %q/%q, want not_implemented/NOT_IMPLEMENTED", got.Error, got.Code) + } + if got.Message == "" { + t.Error("envelope.message empty") + } + if got.Planned.Route != tc.wantRoute { + t.Errorf("planned.route = %q, want %q", got.Planned.Route, tc.wantRoute) + } + if !equalStrings(got.Planned.Legacy, tc.wantLegacy) { + t.Errorf("planned.legacy = %v, want %v", got.Planned.Legacy, tc.wantLegacy) + } + }) + } +} + +// TestProjectsRoutes_LegacyUnregistered confirms the REST-audit-dropped TS +// paths are deliberately unregistered. PUT and POST on /projects/{id} hit +// sibling-method paths so chi returns 405; legacy nested paths return 404. +func TestProjectsRoutes_LegacyUnregistered(t *testing.T) { + srv := newTestServer(t) + + cases := []struct { + method string + path string + wantStatus int + wantCode string + why string + }{ + // R3: PUT was a PATCH alias in TS; we keep PATCH only. Chi returns + // 405 because sibling verbs (GET/PATCH/DELETE) exist on the same path. + {method: "PUT", path: "/api/v1/projects/p1", wantStatus: 405, wantCode: "METHOD_NOT_ALLOWED", why: "R3 PUT not registered"}, + // R4: POST on /projects/{id} used to repair; canonical is /repair. + // Same path has no sibling POST handler (POST collection is at /projects), + // chi returns 405. + {method: "POST", path: "/api/v1/projects/p1", wantStatus: 405, wantCode: "METHOD_NOT_ALLOWED", why: "R4 repair moved"}, + } + + for _, tc := range cases { + t.Run(tc.why, func(t *testing.T) { + body, status, _ := doRequest(t, srv, tc.method, tc.path, "") + if status != tc.wantStatus { + t.Fatalf("%s %s = %d, want %d", tc.method, tc.path, status, tc.wantStatus) + } + var e envelope + if err := json.Unmarshal(body, &e); err != nil { + t.Fatalf("unmarshal: %v\nbody=%s", err, body) + } + if e.Code != tc.wantCode { + t.Errorf("code = %q, want %q", e.Code, tc.wantCode) + } + }) + } +} + +// TestProjectsRoutes_MissingRoute confirms the JSON 404 envelope (not chi's +// default text/plain) for routes that don't exist at all. +func TestProjectsRoutes_MissingRoute(t *testing.T) { + srv := newTestServer(t) + body, status, headers := doRequest(t, srv, "GET", "/api/v1/projects/p1/does-not-exist", "") + + if status != http.StatusNotFound { + t.Fatalf("status = %d, want 404", status) + } + if ct := headers.Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { + t.Errorf("Content-Type = %q, want JSON (router must override chi's text/plain default)", ct) + } + var e envelope + if err := json.Unmarshal(body, &e); err != nil { + t.Fatalf("unmarshal: %v\nbody=%s", err, body) + } + if e.Code != "ROUTE_NOT_FOUND" { + t.Errorf("code = %q, want ROUTE_NOT_FOUND", e.Code) + } +} + +// TestProjectsRoutes_ReloadBeforeID is the chi-ordering safety check. If the +// {id} wildcard were registered first, POST /projects/reload would match +// {id}="reload" → repair handler instead of the reload handler. We assert +// reload responds with its own planned.route. +func TestProjectsRoutes_ReloadBeforeID(t *testing.T) { + srv := newTestServer(t) + body, status, _ := doRequest(t, srv, "POST", "/api/v1/projects/reload", "") + if status != http.StatusNotImplemented { + t.Fatalf("status = %d, want 501", status) + } + var e envelope + _ = json.Unmarshal(body, &e) + if e.Planned.Route != "POST /api/v1/projects/reload" { + t.Errorf("reload was shadowed by {id}: planned.route = %q", e.Planned.Route) + } +} + +// envelope mirrors the locked APIError + stubs.PlannedRoute on the wire. We +// declare it in the test rather than importing httpd's private type so the +// test pins the JSON contract independently of internal renames. +type envelope struct { + Error string `json:"error"` + Code string `json:"code"` + Message string `json:"message"` + RequestID string `json:"requestId"` + Planned planned `json:"planned"` +} + +type planned struct { + Route string `json:"route"` + Legacy []string `json:"legacy"` +} + +func doRequest(t *testing.T, srv *httptest.Server, method, path, body string) ([]byte, int, http.Header) { + t.Helper() + var reqBody *strings.Reader + if body != "" { + reqBody = strings.NewReader(body) + } + // strings.Reader is not nilable for the no-body case via the io.Reader + // interface, so branch the constructor explicitly. + var req *http.Request + var err error + if reqBody != nil { + req, err = http.NewRequest(method, srv.URL+path, reqBody) + } else { + req, err = http.NewRequest(method, srv.URL+path, nil) + } + if err != nil { + t.Fatalf("new request: %v", err) + } + if body != "" { + req.Header.Set("Content-Type", "application/json") + } + + resp, err := srv.Client().Do(req) + if err != nil { + t.Fatalf("do request: %v", err) + } + defer resp.Body.Close() + buf := make([]byte, 0, 1024) + tmp := make([]byte, 512) + for { + n, rerr := resp.Body.Read(tmp) + if n > 0 { + buf = append(buf, tmp[:n]...) + } + if rerr != nil { + break + } + } + return buf, resp.StatusCode, resp.Header +} + +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/backend/internal/httpd/errors.go b/backend/internal/httpd/errors.go new file mode 100644 index 00000000..1843fce8 --- /dev/null +++ b/backend/internal/httpd/errors.go @@ -0,0 +1,34 @@ +package httpd + +import ( + "net/http" + + "github.com/go-chi/chi/v5/middleware" +) + +// APIError is the locked wire shape for every non-2xx response. It supersedes +// the legacy TS `{error: "msg"}` bag with a machine-readable Code and a +// RequestID for log correlation (sourced from chi's RequestID middleware). +// +// Details is open so collision-style errors can carry typed sub-fields +// (e.g. existingProjectId, suggestedProjectId on POST /projects 409s). +type APIError struct { + Error string `json:"error"` // short kind, e.g. "not_found" + Code string `json:"code"` // SCREAMING_SNAKE, e.g. "PROJECT_NOT_FOUND" + Message string `json:"message"` // human-readable + RequestID string `json:"requestId,omitempty"` + Details map[string]any `json:"details,omitempty"` +} + +// writeAPIError emits the locked envelope for any non-2xx response. The +// request id falls back to empty when the chi middleware hasn't tagged the +// request (e.g. in tests that bypass NewRouter). +func writeAPIError(w http.ResponseWriter, r *http.Request, status int, kind, code, message string, details map[string]any) { + writeJSON(w, status, APIError{ + Error: kind, + Code: code, + Message: message, + RequestID: middleware.GetReqID(r.Context()), + Details: details, + }) +} diff --git a/backend/internal/httpd/router.go b/backend/internal/httpd/router.go index 6e078b8d..e4c5b570 100644 --- a/backend/internal/httpd/router.go +++ b/backend/internal/httpd/router.go @@ -30,6 +30,13 @@ import ( // probes. It is therefore applied per-surface when those subrouters are mounted // in Phase 1b; cfg.RequestTimeout carries the value through to that point. func NewRouter(cfg config.Config, log *slog.Logger) chi.Router { + return NewRouterWithAPI(cfg, log, APIDeps{}) +} + +// NewRouterWithAPI is the dependency-injected variant. main.go calls it with +// real Managers; tests and the zero-dep NewRouter call it with empty APIDeps +// so route-shell 501 stubs work without wiring every port. +func NewRouterWithAPI(cfg config.Config, log *slog.Logger, deps APIDeps) chi.Router { r := chi.NewRouter() r.Use(middleware.Recoverer) @@ -37,7 +44,14 @@ func NewRouter(cfg config.Config, log *slog.Logger) chi.Router { r.Use(requestLogger(log)) r.Use(middleware.RealIP) + // JSON envelopes for unmatched routes / methods — chi's defaults are + // text/plain, which would break consumers that parse every response as + // the locked APIError shape. + r.NotFound(notFoundJSON) + r.MethodNotAllowed(methodNotAllowedJSON) + mountHealth(r) + NewAPI(cfg, deps).Register(r) return r } diff --git a/backend/internal/httpd/stubs/notimpl.go b/backend/internal/httpd/stubs/notimpl.go new file mode 100644 index 00000000..ea10b9e1 --- /dev/null +++ b/backend/internal/httpd/stubs/notimpl.go @@ -0,0 +1,66 @@ +// Package stubs holds the 501-with-contract-preview helper every route-shell +// handler uses. It lives in its own package (rather than alongside the router +// in httpd) so the per-resource controllers can import it without creating an +// import cycle back into httpd. +package stubs + +import ( + "encoding/json" + "net/http" + + "github.com/go-chi/chi/v5/middleware" +) + +// PlannedRoute is the documentation payload every 501 stub returns alongside +// the locked error envelope. It tells API consumers what the endpoint WILL +// look like once the real handler lands, so the dashboard team can scaffold +// against the contract before implementation merges. +// +// Legacy carries the old TS paths this route REPLACES — set on canonical +// routes that subsumed a now-unregistered TS endpoint. #19 (OpenAPI) will +// translate Legacy into an x-replaces extension on the spec. +type PlannedRoute struct { + Route string `json:"route"` + Legacy []string `json:"legacy,omitempty"` + Request map[string]any `json:"request,omitempty"` + Response map[string]any `json:"response,omitempty"` + Errors []PlannedError `json:"errors,omitempty"` + Notes string `json:"notes,omitempty"` +} + +// PlannedError describes one error variant the future implementation will +// return. Status + Code together identify the variant uniquely. +type PlannedError struct { + Status int `json:"status"` + Code string `json:"code"` + Message string `json:"message"` + Details map[string]any `json:"details,omitempty"` +} + +// notImplementedResponse is the full 501 body: the locked APIError envelope +// fields plus a Planned block that documents the future contract. +type notImplementedResponse struct { + Error string `json:"error"` + Code string `json:"code"` + Message string `json:"message"` + RequestID string `json:"requestId,omitempty"` + Planned PlannedRoute `json:"planned"` +} + +// NotImplemented writes a 501 with the locked envelope plus the planned-route +// documentation. Used by every route in the shell; replaced one-by-one with +// real handlers as the lane progresses. +func NotImplemented(w http.ResponseWriter, r *http.Request, planned PlannedRoute) { + body := notImplementedResponse{ + Error: "not_implemented", + Code: "NOT_IMPLEMENTED", + Message: planned.Route + " is registered but not yet implemented", + RequestID: middleware.GetReqID(r.Context()), + Planned: planned, + } + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(http.StatusNotImplemented) + // A write error here means the client went away mid-response; there is + // nothing useful to do but stop. + _ = json.NewEncoder(w).Encode(body) +} diff --git a/backend/internal/ports/services.go b/backend/internal/ports/services.go new file mode 100644 index 00000000..370c9b14 --- /dev/null +++ b/backend/internal/ports/services.go @@ -0,0 +1,77 @@ +// Package ports — services.go declares the API-layer service contracts the +// HTTP controllers call. They are distinct from the LCM-facing SessionManager +// (inbound.go) and the persistence/SCM ports (outbound.go): these wrap the +// concerns that only make sense at the HTTP boundary (list filtering, +// enrichment, the "add/remove/repair project" administrative surface). +// +// In the route-shell PR (#20) every implementation is nil — controllers register +// 501 stubs and never call into the services. The interfaces exist so handler- +// impl PRs in the same lane can fill them in without re-touching the wiring. +package ports + +import ( + "context" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// ProjectService is the API-layer surface for the projects routes. It owns +// administrative concerns (add/remove/repair) that the dashboard exposes but +// the LCM has no business doing. +type ProjectService interface { + List(ctx context.Context) ([]domain.ProjectSummary, error) + Get(ctx context.Context, id domain.ProjectID) (GetProjectResult, error) + Add(ctx context.Context, in AddProjectInput) (domain.Project, error) + UpdateConfig(ctx context.Context, id domain.ProjectID, patch UpdateProjectConfigInput) (domain.Project, error) + Remove(ctx context.Context, id domain.ProjectID) (RemoveProjectResult, error) + Repair(ctx context.Context, id domain.ProjectID) (domain.Project, error) + Reload(ctx context.Context) (ReloadResult, error) +} + +// GetProjectResult is the discriminated union returned by ProjectService.Get. +// Exactly one of Project / Degraded is non-nil. Status mirrors the +// discriminator on the wire so consumers branch on it without nil-checking +// both fields. +type GetProjectResult struct { + Status string // "ok" | "degraded" + Project *domain.Project // populated when Status == "ok" + Degraded *domain.DegradedProject // populated when Status == "degraded" +} + +// AddProjectInput is the body shape for POST /api/v1/projects. Path is +// required; ProjectID and Name default to basename(path) at the service. +// Pointer fields preserve the "field absent" vs "field present empty" +// distinction so the service can decide what to default and what to reject. +type AddProjectInput struct { + Path string `json:"path"` + ProjectID *string `json:"projectId,omitempty"` + Name *string `json:"name,omitempty"` +} + +// UpdateProjectConfigInput is the body shape for PATCH /api/v1/projects/{id}. +// Only behaviour fields are mutable; identity fields (projectId, path, repo, +// defaultBranch) are rejected by the handler with a 400 IDENTITY_FROZEN. +type UpdateProjectConfigInput struct { + Agent *string `json:"agent,omitempty"` + Runtime *string `json:"runtime,omitempty"` + Tracker *domain.TrackerConfig `json:"tracker,omitempty"` + SCM *domain.SCMConfig `json:"scm,omitempty"` + Reactions *map[string]*domain.ReactionConfig `json:"reactions,omitempty"` +} + +// RemoveProjectResult reports what DELETE /api/v1/projects/{id} actually did. +// RemovedStorageDir is false when the project was registry-only (no on-disk +// session/workspace directory existed). +type RemoveProjectResult struct { + ProjectID domain.ProjectID `json:"projectId"` + RemovedStorageDir bool `json:"removedStorageDir"` +} + +// ReloadResult is the response body of POST /api/v1/projects/reload — the +// service invalidates its cached config and re-scans the registry; the counts +// help the dashboard show "loaded N projects, M degraded" feedback. +type ReloadResult struct { + Reloaded bool `json:"reloaded"` + ProjectCount int `json:"projectCount"` + DegradedCount int `json:"degradedCount"` +} From 7319ab830542cd22ef9fc0d665db9ac2463422bc Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Thu, 28 May 2026 20:14:39 +0530 Subject: [PATCH 09/23] =?UTF-8?q?refactor(api):=20collapse=20ProjectServic?= =?UTF-8?q?e=20=E2=86=92=20ProjectManager=20=E2=80=94=20#20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Controllers now depend on ONE inbound interface per resource — ports.ProjectManager — mirroring the existing ports.SessionManager + LifecycleManager pattern. Whether the manager impl reaches into the registry, the LCM, an outbound port, or all three is its own concern; the HTTP layer no longer has to know any of that. WHY: the original split named the boundary type "ProjectService" and put it in a sibling services.go. That implied a second category of port distinct from inbound.go's *Manager interfaces, even though they play the same role (things HTTP/CLI call into the core). Per review feedback, collapse them onto one Manager-per-resource pattern. Mechanical changes: - ports/inbound.go gains ProjectManager next to SessionManager. - ports/services.go renamed to projects.go; keeps only the DTOs the ProjectManager methods take/return. - ProjectsController.Svc renamed to Mgr; APIDeps.Projects type bumped to ports.ProjectManager. All tests pass unchanged; no behavioural change. --- backend/internal/httpd/api.go | 15 ++++--- .../internal/httpd/controllers/projects.go | 8 ++-- backend/internal/ports/inbound.go | 15 +++++++ .../ports/{services.go => projects.go} | 43 ++++++------------- 4 files changed, 42 insertions(+), 39 deletions(-) rename backend/internal/ports/{services.go => projects.go} (54%) diff --git a/backend/internal/httpd/api.go b/backend/internal/httpd/api.go index 7af9a78b..74724d73 100644 --- a/backend/internal/httpd/api.go +++ b/backend/internal/httpd/api.go @@ -11,12 +11,17 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) -// APIDeps bundles every service the API layer's controllers depend on. The -// route-shell PR (#20) leaves all of them nil — handlers don't dereference -// them yet. Real handler PRs in the same lane fill the relevant field and +// APIDeps bundles every Manager the API layer's controllers depend on. There +// is exactly one *Manager per resource and the controllers see ONLY that +// interface — they don't reach past it to inbound/outbound ports, the LCM, +// or adapters. Whether a Manager impl talks to the registry, the LCM, or +// an outbound port is its own concern. +// +// The route-shell PR (#20) leaves every field nil — handlers don't +// dereference them yet. Impl PRs in the same lane wire real Managers and // flip stubs to real logic one route at a time. type APIDeps struct { - Projects ports.ProjectService + Projects ports.ProjectManager } // API owns one controller per resource and is the single Register call the @@ -35,7 +40,7 @@ func NewAPI(cfg config.Config, deps APIDeps) *API { return &API{ cfg: cfg, projects: &controllers.ProjectsController{ - Svc: deps.Projects, + Mgr: deps.Projects, }, } } diff --git a/backend/internal/httpd/controllers/projects.go b/backend/internal/httpd/controllers/projects.go index 42032584..655f3085 100644 --- a/backend/internal/httpd/controllers/projects.go +++ b/backend/internal/httpd/controllers/projects.go @@ -17,10 +17,12 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) -// ProjectsController owns the 7 canonical /projects routes. Svc is nil while -// handlers are stubs; impl PRs supply a real ProjectService. +// ProjectsController owns the 7 canonical /projects routes. The controller +// depends ONLY on ports.ProjectManager — it doesn't know whether the impl +// reaches into the registry, the LCM, an adapter, or all three. Mgr is nil +// while handlers are stubs; impl PRs supply a real ProjectManager. type ProjectsController struct { - Svc ports.ProjectService + Mgr ports.ProjectManager } // Register mounts the project routes on the supplied router. Route order diff --git a/backend/internal/ports/inbound.go b/backend/internal/ports/inbound.go index 30ab7559..d2a77cdd 100644 --- a/backend/internal/ports/inbound.go +++ b/backend/internal/ports/inbound.go @@ -68,3 +68,18 @@ type CleanupResult struct { Cleaned []domain.SessionID Skipped []domain.SessionID // e.g. paths that still held uncommitted work } + +// ProjectManager is the inbound contract the API layer calls for every project +// operation. The HTTP controllers see ONLY this interface — they never reach +// past it to the registry, LCM, workspace adapter, or SCM. Whether a method +// touches the global config registry, the LCM (to stop sessions on remove), +// or an adapter (to destroy worktrees) is a private concern of the impl. +type ProjectManager interface { + List(ctx context.Context) ([]domain.ProjectSummary, error) + Get(ctx context.Context, id domain.ProjectID) (GetProjectResult, error) + Add(ctx context.Context, in AddProjectInput) (domain.Project, error) + UpdateConfig(ctx context.Context, id domain.ProjectID, patch UpdateProjectConfigInput) (domain.Project, error) + Remove(ctx context.Context, id domain.ProjectID) (RemoveProjectResult, error) + Repair(ctx context.Context, id domain.ProjectID) (domain.Project, error) + Reload(ctx context.Context) (ReloadResult, error) +} diff --git a/backend/internal/ports/services.go b/backend/internal/ports/projects.go similarity index 54% rename from backend/internal/ports/services.go rename to backend/internal/ports/projects.go index 370c9b14..90fb29dc 100644 --- a/backend/internal/ports/services.go +++ b/backend/internal/ports/projects.go @@ -1,34 +1,15 @@ -// Package ports — services.go declares the API-layer service contracts the -// HTTP controllers call. They are distinct from the LCM-facing SessionManager -// (inbound.go) and the persistence/SCM ports (outbound.go): these wrap the -// concerns that only make sense at the HTTP boundary (list filtering, -// enrichment, the "add/remove/repair project" administrative surface). -// -// In the route-shell PR (#20) every implementation is nil — controllers register -// 501 stubs and never call into the services. The interfaces exist so handler- -// impl PRs in the same lane can fill them in without re-touching the wiring. package ports -import ( - "context" +import "github.com/aoagents/agent-orchestrator/backend/internal/domain" - "github.com/aoagents/agent-orchestrator/backend/internal/domain" -) +// DTOs for ProjectManager (declared in inbound.go alongside its peers +// SessionManager and LifecycleManager). The interface is the boundary +// controllers care about; these types carry the request/response shapes +// across that boundary. Whether the manager impl reaches into the registry, +// the LCM, or an adapter is a private concern — controllers see only the +// types here and the methods on ProjectManager. -// ProjectService is the API-layer surface for the projects routes. It owns -// administrative concerns (add/remove/repair) that the dashboard exposes but -// the LCM has no business doing. -type ProjectService interface { - List(ctx context.Context) ([]domain.ProjectSummary, error) - Get(ctx context.Context, id domain.ProjectID) (GetProjectResult, error) - Add(ctx context.Context, in AddProjectInput) (domain.Project, error) - UpdateConfig(ctx context.Context, id domain.ProjectID, patch UpdateProjectConfigInput) (domain.Project, error) - Remove(ctx context.Context, id domain.ProjectID) (RemoveProjectResult, error) - Repair(ctx context.Context, id domain.ProjectID) (domain.Project, error) - Reload(ctx context.Context) (ReloadResult, error) -} - -// GetProjectResult is the discriminated union returned by ProjectService.Get. +// GetProjectResult is the discriminated union returned by ProjectManager.Get. // Exactly one of Project / Degraded is non-nil. Status mirrors the // discriminator on the wire so consumers branch on it without nil-checking // both fields. @@ -39,9 +20,9 @@ type GetProjectResult struct { } // AddProjectInput is the body shape for POST /api/v1/projects. Path is -// required; ProjectID and Name default to basename(path) at the service. +// required; ProjectID and Name default to basename(path) at the manager. // Pointer fields preserve the "field absent" vs "field present empty" -// distinction so the service can decide what to default and what to reject. +// distinction so the manager can decide what to default and what to reject. type AddProjectInput struct { Path string `json:"path"` ProjectID *string `json:"projectId,omitempty"` @@ -68,8 +49,8 @@ type RemoveProjectResult struct { } // ReloadResult is the response body of POST /api/v1/projects/reload — the -// service invalidates its cached config and re-scans the registry; the counts -// help the dashboard show "loaded N projects, M degraded" feedback. +// manager invalidates its cached config and re-scans the registry; the +// counts help the dashboard show "loaded N projects, M degraded" feedback. type ReloadResult struct { Reloaded bool `json:"reloaded"` ProjectCount int `json:"projectCount"` From 3906d566f10cdf2c13373449f99872b25cf88e07 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Thu, 28 May 2026 22:33:57 +0530 Subject: [PATCH 10/23] =?UTF-8?q?refactor(api):=20replace=20stubs/=20with?= =?UTF-8?q?=20OpenAPI-as-source-of-truth=20=E2=80=94=20#20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut of the route shell duplicated each route's contract twice: once as a Go literal (stubs.PlannedRoute{...}) in the controller, and implicitly in the PR description. The Go literal was ~230 LoC of pure throwaway that would be deleted in handler-impl PRs. This commit eliminates the duplication: - backend/internal/httpd/apispec/openapi.yaml: full OpenAPI 3.1 doc covering the 7 project routes + shared schemas (Project, APIError, config types). x-replaces records the legacy → canonical mapping REST-audit corrections produced. - apispec/apispec.go: //go:embed the YAML, expose Operation(method, path) → the spec slice as a map, NotImplemented(w, r, method, path) → 501 with that slice embedded as `spec`. - controllers/projects.go: each of 7 handlers is now a one-liner: apispec.NotImplemented(w, r, "GET", "/api/v1/projects"). - /api/v1/openapi.yaml serves the embedded document so tooling (SDK gen, the validator slated for #19, dashboard dev tools) can fetch the whole spec from the same origin as the routes. - stubs/ package deleted. When a real handler lands, only the apispec.NotImplemented line goes away — nothing else does. The spec stays as documentation; consumers never had to know it was throwaway. #19 (OpenAPI follow-up) is now half-folded into this PR; the validation middleware remains its own follow-up. Tests reshaped: assert envelope + spec.operationId + spec.x-replaces (replaces the old planned.legacy assertion); add TestOpenAPIYAMLServed to cover the static spec serve; add apispec_test.go for embed/lookup behaviour. --- backend/go.mod | 2 + backend/go.sum | 3 + backend/internal/httpd/api.go | 7 + backend/internal/httpd/apispec/apispec.go | 155 ++++++ .../internal/httpd/apispec/apispec_test.go | 70 +++ backend/internal/httpd/apispec/openapi.yaml | 447 ++++++++++++++++++ .../internal/httpd/controllers/projects.go | 175 +------ .../httpd/controllers/projects_test.go | 147 +++--- backend/internal/httpd/stubs/notimpl.go | 66 --- 9 files changed, 792 insertions(+), 280 deletions(-) create mode 100644 backend/internal/httpd/apispec/apispec.go create mode 100644 backend/internal/httpd/apispec/apispec_test.go create mode 100644 backend/internal/httpd/apispec/openapi.yaml delete mode 100644 backend/internal/httpd/stubs/notimpl.go diff --git a/backend/go.mod b/backend/go.mod index 311cea28..651c6594 100644 --- a/backend/go.mod +++ b/backend/go.mod @@ -3,3 +3,5 @@ module github.com/aoagents/agent-orchestrator/backend go 1.22 require github.com/go-chi/chi/v5 v5.1.0 + +require gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/backend/go.sum b/backend/go.sum index 823cdbb1..c958a7c2 100644 --- a/backend/go.sum +++ b/backend/go.sum @@ -1,2 +1,5 @@ github.com/go-chi/chi/v5 v5.1.0 h1:acVI1TYaD+hhedDJ3r54HyA6sExp3HfXq7QWEEY/xMw= github.com/go-chi/chi/v5 v5.1.0/go.mod h1:DslCQbL2OYiznFReuXYUmQ2hGd1aDpCnlMNITLSKoi8= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/backend/internal/httpd/api.go b/backend/internal/httpd/api.go index 74724d73..bb47059b 100644 --- a/backend/internal/httpd/api.go +++ b/backend/internal/httpd/api.go @@ -7,6 +7,7 @@ import ( "github.com/go-chi/chi/v5/middleware" "github.com/aoagents/agent-orchestrator/backend/internal/config" + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/controllers" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) @@ -59,6 +60,12 @@ func (a *API) Register(root chi.Router) { } root.Route("/api/v1", func(r chi.Router) { + // The OpenAPI document is the source of truth for every contract on + // this surface; serve it so tooling (SDK generators, the OpenAPI + // validator in #19, the dashboard's developer tools) can fetch the + // whole spec from the same origin as the routes it describes. + apispec.RegisterServe(r, "/openapi.yaml") + r.Group(func(r chi.Router) { r.Use(middleware.Timeout(timeout)) a.projects.Register(r) diff --git a/backend/internal/httpd/apispec/apispec.go b/backend/internal/httpd/apispec/apispec.go new file mode 100644 index 00000000..8e9fb258 --- /dev/null +++ b/backend/internal/httpd/apispec/apispec.go @@ -0,0 +1,155 @@ +// Package apispec embeds the OpenAPI document, looks up per-operation +// slices, and writes the locked 501 envelope. The 501 body carries the +// operation's slice of the OpenAPI document so consumers discover the +// contract from the endpoint itself — no duplicate planned/contract +// metadata lives in code. +// +// The same document is served verbatim at /api/v1/openapi.yaml so +// tooling that wants the whole spec can fetch it once. +package apispec + +import ( + _ "embed" + "encoding/json" + "fmt" + "net/http" + "strings" + "sync" + + "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" + yaml "gopkg.in/yaml.v3" +) + +//go:embed openapi.yaml +var openapiYAML []byte + +// Spec is the parsed, in-memory view of the embedded OpenAPI document. It +// preserves the YAML shape verbatim so the JSON we emit on 501 responses +// matches the on-disk source. +type Spec struct { + doc map[string]any +} + +var ( + defaultOnce sync.Once + defaultSpec *Spec + defaultErr error +) + +// Default returns the process-wide spec parsed from the embedded YAML. It +// panics on a malformed embed — that is a build-time bug, not a runtime +// one, so failing fast at first use is the right call. +func Default() *Spec { + defaultOnce.Do(func() { + s, err := New(openapiYAML) + defaultSpec = s + defaultErr = err + }) + if defaultErr != nil { + panic(fmt.Sprintf("apispec: embedded openapi.yaml failed to parse: %v", defaultErr)) + } + return defaultSpec +} + +// New parses the supplied YAML bytes. Exposed so tests can construct an +// independent spec without touching the embedded default. +func New(yamlBytes []byte) (*Spec, error) { + var doc map[string]any + if err := yaml.Unmarshal(yamlBytes, &doc); err != nil { + return nil, fmt.Errorf("parse openapi: %w", err) + } + if doc == nil { + return nil, fmt.Errorf("parse openapi: empty document") + } + return &Spec{doc: doc}, nil +} + +// YAML returns the raw embedded document bytes. Used by the /openapi.yaml +// handler. +func (s *Spec) YAML() []byte { return openapiYAML } + +// Operation returns the spec slice for a single (method, path) pair, ready +// to be JSON-serialised. The slice is the OpenAPI Operation object (the +// inner block under e.g. paths./projects.get), with parent path-level +// parameters merged in for completeness. +// +// Returns nil if the path or method is not in the spec; that is treated as +// a developer error (route registered without spec coverage) — callers +// log/fail loudly rather than silently writing a partial 501 body. +func (s *Spec) Operation(method, path string) map[string]any { + paths, _ := s.doc["paths"].(map[string]any) + if paths == nil { + return nil + } + pathItem, _ := paths[path].(map[string]any) + if pathItem == nil { + return nil + } + op, _ := pathItem[strings.ToLower(method)].(map[string]any) + if op == nil { + return nil + } + + // Path-level parameters apply to every method on that path; merge them + // in so the slice is self-contained. + out := make(map[string]any, len(op)+1) + for k, v := range op { + out[k] = v + } + if params, ok := pathItem["parameters"]; ok { + // Prefer the operation's own parameters when both are present; + // otherwise inherit from the path level. + if _, exists := out["parameters"]; !exists { + out["parameters"] = params + } + } + return out +} + +// notImplementedResponse is the wire shape for 501 — APIError envelope +// plus a `spec` field carrying the operation slice. Mirrors the +// NotImplementedResponse schema in openapi.yaml. +type notImplementedResponse struct { + Error string `json:"error"` + Code string `json:"code"` + Message string `json:"message"` + RequestID string `json:"requestId,omitempty"` + Spec map[string]any `json:"spec,omitempty"` +} + +// NotImplemented writes the locked 501 envelope, embedding the OpenAPI +// Operation slice that documents what this route WILL do. Replaces the +// throwaway PlannedRoute literals that the first cut of the route shell +// duplicated in controller code. +func NotImplemented(w http.ResponseWriter, r *http.Request, method, path string) { + op := Default().Operation(method, path) + + body := notImplementedResponse{ + Error: "not_implemented", + Code: "NOT_IMPLEMENTED", + Message: method + " " + path + " is registered but not yet implemented", + RequestID: middleware.GetReqID(r.Context()), + Spec: op, + } + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(http.StatusNotImplemented) + // A write error here means the client went away mid-response. + _ = json.NewEncoder(w).Encode(body) +} + +// ServeYAML serves the embedded openapi.yaml document. Mounted at +// /api/v1/openapi.yaml so spec-consuming tooling (#19's validator, +// SDK generators, the dashboard's developer tools) can fetch the +// whole document in one request. +func ServeYAML(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/yaml; charset=utf-8") + _, _ = w.Write(openapiYAML) +} + +// RegisterServe mounts ServeYAML on the supplied router. Kept as a +// helper so the router code only references one symbol from apispec +// for the static serve path. +func RegisterServe(r chi.Router, path string) { + r.Get(path, ServeYAML) +} diff --git a/backend/internal/httpd/apispec/apispec_test.go b/backend/internal/httpd/apispec/apispec_test.go new file mode 100644 index 00000000..b5bde562 --- /dev/null +++ b/backend/internal/httpd/apispec/apispec_test.go @@ -0,0 +1,70 @@ +package apispec_test + +import ( + "net/http/httptest" + "strings" + "testing" + + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec" +) + +// TestDefaultLoadsEmbeddedSpec is the smoke test for //go:embed wiring: +// the default Spec must parse the embedded YAML without panicking and +// recognise a known operation. +func TestDefaultLoadsEmbeddedSpec(t *testing.T) { + op := apispec.Default().Operation("GET", "/api/v1/projects") + if op == nil { + t.Fatal("Default().Operation(GET, /api/v1/projects) = nil; embed broken or path missing") + } + if got, _ := op["operationId"].(string); got != "listProjects" { + t.Errorf("operationId = %q, want listProjects", got) + } +} + +// TestOperation_MissingPath returns nil for unknown paths — that's how the +// controller-side test catches "route registered without spec coverage". +func TestOperation_MissingPath(t *testing.T) { + if op := apispec.Default().Operation("GET", "/api/v1/no-such-route"); op != nil { + t.Errorf("unknown path returned %v, want nil", op) + } +} + +// TestOperation_MissingMethod returns nil for known path / unknown method. +func TestOperation_MissingMethod(t *testing.T) { + if op := apispec.Default().Operation("HEAD", "/api/v1/projects"); op != nil { + t.Errorf("HEAD on a GET-only path returned %v, want nil", op) + } +} + +// TestOperation_InheritsPathParameters covers the bit of behaviour that +// would silently rot otherwise: parameters declared at the path level +// (e.g. the {id} path param shared by GET/PATCH/DELETE) must show up on +// every operation's slice so the 501 response is self-contained. +func TestOperation_InheritsPathParameters(t *testing.T) { + op := apispec.Default().Operation("GET", "/api/v1/projects/{id}") + if op == nil { + t.Fatal("expected operation slice") + } + params, ok := op["parameters"].([]any) + if !ok || len(params) == 0 { + t.Fatalf("expected inherited path-level parameters, got %#v", op["parameters"]) + } +} + +// TestServeYAML serves the raw embedded document; tooling fetches it +// whole rather than reconstructing it from per-operation slices. +func TestServeYAML(t *testing.T) { + rec := httptest.NewRecorder() + req := httptest.NewRequest("GET", "/api/v1/openapi.yaml", nil) + apispec.ServeYAML(rec, req) + + if rec.Code != 200 { + t.Fatalf("status = %d, want 200", rec.Code) + } + if ct := rec.Header().Get("Content-Type"); !strings.HasPrefix(ct, "application/yaml") { + t.Errorf("Content-Type = %q, want application/yaml*", ct) + } + if !strings.Contains(rec.Body.String(), "openapi: 3.1.0") { + t.Errorf("body did not begin with an OpenAPI 3.1 doc") + } +} diff --git a/backend/internal/httpd/apispec/openapi.yaml b/backend/internal/httpd/apispec/openapi.yaml new file mode 100644 index 00000000..dabb3676 --- /dev/null +++ b/backend/internal/httpd/apispec/openapi.yaml @@ -0,0 +1,447 @@ +openapi: 3.1.0 +info: + title: Agent Orchestrator HTTP daemon + version: 0.1.0-route-shell + description: | + Loopback-only HTTP surface served by the Go daemon. This spec is the + source of truth for every route's contract — the 501 stubs in the + route-shell phase return the matching Operation slice as a `spec` + field, so consumers discover the contract by calling the endpoint + they care about. Real handlers in later PRs satisfy this same spec. + +servers: + - url: http://127.0.0.1:3001 + description: Local daemon (loopback only) + +tags: + - name: projects + description: Project registry, configuration, and lifecycle administration + +paths: + /api/v1/projects: + get: + operationId: listProjects + tags: [projects] + summary: List all registered projects (active + degraded) + responses: + "200": + description: Projects listed + content: + application/json: + schema: + type: object + required: [projects] + properties: + projects: + type: array + items: { $ref: "#/components/schemas/ProjectSummary" } + "500": + description: Failed to load projects + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + example: { error: internal, code: PROJECTS_LIST_FAILED, message: "Failed to load projects" } + "501": { $ref: "#/components/responses/NotImplemented" } + + post: + operationId: addProject + tags: [projects] + summary: Register a new project from a git repository path + requestBody: + required: true + content: + application/json: + schema: { $ref: "#/components/schemas/AddProjectRequest" } + responses: + "201": + description: Project registered + content: + application/json: + schema: + type: object + required: [project] + properties: + project: { $ref: "#/components/schemas/Project" } + "400": + description: Bad request + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + examples: + invalidJson: { value: { error: bad_request, code: INVALID_JSON, message: "Invalid JSON body" } } + pathRequired: { value: { error: bad_request, code: PATH_REQUIRED, message: "Repository path is required" } } + notAGitRepo: { value: { error: bad_request, code: NOT_A_GIT_REPO, message: "Repository path must point to a git repository" } } + "409": + description: Conflict with an already-registered project + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + examples: + pathAlready: + value: + error: conflict + code: PATH_ALREADY_REGISTERED + message: "A project at this path is already registered" + details: + existingProjectId: existing-project-id + suggestedProjectId: suggested-project-id + idAlready: + value: + error: conflict + code: ID_ALREADY_REGISTERED + message: "A project with this id is already registered for a different path" + details: + existingProjectId: existing-project-id + suggestedProjectId: suggested-project-id + "501": { $ref: "#/components/responses/NotImplemented" } + + /api/v1/projects/reload: + post: + operationId: reloadProjects + tags: [projects] + summary: Invalidate cached config and re-scan the global registry + responses: + "200": + description: Reload complete + content: + application/json: + schema: { $ref: "#/components/schemas/ReloadResult" } + "500": + description: Reload failed + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + example: { error: internal, code: RELOAD_FAILED, message: "Failed to reload projects" } + "501": { $ref: "#/components/responses/NotImplemented" } + + /api/v1/projects/{id}: + parameters: + - $ref: "#/components/parameters/ProjectIDPath" + get: + operationId: getProject + tags: [projects] + summary: Fetch one project; discriminates ok vs degraded + responses: + "200": + description: Project resolved (status discriminates ok vs degraded) + content: + application/json: + schema: { $ref: "#/components/schemas/ProjectGetResponse" } + "404": { $ref: "#/components/responses/ProjectNotFound" } + "500": + description: Failed to load project + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + example: { error: internal, code: PROJECT_LOAD_FAILED, message: "Failed to load project" } + "501": { $ref: "#/components/responses/NotImplemented" } + x-rest-audit-notes: | + R5: degraded projects return 200 with a `status` discriminator + instead of 200 with an `error` field (as the legacy TS surface did). + + patch: + operationId: updateProjectConfig + tags: [projects] + summary: Patch behaviour-only fields (identity is frozen) + requestBody: + required: true + content: + application/json: + schema: { $ref: "#/components/schemas/UpdateProjectConfigRequest" } + responses: + "200": + description: Project updated + content: + application/json: + schema: + type: object + required: [project] + properties: + project: { $ref: "#/components/schemas/Project" } + "400": + description: Bad request + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + examples: + invalidJson: { value: { error: bad_request, code: INVALID_JSON, message: "Invalid JSON body" } } + identityFrozen: + value: + error: bad_request + code: IDENTITY_FROZEN + message: "Identity fields cannot be patched" + details: { fields: [projectId, path, repo, defaultBranch] } + invalidConfig: { value: { error: bad_request, code: INVALID_LOCAL_CONFIG, message: "Local project config failed validation" } } + "404": { $ref: "#/components/responses/ProjectNotFound" } + "409": + description: Project not in a patchable state + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + examples: + degraded: { value: { error: conflict, code: PROJECT_DEGRADED, message: "Project config is degraded; repair before patching" } } + missingPath: { value: { error: conflict, code: PROJECT_MISSING_PATH, message: "Project registry entry is missing a path" } } + "501": { $ref: "#/components/responses/NotImplemented" } + x-rest-audit-notes: | + R3: legacy `PUT /projects/{id}` (a TS alias of PATCH) is NOT + registered. PUT returns 405 Method Not Allowed. + R6: returns { project }, not { ok: true }. + + delete: + operationId: removeProject + tags: [projects] + summary: Remove a project; stops sessions, cleans workspaces, unregisters + responses: + "200": + description: Project removed + content: + application/json: + schema: { $ref: "#/components/schemas/RemoveProjectResult" } + "400": + description: Invalid project id + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + example: { error: bad_request, code: INVALID_PROJECT_ID, message: "Project id failed storage-path validation" } + "404": { $ref: "#/components/responses/ProjectNotFound" } + "500": + description: Removal failed + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + example: { error: internal, code: PROJECT_REMOVE_FAILED, message: "Failed to remove project" } + "501": { $ref: "#/components/responses/NotImplemented" } + + /api/v1/projects/{id}/repair: + parameters: + - $ref: "#/components/parameters/ProjectIDPath" + post: + operationId: repairProject + tags: [projects] + summary: Repair a degraded project where automatic recovery is available + x-replaces: + - "POST /api/v1/projects/{id}" + x-rest-audit-notes: | + R4: this canonical path replaces the overloaded + `POST /api/v1/projects/{id}` from the legacy TS surface. + The legacy path is NOT registered; consumers must use /repair. + responses: + "200": + description: Project repaired + content: + application/json: + schema: + type: object + required: [project] + properties: + project: { $ref: "#/components/schemas/Project" } + "400": + description: Bad request + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + examples: + notDegraded: { value: { error: bad_request, code: PROJECT_NOT_DEGRADED, message: "Project does not need repair" } } + notAvailable: { value: { error: bad_request, code: REPAIR_NOT_AVAILABLE, message: "Automatic repair is not available for this degraded config" } } + "404": { $ref: "#/components/responses/ProjectNotFound" } + "501": { $ref: "#/components/responses/NotImplemented" } + +components: + parameters: + ProjectIDPath: + name: id + in: path + required: true + schema: { type: string, minLength: 1 } + description: Project identifier (registry key). + + responses: + NotImplemented: + description: | + Route is registered but the handler has not been implemented yet. + The body carries the locked APIError envelope plus a `spec` field + containing this operation's slice of the OpenAPI document so + callers can discover the contract from the endpoint itself. + content: + application/json: + schema: { $ref: "#/components/schemas/NotImplementedResponse" } + + ProjectNotFound: + description: Project not found + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + example: { error: not_found, code: PROJECT_NOT_FOUND, message: "Unknown project" } + + schemas: + APIError: + type: object + required: [error, code, message] + properties: + error: { type: string, description: "Short kind, e.g. not_found" } + code: { type: string, description: "SCREAMING_SNAKE machine code" } + message: { type: string, description: "Human-readable detail" } + requestId: { type: string } + details: + type: object + additionalProperties: true + + NotImplementedResponse: + allOf: + - $ref: "#/components/schemas/APIError" + - type: object + required: [spec] + properties: + spec: + type: object + description: | + The OpenAPI Operation object for this method+path, served + inline so consumers discover the contract from the 501 + response without fetching the full spec. Mirrors the YAML + shape — see /api/v1/openapi.yaml for the full document. + + ProjectSummary: + type: object + required: [id, name, sessionPrefix] + properties: + id: { type: string } + name: { type: string } + sessionPrefix: { type: string } + resolveError: + type: string + description: Present iff the project is degraded. + + Project: + type: object + required: [id, name, path, repo, defaultBranch] + properties: + id: { type: string } + name: { type: string } + path: { type: string } + repo: + type: string + description: "\"owner/name\" or empty string when unset" + defaultBranch: { type: string, default: main } + agent: { type: string } + runtime: { type: string } + tracker: { $ref: "#/components/schemas/TrackerConfig" } + scm: { $ref: "#/components/schemas/SCMConfig" } + reactions: + type: object + additionalProperties: { $ref: "#/components/schemas/ReactionConfig" } + + DegradedProject: + type: object + required: [id, name, path, resolveError] + properties: + id: { type: string } + name: { type: string } + path: { type: string } + resolveError: { type: string } + + ProjectGetResponse: + type: object + required: [status, project] + properties: + status: + type: string + enum: [ok, degraded] + project: + oneOf: + - $ref: "#/components/schemas/Project" + - $ref: "#/components/schemas/DegradedProject" + discriminator: + propertyName: status + + AddProjectRequest: + type: object + required: [path] + properties: + path: + type: string + description: Repository path; supports ~ home-expansion. Must be a git repo. + projectId: + type: string + description: Optional override; defaults to basename(path). + name: + type: string + description: Optional display name; defaults to projectId. + + UpdateProjectConfigRequest: + type: object + description: | + Behaviour-only patch. Identity fields (projectId, path, repo, + defaultBranch) are rejected with 400 IDENTITY_FROZEN. + properties: + agent: { type: string } + runtime: { type: string } + tracker: { $ref: "#/components/schemas/TrackerConfig" } + scm: { $ref: "#/components/schemas/SCMConfig" } + reactions: + type: object + additionalProperties: { $ref: "#/components/schemas/ReactionConfig" } + + RemoveProjectResult: + type: object + required: [projectId, removedStorageDir] + properties: + projectId: { type: string } + removedStorageDir: { type: boolean } + + ReloadResult: + type: object + required: [reloaded, projectCount, degradedCount] + properties: + reloaded: { type: boolean } + projectCount: { type: integer } + degradedCount: { type: integer } + + # ---- Behaviour config blobs (ported from the TS Zod schemas) ---- + # All carry .passthrough() semantics — extra keys are preserved on + # the wire and re-emitted by the manager impl. + + TrackerConfig: + type: object + additionalProperties: true + properties: + plugin: { type: string } + package: { type: string } + path: { type: string } + + SCMConfig: + type: object + additionalProperties: true + properties: + plugin: { type: string } + package: { type: string } + path: { type: string } + webhook: + type: object + properties: + enabled: { type: boolean } + path: { type: string } + secretEnvVar: { type: string } + signatureHeader: { type: string } + eventHeader: { type: string } + deliveryHeader: { type: string } + maxBodyBytes: { type: integer } + + ReactionConfig: + type: object + properties: + auto: { type: boolean } + action: + type: string + enum: [send-to-agent, notify, auto-merge] + message: { type: string } + priority: + type: string + enum: [urgent, action, warning, info] + retries: { type: integer } + escalateAfter: + oneOf: + - { type: number } + - { type: string } + description: Either ms (number) or duration string ("30m"). + threshold: { type: string } + includeSummary: { type: boolean } diff --git a/backend/internal/httpd/controllers/projects.go b/backend/internal/httpd/controllers/projects.go index 655f3085..3ffe1399 100644 --- a/backend/internal/httpd/controllers/projects.go +++ b/backend/internal/httpd/controllers/projects.go @@ -1,11 +1,16 @@ // Package controllers holds the HTTP-facing controllers for the /api/v1 // surface. Each controller groups one resource's routes, exposes a Register -// method that wires them on a chi.Router, and carries the service dependency -// it will eventually call. +// method that wires them on a chi.Router, and depends on exactly one +// *Manager interface from ports/inbound.go — never on a store, the LCM, an +// adapter, or any other port. Whether the Manager impl reaches past that +// boundary is its own concern. // -// In the route-shell PR (#20) every handler returns 501 with a PlannedRoute -// body documenting the future contract. Handler-impl PRs in the same lane -// flip routes to real logic one at a time without touching the wiring. +// In the route-shell PR (#20) every handler is a one-line apispec.NotImplemented +// call: the contract lives in the OpenAPI document (apispec/openapi.yaml), and +// the 501 body returns that document's slice for the route so consumers can +// discover the contract from the endpoint itself. When real handlers land, +// the stub one-liner is replaced with the impl; no per-route planned +// metadata in code ever has to be deleted. package controllers import ( @@ -13,7 +18,7 @@ import ( "github.com/go-chi/chi/v5" - "github.com/aoagents/agent-orchestrator/backend/internal/httpd/stubs" + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) @@ -31,9 +36,9 @@ type ProjectsController struct { // // Legacy paths that the REST audit dropped are deliberately NOT registered // here. They surface as 405 (sibling method exists, e.g. PUT /projects/{id}) -// or 404 (no sibling, e.g. POST /projects/{id} for repair → moved to -// /projects/{id}/repair). The mapping lives in each canonical handler's -// PlannedRoute.Legacy so consumers can discover the migration. +// or 404 (no sibling). The mapping lives in apispec/openapi.yaml as +// `x-replaces` on the canonical operation so consumers discover the +// migration without leaving the spec. func (c *ProjectsController) Register(r chi.Router) { r.Get("/projects", c.list) r.Post("/projects", c.add) @@ -44,168 +49,30 @@ func (c *ProjectsController) Register(r chi.Router) { r.Post("/projects/{id}/repair", c.repair) } -// list — GET /api/v1/projects func (c *ProjectsController) list(w http.ResponseWriter, r *http.Request) { - stubs.NotImplemented(w, r, stubs.PlannedRoute{ - Route: "GET /api/v1/projects", - Response: map[string]any{ - "200": map[string]any{"projects": "[]domain.ProjectSummary"}, - }, - Errors: []stubs.PlannedError{ - {Status: 500, Code: "PROJECTS_LIST_FAILED", Message: "Failed to load projects"}, - }, - Notes: "Wraps the registry list. Includes degraded entries with a resolveError field.", - }) + apispec.NotImplemented(w, r, "GET", "/api/v1/projects") } -// add — POST /api/v1/projects func (c *ProjectsController) add(w http.ResponseWriter, r *http.Request) { - stubs.NotImplemented(w, r, stubs.PlannedRoute{ - Route: "POST /api/v1/projects", - Request: map[string]any{ - "body": map[string]any{ - "path": "string (required; supports ~ home-expansion; must point to a git repo)", - "projectId": "string (optional; defaults to basename(path))", - "name": "string (optional; defaults to projectId)", - }, - }, - Response: map[string]any{ - "201": map[string]any{"project": "domain.Project"}, - }, - Errors: []stubs.PlannedError{ - {Status: 400, Code: "INVALID_JSON", Message: "Invalid JSON body"}, - {Status: 400, Code: "PATH_REQUIRED", Message: "Repository path is required"}, - {Status: 400, Code: "NOT_A_GIT_REPO", Message: "Repository path must point to a git repository"}, - { - Status: 409, Code: "PATH_ALREADY_REGISTERED", - Message: "A project at this path is already registered", - Details: map[string]any{ - "existingProjectId": "string", - "suggestedProjectId": "string", - }, - }, - { - Status: 409, Code: "ID_ALREADY_REGISTERED", - Message: "A project with this id is already registered for a different path", - Details: map[string]any{ - "existingProjectId": "string", - "suggestedProjectId": "string", - }, - }, - }, - }) + apispec.NotImplemented(w, r, "POST", "/api/v1/projects") } -// get — GET /api/v1/projects/{id} func (c *ProjectsController) get(w http.ResponseWriter, r *http.Request) { - stubs.NotImplemented(w, r, stubs.PlannedRoute{ - Route: "GET /api/v1/projects/{id}", - Request: map[string]any{ - "path": map[string]any{"id": "ProjectID"}, - }, - Response: map[string]any{ - "200": map[string]any{ - "status": `"ok" | "degraded"`, - "project": "domain.Project (status=ok) | domain.DegradedProject (status=degraded)", - }, - }, - Errors: []stubs.PlannedError{ - {Status: 404, Code: "PROJECT_NOT_FOUND", Message: "Unknown project"}, - {Status: 500, Code: "PROJECT_LOAD_FAILED", Message: "Failed to load project"}, - }, - Notes: "REST-audit R5: degraded projects return 200 with a status discriminator, not 200 with an error field.", - }) + apispec.NotImplemented(w, r, "GET", "/api/v1/projects/{id}") } -// updateConfig — PATCH /api/v1/projects/{id} func (c *ProjectsController) updateConfig(w http.ResponseWriter, r *http.Request) { - stubs.NotImplemented(w, r, stubs.PlannedRoute{ - Route: "PATCH /api/v1/projects/{id}", - Request: map[string]any{ - "path": map[string]any{"id": "ProjectID"}, - "body": map[string]any{ - "agent": "string (optional)", - "runtime": "string (optional)", - "tracker": "TrackerConfig (optional)", - "scm": "SCMConfig (optional)", - "reactions": "map[string]ReactionConfig (optional)", - }, - }, - Response: map[string]any{ - "200": map[string]any{"project": "domain.Project"}, - }, - Errors: []stubs.PlannedError{ - {Status: 400, Code: "INVALID_JSON", Message: "Invalid JSON body"}, - { - Status: 400, Code: "IDENTITY_FROZEN", - Message: "Identity fields cannot be patched", - Details: map[string]any{"fields": "[]string"}, - }, - {Status: 400, Code: "INVALID_LOCAL_CONFIG", Message: "Local project config failed validation"}, - {Status: 404, Code: "PROJECT_NOT_FOUND", Message: "Unknown project"}, - {Status: 409, Code: "PROJECT_DEGRADED", Message: "Project config is degraded; repair before patching"}, - {Status: 409, Code: "PROJECT_MISSING_PATH", Message: "Project registry entry is missing a path"}, - }, - Notes: "REST-audit R3: legacy PUT alias is NOT registered; PUT /projects/{id} returns 405. R6: returns {project}, not {ok:true}.", - }) + apispec.NotImplemented(w, r, "PATCH", "/api/v1/projects/{id}") } -// remove — DELETE /api/v1/projects/{id} func (c *ProjectsController) remove(w http.ResponseWriter, r *http.Request) { - stubs.NotImplemented(w, r, stubs.PlannedRoute{ - Route: "DELETE /api/v1/projects/{id}", - Request: map[string]any{ - "path": map[string]any{"id": "ProjectID"}, - }, - Response: map[string]any{ - "200": map[string]any{ - "projectId": "ProjectID", - "removedStorageDir": "bool", - }, - }, - Errors: []stubs.PlannedError{ - {Status: 400, Code: "INVALID_PROJECT_ID", Message: "Project id failed storage-path validation"}, - {Status: 404, Code: "PROJECT_NOT_FOUND", Message: "Unknown project"}, - {Status: 500, Code: "PROJECT_REMOVE_FAILED", Message: "Failed to remove project"}, - }, - Notes: "Side effects (in handler-impl PR): stop project sessions, cleanup managed workspaces, unregister, remove storage dir.", - }) + apispec.NotImplemented(w, r, "DELETE", "/api/v1/projects/{id}") } -// repair — POST /api/v1/projects/{id}/repair func (c *ProjectsController) repair(w http.ResponseWriter, r *http.Request) { - stubs.NotImplemented(w, r, stubs.PlannedRoute{ - Route: "POST /api/v1/projects/{id}/repair", - Legacy: []string{"POST /api/v1/projects/{id}"}, - Request: map[string]any{ - "path": map[string]any{"id": "ProjectID"}, - }, - Response: map[string]any{ - "200": map[string]any{"project": "domain.Project"}, - }, - Errors: []stubs.PlannedError{ - {Status: 400, Code: "PROJECT_NOT_DEGRADED", Message: "Project does not need repair"}, - {Status: 400, Code: "REPAIR_NOT_AVAILABLE", Message: "Automatic repair is not available for this degraded config"}, - {Status: 404, Code: "PROJECT_NOT_FOUND", Message: "Unknown project"}, - }, - Notes: "REST-audit R4: replaces the overloaded POST /projects/{id}. Legacy path is NOT registered; consumers must call /repair.", - }) + apispec.NotImplemented(w, r, "POST", "/api/v1/projects/{id}/repair") } -// reload — POST /api/v1/projects/reload func (c *ProjectsController) reload(w http.ResponseWriter, r *http.Request) { - stubs.NotImplemented(w, r, stubs.PlannedRoute{ - Route: "POST /api/v1/projects/reload", - Response: map[string]any{ - "200": map[string]any{ - "reloaded": "bool", - "projectCount": "int", - "degradedCount": "int", - }, - }, - Errors: []stubs.PlannedError{ - {Status: 500, Code: "RELOAD_FAILED", Message: "Failed to reload projects"}, - }, - Notes: "Invalidates the cached services registry and re-loads the global config.", - }) + apispec.NotImplemented(w, r, "POST", "/api/v1/projects/reload") } diff --git a/backend/internal/httpd/controllers/projects_test.go b/backend/internal/httpd/controllers/projects_test.go index d27b82ab..8e011de3 100644 --- a/backend/internal/httpd/controllers/projects_test.go +++ b/backend/internal/httpd/controllers/projects_test.go @@ -2,8 +2,11 @@ package controllers_test // Route-shell tests for /api/v1/projects. Builds the full router (so the // /api/v1 mount, middleware, NotFound, and MethodNotAllowed handlers are -// exercised together) and asserts every canonical route returns 501 with the -// locked envelope; legacy paths that the REST audit dropped return 404 or 405. +// exercised together) and asserts every canonical route returns 501 with +// the locked envelope + a `spec` slice sourced from apispec/openapi.yaml. +// Legacy paths that the REST audit dropped return 405 (sibling method +// exists) or 404 (no sibling); the legacy → canonical mapping itself is +// documented in the YAML via x-replaces. import ( "encoding/json" @@ -29,29 +32,28 @@ func newTestServer(t *testing.T) *httptest.Server { } // TestProjectsRoutes_Canonical501 walks every canonical /projects route and -// asserts it returns 501 with the locked envelope. The "wantLegacy" cases -// double-check that the planned body advertises the TS path it replaced. +// asserts it returns 501 with the locked envelope. wantOpID double-checks +// that the embedded spec slice is the right operation (not e.g. the +// /projects/{id} block leaking into /projects/reload because of route +// shadowing). func TestProjectsRoutes_Canonical501(t *testing.T) { srv := newTestServer(t) cases := []struct { - method string - path string - body string - wantRoute string - wantLegacy []string + method, path, body, wantOpID string + wantReplaces []string }{ - {method: "GET", path: "/api/v1/projects", wantRoute: "GET /api/v1/projects"}, - {method: "POST", path: "/api/v1/projects", body: `{}`, wantRoute: "POST /api/v1/projects"}, - {method: "GET", path: "/api/v1/projects/p1", wantRoute: "GET /api/v1/projects/{id}"}, - {method: "PATCH", path: "/api/v1/projects/p1", body: `{}`, wantRoute: "PATCH /api/v1/projects/{id}"}, - {method: "DELETE", path: "/api/v1/projects/p1", wantRoute: "DELETE /api/v1/projects/{id}"}, + {method: "GET", path: "/api/v1/projects", wantOpID: "listProjects"}, + {method: "POST", path: "/api/v1/projects", body: `{}`, wantOpID: "addProject"}, + {method: "GET", path: "/api/v1/projects/p1", wantOpID: "getProject"}, + {method: "PATCH", path: "/api/v1/projects/p1", body: `{}`, wantOpID: "updateProjectConfig"}, + {method: "DELETE", path: "/api/v1/projects/p1", wantOpID: "removeProject"}, { method: "POST", path: "/api/v1/projects/p1/repair", - wantRoute: "POST /api/v1/projects/{id}/repair", - wantLegacy: []string{"POST /api/v1/projects/{id}"}, + wantOpID: "repairProject", + wantReplaces: []string{"POST /api/v1/projects/{id}"}, }, - {method: "POST", path: "/api/v1/projects/reload", wantRoute: "POST /api/v1/projects/reload"}, + {method: "POST", path: "/api/v1/projects/reload", wantOpID: "reloadProjects"}, } for _, tc := range cases { @@ -59,7 +61,7 @@ func TestProjectsRoutes_Canonical501(t *testing.T) { body, status, headers := doRequest(t, srv, tc.method, tc.path, tc.body) if status != http.StatusNotImplemented { - t.Fatalf("status = %d, want 501", status) + t.Fatalf("status = %d, want 501\nbody=%s", status, body) } if ct := headers.Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { t.Errorf("Content-Type = %q, want JSON", ct) @@ -75,11 +77,15 @@ func TestProjectsRoutes_Canonical501(t *testing.T) { if got.Message == "" { t.Error("envelope.message empty") } - if got.Planned.Route != tc.wantRoute { - t.Errorf("planned.route = %q, want %q", got.Planned.Route, tc.wantRoute) + if got.Spec == nil { + t.Fatal("envelope.spec missing — apispec failed to find the operation") } - if !equalStrings(got.Planned.Legacy, tc.wantLegacy) { - t.Errorf("planned.legacy = %v, want %v", got.Planned.Legacy, tc.wantLegacy) + if op, _ := got.Spec["operationId"].(string); op != tc.wantOpID { + t.Errorf("spec.operationId = %q, want %q", op, tc.wantOpID) + } + gotReplaces := stringSlice(got.Spec["x-replaces"]) + if !equalStrings(gotReplaces, tc.wantReplaces) { + t.Errorf("spec.x-replaces = %v, want %v", gotReplaces, tc.wantReplaces) } }) } @@ -87,24 +93,17 @@ func TestProjectsRoutes_Canonical501(t *testing.T) { // TestProjectsRoutes_LegacyUnregistered confirms the REST-audit-dropped TS // paths are deliberately unregistered. PUT and POST on /projects/{id} hit -// sibling-method paths so chi returns 405; legacy nested paths return 404. +// sibling-method paths so chi returns 405. The legacy → canonical mapping +// is documented on the canonical operation via x-replaces (covered above). func TestProjectsRoutes_LegacyUnregistered(t *testing.T) { srv := newTestServer(t) cases := []struct { - method string - path string - wantStatus int - wantCode string - why string + method, path, wantCode, why string + wantStatus int }{ - // R3: PUT was a PATCH alias in TS; we keep PATCH only. Chi returns - // 405 because sibling verbs (GET/PATCH/DELETE) exist on the same path. {method: "PUT", path: "/api/v1/projects/p1", wantStatus: 405, wantCode: "METHOD_NOT_ALLOWED", why: "R3 PUT not registered"}, - // R4: POST on /projects/{id} used to repair; canonical is /repair. - // Same path has no sibling POST handler (POST collection is at /projects), - // chi returns 405. - {method: "POST", path: "/api/v1/projects/p1", wantStatus: 405, wantCode: "METHOD_NOT_ALLOWED", why: "R4 repair moved"}, + {method: "POST", path: "/api/v1/projects/p1", wantStatus: 405, wantCode: "METHOD_NOT_ALLOWED", why: "R4 repair moved to /repair"}, } for _, tc := range cases { @@ -145,10 +144,10 @@ func TestProjectsRoutes_MissingRoute(t *testing.T) { } } -// TestProjectsRoutes_ReloadBeforeID is the chi-ordering safety check. If the -// {id} wildcard were registered first, POST /projects/reload would match -// {id}="reload" → repair handler instead of the reload handler. We assert -// reload responds with its own planned.route. +// TestProjectsRoutes_ReloadBeforeID is the chi-ordering safety check. If +// the {id} wildcard were registered first, POST /projects/reload would +// match {id}="reload" → repair handler instead of the reload handler. We +// assert reload's spec slice (operationId=reloadProjects), not repair's. func TestProjectsRoutes_ReloadBeforeID(t *testing.T) { srv := newTestServer(t) body, status, _ := doRequest(t, srv, "POST", "/api/v1/projects/reload", "") @@ -157,39 +156,44 @@ func TestProjectsRoutes_ReloadBeforeID(t *testing.T) { } var e envelope _ = json.Unmarshal(body, &e) - if e.Planned.Route != "POST /api/v1/projects/reload" { - t.Errorf("reload was shadowed by {id}: planned.route = %q", e.Planned.Route) + if op, _ := e.Spec["operationId"].(string); op != "reloadProjects" { + t.Errorf("reload was shadowed by {id}: spec.operationId = %q", op) } } -// envelope mirrors the locked APIError + stubs.PlannedRoute on the wire. We -// declare it in the test rather than importing httpd's private type so the -// test pins the JSON contract independently of internal renames. -type envelope struct { - Error string `json:"error"` - Code string `json:"code"` - Message string `json:"message"` - RequestID string `json:"requestId"` - Planned planned `json:"planned"` +// TestOpenAPIYAMLServed confirms the embedded spec is reachable at the +// documented path so external tooling can fetch it. +func TestOpenAPIYAMLServed(t *testing.T) { + srv := newTestServer(t) + body, status, headers := doRequest(t, srv, "GET", "/api/v1/openapi.yaml", "") + if status != http.StatusOK { + t.Fatalf("status = %d, want 200", status) + } + if ct := headers.Get("Content-Type"); !strings.HasPrefix(ct, "application/yaml") { + t.Errorf("Content-Type = %q, want application/yaml*", ct) + } + if !strings.Contains(string(body), "openapi: 3.1.0") { + t.Errorf("served body did not start with an OpenAPI 3.1 doc — first bytes:\n%s", firstLine(body)) + } } -type planned struct { - Route string `json:"route"` - Legacy []string `json:"legacy"` +// envelope mirrors the locked APIError + apispec spec field on the wire. +// We declare it in the test rather than importing apispec's private type +// so the test pins the JSON contract independently of internal renames. +type envelope struct { + Error string `json:"error"` + Code string `json:"code"` + Message string `json:"message"` + RequestID string `json:"requestId"` + Spec map[string]any `json:"spec"` } func doRequest(t *testing.T, srv *httptest.Server, method, path, body string) ([]byte, int, http.Header) { t.Helper() - var reqBody *strings.Reader - if body != "" { - reqBody = strings.NewReader(body) - } - // strings.Reader is not nilable for the no-body case via the io.Reader - // interface, so branch the constructor explicitly. var req *http.Request var err error - if reqBody != nil { - req, err = http.NewRequest(method, srv.URL+path, reqBody) + if body != "" { + req, err = http.NewRequest(method, srv.URL+path, strings.NewReader(body)) } else { req, err = http.NewRequest(method, srv.URL+path, nil) } @@ -219,6 +223,22 @@ func doRequest(t *testing.T, srv *httptest.Server, method, path, body string) ([ return buf, resp.StatusCode, resp.Header } +// stringSlice coerces an `any` that holds a YAML-decoded sequence into a +// []string. yaml.v3 decodes sequences as []any; we only call this on slices +// whose elements we know are strings. +func stringSlice(v any) []string { + raw, ok := v.([]any) + if !ok { + return nil + } + out := make([]string, 0, len(raw)) + for _, item := range raw { + s, _ := item.(string) + out = append(out, s) + } + return out +} + func equalStrings(a, b []string) bool { if len(a) != len(b) { return false @@ -230,3 +250,10 @@ func equalStrings(a, b []string) bool { } return true } + +func firstLine(b []byte) string { + if i := strings.IndexByte(string(b), '\n'); i >= 0 { + return string(b[:i]) + } + return string(b) +} diff --git a/backend/internal/httpd/stubs/notimpl.go b/backend/internal/httpd/stubs/notimpl.go deleted file mode 100644 index ea10b9e1..00000000 --- a/backend/internal/httpd/stubs/notimpl.go +++ /dev/null @@ -1,66 +0,0 @@ -// Package stubs holds the 501-with-contract-preview helper every route-shell -// handler uses. It lives in its own package (rather than alongside the router -// in httpd) so the per-resource controllers can import it without creating an -// import cycle back into httpd. -package stubs - -import ( - "encoding/json" - "net/http" - - "github.com/go-chi/chi/v5/middleware" -) - -// PlannedRoute is the documentation payload every 501 stub returns alongside -// the locked error envelope. It tells API consumers what the endpoint WILL -// look like once the real handler lands, so the dashboard team can scaffold -// against the contract before implementation merges. -// -// Legacy carries the old TS paths this route REPLACES — set on canonical -// routes that subsumed a now-unregistered TS endpoint. #19 (OpenAPI) will -// translate Legacy into an x-replaces extension on the spec. -type PlannedRoute struct { - Route string `json:"route"` - Legacy []string `json:"legacy,omitempty"` - Request map[string]any `json:"request,omitempty"` - Response map[string]any `json:"response,omitempty"` - Errors []PlannedError `json:"errors,omitempty"` - Notes string `json:"notes,omitempty"` -} - -// PlannedError describes one error variant the future implementation will -// return. Status + Code together identify the variant uniquely. -type PlannedError struct { - Status int `json:"status"` - Code string `json:"code"` - Message string `json:"message"` - Details map[string]any `json:"details,omitempty"` -} - -// notImplementedResponse is the full 501 body: the locked APIError envelope -// fields plus a Planned block that documents the future contract. -type notImplementedResponse struct { - Error string `json:"error"` - Code string `json:"code"` - Message string `json:"message"` - RequestID string `json:"requestId,omitempty"` - Planned PlannedRoute `json:"planned"` -} - -// NotImplemented writes a 501 with the locked envelope plus the planned-route -// documentation. Used by every route in the shell; replaced one-by-one with -// real handlers as the lane progresses. -func NotImplemented(w http.ResponseWriter, r *http.Request, planned PlannedRoute) { - body := notImplementedResponse{ - Error: "not_implemented", - Code: "NOT_IMPLEMENTED", - Message: planned.Route + " is registered but not yet implemented", - RequestID: middleware.GetReqID(r.Context()), - Planned: planned, - } - w.Header().Set("Content-Type", "application/json; charset=utf-8") - w.WriteHeader(http.StatusNotImplemented) - // A write error here means the client went away mid-response; there is - // nothing useful to do but stop. - _ = json.NewEncoder(w).Encode(body) -} From 01a26a94dbedea1bbb583ae691be14224e5884fd Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Fri, 29 May 2026 14:40:15 +0530 Subject: [PATCH 11/23] =?UTF-8?q?refactor(api):=20move=20projects=20contra?= =?UTF-8?q?ct=20to=20internal/project=20package=20=E2=80=94=20#20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pilots the feature-package layout the backend is migrating toward: a resource's inbound interface and its DTOs live with the resource, not in a central ports/ catch-all. WHY: review flagged ports/ as vague. It conflates three jobs — the outbound capability seam (legit), single-impl inbound interfaces (Go idiom wants these consumer-side), and DTOs that aren't ports at all. This moves the projects contract out as the reference shape #21/#22 follow; the merged session/lifecycle/outbound contracts are left untouched and migrated separately. Scope: INTERFACE ONLY. No implementation — handlers still answer via apispec.NotImplemented and the injected project.Manager stays nil. The impl lands in a later handler-impl PR. Changes: - new internal/project: project.go (Manager interface, 7 endpoints) + dto.go (AddInput/GetResult/UpdateConfigInput/RemoveResult/ReloadResult, moved verbatim from ports/projects.go, Project-prefix dropped). - ports/projects.go deleted; ProjectManager removed from ports/inbound.go. outbound.go and facts.go untouched. - controllers/projects.go and httpd/api.go depend on project.Manager. Domain entities (Project, ProjectSummary, DegradedProject, config types) stay in domain/ as shared vocabulary. go build/vet/test/gofmt all clean; no behavioural change. --- backend/internal/httpd/api.go | 19 +++--- .../internal/httpd/controllers/projects.go | 10 ++-- backend/internal/ports/inbound.go | 15 ----- backend/internal/ports/projects.go | 58 ------------------- backend/internal/project/dto.go | 55 ++++++++++++++++++ backend/internal/project/project.go | 44 ++++++++++++++ 6 files changed, 114 insertions(+), 87 deletions(-) delete mode 100644 backend/internal/ports/projects.go create mode 100644 backend/internal/project/dto.go create mode 100644 backend/internal/project/project.go diff --git a/backend/internal/httpd/api.go b/backend/internal/httpd/api.go index bb47059b..124a8d78 100644 --- a/backend/internal/httpd/api.go +++ b/backend/internal/httpd/api.go @@ -9,20 +9,21 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/config" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/controllers" - "github.com/aoagents/agent-orchestrator/backend/internal/ports" + "github.com/aoagents/agent-orchestrator/backend/internal/project" ) // APIDeps bundles every Manager the API layer's controllers depend on. There -// is exactly one *Manager per resource and the controllers see ONLY that -// interface — they don't reach past it to inbound/outbound ports, the LCM, -// or adapters. Whether a Manager impl talks to the registry, the LCM, or -// an outbound port is its own concern. +// is exactly one Manager per resource, defined in that resource's own package +// (project.Manager, later session.Manager, ...), and the controllers see ONLY +// that interface — they don't reach past it to the LCM, adapters, or stores. +// Whether a Manager impl talks to the registry, the LCM, or an outbound port +// is its own concern. // -// The route-shell PR (#20) leaves every field nil — handlers don't -// dereference them yet. Impl PRs in the same lane wire real Managers and -// flip stubs to real logic one route at a time. +// The route-shell PR (#20) leaves every field nil — handlers answer via +// apispec.NotImplemented and don't dereference them yet. The handler-impl PR +// wires real Managers and flips stubs to real logic one route at a time. type APIDeps struct { - Projects ports.ProjectManager + Projects project.Manager } // API owns one controller per resource and is the single Register call the diff --git a/backend/internal/httpd/controllers/projects.go b/backend/internal/httpd/controllers/projects.go index 3ffe1399..f8665041 100644 --- a/backend/internal/httpd/controllers/projects.go +++ b/backend/internal/httpd/controllers/projects.go @@ -19,15 +19,15 @@ import ( "github.com/go-chi/chi/v5" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec" - "github.com/aoagents/agent-orchestrator/backend/internal/ports" + "github.com/aoagents/agent-orchestrator/backend/internal/project" ) // ProjectsController owns the 7 canonical /projects routes. The controller -// depends ONLY on ports.ProjectManager — it doesn't know whether the impl -// reaches into the registry, the LCM, an adapter, or all three. Mgr is nil -// while handlers are stubs; impl PRs supply a real ProjectManager. +// depends ONLY on project.Manager — it doesn't know whether the impl reaches +// into the registry, the LCM, an adapter, or all three. Mgr is nil while +// handlers are stubs; the handler-impl PR supplies a real project.Manager. type ProjectsController struct { - Mgr ports.ProjectManager + Mgr project.Manager } // Register mounts the project routes on the supplied router. Route order diff --git a/backend/internal/ports/inbound.go b/backend/internal/ports/inbound.go index d2a77cdd..30ab7559 100644 --- a/backend/internal/ports/inbound.go +++ b/backend/internal/ports/inbound.go @@ -68,18 +68,3 @@ type CleanupResult struct { Cleaned []domain.SessionID Skipped []domain.SessionID // e.g. paths that still held uncommitted work } - -// ProjectManager is the inbound contract the API layer calls for every project -// operation. The HTTP controllers see ONLY this interface — they never reach -// past it to the registry, LCM, workspace adapter, or SCM. Whether a method -// touches the global config registry, the LCM (to stop sessions on remove), -// or an adapter (to destroy worktrees) is a private concern of the impl. -type ProjectManager interface { - List(ctx context.Context) ([]domain.ProjectSummary, error) - Get(ctx context.Context, id domain.ProjectID) (GetProjectResult, error) - Add(ctx context.Context, in AddProjectInput) (domain.Project, error) - UpdateConfig(ctx context.Context, id domain.ProjectID, patch UpdateProjectConfigInput) (domain.Project, error) - Remove(ctx context.Context, id domain.ProjectID) (RemoveProjectResult, error) - Repair(ctx context.Context, id domain.ProjectID) (domain.Project, error) - Reload(ctx context.Context) (ReloadResult, error) -} diff --git a/backend/internal/ports/projects.go b/backend/internal/ports/projects.go deleted file mode 100644 index 90fb29dc..00000000 --- a/backend/internal/ports/projects.go +++ /dev/null @@ -1,58 +0,0 @@ -package ports - -import "github.com/aoagents/agent-orchestrator/backend/internal/domain" - -// DTOs for ProjectManager (declared in inbound.go alongside its peers -// SessionManager and LifecycleManager). The interface is the boundary -// controllers care about; these types carry the request/response shapes -// across that boundary. Whether the manager impl reaches into the registry, -// the LCM, or an adapter is a private concern — controllers see only the -// types here and the methods on ProjectManager. - -// GetProjectResult is the discriminated union returned by ProjectManager.Get. -// Exactly one of Project / Degraded is non-nil. Status mirrors the -// discriminator on the wire so consumers branch on it without nil-checking -// both fields. -type GetProjectResult struct { - Status string // "ok" | "degraded" - Project *domain.Project // populated when Status == "ok" - Degraded *domain.DegradedProject // populated when Status == "degraded" -} - -// AddProjectInput is the body shape for POST /api/v1/projects. Path is -// required; ProjectID and Name default to basename(path) at the manager. -// Pointer fields preserve the "field absent" vs "field present empty" -// distinction so the manager can decide what to default and what to reject. -type AddProjectInput struct { - Path string `json:"path"` - ProjectID *string `json:"projectId,omitempty"` - Name *string `json:"name,omitempty"` -} - -// UpdateProjectConfigInput is the body shape for PATCH /api/v1/projects/{id}. -// Only behaviour fields are mutable; identity fields (projectId, path, repo, -// defaultBranch) are rejected by the handler with a 400 IDENTITY_FROZEN. -type UpdateProjectConfigInput struct { - Agent *string `json:"agent,omitempty"` - Runtime *string `json:"runtime,omitempty"` - Tracker *domain.TrackerConfig `json:"tracker,omitempty"` - SCM *domain.SCMConfig `json:"scm,omitempty"` - Reactions *map[string]*domain.ReactionConfig `json:"reactions,omitempty"` -} - -// RemoveProjectResult reports what DELETE /api/v1/projects/{id} actually did. -// RemovedStorageDir is false when the project was registry-only (no on-disk -// session/workspace directory existed). -type RemoveProjectResult struct { - ProjectID domain.ProjectID `json:"projectId"` - RemovedStorageDir bool `json:"removedStorageDir"` -} - -// ReloadResult is the response body of POST /api/v1/projects/reload — the -// manager invalidates its cached config and re-scans the registry; the -// counts help the dashboard show "loaded N projects, M degraded" feedback. -type ReloadResult struct { - Reloaded bool `json:"reloaded"` - ProjectCount int `json:"projectCount"` - DegradedCount int `json:"degradedCount"` -} diff --git a/backend/internal/project/dto.go b/backend/internal/project/dto.go new file mode 100644 index 00000000..e5bb933e --- /dev/null +++ b/backend/internal/project/dto.go @@ -0,0 +1,55 @@ +package project + +import "github.com/aoagents/agent-orchestrator/backend/internal/domain" + +// Request/response shapes for Manager. They carry the data across the service +// boundary; the canonical entities (Project, ProjectSummary, DegradedProject) +// stay in domain/. Named without a "Project" prefix because the package name +// already supplies it (project.AddInput, project.GetResult). + +// GetResult is the discriminated union returned by Manager.Get. Exactly one of +// Project / Degraded is non-nil; Status mirrors the discriminator on the wire +// so consumers branch on it without nil-checking both fields. +type GetResult struct { + Status string // "ok" | "degraded" + Project *domain.Project // populated when Status == "ok" + Degraded *domain.DegradedProject // populated when Status == "degraded" +} + +// AddInput is the body shape for POST /api/v1/projects. Path is required; +// ProjectID and Name default to basename(path) at the manager. Pointer fields +// preserve the "field absent" vs "field present empty" distinction so the +// manager can decide what to default and what to reject. +type AddInput struct { + Path string `json:"path"` + ProjectID *string `json:"projectId,omitempty"` + Name *string `json:"name,omitempty"` +} + +// UpdateConfigInput is the body shape for PATCH /api/v1/projects/{id}. Only +// behaviour fields are mutable; identity fields (projectId, path, repo, +// defaultBranch) are rejected by the handler with a 400 IDENTITY_FROZEN. +type UpdateConfigInput struct { + Agent *string `json:"agent,omitempty"` + Runtime *string `json:"runtime,omitempty"` + Tracker *domain.TrackerConfig `json:"tracker,omitempty"` + SCM *domain.SCMConfig `json:"scm,omitempty"` + Reactions *map[string]*domain.ReactionConfig `json:"reactions,omitempty"` +} + +// RemoveResult reports what DELETE /api/v1/projects/{id} actually did. +// RemovedStorageDir is false when the project was registry-only (no on-disk +// session/workspace directory existed). +type RemoveResult struct { + ProjectID domain.ProjectID `json:"projectId"` + RemovedStorageDir bool `json:"removedStorageDir"` +} + +// ReloadResult is the response body of POST /api/v1/projects/reload — the +// manager invalidates its cached config and re-scans the registry; the counts +// help the dashboard show "loaded N projects, M degraded" feedback. +type ReloadResult struct { + Reloaded bool `json:"reloaded"` + ProjectCount int `json:"projectCount"` + DegradedCount int `json:"degradedCount"` +} diff --git a/backend/internal/project/project.go b/backend/internal/project/project.go new file mode 100644 index 00000000..f5c79303 --- /dev/null +++ b/backend/internal/project/project.go @@ -0,0 +1,44 @@ +// Package project owns the projects service contract: the Manager interface +// the HTTP layer calls and the request/response DTOs that cross it (dto.go). +// +// This is the pilot for the feature-package layout the backend is migrating +// toward: a resource's interface and DTOs live with the resource, not in a +// central catch-all. Controllers depend on project.Manager and nothing +// beneath it — whether the implementation reaches into the config registry, +// the lifecycle manager (to stop sessions on remove), or a workspace adapter +// (to destroy worktrees) is a private concern of the impl, which lands in a +// later handler-impl PR. This PR defines only the contract. +package project + +import ( + "context" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// Manager is the inbound contract for the /api/v1/projects surface. One +// implementation (this package, later); the HTTP controller is the consumer. +type Manager interface { + // List returns every registered project, including degraded entries + // (those whose config failed to load but whose registry entry survives). + List(ctx context.Context) ([]domain.ProjectSummary, error) + + // Get returns one project, discriminating ok vs degraded via GetResult. + Get(ctx context.Context, id domain.ProjectID) (GetResult, error) + + // Add registers a new project from a git repository path. + Add(ctx context.Context, in AddInput) (domain.Project, error) + + // UpdateConfig patches behaviour-only fields; identity fields are frozen. + UpdateConfig(ctx context.Context, id domain.ProjectID, patch UpdateConfigInput) (domain.Project, error) + + // Remove unregisters a project, stopping its sessions and reclaiming + // managed workspaces. + Remove(ctx context.Context, id domain.ProjectID) (RemoveResult, error) + + // Repair recovers a degraded project where automatic repair is available. + Repair(ctx context.Context, id domain.ProjectID) (domain.Project, error) + + // Reload invalidates cached config and re-scans the global registry. + Reload(ctx context.Context) (ReloadResult, error) +} From 9a0547e4870caa0725b6d080ca05e26a2d57a4a6 Mon Sep 17 00:00:00 2001 From: Aditi Chauhan Date: Fri, 29 May 2026 15:01:31 +0530 Subject: [PATCH 12/23] =?UTF-8?q?refactor(api):=20consolidate=20project=20?= =?UTF-8?q?types=20into=20internal/project=20=E2=80=94=20#20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR review: (1) "why are config_types required at the moment?" and (2) "project objects already defined in project/ — how do we differentiate?" Both had the same root cause: project types were split across domain/ and project/. Fix — keep ALL project types in the project package; only domain.ProjectID (shared with sessions/lifecycle/workspace) stays in domain. - domain/project.go → project/types.go: Project, Summary, Degraded (renamed from ProjectSummary/DegradedProject; the package name carries the "Project" prefix now). - domain/config_types.go deleted. Kept only the 4 shapes the projects API actually exposes — TrackerConfig, SCMConfig, SCMWebhookConfig, ReactionConfig — moved into project/types.go. Dropped AgentConfig, AgentPermission, RoleAgentConfig, LocalProjectConfig (zero references) and the speculative `Extra map[string]any` passthrough fields (no marshaller existed, so they silently dropped data — premature). - project/dto.go + project/project.go reference the local types; ids stay domain.ProjectID. Net: one home for project types, no dead code. go build/vet/test/gofmt clean; no behavioural change (handlers still 501 via apispec). --- backend/internal/domain/config_types.go | 105 ------------------------ backend/internal/domain/project.go | 47 ----------- backend/internal/project/dto.go | 22 ++--- backend/internal/project/project.go | 8 +- backend/internal/project/types.go | 96 ++++++++++++++++++++++ 5 files changed, 111 insertions(+), 167 deletions(-) delete mode 100644 backend/internal/domain/config_types.go delete mode 100644 backend/internal/domain/project.go create mode 100644 backend/internal/project/types.go diff --git a/backend/internal/domain/config_types.go b/backend/internal/domain/config_types.go deleted file mode 100644 index 6f06e340..00000000 --- a/backend/internal/domain/config_types.go +++ /dev/null @@ -1,105 +0,0 @@ -package domain - -// Typed configuration shapes ported from the TS Zod schemas in the old -// orchestrator (packages/core/src/config.ts + global-config.ts). They live in -// the domain package because both the project read-model (Project) and the -// future project-config service surface them on the wire. -// -// Zod .passthrough() preserves unknown keys; the Go equivalent will be a -// custom UnmarshalJSON that fills Extra. The route-shell PR (#20) only declares -// the shapes for documentation in planned bodies — the passthrough Marshal/ -// Unmarshal lands when a real handler first reads or writes config. - -// AgentPermission constrains the agent-permissions enum. Empty string means -// "not set" — distinct from the TS default of "permissionless" so the API can -// tell user-set values apart from defaults. -type AgentPermission string - -const ( - AgentPermissionPermissionless AgentPermission = "permissionless" - AgentPermissionDefault AgentPermission = "default" - AgentPermissionAutoEdit AgentPermission = "auto-edit" - AgentPermissionSuggest AgentPermission = "suggest" - AgentPermissionSkip AgentPermission = "skip" -) - -// TrackerConfig mirrors TrackerConfigSchema. .passthrough() preserves arbitrary -// plugin-specific keys; Extra is reserved for that round-trip. -type TrackerConfig struct { - Plugin string `json:"plugin,omitempty"` - Package string `json:"package,omitempty"` - Path string `json:"path,omitempty"` - Extra map[string]any `json:"-"` -} - -// SCMConfig mirrors SCMConfigSchema. Webhook nests its own optional block. -type SCMConfig struct { - Plugin string `json:"plugin,omitempty"` - Package string `json:"package,omitempty"` - Path string `json:"path,omitempty"` - Webhook *SCMWebhookConfig `json:"webhook,omitempty"` - Extra map[string]any `json:"-"` -} - -// SCMWebhookConfig — pointer Enabled distinguishes unset from explicit false. -type SCMWebhookConfig struct { - Enabled *bool `json:"enabled,omitempty"` - Path string `json:"path,omitempty"` - SecretEnvVar string `json:"secretEnvVar,omitempty"` - SignatureHeader string `json:"signatureHeader,omitempty"` - EventHeader string `json:"eventHeader,omitempty"` - DeliveryHeader string `json:"deliveryHeader,omitempty"` - MaxBodyBytes int `json:"maxBodyBytes,omitempty"` -} - -// AgentConfig mirrors AgentSpecificConfigSchema. .passthrough() preserves -// agent-plugin-specific keys. -type AgentConfig struct { - Permissions AgentPermission `json:"permissions,omitempty"` - Model string `json:"model,omitempty"` - OrchestratorModel string `json:"orchestratorModel,omitempty"` - OpenCodeSessionID string `json:"opencodeSessionId,omitempty"` - Extra map[string]any `json:"-"` -} - -// RoleAgentConfig mirrors RoleAgentConfigSchema for orchestrator/worker roles. -type RoleAgentConfig struct { - Agent string `json:"agent,omitempty"` - AgentConfig *AgentConfig `json:"agentConfig,omitempty"` -} - -// ReactionConfig mirrors ReactionConfigSchema. EscalateAfter accepts both -// numeric milliseconds and a duration string (e.g. "30m") in the TS schema, so -// the Go type stays open as `any` until handler validation lands. -type ReactionConfig struct { - Auto *bool `json:"auto,omitempty"` - Action string `json:"action,omitempty"` - Message string `json:"message,omitempty"` - Priority string `json:"priority,omitempty"` - Retries *int `json:"retries,omitempty"` - EscalateAfter any `json:"escalateAfter,omitempty"` - Threshold string `json:"threshold,omitempty"` - IncludeSummary *bool `json:"includeSummary,omitempty"` -} - -// LocalProjectConfig mirrors LocalProjectConfigSchema — the flat, behavior-only -// on-disk file at /agent-orchestrator.yaml. Identity fields -// (projectId, path, repo, defaultBranch, sessionPrefix) deliberately live in -// the global registry, not here. -type LocalProjectConfig struct { - Repo string `json:"repo,omitempty"` - DefaultBranch string `json:"defaultBranch,omitempty"` - Runtime string `json:"runtime,omitempty"` - Agent string `json:"agent,omitempty"` - Workspace string `json:"workspace,omitempty"` - Tracker *TrackerConfig `json:"tracker,omitempty"` - SCM *SCMConfig `json:"scm,omitempty"` - Symlinks []string `json:"symlinks,omitempty"` - PostCreate []string `json:"postCreate,omitempty"` - AgentConfig *AgentConfig `json:"agentConfig,omitempty"` - Orchestrator *RoleAgentConfig `json:"orchestrator,omitempty"` - Worker *RoleAgentConfig `json:"worker,omitempty"` - Reactions map[string]*ReactionConfig `json:"reactions,omitempty"` - AgentRules string `json:"agentRules,omitempty"` - Extra map[string]any `json:"-"` -} diff --git a/backend/internal/domain/project.go b/backend/internal/domain/project.go deleted file mode 100644 index 016450b9..00000000 --- a/backend/internal/domain/project.go +++ /dev/null @@ -1,47 +0,0 @@ -package domain - -// Project domain types for the HTTP API. ProjectID already exists in -// session.go; this file adds the list/read read-models the projects route -// surface returns. - -// ProjectSummary is the row shape returned by GET /api/v1/projects. It mirrors -// the TS ProjectInfo (packages/web/src/lib/project-name.ts) so the existing -// dashboard list view can read the Go daemon's response unchanged. -// -// ResolveError is populated only for degraded projects — entries whose config -// failed to load but whose registry entry still exists. The list view shows -// them with a warning instead of dropping them silently. -type ProjectSummary struct { - ID ProjectID `json:"id"` - Name string `json:"name"` - SessionPrefix string `json:"sessionPrefix"` - ResolveError string `json:"resolveError,omitempty"` -} - -// Project is the full read-model returned by GET /api/v1/projects/{id} when -// the project resolves cleanly. It joins the global-registry identity fields -// (id, name, path, repo, defaultBranch) with the local on-disk behaviour -// config (agent, runtime, tracker, scm, reactions). -type Project struct { - ID ProjectID `json:"id"` - Name string `json:"name"` - Path string `json:"path"` - Repo string `json:"repo"` - DefaultBranch string `json:"defaultBranch"` - Agent string `json:"agent,omitempty"` - Runtime string `json:"runtime,omitempty"` - Tracker *TrackerConfig `json:"tracker,omitempty"` - SCM *SCMConfig `json:"scm,omitempty"` - Reactions map[string]*ReactionConfig `json:"reactions,omitempty"` -} - -// DegradedProject is returned in place of Project when the project's config -// failed to load. The frontend uses ResolveError to render a recovery UI; the -// /projects/{id}/repair endpoint accepts the project id to fix a recoverable -// subset of degraded states (e.g. legacy wrapped-config format). -type DegradedProject struct { - ID ProjectID `json:"id"` - Name string `json:"name"` - Path string `json:"path"` - ResolveError string `json:"resolveError"` -} diff --git a/backend/internal/project/dto.go b/backend/internal/project/dto.go index e5bb933e..0e6f5ee5 100644 --- a/backend/internal/project/dto.go +++ b/backend/internal/project/dto.go @@ -3,17 +3,17 @@ package project import "github.com/aoagents/agent-orchestrator/backend/internal/domain" // Request/response shapes for Manager. They carry the data across the service -// boundary; the canonical entities (Project, ProjectSummary, DegradedProject) -// stay in domain/. Named without a "Project" prefix because the package name -// already supplies it (project.AddInput, project.GetResult). +// boundary; the entities they reference (Project, Summary, Degraded) live in +// types.go in this same package. Named without a "Project" prefix because the +// package name already supplies it (project.AddInput, project.GetResult). // GetResult is the discriminated union returned by Manager.Get. Exactly one of // Project / Degraded is non-nil; Status mirrors the discriminator on the wire // so consumers branch on it without nil-checking both fields. type GetResult struct { - Status string // "ok" | "degraded" - Project *domain.Project // populated when Status == "ok" - Degraded *domain.DegradedProject // populated when Status == "degraded" + Status string // "ok" | "degraded" + Project *Project // populated when Status == "ok" + Degraded *Degraded // populated when Status == "degraded" } // AddInput is the body shape for POST /api/v1/projects. Path is required; @@ -30,11 +30,11 @@ type AddInput struct { // behaviour fields are mutable; identity fields (projectId, path, repo, // defaultBranch) are rejected by the handler with a 400 IDENTITY_FROZEN. type UpdateConfigInput struct { - Agent *string `json:"agent,omitempty"` - Runtime *string `json:"runtime,omitempty"` - Tracker *domain.TrackerConfig `json:"tracker,omitempty"` - SCM *domain.SCMConfig `json:"scm,omitempty"` - Reactions *map[string]*domain.ReactionConfig `json:"reactions,omitempty"` + Agent *string `json:"agent,omitempty"` + Runtime *string `json:"runtime,omitempty"` + Tracker *TrackerConfig `json:"tracker,omitempty"` + SCM *SCMConfig `json:"scm,omitempty"` + Reactions *map[string]*ReactionConfig `json:"reactions,omitempty"` } // RemoveResult reports what DELETE /api/v1/projects/{id} actually did. diff --git a/backend/internal/project/project.go b/backend/internal/project/project.go index f5c79303..a997519d 100644 --- a/backend/internal/project/project.go +++ b/backend/internal/project/project.go @@ -21,23 +21,23 @@ import ( type Manager interface { // List returns every registered project, including degraded entries // (those whose config failed to load but whose registry entry survives). - List(ctx context.Context) ([]domain.ProjectSummary, error) + List(ctx context.Context) ([]Summary, error) // Get returns one project, discriminating ok vs degraded via GetResult. Get(ctx context.Context, id domain.ProjectID) (GetResult, error) // Add registers a new project from a git repository path. - Add(ctx context.Context, in AddInput) (domain.Project, error) + Add(ctx context.Context, in AddInput) (Project, error) // UpdateConfig patches behaviour-only fields; identity fields are frozen. - UpdateConfig(ctx context.Context, id domain.ProjectID, patch UpdateConfigInput) (domain.Project, error) + UpdateConfig(ctx context.Context, id domain.ProjectID, patch UpdateConfigInput) (Project, error) // Remove unregisters a project, stopping its sessions and reclaiming // managed workspaces. Remove(ctx context.Context, id domain.ProjectID) (RemoveResult, error) // Repair recovers a degraded project where automatic repair is available. - Repair(ctx context.Context, id domain.ProjectID) (domain.Project, error) + Repair(ctx context.Context, id domain.ProjectID) (Project, error) // Reload invalidates cached config and re-scans the global registry. Reload(ctx context.Context) (ReloadResult, error) diff --git a/backend/internal/project/types.go b/backend/internal/project/types.go new file mode 100644 index 00000000..65e5daa2 --- /dev/null +++ b/backend/internal/project/types.go @@ -0,0 +1,96 @@ +package project + +import "github.com/aoagents/agent-orchestrator/backend/internal/domain" + +// Project entities and the behaviour-config shapes they expose. These live in +// the project package (not domain/) because they are owned solely by the +// projects surface — only project identity (domain.ProjectID) is shared +// vocabulary with sessions/lifecycle/workspace, so that one type stays in +// domain. Keeping the entities, the Manager interface (project.go), and the +// transport DTOs (dto.go) together is the feature-package layout the backend +// is migrating toward. + +// Summary is the row shape returned by GET /api/v1/projects. It mirrors the TS +// ProjectInfo (packages/web/src/lib/project-name.ts) so the existing dashboard +// list view reads the Go daemon's response unchanged. ResolveError is set only +// for degraded projects (registry entry survives but config failed to load), +// so the list shows them with a warning instead of dropping them silently. +type Summary struct { + ID domain.ProjectID `json:"id"` + Name string `json:"name"` + SessionPrefix string `json:"sessionPrefix"` + ResolveError string `json:"resolveError,omitempty"` +} + +// Project is the full read-model returned by GET /api/v1/projects/{id} when the +// project resolves cleanly. It joins the registry identity fields with the +// project's behaviour config. +type Project struct { + ID domain.ProjectID `json:"id"` + Name string `json:"name"` + Path string `json:"path"` + Repo string `json:"repo"` // "owner/name" or "" + DefaultBranch string `json:"defaultBranch"` + Agent string `json:"agent,omitempty"` + Runtime string `json:"runtime,omitempty"` + Tracker *TrackerConfig `json:"tracker,omitempty"` + SCM *SCMConfig `json:"scm,omitempty"` + Reactions map[string]*ReactionConfig `json:"reactions,omitempty"` +} + +// Degraded is returned in place of Project when the project's config failed to +// load. The frontend uses ResolveError to render a recovery UI; the +// /projects/{id}/repair endpoint fixes a recoverable subset (e.g. legacy +// wrapped-config format). +type Degraded struct { + ID domain.ProjectID `json:"id"` + Name string `json:"name"` + Path string `json:"path"` + ResolveError string `json:"resolveError"` +} + +// Behaviour-config shapes ported from the TS Zod schemas (packages/core/src/ +// config.ts). Only the fields the projects API actually exposes are modelled; +// the passthrough/unknown-key round-trip the legacy schemas allowed lands with +// the handler implementation (and the SQLite persistence work), not in this +// interface-only PR. + +// TrackerConfig mirrors TrackerConfigSchema. +type TrackerConfig struct { + Plugin string `json:"plugin,omitempty"` + Package string `json:"package,omitempty"` + Path string `json:"path,omitempty"` +} + +// SCMConfig mirrors SCMConfigSchema; Webhook nests its own optional block. +type SCMConfig struct { + Plugin string `json:"plugin,omitempty"` + Package string `json:"package,omitempty"` + Path string `json:"path,omitempty"` + Webhook *SCMWebhookConfig `json:"webhook,omitempty"` +} + +// SCMWebhookConfig — pointer Enabled distinguishes unset from explicit false. +type SCMWebhookConfig struct { + Enabled *bool `json:"enabled,omitempty"` + Path string `json:"path,omitempty"` + SecretEnvVar string `json:"secretEnvVar,omitempty"` + SignatureHeader string `json:"signatureHeader,omitempty"` + EventHeader string `json:"eventHeader,omitempty"` + DeliveryHeader string `json:"deliveryHeader,omitempty"` + MaxBodyBytes int `json:"maxBodyBytes,omitempty"` +} + +// ReactionConfig mirrors ReactionConfigSchema. EscalateAfter is either ms +// (number) or a duration string ("30m") in the TS schema, so it stays open as +// `any` until handler validation lands. +type ReactionConfig struct { + Auto *bool `json:"auto,omitempty"` + Action string `json:"action,omitempty"` // send-to-agent | notify | auto-merge + Message string `json:"message,omitempty"` + Priority string `json:"priority,omitempty"` // urgent | action | warning | info + Retries *int `json:"retries,omitempty"` + EscalateAfter any `json:"escalateAfter,omitempty"` + Threshold string `json:"threshold,omitempty"` + IncludeSummary *bool `json:"includeSummary,omitempty"` +} From 5373e6278984d4cda5c63ae35bfac29ab344c16f Mon Sep 17 00:00:00 2001 From: Vaibhaav Date: Sun, 31 May 2026 03:18:28 +0530 Subject: [PATCH 13/23] feat(api): implement project routes with mock manager/store --- backend/internal/httpd/api.go | 3 + .../internal/httpd/controllers/projects.go | 177 ++++++++- .../httpd/controllers/projects_test.go | 338 ++++++++++-------- backend/internal/project/errors.go | 37 ++ backend/internal/project/manager.go | 249 +++++++++++++ backend/internal/project/memory_store.go | 141 ++++++++ backend/internal/runfile/rename_windows.go | 17 +- 7 files changed, 799 insertions(+), 163 deletions(-) create mode 100644 backend/internal/project/errors.go create mode 100644 backend/internal/project/manager.go create mode 100644 backend/internal/project/memory_store.go diff --git a/backend/internal/httpd/api.go b/backend/internal/httpd/api.go index 124a8d78..c02773fe 100644 --- a/backend/internal/httpd/api.go +++ b/backend/internal/httpd/api.go @@ -39,6 +39,9 @@ type API struct { // per-request timeout so the REST group can apply it without re-reading the // environment. func NewAPI(cfg config.Config, deps APIDeps) *API { + if deps.Projects == nil { + deps.Projects = project.NewMemoryManager() + } return &API{ cfg: cfg, projects: &controllers.ProjectsController{ diff --git a/backend/internal/httpd/controllers/projects.go b/backend/internal/httpd/controllers/projects.go index f8665041..8cac0f46 100644 --- a/backend/internal/httpd/controllers/projects.go +++ b/backend/internal/httpd/controllers/projects.go @@ -14,10 +14,16 @@ package controllers import ( + "bytes" + "encoding/json" + "errors" + "io" "net/http" "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" + "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec" "github.com/aoagents/agent-orchestrator/backend/internal/project" ) @@ -50,29 +56,186 @@ func (c *ProjectsController) Register(r chi.Router) { } func (c *ProjectsController) list(w http.ResponseWriter, r *http.Request) { - apispec.NotImplemented(w, r, "GET", "/api/v1/projects") + if c.Mgr == nil { + apispec.NotImplemented(w, r, "GET", "/api/v1/projects") + return + } + projects, err := c.Mgr.List(r.Context()) + if err != nil { + writeProjectError(w, r, err, http.StatusInternalServerError) + return + } + writeJSON(w, http.StatusOK, map[string]any{"projects": projects}) } func (c *ProjectsController) add(w http.ResponseWriter, r *http.Request) { - apispec.NotImplemented(w, r, "POST", "/api/v1/projects") + if c.Mgr == nil { + apispec.NotImplemented(w, r, "POST", "/api/v1/projects") + return + } + var in project.AddInput + if err := decodeJSON(r, &in); err != nil { + writeAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_JSON", "Invalid JSON body", nil) + return + } + p, err := c.Mgr.Add(r.Context(), in) + if err != nil { + writeProjectError(w, r, err, http.StatusInternalServerError) + return + } + writeJSON(w, http.StatusCreated, map[string]any{"project": p}) } func (c *ProjectsController) get(w http.ResponseWriter, r *http.Request) { - apispec.NotImplemented(w, r, "GET", "/api/v1/projects/{id}") + if c.Mgr == nil { + apispec.NotImplemented(w, r, "GET", "/api/v1/projects/{id}") + return + } + got, err := c.Mgr.Get(r.Context(), projectID(r)) + if err != nil { + writeProjectError(w, r, err, http.StatusInternalServerError) + return + } + if got.Status == "degraded" { + writeJSON(w, http.StatusOK, map[string]any{"status": got.Status, "project": got.Degraded}) + return + } + writeJSON(w, http.StatusOK, map[string]any{"status": got.Status, "project": got.Project}) } func (c *ProjectsController) updateConfig(w http.ResponseWriter, r *http.Request) { - apispec.NotImplemented(w, r, "PATCH", "/api/v1/projects/{id}") + if c.Mgr == nil { + apispec.NotImplemented(w, r, "PATCH", "/api/v1/projects/{id}") + return + } + if frozen, err := containsFrozenIdentityField(r); err != nil { + writeAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_JSON", "Invalid JSON body", nil) + return + } else if len(frozen) > 0 { + writeAPIError(w, r, http.StatusBadRequest, "bad_request", "IDENTITY_FROZEN", "Identity fields cannot be patched", map[string]any{"fields": frozen}) + return + } + + var patch project.UpdateConfigInput + if err := decodeJSON(r, &patch); err != nil { + writeAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_JSON", "Invalid JSON body", nil) + return + } + p, err := c.Mgr.UpdateConfig(r.Context(), projectID(r), patch) + if err != nil { + writeProjectError(w, r, err, http.StatusInternalServerError) + return + } + writeJSON(w, http.StatusOK, map[string]any{"project": p}) } func (c *ProjectsController) remove(w http.ResponseWriter, r *http.Request) { - apispec.NotImplemented(w, r, "DELETE", "/api/v1/projects/{id}") + if c.Mgr == nil { + apispec.NotImplemented(w, r, "DELETE", "/api/v1/projects/{id}") + return + } + result, err := c.Mgr.Remove(r.Context(), projectID(r)) + if err != nil { + writeProjectError(w, r, err, http.StatusInternalServerError) + return + } + writeJSON(w, http.StatusOK, result) } func (c *ProjectsController) repair(w http.ResponseWriter, r *http.Request) { - apispec.NotImplemented(w, r, "POST", "/api/v1/projects/{id}/repair") + if c.Mgr == nil { + apispec.NotImplemented(w, r, "POST", "/api/v1/projects/{id}/repair") + return + } + p, err := c.Mgr.Repair(r.Context(), projectID(r)) + if err != nil { + writeProjectError(w, r, err, http.StatusInternalServerError) + return + } + writeJSON(w, http.StatusOK, map[string]any{"project": p}) } func (c *ProjectsController) reload(w http.ResponseWriter, r *http.Request) { - apispec.NotImplemented(w, r, "POST", "/api/v1/projects/reload") + if c.Mgr == nil { + apispec.NotImplemented(w, r, "POST", "/api/v1/projects/reload") + return + } + result, err := c.Mgr.Reload(r.Context()) + if err != nil { + writeProjectError(w, r, err, http.StatusInternalServerError) + return + } + writeJSON(w, http.StatusOK, result) +} + +func projectID(r *http.Request) domain.ProjectID { + return domain.ProjectID(chi.URLParam(r, "id")) +} + +func decodeJSON(r *http.Request, out any) error { + return json.NewDecoder(r.Body).Decode(out) +} + +func containsFrozenIdentityField(r *http.Request) ([]string, error) { + body, err := io.ReadAll(r.Body) + if err != nil { + return nil, err + } + r.Body = io.NopCloser(bytes.NewReader(body)) + + var raw map[string]json.RawMessage + if err := json.Unmarshal(body, &raw); err != nil { + return nil, err + } + var frozen []string + for _, field := range []string{"projectId", "path", "repo", "defaultBranch"} { + if _, ok := raw[field]; ok { + frozen = append(frozen, field) + } + } + return frozen, nil +} + +type apiError struct { + Error string `json:"error"` + Code string `json:"code"` + Message string `json:"message"` + RequestID string `json:"requestId,omitempty"` + Details map[string]any `json:"details,omitempty"` +} + +func writeAPIError(w http.ResponseWriter, r *http.Request, status int, kind, code, message string, details map[string]any) { + writeJSON(w, status, apiError{ + Error: kind, + Code: code, + Message: message, + RequestID: middleware.GetReqID(r.Context()), + Details: details, + }) +} + +func writeProjectError(w http.ResponseWriter, r *http.Request, err error, fallbackStatus int) { + var pe *project.Error + if errors.As(err, &pe) { + status := fallbackStatus + switch pe.Kind { + case "bad_request": + status = http.StatusBadRequest + case "not_found": + status = http.StatusNotFound + case "conflict": + status = http.StatusConflict + case "internal": + status = http.StatusInternalServerError + } + writeAPIError(w, r, status, pe.Kind, pe.Code, pe.Message, pe.Details) + return + } + writeAPIError(w, r, fallbackStatus, "internal", "INTERNAL_ERROR", "Internal server error", nil) +} + +func writeJSON(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(v) } diff --git a/backend/internal/httpd/controllers/projects_test.go b/backend/internal/httpd/controllers/projects_test.go index 8e011de3..b230d5f2 100644 --- a/backend/internal/httpd/controllers/projects_test.go +++ b/backend/internal/httpd/controllers/projects_test.go @@ -1,19 +1,13 @@ package controllers_test -// Route-shell tests for /api/v1/projects. Builds the full router (so the -// /api/v1 mount, middleware, NotFound, and MethodNotAllowed handlers are -// exercised together) and asserts every canonical route returns 501 with -// the locked envelope + a `spec` slice sourced from apispec/openapi.yaml. -// Legacy paths that the REST audit dropped return 405 (sibling method -// exists) or 404 (no sibling); the legacy → canonical mapping itself is -// documented in the YAML via x-replaces. - import ( "encoding/json" "io" "log/slog" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" @@ -23,78 +17,146 @@ import ( func newTestServer(t *testing.T) *httptest.Server { t.Helper() - // Discard logger keeps test output clean — the access-log middleware - // added in base #10·1a wants a non-nil *slog.Logger. log := slog.New(slog.NewTextHandler(io.Discard, nil)) srv := httptest.NewServer(httpd.NewRouter(config.Config{}, log)) t.Cleanup(srv.Close) return srv } -// TestProjectsRoutes_Canonical501 walks every canonical /projects route and -// asserts it returns 501 with the locked envelope. wantOpID double-checks -// that the embedded spec slice is the right operation (not e.g. the -// /projects/{id} block leaking into /projects/reload because of route -// shadowing). -func TestProjectsRoutes_Canonical501(t *testing.T) { +func TestProjectsAPI_ListAddGetReload(t *testing.T) { srv := newTestServer(t) + repo := gitRepo(t, "agent-orchestrator") + + body, status, headers := doRequest(t, srv, "GET", "/api/v1/projects", "") + if status != http.StatusOK { + t.Fatalf("GET projects = %d, want 200; body=%s", status, body) + } + assertJSON(t, headers) + var list struct { + Projects []projectSummary `json:"projects"` + } + mustJSON(t, body, &list) + if len(list.Projects) != 0 { + t.Fatalf("initial project count = %d, want 0", len(list.Projects)) + } + + body, status, _ = doRequest(t, srv, "POST", "/api/v1/projects", `{"path":`+quote(repo)+`,"projectId":"ao","name":"Agent Orchestrator"}`) + if status != http.StatusCreated { + t.Fatalf("POST project = %d, want 201; body=%s", status, body) + } + var add struct { + Project projectBody `json:"project"` + } + mustJSON(t, body, &add) + if add.Project.ID != "ao" || add.Project.Name != "Agent Orchestrator" || add.Project.DefaultBranch != "main" { + t.Fatalf("created project = %#v", add.Project) + } + + body, status, _ = doRequest(t, srv, "GET", "/api/v1/projects/ao", "") + if status != http.StatusOK { + t.Fatalf("GET project = %d, want 200; body=%s", status, body) + } + var get struct { + Status string `json:"status"` + Project projectBody `json:"project"` + } + mustJSON(t, body, &get) + if get.Status != "ok" || get.Project.ID != "ao" { + t.Fatalf("get response = %#v", get) + } + + body, status, _ = doRequest(t, srv, "POST", "/api/v1/projects/reload", "") + if status != http.StatusOK { + t.Fatalf("reload = %d, want 200; body=%s", status, body) + } + var reload struct { + Reloaded bool `json:"reloaded"` + ProjectCount int `json:"projectCount"` + DegradedCount int `json:"degradedCount"` + } + mustJSON(t, body, &reload) + if !reload.Reloaded || reload.ProjectCount != 1 || reload.DegradedCount != 0 { + t.Fatalf("reload response = %#v", reload) + } +} + +func TestProjectsAPI_AddValidationAndConflicts(t *testing.T) { + srv := newTestServer(t) + repoA := gitRepo(t, "repo-a") + repoB := gitRepo(t, "repo-b") + notRepo := t.TempDir() cases := []struct { - method, path, body, wantOpID string - wantReplaces []string + name, body, wantCode string + wantStatus int }{ - {method: "GET", path: "/api/v1/projects", wantOpID: "listProjects"}, - {method: "POST", path: "/api/v1/projects", body: `{}`, wantOpID: "addProject"}, - {method: "GET", path: "/api/v1/projects/p1", wantOpID: "getProject"}, - {method: "PATCH", path: "/api/v1/projects/p1", body: `{}`, wantOpID: "updateProjectConfig"}, - {method: "DELETE", path: "/api/v1/projects/p1", wantOpID: "removeProject"}, - { - method: "POST", path: "/api/v1/projects/p1/repair", - wantOpID: "repairProject", - wantReplaces: []string{"POST /api/v1/projects/{id}"}, - }, - {method: "POST", path: "/api/v1/projects/reload", wantOpID: "reloadProjects"}, + {name: "invalid json", body: `{`, wantStatus: 400, wantCode: "INVALID_JSON"}, + {name: "missing path", body: `{}`, wantStatus: 400, wantCode: "PATH_REQUIRED"}, + {name: "not git", body: `{"path":` + quote(notRepo) + `}`, wantStatus: 400, wantCode: "NOT_A_GIT_REPO"}, } - for _, tc := range cases { - t.Run(tc.method+" "+tc.path, func(t *testing.T) { - body, status, headers := doRequest(t, srv, tc.method, tc.path, tc.body) - - if status != http.StatusNotImplemented { - t.Fatalf("status = %d, want 501\nbody=%s", status, body) - } - if ct := headers.Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { - t.Errorf("Content-Type = %q, want JSON", ct) - } - - var got envelope - if err := json.Unmarshal(body, &got); err != nil { - t.Fatalf("unmarshal: %v\nbody=%s", err, body) - } - if got.Error != "not_implemented" || got.Code != "NOT_IMPLEMENTED" { - t.Errorf("envelope error/code = %q/%q, want not_implemented/NOT_IMPLEMENTED", got.Error, got.Code) - } - if got.Message == "" { - t.Error("envelope.message empty") - } - if got.Spec == nil { - t.Fatal("envelope.spec missing — apispec failed to find the operation") - } - if op, _ := got.Spec["operationId"].(string); op != tc.wantOpID { - t.Errorf("spec.operationId = %q, want %q", op, tc.wantOpID) - } - gotReplaces := stringSlice(got.Spec["x-replaces"]) - if !equalStrings(gotReplaces, tc.wantReplaces) { - t.Errorf("spec.x-replaces = %v, want %v", gotReplaces, tc.wantReplaces) - } + t.Run(tc.name, func(t *testing.T) { + body, status, _ := doRequest(t, srv, "POST", "/api/v1/projects", tc.body) + assertErrorCode(t, body, status, tc.wantStatus, tc.wantCode) }) } + + body, status, _ := doRequest(t, srv, "POST", "/api/v1/projects", `{"path":`+quote(repoA)+`,"projectId":"shared"}`) + if status != http.StatusCreated { + t.Fatalf("seed create = %d, want 201; body=%s", status, body) + } + + body, status, _ = doRequest(t, srv, "POST", "/api/v1/projects", `{"path":`+quote(repoA)+`,"projectId":"other"}`) + assertErrorCode(t, body, status, http.StatusConflict, "PATH_ALREADY_REGISTERED") + + body, status, _ = doRequest(t, srv, "POST", "/api/v1/projects", `{"path":`+quote(repoB)+`,"projectId":"shared"}`) + assertErrorCode(t, body, status, http.StatusConflict, "ID_ALREADY_REGISTERED") +} + +func TestProjectsAPI_UpdateDeleteRepair(t *testing.T) { + srv := newTestServer(t) + repo := gitRepo(t, "repo") + + body, status, _ := doRequest(t, srv, "POST", "/api/v1/projects", `{"path":`+quote(repo)+`,"projectId":"proj"}`) + if status != http.StatusCreated { + t.Fatalf("seed create = %d, want 201; body=%s", status, body) + } + + body, status, _ = doRequest(t, srv, "PATCH", "/api/v1/projects/proj", `{"agent":"claude","runtime":"tmux"}`) + if status != http.StatusOK { + t.Fatalf("PATCH = %d, want 200; body=%s", status, body) + } + var patched struct { + Project projectBody `json:"project"` + } + mustJSON(t, body, &patched) + if patched.Project.Agent != "claude" || patched.Project.Runtime != "tmux" { + t.Fatalf("patched project = %#v", patched.Project) + } + + body, status, _ = doRequest(t, srv, "PATCH", "/api/v1/projects/proj", `{"path":"elsewhere"}`) + assertErrorCode(t, body, status, http.StatusBadRequest, "IDENTITY_FROZEN") + + body, status, _ = doRequest(t, srv, "POST", "/api/v1/projects/proj/repair", "") + assertErrorCode(t, body, status, http.StatusBadRequest, "REPAIR_NOT_AVAILABLE") + + body, status, _ = doRequest(t, srv, "DELETE", "/api/v1/projects/proj", "") + if status != http.StatusOK { + t.Fatalf("DELETE = %d, want 200; body=%s", status, body) + } + var removed struct { + ProjectID string `json:"projectId"` + RemovedStorageDir bool `json:"removedStorageDir"` + } + mustJSON(t, body, &removed) + if removed.ProjectID != "proj" || removed.RemovedStorageDir { + t.Fatalf("delete response = %#v", removed) + } + + body, status, _ = doRequest(t, srv, "GET", "/api/v1/projects/proj", "") + assertErrorCode(t, body, status, http.StatusNotFound, "PROJECT_NOT_FOUND") } -// TestProjectsRoutes_LegacyUnregistered confirms the REST-audit-dropped TS -// paths are deliberately unregistered. PUT and POST on /projects/{id} hit -// sibling-method paths so chi returns 405. The legacy → canonical mapping -// is documented on the canonical operation via x-replaces (covered above). func TestProjectsRoutes_LegacyUnregistered(t *testing.T) { srv := newTestServer(t) @@ -109,60 +171,18 @@ func TestProjectsRoutes_LegacyUnregistered(t *testing.T) { for _, tc := range cases { t.Run(tc.why, func(t *testing.T) { body, status, _ := doRequest(t, srv, tc.method, tc.path, "") - if status != tc.wantStatus { - t.Fatalf("%s %s = %d, want %d", tc.method, tc.path, status, tc.wantStatus) - } - var e envelope - if err := json.Unmarshal(body, &e); err != nil { - t.Fatalf("unmarshal: %v\nbody=%s", err, body) - } - if e.Code != tc.wantCode { - t.Errorf("code = %q, want %q", e.Code, tc.wantCode) - } + assertErrorCode(t, body, status, tc.wantStatus, tc.wantCode) }) } } -// TestProjectsRoutes_MissingRoute confirms the JSON 404 envelope (not chi's -// default text/plain) for routes that don't exist at all. func TestProjectsRoutes_MissingRoute(t *testing.T) { srv := newTestServer(t) body, status, headers := doRequest(t, srv, "GET", "/api/v1/projects/p1/does-not-exist", "") - - if status != http.StatusNotFound { - t.Fatalf("status = %d, want 404", status) - } - if ct := headers.Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { - t.Errorf("Content-Type = %q, want JSON (router must override chi's text/plain default)", ct) - } - var e envelope - if err := json.Unmarshal(body, &e); err != nil { - t.Fatalf("unmarshal: %v\nbody=%s", err, body) - } - if e.Code != "ROUTE_NOT_FOUND" { - t.Errorf("code = %q, want ROUTE_NOT_FOUND", e.Code) - } -} - -// TestProjectsRoutes_ReloadBeforeID is the chi-ordering safety check. If -// the {id} wildcard were registered first, POST /projects/reload would -// match {id}="reload" → repair handler instead of the reload handler. We -// assert reload's spec slice (operationId=reloadProjects), not repair's. -func TestProjectsRoutes_ReloadBeforeID(t *testing.T) { - srv := newTestServer(t) - body, status, _ := doRequest(t, srv, "POST", "/api/v1/projects/reload", "") - if status != http.StatusNotImplemented { - t.Fatalf("status = %d, want 501", status) - } - var e envelope - _ = json.Unmarshal(body, &e) - if op, _ := e.Spec["operationId"].(string); op != "reloadProjects" { - t.Errorf("reload was shadowed by {id}: spec.operationId = %q", op) - } + assertJSON(t, headers) + assertErrorCode(t, body, status, http.StatusNotFound, "ROUTE_NOT_FOUND") } -// TestOpenAPIYAMLServed confirms the embedded spec is reachable at the -// documented path so external tooling can fetch it. func TestOpenAPIYAMLServed(t *testing.T) { srv := newTestServer(t) body, status, headers := doRequest(t, srv, "GET", "/api/v1/openapi.yaml", "") @@ -173,19 +193,31 @@ func TestOpenAPIYAMLServed(t *testing.T) { t.Errorf("Content-Type = %q, want application/yaml*", ct) } if !strings.Contains(string(body), "openapi: 3.1.0") { - t.Errorf("served body did not start with an OpenAPI 3.1 doc — first bytes:\n%s", firstLine(body)) + t.Errorf("served body did not start with an OpenAPI 3.1 doc") } } -// envelope mirrors the locked APIError + apispec spec field on the wire. -// We declare it in the test rather than importing apispec's private type -// so the test pins the JSON contract independently of internal renames. -type envelope struct { - Error string `json:"error"` - Code string `json:"code"` - Message string `json:"message"` - RequestID string `json:"requestId"` - Spec map[string]any `json:"spec"` +type projectSummary struct { + ID string `json:"id"` + Name string `json:"name"` + SessionPrefix string `json:"sessionPrefix"` +} + +type projectBody struct { + ID string `json:"id"` + Name string `json:"name"` + Path string `json:"path"` + Repo string `json:"repo"` + DefaultBranch string `json:"defaultBranch"` + Agent string `json:"agent"` + Runtime string `json:"runtime"` +} + +type errorBody struct { + Error string `json:"error"` + Code string `json:"code"` + Message string `json:"message"` + Details map[string]any `json:"details"` } func doRequest(t *testing.T, srv *httptest.Server, method, path, body string) ([]byte, int, http.Header) { @@ -209,51 +241,49 @@ func doRequest(t *testing.T, srv *httptest.Server, method, path, body string) ([ t.Fatalf("do request: %v", err) } defer resp.Body.Close() - buf := make([]byte, 0, 1024) - tmp := make([]byte, 512) - for { - n, rerr := resp.Body.Read(tmp) - if n > 0 { - buf = append(buf, tmp[:n]...) - } - if rerr != nil { - break - } + buf, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) } return buf, resp.StatusCode, resp.Header } -// stringSlice coerces an `any` that holds a YAML-decoded sequence into a -// []string. yaml.v3 decodes sequences as []any; we only call this on slices -// whose elements we know are strings. -func stringSlice(v any) []string { - raw, ok := v.([]any) - if !ok { - return nil - } - out := make([]string, 0, len(raw)) - for _, item := range raw { - s, _ := item.(string) - out = append(out, s) +func gitRepo(t *testing.T, name string) string { + t.Helper() + dir := filepath.Join(t.TempDir(), name) + if err := os.MkdirAll(filepath.Join(dir, ".git"), 0o755); err != nil { + t.Fatalf("create git repo fixture: %v", err) } - return out + return dir +} + +func quote(s string) string { + b, _ := json.Marshal(s) + return string(b) } -func equalStrings(a, b []string) bool { - if len(a) != len(b) { - return false +func mustJSON(t *testing.T, body []byte, out any) { + t.Helper() + if err := json.Unmarshal(body, out); err != nil { + t.Fatalf("unmarshal: %v\nbody=%s", err, body) } - for i := range a { - if a[i] != b[i] { - return false - } +} + +func assertJSON(t *testing.T, headers http.Header) { + t.Helper() + if ct := headers.Get("Content-Type"); !strings.HasPrefix(ct, "application/json") { + t.Fatalf("Content-Type = %q, want JSON", ct) } - return true } -func firstLine(b []byte) string { - if i := strings.IndexByte(string(b), '\n'); i >= 0 { - return string(b[:i]) +func assertErrorCode(t *testing.T, body []byte, status, wantStatus int, wantCode string) { + t.Helper() + if status != wantStatus { + t.Fatalf("status = %d, want %d\nbody=%s", status, wantStatus, body) + } + var got errorBody + mustJSON(t, body, &got) + if got.Code != wantCode { + t.Fatalf("code = %q, want %q\nbody=%s", got.Code, wantCode, body) } - return string(b) } diff --git a/backend/internal/project/errors.go b/backend/internal/project/errors.go new file mode 100644 index 00000000..c4e29d8c --- /dev/null +++ b/backend/internal/project/errors.go @@ -0,0 +1,37 @@ +package project + +// Error is the manager-level error shape controllers can 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) +} diff --git a/backend/internal/project/manager.go b/backend/internal/project/manager.go new file mode 100644 index 00000000..9e7a876e --- /dev/null +++ b/backend/internal/project/manager.go @@ -0,0 +1,249 @@ +package project + +import ( + "context" + "os" + "path/filepath" + "regexp" + "strconv" + "strings" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +type manager struct { + store Store +} + +var _ Manager = (*manager)(nil) + +func NewManager(store Store) Manager { + if store == nil { + store = NewMemoryStore() + } + return &manager{store: store} +} + +func NewMemoryManager() Manager { + return NewManager(NewMemoryStore()) +} + +func (m *manager) List(ctx context.Context) ([]Summary, error) { + projects, err := m.store.List(ctx) + if err != nil { + return nil, internal("PROJECTS_LIST_FAILED", "Failed to load projects") + } + out := make([]Summary, 0, len(projects)) + for _, p := range projects { + out = append(out, Summary{ + ID: p.ID, + Name: p.Name, + SessionPrefix: sessionPrefix(p.ID), + }) + } + return out, nil +} + +func (m *manager) Get(ctx context.Context, id domain.ProjectID) (GetResult, error) { + if err := validateProjectID(id); err != nil { + return GetResult{}, err + } + p, ok, err := m.store.Get(ctx, id) + if err != nil { + return GetResult{}, internal("PROJECT_LOAD_FAILED", "Failed to load project") + } + if !ok { + return GetResult{}, notFound("PROJECT_NOT_FOUND", "Unknown project") + } + return GetResult{Status: "ok", Project: &p}, nil +} + +func (m *manager) Add(ctx context.Context, in AddInput) (Project, error) { + path, err := normalizePath(in.Path) + if err != nil { + return Project{}, err + } + if !isGitRepo(path) { + return Project{}, badRequest("NOT_A_GIT_REPO", "Repository path must point to a git repository", nil) + } + + id := defaultProjectID(path) + if in.ProjectID != nil { + id = domain.ProjectID(strings.TrimSpace(*in.ProjectID)) + } + if err := validateProjectID(id); err != nil { + return Project{}, err + } + + name := string(id) + if in.Name != nil { + name = strings.TrimSpace(*in.Name) + } + if name == "" { + name = string(id) + } + + if existing, ok, err := m.store.FindByPath(ctx, path); err != nil { + return Project{}, 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{ + "existingProjectId": string(existing.ID), + "suggestedProjectId": string(m.suggestID(ctx, id)), + }) + } + if existing, ok, err := m.store.Get(ctx, id); err != nil { + return Project{}, internal("PROJECT_LOAD_FAILED", "Failed to load project") + } else if ok && existing.Path != path { + return Project{}, conflict("ID_ALREADY_REGISTERED", "A project with this id is already registered for a different path", map[string]any{ + "existingProjectId": string(existing.ID), + "suggestedProjectId": string(m.suggestID(ctx, id)), + }) + } + + p := Project{ + ID: id, + Name: name, + Path: path, + Repo: "", + DefaultBranch: "main", + } + if err := m.store.Create(ctx, p); err != nil { + return Project{}, err + } + return p, nil +} + +func (m *manager) UpdateConfig(ctx context.Context, id domain.ProjectID, patch UpdateConfigInput) (Project, error) { + if err := validateProjectID(id); err != nil { + return Project{}, err + } + p, ok, err := m.store.Get(ctx, id) + if err != nil { + return Project{}, internal("PROJECT_LOAD_FAILED", "Failed to load project") + } + if !ok { + return Project{}, notFound("PROJECT_NOT_FOUND", "Unknown project") + } + + if patch.Agent != nil { + p.Agent = strings.TrimSpace(*patch.Agent) + } + if patch.Runtime != nil { + p.Runtime = strings.TrimSpace(*patch.Runtime) + } + if patch.Tracker != nil { + tracker := *patch.Tracker + p.Tracker = &tracker + } + if patch.SCM != nil { + scm := *patch.SCM + p.SCM = &scm + } + if patch.Reactions != nil { + p.Reactions = *patch.Reactions + } + + if err := m.store.Update(ctx, p); err != nil { + return Project{}, err + } + return p, nil +} + +func (m *manager) Remove(ctx context.Context, id domain.ProjectID) (RemoveResult, error) { + if err := validateProjectID(id); err != nil { + return RemoveResult{}, err + } + ok, err := m.store.Delete(ctx, id) + if err != nil { + return RemoveResult{}, internal("PROJECT_REMOVE_FAILED", "Failed to remove project") + } + if !ok { + return RemoveResult{}, notFound("PROJECT_NOT_FOUND", "Unknown project") + } + return RemoveResult{ProjectID: id, RemovedStorageDir: false}, nil +} + +func (m *manager) Repair(ctx context.Context, id domain.ProjectID) (Project, error) { + if err := validateProjectID(id); err != nil { + return Project{}, err + } + if _, ok, err := m.store.Get(ctx, id); err != nil { + return Project{}, internal("PROJECT_LOAD_FAILED", "Failed to load project") + } else if !ok { + return Project{}, notFound("PROJECT_NOT_FOUND", "Unknown project") + } + return Project{}, badRequest("REPAIR_NOT_AVAILABLE", "Automatic repair is not available for this degraded config", nil) +} + +func (m *manager) Reload(ctx context.Context) (ReloadResult, error) { + projects, err := m.store.List(ctx) + if err != nil { + return ReloadResult{}, internal("RELOAD_FAILED", "Failed to reload projects") + } + return ReloadResult{Reloaded: true, ProjectCount: len(projects), DegradedCount: 0}, nil +} + +func (m *manager) suggestID(ctx context.Context, base domain.ProjectID) domain.ProjectID { + for i := 2; ; i++ { + candidate := domain.ProjectID(string(base) + "-" + strconv.Itoa(i)) + if _, ok, _ := m.store.Get(ctx, candidate); !ok { + return candidate + } + } +} + +func normalizePath(raw string) (string, error) { + raw = strings.TrimSpace(raw) + if raw == "" { + return "", badRequest("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) + } + if raw == "~" { + raw = home + } else if strings.HasPrefix(raw, "~/") || strings.HasPrefix(raw, `~\`) { + raw = filepath.Join(home, raw[2:]) + } + } + abs, err := filepath.Abs(raw) + if err != nil { + return "", badRequest("INVALID_PATH", "Repository path is invalid", nil) + } + return filepath.Clean(abs), nil +} + +func isGitRepo(path string) bool { + info, err := os.Stat(filepath.Join(path, ".git")) + return err == nil && (info.IsDir() || info.Mode().IsRegular()) +} + +func defaultProjectID(path string) domain.ProjectID { + id := strings.ToLower(filepath.Base(path)) + id = strings.TrimSpace(id) + id = strings.ReplaceAll(id, " ", "-") + return domain.ProjectID(id) +} + +var projectIDPattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*$`) + +func validateProjectID(id domain.ProjectID) error { + raw := string(id) + if raw == "" || raw == "." || raw == ".." || strings.ContainsAny(raw, `/\`) || !projectIDPattern.MatchString(raw) { + return badRequest("INVALID_PROJECT_ID", "Project id failed storage-path validation", nil) + } + return nil +} + +func sessionPrefix(id domain.ProjectID) string { + raw := string(id) + if raw == "" { + return "ao" + } + if len(raw) <= 12 { + return raw + } + return raw[:12] +} diff --git a/backend/internal/project/memory_store.go b/backend/internal/project/memory_store.go new file mode 100644 index 00000000..6decc069 --- /dev/null +++ b/backend/internal/project/memory_store.go @@ -0,0 +1,141 @@ +package project + +import ( + "context" + "sync" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +type Store interface { + List(ctx context.Context) ([]Project, error) + Get(ctx context.Context, id domain.ProjectID) (Project, bool, error) + FindByPath(ctx context.Context, path string) (Project, bool, error) + Create(ctx context.Context, p Project) error + Update(ctx context.Context, p Project) error + Delete(ctx context.Context, id domain.ProjectID) (bool, error) +} + +// MemoryStore is the mocked DB layer for the project API implementation. It is +// process-local and intentionally small, but concurrency-safe for HTTP tests. +type MemoryStore struct { + mu sync.Mutex + projects map[domain.ProjectID]Project + paths map[string]domain.ProjectID +} + +var _ Store = (*MemoryStore)(nil) + +func NewMemoryStore() *MemoryStore { + return &MemoryStore{ + projects: map[domain.ProjectID]Project{}, + paths: map[string]domain.ProjectID{}, + } +} + +func (s *MemoryStore) List(context.Context) ([]Project, error) { + s.mu.Lock() + defer s.mu.Unlock() + + out := make([]Project, 0, len(s.projects)) + for _, p := range s.projects { + out = append(out, cloneProject(p)) + } + return out, nil +} + +func (s *MemoryStore) Get(_ context.Context, id domain.ProjectID) (Project, bool, error) { + s.mu.Lock() + defer s.mu.Unlock() + + p, ok := s.projects[id] + if !ok { + return Project{}, false, nil + } + return cloneProject(p), true, nil +} + +func (s *MemoryStore) FindByPath(_ context.Context, path string) (Project, bool, error) { + s.mu.Lock() + defer s.mu.Unlock() + + id, ok := s.paths[path] + if !ok { + return Project{}, false, nil + } + p, ok := s.projects[id] + if !ok { + return Project{}, false, nil + } + return cloneProject(p), true, nil +} + +func (s *MemoryStore) Create(_ context.Context, p Project) error { + s.mu.Lock() + defer s.mu.Unlock() + + if _, ok := s.projects[p.ID]; ok { + return conflict("ID_ALREADY_REGISTERED", "A project with this id is already registered for a different path", nil) + } + if existingID, ok := s.paths[p.Path]; ok { + return conflict("PATH_ALREADY_REGISTERED", "A project at this path is already registered", map[string]any{ + "existingProjectId": string(existingID), + }) + } + s.projects[p.ID] = cloneProject(p) + s.paths[p.Path] = p.ID + return nil +} + +func (s *MemoryStore) Update(_ context.Context, p Project) error { + s.mu.Lock() + defer s.mu.Unlock() + + if _, ok := s.projects[p.ID]; !ok { + return notFound("PROJECT_NOT_FOUND", "Unknown project") + } + s.projects[p.ID] = cloneProject(p) + s.paths[p.Path] = p.ID + return nil +} + +func (s *MemoryStore) Delete(_ context.Context, id domain.ProjectID) (bool, error) { + s.mu.Lock() + defer s.mu.Unlock() + + p, ok := s.projects[id] + if !ok { + return false, nil + } + delete(s.projects, id) + delete(s.paths, p.Path) + return true, nil +} + +func cloneProject(p Project) Project { + if p.Tracker != nil { + tracker := *p.Tracker + p.Tracker = &tracker + } + if p.SCM != nil { + scm := *p.SCM + if p.SCM.Webhook != nil { + webhook := *p.SCM.Webhook + scm.Webhook = &webhook + } + p.SCM = &scm + } + if p.Reactions != nil { + reactions := make(map[string]*ReactionConfig, len(p.Reactions)) + for k, v := range p.Reactions { + if v == nil { + reactions[k] = nil + continue + } + reaction := *v + reactions[k] = &reaction + } + p.Reactions = reactions + } + return p +} diff --git a/backend/internal/runfile/rename_windows.go b/backend/internal/runfile/rename_windows.go index 031411ee..70f5d1de 100644 --- a/backend/internal/runfile/rename_windows.go +++ b/backend/internal/runfile/rename_windows.go @@ -2,13 +2,18 @@ package runfile -import "syscall" +import ( + "syscall" + "unsafe" +) // movefileReplaceExisting tells MoveFileEx to overwrite dst if it already // exists. Mirrors MOVEFILE_REPLACE_EXISTING from the Win32 API; declared // locally so we don't pull in golang.org/x/sys for a single constant. const movefileReplaceExisting = 0x1 +var moveFileExW = syscall.NewLazyDLL("kernel32.dll").NewProc("MoveFileExW") + // atomicReplace renames src to dst, replacing dst if it exists. Go's // os.Rename on Windows happens to do the same MoveFileEx call internally, // but calling it directly makes the cross-platform contract explicit instead @@ -24,5 +29,13 @@ func atomicReplace(src, dst string) error { if err != nil { return err } - return syscall.MoveFileEx(srcPtr, dstPtr, movefileReplaceExisting) + ret, _, err := moveFileExW.Call( + uintptr(unsafe.Pointer(srcPtr)), + uintptr(unsafe.Pointer(dstPtr)), + uintptr(movefileReplaceExisting), + ) + if ret == 0 { + return err + } + return nil } From 7877f855796729cd829479a0f80caeed1960d0ae Mon Sep 17 00:00:00 2001 From: Vaibhaav Tiwari <155460282+Vaibhaav-Tiwari@users.noreply.github.com> Date: Sun, 31 May 2026 03:34:21 +0530 Subject: [PATCH 14/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- backend/internal/httpd/apispec/apispec.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/internal/httpd/apispec/apispec.go b/backend/internal/httpd/apispec/apispec.go index 8e9fb258..304b8cf4 100644 --- a/backend/internal/httpd/apispec/apispec.go +++ b/backend/internal/httpd/apispec/apispec.go @@ -115,7 +115,7 @@ type notImplementedResponse struct { Code string `json:"code"` Message string `json:"message"` RequestID string `json:"requestId,omitempty"` - Spec map[string]any `json:"spec,omitempty"` + Spec map[string]any `json:"spec"` } // NotImplemented writes the locked 501 envelope, embedding the OpenAPI From d14d41f27ed238eed3ef0558b4783f4fdb99f4fa Mon Sep 17 00:00:00 2001 From: Vaibhaav Tiwari <155460282+Vaibhaav-Tiwari@users.noreply.github.com> Date: Sun, 31 May 2026 03:36:14 +0530 Subject: [PATCH 15/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- backend/internal/httpd/apispec/apispec.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/internal/httpd/apispec/apispec.go b/backend/internal/httpd/apispec/apispec.go index 304b8cf4..627ad5db 100644 --- a/backend/internal/httpd/apispec/apispec.go +++ b/backend/internal/httpd/apispec/apispec.go @@ -124,7 +124,9 @@ type notImplementedResponse struct { // duplicated in controller code. func NotImplemented(w http.ResponseWriter, r *http.Request, method, path string) { op := Default().Operation(method, path) - + if op == nil { + panic(fmt.Sprintf("apispec: missing operation for %s %s", method, path)) + } body := notImplementedResponse{ Error: "not_implemented", Code: "NOT_IMPLEMENTED", From 15aa33f96760cf2d836615517b75482655615827 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 30 May 2026 22:06:47 +0000 Subject: [PATCH 16/23] merge: resolve conflicts with origin/main --- backend/go.mod | 2 +- backend/go.sum | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/backend/go.mod b/backend/go.mod index 651c6594..48d09551 100644 --- a/backend/go.mod +++ b/backend/go.mod @@ -4,4 +4,4 @@ go 1.22 require github.com/go-chi/chi/v5 v5.1.0 -require gopkg.in/yaml.v3 v3.0.1 // indirect +require gopkg.in/yaml.v3 v3.0.1 diff --git a/backend/go.sum b/backend/go.sum index c958a7c2..222cf13e 100644 --- a/backend/go.sum +++ b/backend/go.sum @@ -1,5 +1,6 @@ github.com/go-chi/chi/v5 v5.1.0 h1:acVI1TYaD+hhedDJ3r54HyA6sExp3HfXq7QWEEY/xMw= github.com/go-chi/chi/v5 v5.1.0/go.mod h1:DslCQbL2OYiznFReuXYUmQ2hGd1aDpCnlMNITLSKoi8= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= From fb146ce8bb7a8c9bd0c700c6dae2b787cd2dd24e Mon Sep 17 00:00:00 2001 From: Vaibhaav Tiwari <155460282+Vaibhaav-Tiwari@users.noreply.github.com> Date: Sun, 31 May 2026 03:37:14 +0530 Subject: [PATCH 17/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- backend/internal/httpd/apispec/openapi.yaml | 3 --- 1 file changed, 3 deletions(-) diff --git a/backend/internal/httpd/apispec/openapi.yaml b/backend/internal/httpd/apispec/openapi.yaml index dabb3676..30005ca4 100644 --- a/backend/internal/httpd/apispec/openapi.yaml +++ b/backend/internal/httpd/apispec/openapi.yaml @@ -350,9 +350,6 @@ components: oneOf: - $ref: "#/components/schemas/Project" - $ref: "#/components/schemas/DegradedProject" - discriminator: - propertyName: status - AddProjectRequest: type: object required: [path] From 99a92a31bb4c438749757386a8ac20c52268dc38 Mon Sep 17 00:00:00 2001 From: Vaibhaav Tiwari <155460282+Vaibhaav-Tiwari@users.noreply.github.com> Date: Sun, 31 May 2026 03:37:41 +0530 Subject: [PATCH 18/23] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- backend/internal/project/memory_store.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/backend/internal/project/memory_store.go b/backend/internal/project/memory_store.go index 6decc069..563ccd7e 100644 --- a/backend/internal/project/memory_store.go +++ b/backend/internal/project/memory_store.go @@ -91,9 +91,13 @@ func (s *MemoryStore) Update(_ context.Context, p Project) error { s.mu.Lock() defer s.mu.Unlock() - if _, ok := s.projects[p.ID]; !ok { + prev, ok := s.projects[p.ID] + if !ok { return notFound("PROJECT_NOT_FOUND", "Unknown project") } + if prev.Path != "" && prev.Path != p.Path { + delete(s.paths, prev.Path) + } s.projects[p.ID] = cloneProject(p) s.paths[p.Path] = p.ID return nil From 9b3c6770d8179542dbdad1369b0d2fae14806fdb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 30 May 2026 22:12:41 +0000 Subject: [PATCH 19/23] refactor(httpd): share JSON/API error envelope helpers --- .../internal/httpd/controllers/projects.go | 54 ++++++------------- backend/internal/httpd/envelope/envelope.go | 35 ++++++++++++ backend/internal/httpd/errors.go | 18 ++----- backend/internal/httpd/json.go | 9 ++-- 4 files changed, 56 insertions(+), 60 deletions(-) create mode 100644 backend/internal/httpd/envelope/envelope.go diff --git a/backend/internal/httpd/controllers/projects.go b/backend/internal/httpd/controllers/projects.go index 8cac0f46..d377eeb9 100644 --- a/backend/internal/httpd/controllers/projects.go +++ b/backend/internal/httpd/controllers/projects.go @@ -21,10 +21,10 @@ import ( "net/http" "github.com/go-chi/chi/v5" - "github.com/go-chi/chi/v5/middleware" "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec" + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/envelope" "github.com/aoagents/agent-orchestrator/backend/internal/project" ) @@ -65,7 +65,7 @@ func (c *ProjectsController) list(w http.ResponseWriter, r *http.Request) { writeProjectError(w, r, err, http.StatusInternalServerError) return } - writeJSON(w, http.StatusOK, map[string]any{"projects": projects}) + envelope.WriteJSON(w, http.StatusOK, map[string]any{"projects": projects}) } func (c *ProjectsController) add(w http.ResponseWriter, r *http.Request) { @@ -75,7 +75,7 @@ func (c *ProjectsController) add(w http.ResponseWriter, r *http.Request) { } var in project.AddInput if err := decodeJSON(r, &in); err != nil { - writeAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_JSON", "Invalid JSON body", nil) + envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_JSON", "Invalid JSON body", nil) return } p, err := c.Mgr.Add(r.Context(), in) @@ -83,7 +83,7 @@ func (c *ProjectsController) add(w http.ResponseWriter, r *http.Request) { writeProjectError(w, r, err, http.StatusInternalServerError) return } - writeJSON(w, http.StatusCreated, map[string]any{"project": p}) + envelope.WriteJSON(w, http.StatusCreated, map[string]any{"project": p}) } func (c *ProjectsController) get(w http.ResponseWriter, r *http.Request) { @@ -97,10 +97,10 @@ func (c *ProjectsController) get(w http.ResponseWriter, r *http.Request) { return } if got.Status == "degraded" { - writeJSON(w, http.StatusOK, map[string]any{"status": got.Status, "project": got.Degraded}) + envelope.WriteJSON(w, http.StatusOK, map[string]any{"status": got.Status, "project": got.Degraded}) return } - writeJSON(w, http.StatusOK, map[string]any{"status": got.Status, "project": got.Project}) + envelope.WriteJSON(w, http.StatusOK, map[string]any{"status": got.Status, "project": got.Project}) } func (c *ProjectsController) updateConfig(w http.ResponseWriter, r *http.Request) { @@ -109,16 +109,16 @@ func (c *ProjectsController) updateConfig(w http.ResponseWriter, r *http.Request return } if frozen, err := containsFrozenIdentityField(r); err != nil { - writeAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_JSON", "Invalid JSON body", nil) + envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_JSON", "Invalid JSON body", nil) return } else if len(frozen) > 0 { - writeAPIError(w, r, http.StatusBadRequest, "bad_request", "IDENTITY_FROZEN", "Identity fields cannot be patched", map[string]any{"fields": frozen}) + envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "IDENTITY_FROZEN", "Identity fields cannot be patched", map[string]any{"fields": frozen}) return } var patch project.UpdateConfigInput if err := decodeJSON(r, &patch); err != nil { - writeAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_JSON", "Invalid JSON body", nil) + envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_JSON", "Invalid JSON body", nil) return } p, err := c.Mgr.UpdateConfig(r.Context(), projectID(r), patch) @@ -126,7 +126,7 @@ func (c *ProjectsController) updateConfig(w http.ResponseWriter, r *http.Request writeProjectError(w, r, err, http.StatusInternalServerError) return } - writeJSON(w, http.StatusOK, map[string]any{"project": p}) + envelope.WriteJSON(w, http.StatusOK, map[string]any{"project": p}) } func (c *ProjectsController) remove(w http.ResponseWriter, r *http.Request) { @@ -139,7 +139,7 @@ func (c *ProjectsController) remove(w http.ResponseWriter, r *http.Request) { writeProjectError(w, r, err, http.StatusInternalServerError) return } - writeJSON(w, http.StatusOK, result) + envelope.WriteJSON(w, http.StatusOK, result) } func (c *ProjectsController) repair(w http.ResponseWriter, r *http.Request) { @@ -152,7 +152,7 @@ func (c *ProjectsController) repair(w http.ResponseWriter, r *http.Request) { writeProjectError(w, r, err, http.StatusInternalServerError) return } - writeJSON(w, http.StatusOK, map[string]any{"project": p}) + envelope.WriteJSON(w, http.StatusOK, map[string]any{"project": p}) } func (c *ProjectsController) reload(w http.ResponseWriter, r *http.Request) { @@ -165,7 +165,7 @@ func (c *ProjectsController) reload(w http.ResponseWriter, r *http.Request) { writeProjectError(w, r, err, http.StatusInternalServerError) return } - writeJSON(w, http.StatusOK, result) + envelope.WriteJSON(w, http.StatusOK, result) } func projectID(r *http.Request) domain.ProjectID { @@ -196,24 +196,6 @@ func containsFrozenIdentityField(r *http.Request) ([]string, error) { return frozen, nil } -type apiError struct { - Error string `json:"error"` - Code string `json:"code"` - Message string `json:"message"` - RequestID string `json:"requestId,omitempty"` - Details map[string]any `json:"details,omitempty"` -} - -func writeAPIError(w http.ResponseWriter, r *http.Request, status int, kind, code, message string, details map[string]any) { - writeJSON(w, status, apiError{ - Error: kind, - Code: code, - Message: message, - RequestID: middleware.GetReqID(r.Context()), - Details: details, - }) -} - func writeProjectError(w http.ResponseWriter, r *http.Request, err error, fallbackStatus int) { var pe *project.Error if errors.As(err, &pe) { @@ -228,14 +210,8 @@ func writeProjectError(w http.ResponseWriter, r *http.Request, err error, fallba case "internal": status = http.StatusInternalServerError } - writeAPIError(w, r, status, pe.Kind, pe.Code, pe.Message, pe.Details) + envelope.WriteAPIError(w, r, status, pe.Kind, pe.Code, pe.Message, pe.Details) return } - writeAPIError(w, r, fallbackStatus, "internal", "INTERNAL_ERROR", "Internal server error", nil) -} - -func writeJSON(w http.ResponseWriter, status int, v any) { - w.Header().Set("Content-Type", "application/json; charset=utf-8") - w.WriteHeader(status) - _ = json.NewEncoder(w).Encode(v) + envelope.WriteAPIError(w, r, fallbackStatus, "internal", "INTERNAL_ERROR", "Internal server error", nil) } diff --git a/backend/internal/httpd/envelope/envelope.go b/backend/internal/httpd/envelope/envelope.go new file mode 100644 index 00000000..3e1b2ade --- /dev/null +++ b/backend/internal/httpd/envelope/envelope.go @@ -0,0 +1,35 @@ +package envelope + +import ( + "encoding/json" + "net/http" + + "github.com/go-chi/chi/v5/middleware" +) + +// APIError is the locked wire shape for every non-2xx response. +type APIError struct { + Error string `json:"error"` + Code string `json:"code"` + Message string `json:"message"` + RequestID string `json:"requestId,omitempty"` + Details map[string]any `json:"details,omitempty"` +} + +// WriteJSON serialises v as JSON with the given status. +func WriteJSON(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(v) +} + +// WriteAPIError emits the locked envelope for any non-2xx response. +func WriteAPIError(w http.ResponseWriter, r *http.Request, status int, kind, code, message string, details map[string]any) { + WriteJSON(w, status, APIError{ + Error: kind, + Code: code, + Message: message, + RequestID: middleware.GetReqID(r.Context()), + Details: details, + }) +} diff --git a/backend/internal/httpd/errors.go b/backend/internal/httpd/errors.go index 1843fce8..8b41c99f 100644 --- a/backend/internal/httpd/errors.go +++ b/backend/internal/httpd/errors.go @@ -3,7 +3,7 @@ package httpd import ( "net/http" - "github.com/go-chi/chi/v5/middleware" + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/envelope" ) // APIError is the locked wire shape for every non-2xx response. It supersedes @@ -12,23 +12,11 @@ import ( // // Details is open so collision-style errors can carry typed sub-fields // (e.g. existingProjectId, suggestedProjectId on POST /projects 409s). -type APIError struct { - Error string `json:"error"` // short kind, e.g. "not_found" - Code string `json:"code"` // SCREAMING_SNAKE, e.g. "PROJECT_NOT_FOUND" - Message string `json:"message"` // human-readable - RequestID string `json:"requestId,omitempty"` - Details map[string]any `json:"details,omitempty"` -} +type APIError = envelope.APIError // writeAPIError emits the locked envelope for any non-2xx response. The // request id falls back to empty when the chi middleware hasn't tagged the // request (e.g. in tests that bypass NewRouter). func writeAPIError(w http.ResponseWriter, r *http.Request, status int, kind, code, message string, details map[string]any) { - writeJSON(w, status, APIError{ - Error: kind, - Code: code, - Message: message, - RequestID: middleware.GetReqID(r.Context()), - Details: details, - }) + envelope.WriteAPIError(w, r, status, kind, code, message, details) } diff --git a/backend/internal/httpd/json.go b/backend/internal/httpd/json.go index 9b87461f..64ccb340 100644 --- a/backend/internal/httpd/json.go +++ b/backend/internal/httpd/json.go @@ -1,17 +1,14 @@ package httpd import ( - "encoding/json" "net/http" + + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/envelope" ) // writeJSON serialises v as JSON with the given status. It is the single JSON // writer for the skeleton; the typed error envelope (open item Q1.3) will build // on this in a later phase. func writeJSON(w http.ResponseWriter, status int, v any) { - w.Header().Set("Content-Type", "application/json; charset=utf-8") - w.WriteHeader(status) - // A write error here means the client went away mid-response; there is - // nothing useful to do but stop. - _ = json.NewEncoder(w).Encode(v) + envelope.WriteJSON(w, status, v) } From 323adaf87866f4c27a6e64ae0bcb75e9ef6d6582 Mon Sep 17 00:00:00 2001 From: Vaibhaav Date: Sun, 31 May 2026 11:28:33 +0530 Subject: [PATCH 20/23] fix(api): align project mock store with sqlite schema --- .../httpd/controllers/projects_test.go | 18 ++- backend/internal/project/manager.go | 98 ++++++------- backend/internal/project/memory_store.go | 129 +++++++----------- 3 files changed, 112 insertions(+), 133 deletions(-) diff --git a/backend/internal/httpd/controllers/projects_test.go b/backend/internal/httpd/controllers/projects_test.go index b230d5f2..bb91e86c 100644 --- a/backend/internal/httpd/controllers/projects_test.go +++ b/backend/internal/httpd/controllers/projects_test.go @@ -130,7 +130,7 @@ func TestProjectsAPI_UpdateDeleteRepair(t *testing.T) { Project projectBody `json:"project"` } mustJSON(t, body, &patched) - if patched.Project.Agent != "claude" || patched.Project.Runtime != "tmux" { + if patched.Project.Agent != "" || patched.Project.Runtime != "" { t.Fatalf("patched project = %#v", patched.Project) } @@ -154,7 +154,21 @@ func TestProjectsAPI_UpdateDeleteRepair(t *testing.T) { } body, status, _ = doRequest(t, srv, "GET", "/api/v1/projects/proj", "") - assertErrorCode(t, body, status, http.StatusNotFound, "PROJECT_NOT_FOUND") + if status != http.StatusOK { + t.Fatalf("GET archived project = %d, want 200; body=%s", status, body) + } + + body, status, _ = doRequest(t, srv, "GET", "/api/v1/projects", "") + if status != http.StatusOK { + t.Fatalf("GET projects after archive = %d, want 200; body=%s", status, body) + } + var list struct { + Projects []projectSummary `json:"projects"` + } + mustJSON(t, body, &list) + if len(list.Projects) != 0 { + t.Fatalf("active projects after archive = %d, want 0", len(list.Projects)) + } } func TestProjectsRoutes_LegacyUnregistered(t *testing.T) { diff --git a/backend/internal/project/manager.go b/backend/internal/project/manager.go index 9e7a876e..46144942 100644 --- a/backend/internal/project/manager.go +++ b/backend/internal/project/manager.go @@ -7,6 +7,7 @@ import ( "regexp" "strconv" "strings" + "time" "github.com/aoagents/agent-orchestrator/backend/internal/domain" ) @@ -34,11 +35,11 @@ func (m *manager) List(ctx context.Context) ([]Summary, error) { return nil, internal("PROJECTS_LIST_FAILED", "Failed to load projects") } out := make([]Summary, 0, len(projects)) - for _, p := range projects { + for _, row := range projects { out = append(out, Summary{ - ID: p.ID, - Name: p.Name, - SessionPrefix: sessionPrefix(p.ID), + ID: domain.ProjectID(row.ID), + Name: displayName(row), + SessionPrefix: sessionPrefix(row.ID), }) } return out, nil @@ -48,13 +49,14 @@ func (m *manager) Get(ctx context.Context, id domain.ProjectID) (GetResult, erro if err := validateProjectID(id); err != nil { return GetResult{}, err } - p, ok, err := m.store.Get(ctx, id) + row, ok, err := m.store.Get(ctx, string(id)) if err != nil { return GetResult{}, internal("PROJECT_LOAD_FAILED", "Failed to load project") } if !ok { return GetResult{}, notFound("PROJECT_NOT_FOUND", "Unknown project") } + p := projectFromRow(row) return GetResult{Status: "ok", Project: &p}, nil } @@ -87,37 +89,36 @@ func (m *manager) Add(ctx context.Context, in AddInput) (Project, error) { return Project{}, 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{ - "existingProjectId": string(existing.ID), + "existingProjectId": existing.ID, "suggestedProjectId": string(m.suggestID(ctx, id)), }) } - if existing, ok, err := m.store.Get(ctx, id); err != nil { + if existing, ok, err := m.store.Get(ctx, string(id)); err != nil { return Project{}, internal("PROJECT_LOAD_FAILED", "Failed to load project") } else if ok && existing.Path != path { return Project{}, conflict("ID_ALREADY_REGISTERED", "A project with this id is already registered for a different path", map[string]any{ - "existingProjectId": string(existing.ID), + "existingProjectId": existing.ID, "suggestedProjectId": string(m.suggestID(ctx, id)), }) } - p := Project{ - ID: id, - Name: name, - Path: path, - Repo: "", - DefaultBranch: "main", + row := ProjectRow{ + ID: string(id), + Path: path, + DisplayName: name, + RegisteredAt: time.Now(), } - if err := m.store.Create(ctx, p); err != nil { + if err := m.store.Upsert(ctx, row); err != nil { return Project{}, err } - return p, nil + return projectFromRow(row), nil } func (m *manager) UpdateConfig(ctx context.Context, id domain.ProjectID, patch UpdateConfigInput) (Project, error) { if err := validateProjectID(id); err != nil { return Project{}, err } - p, ok, err := m.store.Get(ctx, id) + row, ok, err := m.store.Get(ctx, string(id)) if err != nil { return Project{}, internal("PROJECT_LOAD_FAILED", "Failed to load project") } @@ -125,35 +126,20 @@ func (m *manager) UpdateConfig(ctx context.Context, id domain.ProjectID, patch U return Project{}, notFound("PROJECT_NOT_FOUND", "Unknown project") } - if patch.Agent != nil { - p.Agent = strings.TrimSpace(*patch.Agent) - } - if patch.Runtime != nil { - p.Runtime = strings.TrimSpace(*patch.Runtime) - } - if patch.Tracker != nil { - tracker := *patch.Tracker - p.Tracker = &tracker - } - if patch.SCM != nil { - scm := *patch.SCM - p.SCM = &scm - } - if patch.Reactions != nil { - p.Reactions = *patch.Reactions - } - - if err := m.store.Update(ctx, p); err != nil { + // The PR #37 schema stores only registry identity columns. Behaviour config + // fields are accepted for API compatibility, but are not durable until a + // dedicated config store is wired in. + if err := m.store.Upsert(ctx, row); err != nil { return Project{}, err } - return p, nil + return projectFromRow(row), nil } func (m *manager) Remove(ctx context.Context, id domain.ProjectID) (RemoveResult, error) { if err := validateProjectID(id); err != nil { return RemoveResult{}, err } - ok, err := m.store.Delete(ctx, id) + ok, err := m.store.Archive(ctx, string(id), time.Now()) if err != nil { return RemoveResult{}, internal("PROJECT_REMOVE_FAILED", "Failed to remove project") } @@ -167,7 +153,7 @@ func (m *manager) Repair(ctx context.Context, id domain.ProjectID) (Project, err if err := validateProjectID(id); err != nil { return Project{}, err } - if _, ok, err := m.store.Get(ctx, id); err != nil { + if _, ok, err := m.store.Get(ctx, string(id)); err != nil { return Project{}, internal("PROJECT_LOAD_FAILED", "Failed to load project") } else if !ok { return Project{}, notFound("PROJECT_NOT_FOUND", "Unknown project") @@ -184,14 +170,31 @@ func (m *manager) Reload(ctx context.Context) (ReloadResult, error) { } func (m *manager) suggestID(ctx context.Context, base domain.ProjectID) domain.ProjectID { - for i := 2; ; i++ { - candidate := domain.ProjectID(string(base) + "-" + strconv.Itoa(i)) - if _, ok, _ := m.store.Get(ctx, candidate); !ok { + for i := 1; ; i++ { + candidate := domain.ProjectID(string(base) + strconv.Itoa(i)) + if _, ok, _ := m.store.Get(ctx, string(candidate)); !ok { return candidate } } } +func projectFromRow(row ProjectRow) Project { + return Project{ + ID: domain.ProjectID(row.ID), + Name: displayName(row), + Path: row.Path, + Repo: row.RepoOriginURL, + DefaultBranch: "main", + } +} + +func displayName(row ProjectRow) string { + if strings.TrimSpace(row.DisplayName) != "" { + return row.DisplayName + } + return row.ID +} + func normalizePath(raw string) (string, error) { raw = strings.TrimSpace(raw) if raw == "" { @@ -237,13 +240,12 @@ func validateProjectID(id domain.ProjectID) error { return nil } -func sessionPrefix(id domain.ProjectID) string { - raw := string(id) - if raw == "" { +func sessionPrefix(id string) string { + if id == "" { return "ao" } - if len(raw) <= 12 { - return raw + if len(id) <= 12 { + return id } - return raw[:12] + return id[:12] } diff --git a/backend/internal/project/memory_store.go b/backend/internal/project/memory_store.go index 563ccd7e..945a7826 100644 --- a/backend/internal/project/memory_store.go +++ b/backend/internal/project/memory_store.go @@ -3,143 +3,106 @@ package project import ( "context" "sync" - - "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "time" ) +// ProjectRow mirrors the project table shape from the sqlite storage PR. The +// memory store is intentionally row-based so the API layer does not depend on a +// richer mock model than the real DB will provide. +type ProjectRow struct { + ID string + Path string + RepoOriginURL string + DisplayName string + RegisteredAt time.Time + ArchivedAt time.Time +} + type Store interface { - List(ctx context.Context) ([]Project, error) - Get(ctx context.Context, id domain.ProjectID) (Project, bool, error) - FindByPath(ctx context.Context, path string) (Project, bool, error) - Create(ctx context.Context, p Project) error - Update(ctx context.Context, p Project) error - Delete(ctx context.Context, id domain.ProjectID) (bool, error) + List(ctx context.Context) ([]ProjectRow, error) + Get(ctx context.Context, id string) (ProjectRow, bool, error) + FindByPath(ctx context.Context, path string) (ProjectRow, bool, error) + Upsert(ctx context.Context, row ProjectRow) error + Archive(ctx context.Context, id string, at time.Time) (bool, error) } // MemoryStore is the mocked DB layer for the project API implementation. It is // process-local and intentionally small, but concurrency-safe for HTTP tests. type MemoryStore struct { mu sync.Mutex - projects map[domain.ProjectID]Project - paths map[string]domain.ProjectID + projects map[string]ProjectRow + paths map[string]string } var _ Store = (*MemoryStore)(nil) func NewMemoryStore() *MemoryStore { return &MemoryStore{ - projects: map[domain.ProjectID]Project{}, - paths: map[string]domain.ProjectID{}, + projects: map[string]ProjectRow{}, + paths: map[string]string{}, } } -func (s *MemoryStore) List(context.Context) ([]Project, error) { +func (s *MemoryStore) List(context.Context) ([]ProjectRow, error) { s.mu.Lock() defer s.mu.Unlock() - out := make([]Project, 0, len(s.projects)) - for _, p := range s.projects { - out = append(out, cloneProject(p)) + out := make([]ProjectRow, 0, len(s.projects)) + for _, row := range s.projects { + if row.ArchivedAt.IsZero() { + out = append(out, row) + } } return out, nil } -func (s *MemoryStore) Get(_ context.Context, id domain.ProjectID) (Project, bool, error) { +func (s *MemoryStore) Get(_ context.Context, id string) (ProjectRow, bool, error) { s.mu.Lock() defer s.mu.Unlock() - p, ok := s.projects[id] + row, ok := s.projects[id] if !ok { - return Project{}, false, nil + return ProjectRow{}, false, nil } - return cloneProject(p), true, nil + return row, true, nil } -func (s *MemoryStore) FindByPath(_ context.Context, path string) (Project, bool, error) { +func (s *MemoryStore) FindByPath(_ context.Context, path string) (ProjectRow, bool, error) { s.mu.Lock() defer s.mu.Unlock() id, ok := s.paths[path] if !ok { - return Project{}, false, nil + return ProjectRow{}, false, nil } - p, ok := s.projects[id] + row, ok := s.projects[id] if !ok { - return Project{}, false, nil + return ProjectRow{}, false, nil } - return cloneProject(p), true, nil + return row, true, nil } -func (s *MemoryStore) Create(_ context.Context, p Project) error { +func (s *MemoryStore) Upsert(_ context.Context, row ProjectRow) error { s.mu.Lock() defer s.mu.Unlock() - if _, ok := s.projects[p.ID]; ok { - return conflict("ID_ALREADY_REGISTERED", "A project with this id is already registered for a different path", nil) - } - if existingID, ok := s.paths[p.Path]; ok { - return conflict("PATH_ALREADY_REGISTERED", "A project at this path is already registered", map[string]any{ - "existingProjectId": string(existingID), - }) + if existing, ok := s.projects[row.ID]; ok && existing.Path != row.Path { + delete(s.paths, existing.Path) } - s.projects[p.ID] = cloneProject(p) - s.paths[p.Path] = p.ID + s.projects[row.ID] = row + s.paths[row.Path] = row.ID return nil } -func (s *MemoryStore) Update(_ context.Context, p Project) error { +func (s *MemoryStore) Archive(_ context.Context, id string, at time.Time) (bool, error) { s.mu.Lock() defer s.mu.Unlock() - prev, ok := s.projects[p.ID] - if !ok { - return notFound("PROJECT_NOT_FOUND", "Unknown project") - } - if prev.Path != "" && prev.Path != p.Path { - delete(s.paths, prev.Path) - } - s.projects[p.ID] = cloneProject(p) - s.paths[p.Path] = p.ID - return nil -} - -func (s *MemoryStore) Delete(_ context.Context, id domain.ProjectID) (bool, error) { - s.mu.Lock() - defer s.mu.Unlock() - - p, ok := s.projects[id] + row, ok := s.projects[id] if !ok { return false, nil } - delete(s.projects, id) - delete(s.paths, p.Path) + row.ArchivedAt = at + s.projects[id] = row return true, nil } - -func cloneProject(p Project) Project { - if p.Tracker != nil { - tracker := *p.Tracker - p.Tracker = &tracker - } - if p.SCM != nil { - scm := *p.SCM - if p.SCM.Webhook != nil { - webhook := *p.SCM.Webhook - scm.Webhook = &webhook - } - p.SCM = &scm - } - if p.Reactions != nil { - reactions := make(map[string]*ReactionConfig, len(p.Reactions)) - for k, v := range p.Reactions { - if v == nil { - reactions[k] = nil - continue - } - reaction := *v - reactions[k] = &reaction - } - p.Reactions = reactions - } - return p -} From d46bf7745ff7a41f441fff8411f7137703097197 Mon Sep 17 00:00:00 2001 From: Vaibhaav Date: Sun, 31 May 2026 14:34:08 +0530 Subject: [PATCH 21/23] fix(api): address project API review semantics --- backend/internal/httpd/api.go | 3 -- backend/internal/httpd/apispec/openapi.yaml | 38 ++++++++++--------- .../internal/httpd/controllers/projects.go | 2 + .../httpd/controllers/projects_test.go | 32 ++++++++++------ backend/internal/httpd/router.go | 4 +- backend/internal/project/errors.go | 4 ++ backend/internal/project/manager.go | 26 +++++++------ 7 files changed, 63 insertions(+), 46 deletions(-) diff --git a/backend/internal/httpd/api.go b/backend/internal/httpd/api.go index c02773fe..124a8d78 100644 --- a/backend/internal/httpd/api.go +++ b/backend/internal/httpd/api.go @@ -39,9 +39,6 @@ type API struct { // per-request timeout so the REST group can apply it without re-reading the // environment. func NewAPI(cfg config.Config, deps APIDeps) *API { - if deps.Projects == nil { - deps.Projects = project.NewMemoryManager() - } return &API{ cfg: cfg, projects: &controllers.ProjectsController{ diff --git a/backend/internal/httpd/apispec/openapi.yaml b/backend/internal/httpd/apispec/openapi.yaml index 30005ca4..2b60a3a5 100644 --- a/backend/internal/httpd/apispec/openapi.yaml +++ b/backend/internal/httpd/apispec/openapi.yaml @@ -22,7 +22,7 @@ paths: get: operationId: listProjects tags: [projects] - summary: List all registered projects (active + degraded) + summary: List active registered projects responses: "200": description: Projects listed @@ -138,26 +138,19 @@ paths: x-rest-audit-notes: | R5: degraded projects return 200 with a `status` discriminator instead of 200 with an `error` field (as the legacy TS surface did). + Archived projects are hidden from list responses but still resolve by + id so historical sessions can keep their project_id reference. patch: operationId: updateProjectConfig tags: [projects] - summary: Patch behaviour-only fields (identity is frozen) + summary: Patch behaviour-only fields (not implemented until config persistence lands) requestBody: required: true content: application/json: schema: { $ref: "#/components/schemas/UpdateProjectConfigRequest" } responses: - "200": - description: Project updated - content: - application/json: - schema: - type: object - required: [project] - properties: - project: { $ref: "#/components/schemas/Project" } "400": description: Bad request content: @@ -181,19 +174,26 @@ paths: examples: degraded: { value: { error: conflict, code: PROJECT_DEGRADED, message: "Project config is degraded; repair before patching" } } missingPath: { value: { error: conflict, code: PROJECT_MISSING_PATH, message: "Project registry entry is missing a path" } } - "501": { $ref: "#/components/responses/NotImplemented" } + "501": + description: Behaviour config persistence is not wired yet + content: + application/json: + schema: { $ref: "#/components/schemas/APIError" } + example: { error: not_implemented, code: PROJECT_CONFIG_NOT_IMPLEMENTED, message: "Project config patching is not available until config persistence is wired" } x-rest-audit-notes: | R3: legacy `PUT /projects/{id}` (a TS alias of PATCH) is NOT registered. PUT returns 405 Method Not Allowed. - R6: returns { project }, not { ok: true }. + R6: when config persistence lands this route returns { project }, not + { ok: true }. Until then, config patches return 501 instead of + pretending to persist fields the current project store cannot store. delete: operationId: removeProject tags: [projects] - summary: Remove a project; stops sessions, cleans workspaces, unregisters + summary: Archive a project; hides it from active lists while preserving id references responses: "200": - description: Project removed + description: Project archived content: application/json: schema: { $ref: "#/components/schemas/RemoveProjectResult" } @@ -368,7 +368,9 @@ components: type: object description: | Behaviour-only patch. Identity fields (projectId, path, repo, - defaultBranch) are rejected with 400 IDENTITY_FROZEN. + defaultBranch) are rejected with 400 IDENTITY_FROZEN. The current Go + handler returns 501 PROJECT_CONFIG_NOT_IMPLEMENTED until config + persistence exists. properties: agent: { type: string } runtime: { type: string } @@ -394,8 +396,8 @@ components: degradedCount: { type: integer } # ---- Behaviour config blobs (ported from the TS Zod schemas) ---- - # All carry .passthrough() semantics — extra keys are preserved on - # the wire and re-emitted by the manager impl. + # These are the known config shapes only. The current Go handler does not + # preserve unknown passthrough keys until config persistence is implemented. TrackerConfig: type: object diff --git a/backend/internal/httpd/controllers/projects.go b/backend/internal/httpd/controllers/projects.go index d377eeb9..8fa9db1f 100644 --- a/backend/internal/httpd/controllers/projects.go +++ b/backend/internal/httpd/controllers/projects.go @@ -207,6 +207,8 @@ func writeProjectError(w http.ResponseWriter, r *http.Request, err error, fallba status = http.StatusNotFound case "conflict": status = http.StatusConflict + case "not_implemented": + status = http.StatusNotImplemented case "internal": status = http.StatusInternalServerError } diff --git a/backend/internal/httpd/controllers/projects_test.go b/backend/internal/httpd/controllers/projects_test.go index bb91e86c..4a6d19e1 100644 --- a/backend/internal/httpd/controllers/projects_test.go +++ b/backend/internal/httpd/controllers/projects_test.go @@ -7,22 +7,36 @@ import ( "net/http" "net/http/httptest" "os" + "os/exec" "path/filepath" "strings" "testing" "github.com/aoagents/agent-orchestrator/backend/internal/config" "github.com/aoagents/agent-orchestrator/backend/internal/httpd" + "github.com/aoagents/agent-orchestrator/backend/internal/project" ) func newTestServer(t *testing.T) *httptest.Server { t.Helper() log := slog.New(slog.NewTextHandler(io.Discard, nil)) - srv := httptest.NewServer(httpd.NewRouter(config.Config{}, log)) + srv := httptest.NewServer(httpd.NewRouterWithAPI(config.Config{}, log, httpd.APIDeps{ + Projects: project.NewMemoryManager(), + })) t.Cleanup(srv.Close) return srv } +func TestProjectsRoutes_DefaultToStubsWithoutManager(t *testing.T) { + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + srv := httptest.NewServer(httpd.NewRouter(config.Config{}, log)) + t.Cleanup(srv.Close) + + body, status, headers := doRequest(t, srv, "GET", "/api/v1/projects", "") + assertJSON(t, headers) + assertErrorCode(t, body, status, http.StatusNotImplemented, "NOT_IMPLEMENTED") +} + func TestProjectsAPI_ListAddGetReload(t *testing.T) { srv := newTestServer(t) repo := gitRepo(t, "agent-orchestrator") @@ -123,16 +137,7 @@ func TestProjectsAPI_UpdateDeleteRepair(t *testing.T) { } body, status, _ = doRequest(t, srv, "PATCH", "/api/v1/projects/proj", `{"agent":"claude","runtime":"tmux"}`) - if status != http.StatusOK { - t.Fatalf("PATCH = %d, want 200; body=%s", status, body) - } - var patched struct { - Project projectBody `json:"project"` - } - mustJSON(t, body, &patched) - if patched.Project.Agent != "" || patched.Project.Runtime != "" { - t.Fatalf("patched project = %#v", patched.Project) - } + assertErrorCode(t, body, status, http.StatusNotImplemented, "PROJECT_CONFIG_NOT_IMPLEMENTED") body, status, _ = doRequest(t, srv, "PATCH", "/api/v1/projects/proj", `{"path":"elsewhere"}`) assertErrorCode(t, body, status, http.StatusBadRequest, "IDENTITY_FROZEN") @@ -265,9 +270,12 @@ func doRequest(t *testing.T, srv *httptest.Server, method, path, body string) ([ func gitRepo(t *testing.T, name string) string { t.Helper() dir := filepath.Join(t.TempDir(), name) - if err := os.MkdirAll(filepath.Join(dir, ".git"), 0o755); err != nil { + if err := os.MkdirAll(dir, 0o755); err != nil { t.Fatalf("create git repo fixture: %v", err) } + if out, err := exec.Command("git", "init", dir).CombinedOutput(); err != nil { + t.Fatalf("git init fixture: %v\n%s", err, out) + } return dir } diff --git a/backend/internal/httpd/router.go b/backend/internal/httpd/router.go index e4c5b570..1a076c61 100644 --- a/backend/internal/httpd/router.go +++ b/backend/internal/httpd/router.go @@ -34,8 +34,8 @@ func NewRouter(cfg config.Config, log *slog.Logger) chi.Router { } // NewRouterWithAPI is the dependency-injected variant. main.go calls it with -// real Managers; tests and the zero-dep NewRouter call it with empty APIDeps -// so route-shell 501 stubs work without wiring every port. +// real Managers when they exist; tests/dev wiring inject mocks explicitly. +// Missing Managers intentionally keep the route-shell 501 behavior. func NewRouterWithAPI(cfg config.Config, log *slog.Logger, deps APIDeps) chi.Router { r := chi.NewRouter() diff --git a/backend/internal/project/errors.go b/backend/internal/project/errors.go index c4e29d8c..f6687e1f 100644 --- a/backend/internal/project/errors.go +++ b/backend/internal/project/errors.go @@ -32,6 +32,10 @@ func conflict(code, message string, details map[string]any) *Error { return newError("conflict", code, message, details) } +func notImplemented(code, message string) *Error { + return newError("not_implemented", code, message, nil) +} + func internal(code, message string) *Error { return newError("internal", code, message, nil) } diff --git a/backend/internal/project/manager.go b/backend/internal/project/manager.go index 46144942..9ad33a3a 100644 --- a/backend/internal/project/manager.go +++ b/backend/internal/project/manager.go @@ -3,6 +3,7 @@ package project import ( "context" "os" + "os/exec" "path/filepath" "regexp" "strconv" @@ -114,11 +115,11 @@ func (m *manager) Add(ctx context.Context, in AddInput) (Project, error) { return projectFromRow(row), nil } -func (m *manager) UpdateConfig(ctx context.Context, id domain.ProjectID, patch UpdateConfigInput) (Project, error) { +func (m *manager) UpdateConfig(ctx context.Context, id domain.ProjectID, _ UpdateConfigInput) (Project, error) { if err := validateProjectID(id); err != nil { return Project{}, err } - row, ok, err := m.store.Get(ctx, string(id)) + _, ok, err := m.store.Get(ctx, string(id)) if err != nil { return Project{}, internal("PROJECT_LOAD_FAILED", "Failed to load project") } @@ -126,13 +127,7 @@ func (m *manager) UpdateConfig(ctx context.Context, id domain.ProjectID, patch U return Project{}, notFound("PROJECT_NOT_FOUND", "Unknown project") } - // The PR #37 schema stores only registry identity columns. Behaviour config - // fields are accepted for API compatibility, but are not durable until a - // dedicated config store is wired in. - if err := m.store.Upsert(ctx, row); err != nil { - return Project{}, err - } - return projectFromRow(row), nil + return Project{}, notImplemented("PROJECT_CONFIG_NOT_IMPLEMENTED", "Project config patching is not available until config persistence is wired") } func (m *manager) Remove(ctx context.Context, id domain.ProjectID) (RemoveResult, error) { @@ -219,8 +214,17 @@ func normalizePath(raw string) (string, error) { } func isGitRepo(path string) bool { - info, err := os.Stat(filepath.Join(path, ".git")) - return err == nil && (info.IsDir() || info.Mode().IsRegular()) + cmd := exec.Command("git", "-C", path, "rev-parse", "--show-toplevel") + out, err := cmd.Output() + if err != nil { + return false + } + top := filepath.Clean(strings.TrimSpace(string(out))) + path = filepath.Clean(path) + if strings.EqualFold(top, path) { + return true + } + return top == path } func defaultProjectID(path string) domain.ProjectID { From 0b0b136de4846ee62909d21e082ca4f4225fd8a7 Mon Sep 17 00:00:00 2001 From: Vaibhaav Tiwari <155460282+Vaibhaav-Tiwari@users.noreply.github.com> Date: Sun, 31 May 2026 18:47:02 +0530 Subject: [PATCH 22/23] canonicalize both paths with filepath.EvalSymlinks before comparing --- backend/internal/project/manager.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/backend/internal/project/manager.go b/backend/internal/project/manager.go index 9ad33a3a..7c203621 100644 --- a/backend/internal/project/manager.go +++ b/backend/internal/project/manager.go @@ -221,6 +221,15 @@ func isGitRepo(path string) bool { } top := filepath.Clean(strings.TrimSpace(string(out))) path = filepath.Clean(path) + top, err = filepath.EvalSymlinks(top) + if err != nil { + return false + } + path, err = filepath.EvalSymlinks(path) + if err != nil { + return false + } + if strings.EqualFold(top, path) { return true } From 5046ea81cdd42186f74ed74a238d237f3ed42ee7 Mon Sep 17 00:00:00 2001 From: Vaibhaav Date: Sun, 31 May 2026 20:01:23 +0530 Subject: [PATCH 23/23] style(project): gofmt git repo validation --- backend/internal/project/manager.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/backend/internal/project/manager.go b/backend/internal/project/manager.go index 7c203621..93ca84d9 100644 --- a/backend/internal/project/manager.go +++ b/backend/internal/project/manager.go @@ -222,13 +222,13 @@ func isGitRepo(path string) bool { top := filepath.Clean(strings.TrimSpace(string(out))) path = filepath.Clean(path) top, err = filepath.EvalSymlinks(top) - if err != nil { - return false - } - path, err = filepath.EvalSymlinks(path) - if err != nil { - return false - } + if err != nil { + return false + } + path, err = filepath.EvalSymlinks(path) + if err != nil { + return false + } if strings.EqualFold(top, path) { return true