From 57bb63701d199201e1cf461fc0fa6bc8f154c48d Mon Sep 17 00:00:00 2001 From: yyovil Date: Tue, 2 Jun 2026 18:39:13 +0530 Subject: [PATCH] Add `ao spawn` + `ao project add` (spawn a real worker end-to-end) (#77) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add `ao spawn` and `ao project add`; resolve project repos for worktrees Make a registered project spawnable end-to-end from the CLI: - DB-backed RepoResolver: the daemon resolves a project's on-disk repo path from the projects table (replacing the empty StaticRepoResolver that failed every lookup), so a session's worktree is cut from the right repo. - session_manager defaults an empty spawn branch to ao/ — a fresh, unique branch per session, since gitworktree can't reuse a branch already checked out elsewhere (e.g. main). - `ao project add --path `: register a local git repo (POST /api/v1/projects). - `ao spawn --project [--harness] [--branch] [--prompt] [--issue]`: spawn a worker session (POST /api/v1/sessions); harness defaults to the daemon's AO_AGENT. - Shared postJSON daemon client (reads the run-file for the port, surfaces the API error envelope). Stacked on #65, which lands the agent-adapter + session-manager wiring this depends on. Co-Authored-By: Claude Opus 4.8 (1M context) * Address Copilot review on #77 - `ao spawn` no longer prints a branch the sessions API doesn't return (session metadata is json:"-"), so the output is no longer misleading. - Unregistered/archived/no-path projects now surface a 400 PROJECT_NOT_RESOLVABLE with an actionable message instead of a generic 500: a new sessionmanager.ErrProjectNotResolvable sentinel the resolver wraps and writeSessionError maps. - postJSON reuses the injected Deps.HTTPClient (cloned, with a longer timeout) instead of a fresh client, keeping HTTP behaviour stubbable. - postJSON treats a stale run-file (dead PID) as "not running" via ProcessAlive, matching its docstring. Co-Authored-By: Claude Opus 4.8 (1M context) * Assert the project-not-resolvable sentinel in the resolver test Greptile review: harden TestProjectRepoResolver to verify the unregistered -project error wraps ErrProjectNotResolvable, so a future regression in the sentinel wrapping (which the HTTP 400 mapping relies on) is caught. Co-Authored-By: Claude Opus 4.8 (1M context) * Fix ao spawn 500 on long session ids (zellij socket-path overflow) Root cause: the daemon built the zellij runtime with an empty SocketDir, so zellij fell back to its $TMPDIR-based default (long on macOS). That left almost none of the ~103-byte unix-socket-path budget for the session name, so a long session id (e.g. "aoagents-agent-orchestrator-1", derived from a long project id) was rejected by zellij with "session name must be less than 0 characters". runtime.Create failed, the spawn 500'd, and the worktree was rolled back (leaving an orphan ao/ branch). - New zellij.DefaultSocketDir(): a short, stable per-user socket dir (/tmp/ao-zellij-); the daemon uses it (and MkdirAll's it). - ao spawn's attach hint now prefixes ZELLIJ_SOCKET_DIR so it stays copy-pasteable against the daemon's socket dir. - Regression test guards that the socket dir leaves >= 48 bytes for the session name within the 103-byte limit. Verified: ao spawn against a long-id project now succeeds (session live, worktree created) where it previously 500'd. Co-Authored-By: Claude Opus 4.8 (1M context) * test(cli): guard CLI/daemon DTO drift with an e2e round-trip The CLI keeps its own request structs (spawnRequest, addProjectRequest) separate from the daemon's canonical DTOs (controllers.SpawnSessionRequest, project.AddInput). Nothing verified the JSON field names agreed, so a renamed tag on either side would compile but break at runtime. Drive `ao spawn` and `ao project add` through the real httpd router and controllers (fakes only at the service layer) over a real loopback round trip via postJSON, asserting each field decodes into the right SpawnConfig/AddInput field. Runs in the normal test lane (no extra ports/processes). Co-Authored-By: Claude Opus 4.7 * fix(cli,daemon): address review findings on ao spawn - spawn: print the sanitised zellij session name (zellij.SessionName) in the attach hint; a long/non-conforming session id is registered under a different name, so the raw id sent users to a missing session. - client: surface the daemon error envelope's requestId so a failed command can be correlated with daemon logs. - daemon: don't swallow the zellij socket-dir MkdirAll error — log it, since a failure otherwise surfaces later as an opaque socket-bind error on every spawn. - project: reject an embedded ".." in a project id up front; it passed the id pattern but yielded an invalid branch (ao/a..b-1) and an opaque 500 at spawn. Co-Authored-By: Claude Opus 4.7 --------- Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: harshitsinghbhandari <24b4506@iitb.ac.in> --- .../adapters/runtime/zellij/zellij.go | 27 ++- .../adapters/runtime/zellij/zellij_test.go | 25 ++ backend/internal/cli/client.go | 97 ++++++++ backend/internal/cli/client_test.go | 25 ++ backend/internal/cli/dto_drift_e2e_test.go | 215 ++++++++++++++++++ backend/internal/cli/project.go | 71 ++++++ backend/internal/cli/root.go | 2 + backend/internal/cli/spawn.go | 88 +++++++ backend/internal/cli/spawn_test.go | 37 +++ backend/internal/daemon/daemon.go | 12 +- backend/internal/daemon/lifecycle_wiring.go | 33 ++- backend/internal/daemon/wiring_test.go | 55 +++++ .../internal/httpd/controllers/sessions.go | 2 + backend/internal/service/project/service.go | 5 +- .../internal/service/project/service_test.go | 5 + backend/internal/session_manager/manager.go | 12 +- .../internal/session_manager/manager_test.go | 23 ++ 17 files changed, 724 insertions(+), 10 deletions(-) create mode 100644 backend/internal/cli/client.go create mode 100644 backend/internal/cli/client_test.go create mode 100644 backend/internal/cli/dto_drift_e2e_test.go create mode 100644 backend/internal/cli/project.go create mode 100644 backend/internal/cli/spawn.go create mode 100644 backend/internal/cli/spawn_test.go diff --git a/backend/internal/adapters/runtime/zellij/zellij.go b/backend/internal/adapters/runtime/zellij/zellij.go index f30a21ecf..536ad28a7 100644 --- a/backend/internal/adapters/runtime/zellij/zellij.go +++ b/backend/internal/adapters/runtime/zellij/zellij.go @@ -58,6 +58,19 @@ type Runtime struct { var _ ports.Runtime = (*Runtime)(nil) +// DefaultSocketDir returns a short, stable ZELLIJ_SOCKET_DIR for AO's daemon. +// zellij's own default lives under $TMPDIR (long on macOS), which leaves almost +// none of the ~103-byte unix-socket-path budget for the session name — a long +// session id then fails with "session name must be less than 0 characters". A +// short dir restores ample budget. Empty on Windows, where zellij is not used. +// Pure: callers that run zellij should MkdirAll the result. +func DefaultSocketDir() string { + if runtime.GOOS == "windows" { + return "" + } + return "/tmp/ao-zellij-" + strconv.Itoa(os.Getuid()) +} + type runner interface { Run(ctx context.Context, env []string, name string, args ...string) ([]byte, error) } @@ -330,10 +343,18 @@ func zellijSessionName(id domain.SessionID) (string, error) { if raw == "" { return "", errors.New("zellij runtime: session id is required") } - if sessionIDPattern.MatchString(raw) && len(raw) <= 48 { - return raw, nil + return SessionName(raw), nil +} + +// SessionName returns the zellij session name the runtime registers for a given +// session id — applying the same sanitisation Create does. Callers that print an +// attach hint (e.g. `ao spawn`) must use this rather than the raw id, since a +// long or non-conforming id maps to a different, sanitised session name. +func SessionName(id string) string { + if sessionIDPattern.MatchString(id) && len(id) <= 48 { + return id } - return sanitizedSessionName(raw), nil + return sanitizedSessionName(id) } func sanitizedSessionName(raw string) string { diff --git a/backend/internal/adapters/runtime/zellij/zellij_test.go b/backend/internal/adapters/runtime/zellij/zellij_test.go index c5ef9b1c1..22e2e5092 100644 --- a/backend/internal/adapters/runtime/zellij/zellij_test.go +++ b/backend/internal/adapters/runtime/zellij/zellij_test.go @@ -10,6 +10,7 @@ import ( "testing" "time" + "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) @@ -59,6 +60,30 @@ func TestZellijSessionNameSanitizesIssueRefs(t *testing.T) { } } +// SessionName must return the exact name Create registers a session under, so +// callers that print an attach hint (e.g. `ao spawn`) reference the real +// session. A short, conforming id passes through; a long one is sanitised to a +// different name — printing the raw id there would send users to a missing +// session. +func TestSessionNameMatchesCreateNaming(t *testing.T) { + short := "myproj-1" + if got := SessionName(short); got != short { + t.Fatalf("SessionName(%q) = %q, want it unchanged", short, got) + } + + long := domain.SessionID(strings.Repeat("x", 60) + "-1") + viaCreate, err := zellijSessionName(long) + if err != nil { + t.Fatalf("zellijSessionName: %v", err) + } + if got := SessionName(string(long)); got != viaCreate { + t.Fatalf("SessionName = %q, but Create uses %q", got, viaCreate) + } + if SessionName(string(long)) == string(long) { + t.Fatal("expected a long id to be sanitised to a different name") + } +} + func TestValidateSessionAndPaneID(t *testing.T) { for _, id := range []string{"sess-1", "S_2", "abc123"} { if err := validateSessionID(id); err != nil { diff --git a/backend/internal/cli/client.go b/backend/internal/cli/client.go new file mode 100644 index 000000000..e6de41fe6 --- /dev/null +++ b/backend/internal/cli/client.go @@ -0,0 +1,97 @@ +package cli + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "net/http" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/config" + "github.com/aoagents/agent-orchestrator/backend/internal/runfile" +) + +// commandTimeout bounds a mutating daemon call. Spawns do real work (git +// worktree add, zellij launch, hook install), so it is generous compared to the +// status probe timeout. +const commandTimeout = 2 * time.Minute + +// apiError is the subset of the daemon's JSON error envelope the CLI surfaces. +// RequestID is surfaced so a failed command can be correlated with daemon logs. +type apiError struct { + Message string `json:"message"` + Code string `json:"code"` + RequestID string `json:"requestId"` +} + +// String renders the envelope for the user: " () [request ]", +// omitting whichever parts the daemon left empty. +func (e apiError) String() string { + msg := e.Message + if e.Code != "" { + msg = fmt.Sprintf("%s (%s)", msg, e.Code) + } + if e.RequestID != "" { + msg = fmt.Sprintf("%s [request %s]", msg, e.RequestID) + } + return msg +} + +// postJSON sends body as JSON to POST /api/v1/ on the running daemon and +// decodes a 2xx response into out (out may be nil). A non-2xx response becomes +// an error built from the API error envelope. A missing run-file or a stale one +// (dead PID) yields a clear "not running" message rather than a +// connection-refused dump. +func (c *commandContext) postJSON(ctx context.Context, path string, body, out any) error { + cfg, err := config.Load() + if err != nil { + return err + } + info, err := runfile.Read(cfg.RunFilePath) + if err != nil { + return err + } + if info == nil { + return fmt.Errorf("AO daemon is not running — start it with `ao start`") + } + if !c.deps.ProcessAlive(info.PID) { + return fmt.Errorf("AO daemon is not running (stale run-file at %s) — start it with `ao start`", cfg.RunFilePath) + } + + payload, err := json.Marshal(body) + if err != nil { + return err + } + url := fmt.Sprintf("http://%s:%d/api/v1/%s", config.LoopbackHost, info.Port, path) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(payload)) + if err != nil { + return err + } + req.Header.Set("Content-Type", "application/json") + + // Reuse the injected client's transport (keeps it stubbable in tests) but + // give mutating calls far more headroom than the 2s status-probe timeout. + client := *c.deps.HTTPClient + client.Timeout = commandTimeout + resp, err := client.Do(req) + if err != nil { + return fmt.Errorf("call daemon: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + var e apiError + _ = json.NewDecoder(resp.Body).Decode(&e) + if e.Message == "" { + return fmt.Errorf("daemon returned HTTP %d", resp.StatusCode) + } + return fmt.Errorf("%s", e.String()) + } + if out != nil { + if err := json.NewDecoder(resp.Body).Decode(out); err != nil { + return fmt.Errorf("decode response: %w", err) + } + } + return nil +} diff --git a/backend/internal/cli/client_test.go b/backend/internal/cli/client_test.go new file mode 100644 index 000000000..eb07dee34 --- /dev/null +++ b/backend/internal/cli/client_test.go @@ -0,0 +1,25 @@ +package cli + +import "testing" + +// TestAPIErrorString covers how the CLI renders the daemon's error envelope, +// including the requestId it now surfaces for log correlation. +func TestAPIErrorString(t *testing.T) { + cases := []struct { + name string + in apiError + want string + }{ + {"message only", apiError{Message: "boom"}, "boom"}, + {"message and code", apiError{Message: "boom", Code: "X"}, "boom (X)"}, + {"with request id", apiError{Message: "boom", Code: "X", RequestID: "req-1"}, "boom (X) [request req-1]"}, + {"message and request id", apiError{Message: "boom", RequestID: "req-1"}, "boom [request req-1]"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := tc.in.String(); got != tc.want { + t.Fatalf("String() = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/backend/internal/cli/dto_drift_e2e_test.go b/backend/internal/cli/dto_drift_e2e_test.go new file mode 100644 index 000000000..07c10a656 --- /dev/null +++ b/backend/internal/cli/dto_drift_e2e_test.go @@ -0,0 +1,215 @@ +package cli + +// dto_drift_e2e_test.go is the DTO-drift guard for the `ao spawn` and +// `ao project add` commands. The CLI defines its OWN request structs +// (spawnRequest in spawn.go, addProjectRequest in project.go) that are separate +// copies of the daemon's canonical request DTOs (controllers.SpawnSessionRequest +// and project.AddInput). Nothing else verifies the two sides agree on JSON field +// names — a renamed `json:"..."` tag on either side compiles fine but silently +// breaks at runtime. +// +// This test stands up the REAL daemon HTTP router + REAL controllers (with fakes +// only BELOW the controller, at the service layer) and drives the actual CLI +// commands through the actual postJSON client over a real loopback HTTP round +// trip. If the CLI's JSON field names diverge from what the controllers decode, +// the captured values are wrong/empty and the subtests fail. +// +// (This lives in a separate file from the build-tagged e2e_test.go so it runs in +// the normal `go test ./...` lane — it binds no extra ports beyond httptest and +// spawns no processes.) + +import ( + "bytes" + "context" + "io" + "log/slog" + "net" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/config" + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/httpd" + "github.com/aoagents/agent-orchestrator/backend/internal/httpd/controllers" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" + "github.com/aoagents/agent-orchestrator/backend/internal/runfile" + projectsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/project" + sessionsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/session" +) + +// fakeSessionService captures the ports.SpawnConfig the controller decodes from +// the CLI's request body. Every other method is a no-op so it satisfies the +// controllers.SessionService interface. +type fakeSessionService struct { + spawned ports.SpawnConfig +} + +var _ controllers.SessionService = (*fakeSessionService)(nil) + +func (f *fakeSessionService) List(context.Context, sessionsvc.ListFilter) ([]domain.Session, error) { + return nil, nil +} + +func (f *fakeSessionService) Spawn(_ context.Context, cfg ports.SpawnConfig) (domain.Session, error) { + f.spawned = cfg + return domain.Session{ + SessionRecord: domain.SessionRecord{ID: domain.SessionID(string(cfg.ProjectID) + "-1")}, + Status: domain.StatusIdle, + }, nil +} + +func (f *fakeSessionService) Get(context.Context, domain.SessionID) (domain.Session, error) { + return domain.Session{}, nil +} + +func (f *fakeSessionService) Restore(context.Context, domain.SessionID) (domain.Session, error) { + return domain.Session{}, nil +} + +func (f *fakeSessionService) Kill(context.Context, domain.SessionID) (bool, error) { + return false, nil +} + +func (f *fakeSessionService) Send(context.Context, domain.SessionID, string) error { + return nil +} + +// fakeProjectManager captures the project.AddInput the controller decodes from +// the CLI's request body. Every other method is a no-op so it satisfies the +// projectsvc.Manager interface. +type fakeProjectManager struct { + added projectsvc.AddInput +} + +var _ projectsvc.Manager = (*fakeProjectManager)(nil) + +func (f *fakeProjectManager) List(context.Context) ([]projectsvc.Summary, error) { + return nil, nil +} + +func (f *fakeProjectManager) Get(context.Context, domain.ProjectID) (projectsvc.GetResult, error) { + return projectsvc.GetResult{}, nil +} + +func (f *fakeProjectManager) Add(_ context.Context, in projectsvc.AddInput) (projectsvc.Project, error) { + f.added = in + id := domain.ProjectID("demo") + if in.ProjectID != nil { + id = domain.ProjectID(*in.ProjectID) + } + return projectsvc.Project{ID: id, Path: in.Path}, nil +} + +func (f *fakeProjectManager) Remove(context.Context, domain.ProjectID) (projectsvc.RemoveResult, error) { + return projectsvc.RemoveResult{}, nil +} + +// startDriftTestDaemon stands up the real router+controllers backed by the +// supplied fakes and points the CLI's run-file at it. The CLI discovers the +// server purely via AO_RUN_FILE + the run-file port, so this is a genuine +// loopback round trip through postJSON. +func startDriftTestDaemon(t *testing.T, sessions controllers.SessionService, projects projectsvc.Manager) { + t.Helper() + + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + router := httpd.NewRouterWithAPI(config.Config{}, log, nil, httpd.APIDeps{ + Sessions: sessions, + Projects: projects, + }) + srv := httptest.NewServer(router) + t.Cleanup(srv.Close) + + port := srv.Listener.Addr().(*net.TCPAddr).Port + + rfPath := filepath.Join(t.TempDir(), "running.json") + t.Setenv("AO_RUN_FILE", rfPath) + if err := runfile.Write(rfPath, runfile.Info{PID: os.Getpid(), Port: port, StartedAt: time.Now()}); err != nil { + t.Fatalf("write run-file: %v", err) + } +} + +func TestE2E_SpawnAndProjectAddDTORoundTrip(t *testing.T) { + t.Run("spawn", func(t *testing.T) { + sessions := &fakeSessionService{} + startDriftTestDaemon(t, sessions, &fakeProjectManager{}) + + var out bytes.Buffer + root := NewRootCommand(Deps{ + Out: &out, + Err: &out, + HTTPClient: &http.Client{}, + ProcessAlive: func(int) bool { return true }, + }) + root.SetArgs([]string{ + "spawn", + "--project", "mer", + "--harness", "codex", + "--branch", "feat/x", + "--prompt", "hi", + "--issue", "ISS-1", + }) + if err := root.Execute(); err != nil { + t.Fatalf("spawn execute: %v\noutput: %s", err, out.String()) + } + + got := sessions.spawned + if got.ProjectID != "mer" { + t.Errorf("ProjectID = %q, want %q (CLI json:\"projectId\" vs SpawnSessionRequest)", got.ProjectID, "mer") + } + if got.Harness != "codex" { + t.Errorf("Harness = %q, want %q", got.Harness, "codex") + } + if got.Branch != "feat/x" { + t.Errorf("Branch = %q, want %q", got.Branch, "feat/x") + } + if got.Prompt != "hi" { + t.Errorf("Prompt = %q, want %q", got.Prompt, "hi") + } + if got.IssueID != "ISS-1" { + t.Errorf("IssueID = %q, want %q", got.IssueID, "ISS-1") + } + if !bytes.Contains(out.Bytes(), []byte("spawned session")) { + t.Errorf("output missing %q; got: %s", "spawned session", out.String()) + } + }) + + t.Run("project add", func(t *testing.T) { + projects := &fakeProjectManager{} + startDriftTestDaemon(t, &fakeSessionService{}, projects) + + var out bytes.Buffer + root := NewRootCommand(Deps{ + Out: &out, + Err: &out, + HTTPClient: &http.Client{}, + ProcessAlive: func(int) bool { return true }, + }) + root.SetArgs([]string{ + "project", "add", + "--path", "/repo/mer", + "--id", "demo", + "--name", "Demo", + }) + if err := root.Execute(); err != nil { + t.Fatalf("project add execute: %v\noutput: %s", err, out.String()) + } + + got := projects.added + if got.Path != "/repo/mer" { + t.Errorf("Path = %q, want %q", got.Path, "/repo/mer") + } + if got.ProjectID == nil || *got.ProjectID != "demo" { + t.Errorf("ProjectID = %v, want %q (CLI json:\"projectId\" vs AddInput)", got.ProjectID, "demo") + } + if got.Name == nil || *got.Name != "Demo" { + t.Errorf("Name = %v, want %q", got.Name, "Demo") + } + if !bytes.Contains(out.Bytes(), []byte("registered project")) { + t.Errorf("output missing %q; got: %s", "registered project", out.String()) + } + }) +} diff --git a/backend/internal/cli/project.go b/backend/internal/cli/project.go new file mode 100644 index 000000000..e25d29066 --- /dev/null +++ b/backend/internal/cli/project.go @@ -0,0 +1,71 @@ +package cli + +import ( + "fmt" + + "github.com/spf13/cobra" +) + +type projectAddOptions struct { + path string + id string + name string +} + +// addProjectRequest mirrors the daemon's project AddInput body for +// POST /api/v1/projects. projectId and name are optional (pointers omit them). +type addProjectRequest struct { + Path string `json:"path"` + ProjectID *string `json:"projectId,omitempty"` + Name *string `json:"name,omitempty"` +} + +type projectResult struct { + Project struct { + ID string `json:"id"` + Path string `json:"path"` + } `json:"project"` +} + +func newProjectCommand(ctx *commandContext) *cobra.Command { + cmd := &cobra.Command{ + Use: "project", + Short: "Manage projects", + } + cmd.AddCommand(newProjectAddCommand(ctx)) + return cmd +} + +func newProjectAddCommand(ctx *commandContext) *cobra.Command { + var opts projectAddOptions + cmd := &cobra.Command{ + Use: "add", + Short: "Register a local git repo as a project", + Long: "Register a local git repo as a project so sessions can be spawned in it.\n\n" + + "The path must be an existing git repository on disk.", + Args: noArgs, + RunE: func(cmd *cobra.Command, args []string) error { + if opts.path == "" { + return usageError{fmt.Errorf("--path is required")} + } + req := addProjectRequest{Path: opts.path} + if opts.id != "" { + req.ProjectID = &opts.id + } + if opts.name != "" { + req.Name = &opts.name + } + var res projectResult + if err := ctx.postJSON(cmd.Context(), "projects", req, &res); err != nil { + return err + } + _, err := fmt.Fprintf(cmd.OutOrStdout(), "registered project %s at %s\n", res.Project.ID, res.Project.Path) + return err + }, + } + f := cmd.Flags() + f.StringVar(&opts.path, "path", "", "Absolute path to the local git repo (required)") + f.StringVar(&opts.id, "id", "", "Project id (default: derived by the daemon from the path)") + f.StringVar(&opts.name, "name", "", "Display name") + return cmd +} diff --git a/backend/internal/cli/root.go b/backend/internal/cli/root.go index ce0157389..293d431fb 100644 --- a/backend/internal/cli/root.go +++ b/backend/internal/cli/root.go @@ -147,6 +147,8 @@ func NewRootCommand(deps Deps) *cobra.Command { root.AddCommand(newStopCommand(ctx)) root.AddCommand(newStatusCommand(ctx)) root.AddCommand(newDoctorCommand(ctx)) + root.AddCommand(newSpawnCommand(ctx)) + root.AddCommand(newProjectCommand(ctx)) root.AddCommand(newCompletionCommand()) root.AddCommand(newVersionCommand()) diff --git a/backend/internal/cli/spawn.go b/backend/internal/cli/spawn.go new file mode 100644 index 000000000..534fff78f --- /dev/null +++ b/backend/internal/cli/spawn.go @@ -0,0 +1,88 @@ +package cli + +import ( + "fmt" + + "github.com/spf13/cobra" + + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/runtime/zellij" +) + +type spawnOptions struct { + project string + harness string + branch string + prompt string + issue string + rules string +} + +// spawnRequest mirrors the daemon's SpawnSessionRequest body for +// POST /api/v1/sessions. The CLI keeps its own copy so it need not import httpd. +type spawnRequest struct { + ProjectID string `json:"projectId"` + IssueID string `json:"issueId,omitempty"` + Harness string `json:"harness,omitempty"` + Branch string `json:"branch,omitempty"` + Prompt string `json:"prompt,omitempty"` + AgentRules string `json:"agentRules,omitempty"` +} + +type spawnResult struct { + Session struct { + ID string `json:"id"` + Status string `json:"status"` + } `json:"session"` +} + +func newSpawnCommand(ctx *commandContext) *cobra.Command { + var opts spawnOptions + cmd := &cobra.Command{ + Use: "spawn", + Short: "Spawn a worker agent session in a registered project", + Long: "Spawn a worker agent session in a registered project.\n\n" + + "The session runs the chosen agent (default: the daemon's AO_AGENT) in a\n" + + "fresh git worktree. Register the project first with `ao project add`.", + Args: noArgs, + RunE: func(cmd *cobra.Command, args []string) error { + if opts.project == "" { + return usageError{fmt.Errorf("--project is required")} + } + req := spawnRequest{ + ProjectID: opts.project, + IssueID: opts.issue, + Harness: opts.harness, + Branch: opts.branch, + Prompt: opts.prompt, + AgentRules: opts.rules, + } + var res spawnResult + if err := ctx.postJSON(cmd.Context(), "sessions", req, &res); err != nil { + return err + } + out := cmd.OutOrStdout() + if _, err := fmt.Fprintf(out, "spawned session %s (%s)\n", res.Session.ID, res.Session.Status); err != nil { + return err + } + // The daemon runs zellij under a short, non-default socket dir (see + // zellij.DefaultSocketDir), so a plain `zellij attach` wouldn't find + // the session — prefix the env so the hint is copy-pasteable. Use the + // sanitised name zellij actually registers (zellij.SessionName): a long + // session id maps to a different name than the raw id. + attach := fmt.Sprintf("zellij attach %s", zellij.SessionName(res.Session.ID)) + if dir := zellij.DefaultSocketDir(); dir != "" { + attach = fmt.Sprintf("ZELLIJ_SOCKET_DIR=%s %s", dir, attach) + } + _, err := fmt.Fprintf(out, "attach with: %s\n", attach) + return err + }, + } + f := cmd.Flags() + f.StringVar(&opts.project, "project", "", "Project id to spawn the session in (required)") + f.StringVar(&opts.harness, "harness", "", "Agent harness: claude-code, codex, … (default: the daemon's AO_AGENT)") + f.StringVar(&opts.branch, "branch", "", "Branch for the session worktree (default: ao/)") + f.StringVar(&opts.prompt, "prompt", "", "Initial prompt for the agent") + f.StringVar(&opts.issue, "issue", "", "Issue id to associate with the session") + f.StringVar(&opts.rules, "rules", "", "Agent rules appended to the prompt") + return cmd +} diff --git a/backend/internal/cli/spawn_test.go b/backend/internal/cli/spawn_test.go new file mode 100644 index 000000000..baca6e3ad --- /dev/null +++ b/backend/internal/cli/spawn_test.go @@ -0,0 +1,37 @@ +package cli + +import ( + "bytes" + "strings" + "testing" +) + +// TestSpawnCommand_RequiresProject asserts `ao spawn` rejects a missing +// --project before touching the network, so it fails fast without a daemon. +func TestSpawnCommand_RequiresProject(t *testing.T) { + var out, errb bytes.Buffer + root := NewRootCommand(Deps{Out: &out, Err: &errb}) + root.SetArgs([]string{"spawn"}) + err := root.Execute() + if err == nil { + t.Fatal("expected an error when --project is missing") + } + if !strings.Contains(err.Error(), "--project is required") { + t.Fatalf("error = %v, want it to mention --project is required", err) + } +} + +// TestProjectAddCommand_RequiresPath asserts `ao project add` rejects a missing +// --path before touching the network. +func TestProjectAddCommand_RequiresPath(t *testing.T) { + var out, errb bytes.Buffer + root := NewRootCommand(Deps{Out: &out, Err: &errb}) + root.SetArgs([]string{"project", "add"}) + err := root.Execute() + if err == nil { + t.Fatal("expected an error when --path is missing") + } + if !strings.Contains(err.Error(), "--path is required") { + t.Fatalf("error = %v, want it to mention --path is required", err) + } +} diff --git a/backend/internal/daemon/daemon.go b/backend/internal/daemon/daemon.go index 3b75e4fb0..d38fa4ea1 100644 --- a/backend/internal/daemon/daemon.go +++ b/backend/internal/daemon/daemon.go @@ -62,7 +62,17 @@ func Run() error { // liveness; the CDC broadcaster feeds the session-state channel. The manager // is handed to httpd, which mounts it at /mux. Raw PTY bytes never flow // through the CDC change_log — only session-state events do. - runtimeAdapter := zellij.New(zellij.Options{}) + // zellij's default socket dir is too long on macOS for long session ids + // (see zellij.DefaultSocketDir); use a short, stable one and ensure it exists. + zellijSocketDir := zellij.DefaultSocketDir() + if zellijSocketDir != "" { + if err := os.MkdirAll(zellijSocketDir, 0o700); err != nil { + // Don't abort startup, but surface it: every spawn's zellij session + // would otherwise fail later with an opaque socket-bind error. + log.Warn("could not create zellij socket dir; spawns may fail", "dir", zellijSocketDir, "error", err) + } + } + runtimeAdapter := zellij.New(zellij.Options{SocketDir: zellijSocketDir}) termMgr := terminal.NewManager(runtimeAdapter, cdcPipe.Broadcaster, log) defer termMgr.Close() diff --git a/backend/internal/daemon/lifecycle_wiring.go b/backend/internal/daemon/lifecycle_wiring.go index 34685670f..51232299d 100644 --- a/backend/internal/daemon/lifecycle_wiring.go +++ b/backend/internal/daemon/lifecycle_wiring.go @@ -63,10 +63,10 @@ func startSession(cfg config.Config, runtime ports.Runtime, store *sqlite.Store, // Per-session worktrees live under the data dir, so a single AO_DATA_DIR // override moves all durable per-user state together. ManagedRoot: filepath.Join(cfg.DataDir, "worktrees"), - // An empty resolver fails every project lookup with a clear - // "no repo configured for project" error until the projects table feeds - // repo paths in — better than silently misrouting spawns. - RepoResolver: gitworktree.StaticRepoResolver{}, + // Resolve each project's source repo from the projects table, so a + // session spawned for a registered project materialises its worktree off + // that repo. Unregistered projects fail loudly. + RepoResolver: projectRepoResolver{store: store}, }) if err != nil { return nil, fmt.Errorf("session workspace: %w", err) @@ -145,3 +145,28 @@ func buildAgentResolver(defaultAgent string, log *slog.Logger) (ports.AgentResol log.Info("built per-session agent resolver", "default", defaultAgent, "registered", ids) return resolver, nil } + +// projectRepoResolver resolves a project's on-disk repo path from the projects +// table so gitworktree can materialise per-session worktrees off it. It replaces +// the empty StaticRepoResolver the daemon used before (which failed every +// lookup), turning a registered project into a spawnable one. +type projectRepoResolver struct{ store *sqlite.Store } + +var _ gitworktree.RepoResolver = projectRepoResolver{} + +func (r projectRepoResolver) RepoPath(projectID domain.ProjectID) (string, error) { + rec, ok, err := r.store.GetProject(context.Background(), string(projectID)) + if err != nil { + return "", fmt.Errorf("look up project %q: %w", projectID, err) + } + if !ok { + return "", fmt.Errorf("no project registered with id %q — add one with `ao project add`: %w", projectID, sessionmanager.ErrProjectNotResolvable) + } + if !rec.ArchivedAt.IsZero() { + return "", fmt.Errorf("project %q is archived: %w", projectID, sessionmanager.ErrProjectNotResolvable) + } + if rec.Path == "" { + return "", fmt.Errorf("project %q has no repo path on record: %w", projectID, sessionmanager.ErrProjectNotResolvable) + } + return rec.Path, nil +} diff --git a/backend/internal/daemon/wiring_test.go b/backend/internal/daemon/wiring_test.go index 33ba2839f..d15f9deef 100644 --- a/backend/internal/daemon/wiring_test.go +++ b/backend/internal/daemon/wiring_test.go @@ -2,6 +2,7 @@ package daemon import ( "context" + "errors" "io" "log/slog" "sync" @@ -15,6 +16,7 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/lifecycle" "github.com/aoagents/agent-orchestrator/backend/internal/ports" + sessionmanager "github.com/aoagents/agent-orchestrator/backend/internal/session_manager" "github.com/aoagents/agent-orchestrator/backend/internal/storage/sqlite" ) @@ -132,3 +134,56 @@ func TestWiring_StartSessionBuildsSessionService(t *testing.T) { t.Fatal("startSession returned nil session service") } } + +// TestProjectRepoResolver_ResolvesRegisteredProject asserts the DB-backed repo +// resolver turns a registered project into its on-disk repo path (so spawns +// materialise a worktree), and fails loudly for an unregistered project. +func TestProjectRepoResolver_ResolvesRegisteredProject(t *testing.T) { + store, err := sqlite.Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = store.Close() }) + + ctx := context.Background() + if err := store.UpsertProject(ctx, domain.ProjectRecord{ID: "mer", Path: "/repo/mer", RegisteredAt: time.Now()}); err != nil { + t.Fatal(err) + } + + r := projectRepoResolver{store: store} + got, err := r.RepoPath("mer") + if err != nil { + t.Fatalf("RepoPath(mer): %v", err) + } + if got != "/repo/mer" { + t.Fatalf("RepoPath(mer) = %q, want /repo/mer", got) + } + _, err = r.RepoPath("nope") + if err == nil { + t.Fatal("expected an error for an unregistered project") + } + // Guard the sentinel wrapping so the HTTP 400 mapping can't silently regress. + if !errors.Is(err, sessionmanager.ErrProjectNotResolvable) { + t.Fatalf("unregistered-project error should wrap ErrProjectNotResolvable, got %v", err) + } +} + +// TestDaemonZellijSocketDir_LeavesBudgetForSessionNames guards the fix for the +// zellij "session name must be less than 0 characters" spawn failure: the +// daemon's socket dir must be short enough that a max-length (48-char) session +// name still fits the ~103-byte unix-domain-socket-path budget. zellij's long +// $TMPDIR default (the bug) would fail this. +func TestDaemonZellijSocketDir_LeavesBudgetForSessionNames(t *testing.T) { + dir := zellij.DefaultSocketDir() + if dir == "" { + t.Skip("zellij not used on this platform") + } + const ( + unixSocketPathMax = 103 // sun_path budget zellij enforces on macOS + zellijOverhead = 24 // zellij's version subdir + separators (generous) + maxSessionName = 48 // zellijSessionName's cap + ) + if budget := unixSocketPathMax - len(dir) - zellijOverhead; budget < maxSessionName { + t.Fatalf("zellij socket dir %q too long: %d bytes left for the session name, need >= %d", dir, budget, maxSessionName) + } +} diff --git a/backend/internal/httpd/controllers/sessions.go b/backend/internal/httpd/controllers/sessions.go index c97f388ad..bc88fd721 100644 --- a/backend/internal/httpd/controllers/sessions.go +++ b/backend/internal/httpd/controllers/sessions.go @@ -253,6 +253,8 @@ func writeSessionError(w http.ResponseWriter, r *http.Request, err error) { envelope.WriteAPIError(w, r, http.StatusConflict, "conflict", "SESSION_NOT_RESTORABLE", "Session is not restorable", nil) case errors.Is(err, sessionmanager.ErrIncompleteHandle): envelope.WriteAPIError(w, r, http.StatusConflict, "conflict", "SESSION_INCOMPLETE_HANDLE", "Session is missing runtime or workspace handles", nil) + case errors.Is(err, sessionmanager.ErrProjectNotResolvable): + envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "PROJECT_NOT_RESOLVABLE", "Project is not registered or has no repo — register it with `ao project add`", nil) default: envelope.WriteAPIError(w, r, http.StatusInternalServerError, "internal", "SESSION_OPERATION_FAILED", "Session operation failed", nil) } diff --git a/backend/internal/service/project/service.go b/backend/internal/service/project/service.go index 0eb213abe..89cf5a10b 100644 --- a/backend/internal/service/project/service.go +++ b/backend/internal/service/project/service.go @@ -228,7 +228,10 @@ 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) { + // Reject any "." run: a "." prefix fails the pattern, but an embedded ".." + // (e.g. "a..b") passes it yet yields a branch like "ao/a..b-1" that git's + // check-ref-format rejects — surfacing as an opaque 500 at spawn time. + if raw == "" || raw == "." || strings.Contains(raw, "..") || strings.ContainsAny(raw, `/\`) || !projectIDPattern.MatchString(raw) { return badRequest("INVALID_PROJECT_ID", "Project id failed storage-path validation", nil) } return nil diff --git a/backend/internal/service/project/service_test.go b/backend/internal/service/project/service_test.go index d8ae22ec5..03dedfad8 100644 --- a/backend/internal/service/project/service_test.go +++ b/backend/internal/service/project/service_test.go @@ -125,6 +125,11 @@ func TestManager_AddValidationAndConflicts(t *testing.T) { _, err = m.Add(ctx, project.AddInput{Path: t.TempDir()}) // exists but not a git repo wantCode(t, err, "NOT_A_GIT_REPO") + // An embedded ".." passes the id pattern but would yield an invalid git + // branch (ao/a..b-1) at spawn time; reject it up front as a clear 400. + _, err = m.Add(ctx, project.AddInput{Path: gitRepo(t), ProjectID: ptr("a..b")}) + wantCode(t, err, "INVALID_PROJECT_ID") + repoA, repoB := gitRepo(t), gitRepo(t) if _, err := m.Add(ctx, project.AddInput{Path: repoA, ProjectID: ptr("shared")}); err != nil { t.Fatalf("seed add: %v", err) diff --git a/backend/internal/session_manager/manager.go b/backend/internal/session_manager/manager.go index d1cc1007a..fcf4277f7 100644 --- a/backend/internal/session_manager/manager.go +++ b/backend/internal/session_manager/manager.go @@ -17,6 +17,9 @@ var ( ErrNotFound = errors.New("session: not found") ErrNotRestorable = errors.New("session: not restorable (not terminal)") ErrIncompleteHandle = errors.New("session: incomplete teardown handle") + // ErrProjectNotResolvable means the spawn's project has no usable repo + // (unregistered, archived, or missing a path). The API maps it to a 400. + ErrProjectNotResolvable = errors.New("session: project repo not resolvable") ) // Env vars a spawned process reads to learn who it is. @@ -101,7 +104,14 @@ func (m *Manager) Spawn(ctx context.Context, cfg ports.SpawnConfig) (domain.Sess } id := rec.ID - ws, err := m.workspace.Create(ctx, ports.WorkspaceConfig{ProjectID: cfg.ProjectID, SessionID: id, Branch: cfg.Branch}) + branch := cfg.Branch + if branch == "" { + // A fresh, unique branch per session: gitworktree can't add a worktree on + // a branch already checked out elsewhere (e.g. main), so default to one + // derived from the assigned session id. + branch = "ao/" + string(id) + } + ws, err := m.workspace.Create(ctx, ports.WorkspaceConfig{ProjectID: cfg.ProjectID, SessionID: id, Branch: branch}) if err != nil { m.markSpawnFailedTerminated(ctx, id) return domain.SessionRecord{}, fmt.Errorf("spawn %s: workspace: %w", id, err) diff --git a/backend/internal/session_manager/manager_test.go b/backend/internal/session_manager/manager_test.go index 74bf7bbcf..78900d473 100644 --- a/backend/internal/session_manager/manager_test.go +++ b/backend/internal/session_manager/manager_test.go @@ -245,3 +245,26 @@ func TestCleanup_ReclaimsTerminalWorkspaces(t *testing.T) { t.Fatal("live workspace must not be destroyed") } } + +func TestSpawn_DefaultsBranchFromSessionID(t *testing.T) { + m, st, _, _ := newManager() + s, err := m.Spawn(ctx, ports.SpawnConfig{ProjectID: "mer", Kind: domain.KindWorker}) + if err != nil { + t.Fatal(err) + } + // An empty SpawnConfig.Branch defaults to a unique per-session branch. + if got := st.sessions[s.ID].Metadata.Branch; got != "ao/mer-1" { + t.Fatalf("default branch = %q, want ao/mer-1", got) + } +} + +func TestSpawn_KeepsExplicitBranch(t *testing.T) { + m, st, _, _ := newManager() + s, err := m.Spawn(ctx, ports.SpawnConfig{ProjectID: "mer", Kind: domain.KindWorker, Branch: "feature/x"}) + if err != nil { + t.Fatal(err) + } + if got := st.sessions[s.ID].Metadata.Branch; got != "feature/x" { + t.Fatalf("explicit branch = %q, want feature/x", got) + } +}