diff --git a/apps/penpal/.gitignore b/apps/penpal/.gitignore index 69d05c41..32926571 100644 --- a/apps/penpal/.gitignore +++ b/apps/penpal/.gitignore @@ -1,5 +1,6 @@ # Binary /penpal +/penpal-cli /penpal-server /birdseye .penpal.pid diff --git a/apps/penpal/.mcp.json b/apps/penpal/.mcp.json index 8869c0a9..6044b65c 100644 --- a/apps/penpal/.mcp.json +++ b/apps/penpal/.mcp.json @@ -2,7 +2,7 @@ "mcpServers": { "penpal": { "type": "http", - "url": "http://localhost:18923/mcp" + "url": "http://localhost:8080/mcp" } } } \ No newline at end of file diff --git a/apps/penpal/ERD.md b/apps/penpal/ERD.md index 0ff706c2..d0f08402 100644 --- a/apps/penpal/ERD.md +++ b/apps/penpal/ERD.md @@ -171,7 +171,7 @@ see-also: - **E-PENPAL-COMMENT-ORDER**: `OrderComments()` arranges comments in tree order: root comments sorted by time, replies grouped under their parents, siblings sorted by effective time. A comment's effective time is `WorkingStartedAt` when present, otherwise `CreatedAt`. This ensures agent replies are ordered by when the agent started working, not when the reply was posted. Missing parents fall back to root level. ← [P-PENPAL-THREAD-PANEL](PRODUCT.md#P-PENPAL-THREAD-PANEL), [P-PENPAL-WORKING](PRODUCT.md#P-PENPAL-WORKING) -- **E-PENPAL-CHANGE-SEQ**: A global monotonic sequence number increments on every comment change. `WaitForChangeSince(ctx, sinceSeq)` blocks on a channel until `changeSeq` advances or context cancels. +- **E-PENPAL-CHANGE-SEQ**: A global monotonic sequence number increments on every comment change. `WaitForChangeSince(ctx, sinceSeq)` blocks on a channel until `changeSeq` advances or context cancels. `WaitAndEnrich(ctx, project, worktree, sinceSeq)` combines the wait with enrichment — on change, it loads pending threads and sets working indicators; on timeout, it refreshes working timestamps. Both the MCP `penpal_wait_for_changes` tool and the REST `handleAgentWait` handler delegate to `WaitAndEnrich`. ← [P-PENPAL-WAIT-CHANGES](PRODUCT.md#P-PENPAL-WAIT-CHANGES) --- @@ -181,7 +181,7 @@ see-also: - **E-PENPAL-WORKING**: An in-memory `working` map (keyed by `"project:path:threadID"`) tracks which threads an agent is actively processing. Each entry stores `startedAt` (time) and `afterCommentID` (the last comment ID when the agent started working). Entries expire after 60s. `SetWorking()`/`ClearWorking()` trigger SSE `comments` events. The API thread response includes `workingAfterCommentId` so the frontend renders the indicator after the correct comment. ← [P-PENPAL-WORKING](PRODUCT.md#P-PENPAL-WORKING) -- **E-PENPAL-HEARTBEAT**: An in-memory `heartbeats` map (keyed by `"project:filePath"`) records agent activity. `IsAgentActive()` returns true if heartbeat is <60s old. MCP tool calls record heartbeats. +- **E-PENPAL-HEARTBEAT**: Agent presence is determined solely by `agents.Manager.HasActiveAgent()`, which checks both spawned processes (via the process map) and CLI sessions (via the session manager). The previous heartbeat-based tracking in `comments.Store` has been removed — heartbeats are no longer needed because the agent management system is the single source of truth for agent presence. ← [P-PENPAL-AGENT-PRESENCE](PRODUCT.md#P-PENPAL-AGENT-PRESENCE) --- @@ -216,14 +216,14 @@ see-also: - **E-PENPAL-AGENT-AUTOSTART**: `maybeStartAgent()` is called after `handleCreateThread` and `handleAddComment`. If the new comment's Role is `"human"` and no agent is running, one is started. ← [P-PENPAL-AGENT-LAUNCH](PRODUCT.md#P-PENPAL-AGENT-LAUNCH) -- **E-PENPAL-AGENT-CLEANUP**: On agent exit: temp MCP config is removed, project heartbeats and working indicators are cleared, and the `onChange` callback fires an SSE event. +- **E-PENPAL-AGENT-CLEANUP**: On agent exit: temp MCP config is removed, working indicators are cleared, and the `onChange` callback fires an SSE event. ← [P-PENPAL-AGENT-LAUNCH](PRODUCT.md#P-PENPAL-AGENT-LAUNCH) --- ## HTTP API -- **E-PENPAL-API-ROUTES**: The server exposes REST endpoints: projects CRUD, project files (grouped), recent files, in-review, search, workspaces, sources, open/navigate, threads CRUD, reviews, focus, agents start/stop/status, raw file, view tracking, publish, publish-state, ready, install-tools, claude-path. SPA served from `/app/`. MCP at `/mcp`. +- **E-PENPAL-API-ROUTES**: The server exposes REST endpoints: projects CRUD, project files (grouped), recent files, in-review, search, workspaces, sources, open/navigate, threads CRUD, reviews, focus, agents start/stop/status/attach/wait, raw file, view tracking, publish, publish-state, ready, install-tools, claude-path. SPA served from `/app/`. MCP at `/mcp`. ← [P-PENPAL-RENDER](PRODUCT.md#P-PENPAL-RENDER), [P-PENPAL-MCP](PRODUCT.md#P-PENPAL-MCP) - **E-PENPAL-LAZY-INIT**: First HTTP request triggers `sync.Once` that discovers projects, starts the watcher, then runs `populateProjects()` in a background goroutine. `populateProjects()` refreshes the cache, seeds activity, closes `readyCh`, broadcasts events, then enriches git info. @@ -407,6 +407,31 @@ see-also: --- +## CLI Agent Tools + +- **E-PENPAL-CLI-ATTACH**: `penpal attach ` reads the port file (`ReadPortFile()`), launches the app if not running (same as `penpal open`), resolves the path to an absolute path, and calls `POST /api/agents/attach` with `{"path": "", "force": false}`. The server resolves the path to a project (via `FindProjectByPathWithWorktree`), checks for existing agents (Manager-spawned or external sessions), and either returns `{"project", "worktree", "sessionToken"}` or 409 Conflict. `--force` sends `force: true`, which kills/evicts the existing agent first. The session token is a random UUID generated server-side. + ← [P-PENPAL-CLI-ATTACH](PRODUCT.md#P-PENPAL-CLI-ATTACH), [P-PENPAL-CLI-CONTENTION](PRODUCT.md#P-PENPAL-CLI-CONTENTION) + +- **E-PENPAL-CLI-AGENT-CMDS**: CLI subcommands (`files-in-review`, `list-threads`, `read-thread`, `reply`, `create-thread`, `wait`) each read the port file, call the corresponding REST API endpoint with `?session=` query parameter, and print the JSON response to stdout. The server validates the session token on each request — invalid or evicted tokens return 401. All endpoints record heartbeats for the session's project. `penpal reply` accepts `--body` flag or reads from stdin. `penpal wait` calls `GET /api/agents/wait?project=X&session=T&sinceSeq=N` which uses the same `WaitForChangeSince` mechanism as the MCP `penpal_wait_for_changes` tool. + ← [P-PENPAL-CLI-AGENT-TOOLS](PRODUCT.md#P-PENPAL-CLI-AGENT-TOOLS) + +- **E-PENPAL-SESSION-MGMT**: The server maintains an in-memory `sessions` map (keyed by token) storing project name, worktree, created-at, and last-heartbeat. Sessions expire after 90 seconds without a heartbeat. `POST /api/agents/attach` creates a session after contention checks. Agent contention checks both `agents.Manager.Status(project).Running` and active sessions. `--force` calls `Manager.Stop()` for spawned agents or marks the existing session as evicted. Evicted sessions return 401 on subsequent use. `Manager.Start()` also checks for active CLI sessions and refuses to spawn if one is attached. `Manager.StopAny()` provides a unified stop that terminates spawned agents and evicts CLI sessions in a single call — used by `POST /api/agents/stop`. + ← [P-PENPAL-CLI-CONTENTION](PRODUCT.md#P-PENPAL-CLI-CONTENTION), [P-PENPAL-CLI-ATTACH](PRODUCT.md#P-PENPAL-CLI-ATTACH) + +- **E-PENPAL-AGENT-ACTIVE-UNIFIED**: `AgentActive` in API responses checks both `agents.Manager.Status(project).Running` AND whether an active (non-expired) session exists for the project. This ensures external CLI-attached agents show the same presence indicators as internally-spawned agents. + ← [P-PENPAL-AGENT-PRESENCE](PRODUCT.md#P-PENPAL-AGENT-PRESENCE) + +- **E-PENPAL-AGENT-PARITY**: Both launched and external agents go through the same `claimSession()` contention path in `agents.Manager`. Spawned agents claim a `SessionSpawned` session before launching; CLI agents claim a `SessionCLI` session via `Attach()`. Session ownership is the single source of truth for "who owns this project." The `agents` map is purely a process handle — it does not determine ownership. + ← [P-PENPAL-AGENT-PARITY](PRODUCT.md#P-PENPAL-AGENT-PARITY), [P-PENPAL-CLI-CONTENTION](PRODUCT.md#P-PENPAL-CLI-CONTENTION) + +- **E-PENPAL-SHARED-CODEPATHS**: Shared codepaths are strongly preferred over branching or special-casing by agent type. When launched and external agents need the same behavior (contention, working indicators, comment threading, presence), a single implementation serves both. `SessionKind` (`SessionSpawned` vs `SessionCLI`) is used only where behavior genuinely differs (e.g., heartbeat expiry applies to CLI sessions but not spawned sessions). + ← [P-PENPAL-AGENT-PARITY](PRODUCT.md#P-PENPAL-AGENT-PARITY) + +- **E-PENPAL-AGENT-SELF-ID**: `Session.AgentName` stores the agent's self-reported name. `Attach(projectName, worktree, agentName, force)` and `claimSession()` accept an `agentName` parameter. `Manager.AgentName(project)` returns the agent name from the active session, defaulting to `"agent"`. The REST `POST /api/agents/attach` reads `"agent"` from the JSON body (default `"agent"`); the attach response includes `"agentName"`. The CLI `penpal attach --agent NAME` sends the name. `handleAddComment` and `handleCreateThread` override the `author` field from the session's `AgentName` when `role == "agent"`. MCP tools receive an `agentNameFunc` parameter and use it to derive the author for `penpal_reply` and `penpal_create_thread`. `Manager.Start()` passes `"claude"` as the agent name for spawned agents. + ← [P-PENPAL-AGENT-SELF-ID](PRODUCT.md#P-PENPAL-AGENT-SELF-ID) + +--- + ## Source Management - **E-PENPAL-ADD-SOURCE**: `POST /api/sources` accepts a relative path. Directories create "tree" sources; `.md` files are added to a "files" source. Duplicate detection refuses paths already covered by an existing source. Only `.md` files accepted for individual file sources. Used internally by `penpal open` CLI to auto-add files to their containing project. diff --git a/apps/penpal/PRODUCT.md b/apps/penpal/PRODUCT.md index 06135ebe..5fc07d9f 100644 --- a/apps/penpal/PRODUCT.md +++ b/apps/penpal/PRODUCT.md @@ -302,6 +302,16 @@ Global views aggregate content across all projects. They appear as top-level ite - **P-PENPAL-CLI-OPEN**: The `penpal open ...` command opens one or more files or directories in the Penpal app, launching the app if it's not running. Directories are resolved to their project; `.md` files are auto-added to their containing project if not already tracked (or a new standalone project is created). Non-`.md` files are rejected. +- **P-PENPAL-CLI-ATTACH**: `penpal attach ` registers the calling agent as the active agent for a project. The command discovers the running server, resolves the path to a project, opens the file in the app, and claims agent ownership. If another agent is already attached, the command fails with an error. `--force` evicts the existing agent and takes over. On success, prints JSON with the project name, worktree (if any), and a session token. The session token must be passed to all subsequent CLI commands. + +- **P-PENPAL-CLI-AGENT-TOOLS**: Agents interact with penpal via CLI subcommands that mirror the MCP tools: `penpal files-in-review`, `penpal list-threads`, `penpal read-thread`, `penpal reply`, `penpal create-thread`, and `penpal wait`. Each command requires `--session ` for authentication and prints JSON to stdout. `penpal wait` blocks up to 30 seconds and returns when comments change or on timeout — agents call it in a loop. All commands record agent heartbeats. An invalid or evicted session token causes the command to exit with an error. + +- **P-PENPAL-CLI-CONTENTION**: At most one agent (internal or external) can be active for a project at a time. `penpal attach` fails if an agent is already active. `penpal attach --force` terminates the existing agent (kills an internally-spawned process, or evicts an external session). When an external agent's session is evicted, its next CLI command fails with an "evicted" error. Internally-launched agents and CLI-attached agents use the same contention system. + +- **P-PENPAL-AGENT-PARITY**: Launched (internally-spawned) and external (CLI-attached) agents have the same capabilities and Penpal exhibits the same behavior for both. Working indicators, presence dots, auto-start suppression, stop button behavior, and comment threading all work identically regardless of how the agent connected. + +- **P-PENPAL-AGENT-SELF-ID**: Agents identify themselves by name when attaching. `penpal attach --agent amp` records "amp" as the agent name. The agent name is stored on the session and used as the comment author for all agent-role comments, so comments show the actual agent that wrote them (e.g., "amp" or "claude") rather than a generic label. Internally-spawned agents are always named "claude". If no agent name is provided, it defaults to "agent". + --- ## Real-Time Updates diff --git a/apps/penpal/TESTING.md b/apps/penpal/TESTING.md index bee14564..28200ac9 100644 --- a/apps/penpal/TESTING.md +++ b/apps/penpal/TESTING.md @@ -86,12 +86,15 @@ see-also: | Suggested Replies (P-PENPAL-SUGGESTED-REPLIES) | — | CommentsPanel.test.tsx | — | review-workflow.spec.ts | | MCP Tools (P-PENPAL-MCP) | — | — | mcpserver/tools_test.go, transport_test.go, worktree_test.go | review-workflow.spec.ts | | Wait for Changes (P-PENPAL-WAIT-CHANGES) | — | — | tools_test.go (TestWaitForChanges_Triggered) | — | -| Agent Management (P-PENPAL-AGENT-LAUNCH, STATUS) | stream_test.go | FilePage.test.tsx (auto-start) | api_agents_test.go | — | +| Agent Management (P-PENPAL-AGENT-LAUNCH, STATUS, P-PENPAL-CLI-CONTENTION) | stream_test.go, session_test.go | FilePage.test.tsx (auto-start) | api_agents_test.go | — | +| CLI Attach & Sessions (P-PENPAL-CLI-ATTACH, P-PENPAL-CLI-CONTENTION, E-PENPAL-CLI-ATTACH, E-PENPAL-SESSION-MGMT, E-PENPAL-AGENT-ACTIVE-UNIFIED) | session_test.go (Attach, ValidateSession, HasActiveAgent, StopAny, Detach, RecordSessionHeartbeat) | — | api_agents_test.go (attach success/conflict/force/missing/noManager, wait invalid/missing, stop ok, maybeStartAgent skip) | — | +| Agent Parity (P-PENPAL-AGENT-PARITY, E-PENPAL-AGENT-PARITY, E-PENPAL-SHARED-CODEPATHS) | session_test.go (claimSession for both SessionSpawned and SessionCLI) | — | api_agents_test.go | — | +| Agent Self-ID (P-PENPAL-AGENT-SELF-ID, E-PENPAL-AGENT-SELF-ID) | session_test.go (Attach stores AgentName) | — | api_agents_test.go (attach with agent name, author override) | — | +| CLI Agent Tools (P-PENPAL-CLI-AGENT-TOOLS, E-PENPAL-CLI-AGENT-CMDS) | — | — | api_agents_test.go | — | | Agent Detection (P-PENPAL-AGENT-PRESENCE) | — | — | — | — | | Review Workflow (P-PENPAL-PROJECT-IN-REVIEW, P-PENPAL-GLOBAL-IN-REVIEW) | — | InReviewPage.test.tsx | api_projects_test.go (TestAPIInReview) | — | | Publishing (P-PENPAL-PUBLISH) | blockcell_test.go, render_test.go, state_test.go | — | — | — | | Tabs (P-PENPAL-TABS) | — | useTabs.test.ts, Layout.test.tsx | — | tab-navigation.spec.ts | -| Search (P-PENPAL-SEARCH) | — | SearchPage.test.tsx | — | react-app.spec.ts | | Recent Files (P-PENPAL-PROJECT-RECENT, P-PENPAL-GLOBAL-RECENT, E-PENPAL-ACTIVITY-PERSIST) | activity_test.go | RecentPage.test.tsx | integration_test.go | — | | CLI Open (P-PENPAL-CLI-OPEN) | — | — | api_manage_test.go | cli-open.spec.ts | | Manual Source Management (E-PENPAL-SRC-MANUAL, E-PENPAL-SRC-ALL-MD) | — | — | api_manage_test.go (TestAPISources_AddFileNotBlockedByAllMarkdown) | — | diff --git a/apps/penpal/cmd/penpal-cli/main.go b/apps/penpal/cmd/penpal-cli/main.go index 62937dc0..2d7df364 100644 --- a/apps/penpal/cmd/penpal-cli/main.go +++ b/apps/penpal/cmd/penpal-cli/main.go @@ -3,12 +3,16 @@ package main import ( "bytes" "encoding/json" + "flag" "fmt" + "io" "net/http" + "net/url" "os" "os/exec" "path/filepath" "runtime" + "strings" "time" "github.com/loganj/penpal/internal/config" @@ -17,7 +21,7 @@ import ( func main() { args := os.Args[1:] if len(args) == 0 { - fmt.Fprintf(os.Stderr, "Usage: penpal open ...\n") + printUsage() os.Exit(1) } @@ -28,37 +32,442 @@ func main() { os.Exit(1) } runOpen(args[1:]) + case "attach": + runAttach(args[1:]) + case "files-in-review": + runFilesInReview(args[1:]) + case "list-threads": + runListThreads(args[1:]) + case "read-thread": + runReadThread(args[1:]) + case "reply": + runReply(args[1:]) + case "create-thread": + runCreateThread(args[1:]) + case "wait": + runWait(args[1:]) default: fmt.Fprintf(os.Stderr, "Unknown command: %s\n", args[0]) - fmt.Fprintf(os.Stderr, "Usage: penpal open ...\n") + printUsage() os.Exit(1) } } +func printUsage() { + fmt.Fprintf(os.Stderr, `Usage: penpal [options] + +Commands: + open ... Open files/directories in Penpal + attach [--force] [--agent NAME] Attach as the active agent for a project + files-in-review --project P [--worktree W] + list-threads --project P [--path F] [--status S] [--worktree W] + read-thread --project P --path F --thread-id ID [--worktree W] + reply --session T --project P --path F --thread-id ID [--body B] [--worktree W] + create-thread --session T --project P --path F --selected-text T --body B [--heading-path H] [--worktree W] + wait --session T --project P [--since-seq N] [--worktree W] +`) +} + +// --- open command --- + // runOpen opens paths in the Penpal desktop app, launching it if needed. // E-PENPAL-CLI: reads port file, checks health, calls POST /api/open. func runOpen(paths []string) { + port := ensureServer() + openPaths(port, paths) +} + +// --- attach command --- + +// runAttach registers the calling agent as the active agent for a project. +// E-PENPAL-CLI-ATTACH: resolves path, ensures server, POST /api/agents/attach. +func runAttach(args []string) { + fs := flag.NewFlagSet("attach", flag.ExitOnError) + force := fs.Bool("force", false, "Evict existing agent and take over") + agent := fs.String("agent", "", "Agent name (e.g., amp, claude)") + fs.Parse(args) + + if fs.NArg() < 1 { + fmt.Fprintf(os.Stderr, "Usage: penpal attach [--force] [--agent NAME]\n") + os.Exit(1) + } + + absPath, err := filepath.Abs(fs.Arg(0)) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: could not resolve path: %v\n", err) + os.Exit(1) + } + + port := ensureServer() + + attachBody := map[string]any{ + "path": absPath, + "force": *force, + } + if *agent != "" { + attachBody["agent"] = *agent + } + body, _ := json.Marshal(attachBody) + + resp, err := http.Post( + fmt.Sprintf("http://localhost:%d/api/agents/attach", port), + "application/json", + bytes.NewReader(body), + ) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: could not contact server: %v\n", err) + os.Exit(1) + } + defer resp.Body.Close() + + if resp.StatusCode == http.StatusConflict { + var errResp struct { + Error string `json:"error"` + } + json.NewDecoder(resp.Body).Decode(&errResp) + fmt.Fprintf(os.Stderr, "Error: %s\n", errResp.Error) + os.Exit(1) + } + + if resp.StatusCode != http.StatusOK { + bodyBytes, _ := io.ReadAll(resp.Body) + fmt.Fprintf(os.Stderr, "Error: server returned %d: %s\n", resp.StatusCode, strings.TrimSpace(string(bodyBytes))) + os.Exit(1) + } + + io.Copy(os.Stdout, resp.Body) + fmt.Fprintln(os.Stdout) +} + +// --- files-in-review command --- + +// runFilesInReview queries files with open threads for a project. +// E-PENPAL-CLI-AGENT-CMDS: GET /api/reviews — read-only, no session required. +func runFilesInReview(args []string) { + fs := flag.NewFlagSet("files-in-review", flag.ExitOnError) + session := fs.String("session", "", "Session token (optional, records heartbeat)") + project := fs.String("project", "", "Project qualified name (required)") + worktree := fs.String("worktree", "", "Worktree name") + fs.Parse(args) + + if *project == "" { + fmt.Fprintf(os.Stderr, "Error: --project is required for files-in-review\n") + os.Exit(1) + } + + port := getPort() + u := fmt.Sprintf("http://localhost:%d/api/reviews?project=%s", + port, urlEncode(*project)) + if *session != "" { + u += "&session=" + urlEncode(*session) + } + if *worktree != "" { + u += "&worktree=" + urlEncode(*worktree) + } + + doGet(u) +} + +// --- list-threads command --- + +// runListThreads lists comment threads for a project or file. +// E-PENPAL-CLI-AGENT-CMDS: GET /api/threads — read-only, no session required. +func runListThreads(args []string) { + fs := flag.NewFlagSet("list-threads", flag.ExitOnError) + session := fs.String("session", "", "Session token (optional, records heartbeat)") + project := fs.String("project", "", "Project qualified name (required)") + path := fs.String("path", "", "File path (project-relative)") + status := fs.String("status", "", "Filter by status (open, resolved)") + worktree := fs.String("worktree", "", "Worktree name") + fs.Parse(args) + + if *project == "" { + fmt.Fprintf(os.Stderr, "Error: --project is required for list-threads\n") + os.Exit(1) + } + + port := getPort() + u := fmt.Sprintf("http://localhost:%d/api/threads?project=%s", + port, urlEncode(*project)) + if *session != "" { + u += "&session=" + urlEncode(*session) + } + if *path != "" { + u += "&path=" + urlEncode(*path) + } + if *status != "" { + u += "&status=" + urlEncode(*status) + } + if *worktree != "" { + u += "&worktree=" + urlEncode(*worktree) + } + + doGet(u) +} + +// --- read-thread command --- + +// runReadThread reads a single thread by ID (filters from list-threads response). +// E-PENPAL-CLI-AGENT-CMDS: GET /api/threads — read-only, no session required. +func runReadThread(args []string) { + fs := flag.NewFlagSet("read-thread", flag.ExitOnError) + session := fs.String("session", "", "Session token (optional, records heartbeat)") + project := fs.String("project", "", "Project qualified name (required)") + path := fs.String("path", "", "File path, project-relative (required)") + threadID := fs.String("thread-id", "", "Thread ID (required)") + worktree := fs.String("worktree", "", "Worktree name") + fs.Parse(args) + + if *project == "" || *path == "" || *threadID == "" { + fmt.Fprintf(os.Stderr, "Usage: penpal read-thread --project P --path F --thread-id ID [--worktree W]\n") + os.Exit(1) + } + + port := getPort() + u := fmt.Sprintf("http://localhost:%d/api/threads?project=%s&path=%s", + port, urlEncode(*project), urlEncode(*path)) + if *session != "" { + u += "&session=" + urlEncode(*session) + } + if *worktree != "" { + u += "&worktree=" + urlEncode(*worktree) + } + + resp, err := http.Get(u) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: could not contact server: %v\n", err) + os.Exit(1) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + bodyBytes, _ := io.ReadAll(resp.Body) + fmt.Fprintf(os.Stderr, "Error: server returned %d: %s\n", resp.StatusCode, strings.TrimSpace(string(bodyBytes))) + os.Exit(1) + } + + var threads []json.RawMessage + if err := json.NewDecoder(resp.Body).Decode(&threads); err != nil { + fmt.Fprintf(os.Stderr, "Error: could not decode response: %v\n", err) + os.Exit(1) + } + + for _, raw := range threads { + var t struct { + ID string `json:"id"` + } + if err := json.Unmarshal(raw, &t); err != nil { + continue + } + if t.ID == *threadID { + os.Stdout.Write(raw) + fmt.Fprintln(os.Stdout) + return + } + } + + fmt.Fprintf(os.Stderr, "Error: thread %s not found\n", *threadID) + os.Exit(1) +} + +// --- reply command --- + +// runReply posts a reply to an existing thread. +// E-PENPAL-CLI-AGENT-CMDS: POST /api/threads/{id}/comments with session validation. +func runReply(args []string) { + fs := flag.NewFlagSet("reply", flag.ExitOnError) + session := fs.String("session", "", "Session token (required)") + project := fs.String("project", "", "Project qualified name (required)") + path := fs.String("path", "", "File path, project-relative (required)") + threadID := fs.String("thread-id", "", "Thread ID (required)") + body := fs.String("body", "", "Reply body (reads stdin if not provided)") + worktree := fs.String("worktree", "", "Worktree name") + fs.Parse(args) + + requireFlags(fs.Name(), *session, *project) + if *path == "" || *threadID == "" { + fmt.Fprintf(os.Stderr, "Usage: penpal reply --session T --project P --path F --thread-id ID [--body B] [--worktree W]\n") + os.Exit(1) + } + + replyBody := *body + if replyBody == "" { + data, err := io.ReadAll(os.Stdin) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: could not read from stdin: %v\n", err) + os.Exit(1) + } + replyBody = strings.TrimSpace(string(data)) + } + if replyBody == "" { + fmt.Fprintf(os.Stderr, "Error: reply body is required (--body or stdin)\n") + os.Exit(1) + } + + port := getPort() + u := fmt.Sprintf("http://localhost:%d/api/threads/%s/comments?session=%s", + port, urlEncode(*threadID), urlEncode(*session)) + + payload, _ := json.Marshal(map[string]string{ + "project": *project, + "path": *path, + "author": "claude", + "role": "agent", + "body": replyBody, + "worktree": *worktree, + }) + + resp, err := http.Post(u, "application/json", bytes.NewReader(payload)) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: could not contact server: %v\n", err) + os.Exit(1) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + bodyBytes, _ := io.ReadAll(resp.Body) + fmt.Fprintf(os.Stderr, "Error: server returned %d: %s\n", resp.StatusCode, strings.TrimSpace(string(bodyBytes))) + os.Exit(1) + } + + io.Copy(os.Stdout, resp.Body) + fmt.Fprintln(os.Stdout) +} + +// --- create-thread command --- + +// runCreateThread creates a new comment thread on a file. +// E-PENPAL-CLI-AGENT-CMDS: POST /api/threads with session validation and anchor. +func runCreateThread(args []string) { + fs := flag.NewFlagSet("create-thread", flag.ExitOnError) + session := fs.String("session", "", "Session token (required)") + project := fs.String("project", "", "Project qualified name (required)") + path := fs.String("path", "", "File path, project-relative (required)") + selectedText := fs.String("selected-text", "", "Anchor text (required)") + body := fs.String("body", "", "Comment body (required)") + headingPath := fs.String("heading-path", "", "Heading path for anchor context") + worktree := fs.String("worktree", "", "Worktree name") + fs.Parse(args) + + requireFlags(fs.Name(), *session, *project) + if *path == "" || *selectedText == "" || *body == "" { + fmt.Fprintf(os.Stderr, "Usage: penpal create-thread --session T --project P --path F --selected-text T --body B [--heading-path H] [--worktree W]\n") + os.Exit(1) + } + + port := getPort() + u := fmt.Sprintf("http://localhost:%d/api/threads?session=%s", + port, urlEncode(*session)) + + anchor := map[string]string{ + "selectedText": *selectedText, + } + if *headingPath != "" { + anchor["headingPath"] = *headingPath + } + + payload, _ := json.Marshal(map[string]any{ + "project": *project, + "path": *path, + "anchor": anchor, + "author": "claude", + "role": "agent", + "body": *body, + "worktree": *worktree, + }) + + resp, err := http.Post(u, "application/json", bytes.NewReader(payload)) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: could not contact server: %v\n", err) + os.Exit(1) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + bodyBytes, _ := io.ReadAll(resp.Body) + fmt.Fprintf(os.Stderr, "Error: server returned %d: %s\n", resp.StatusCode, strings.TrimSpace(string(bodyBytes))) + os.Exit(1) + } + + io.Copy(os.Stdout, resp.Body) + fmt.Fprintln(os.Stdout) +} + +// --- wait command --- + +// runWait blocks until comments change or timeout (30s long-poll). +// E-PENPAL-CLI-AGENT-CMDS: GET /api/agents/wait with 35s client timeout. +func runWait(args []string) { + fs := flag.NewFlagSet("wait", flag.ExitOnError) + session := fs.String("session", "", "Session token (required)") + project := fs.String("project", "", "Project qualified name (required)") + sinceSeq := fs.String("since-seq", "", "Sequence number to wait after") + worktree := fs.String("worktree", "", "Worktree name") + fs.Parse(args) + + requireFlags(fs.Name(), *session, *project) + + port := getPort() + u := fmt.Sprintf("http://localhost:%d/api/agents/wait?project=%s&session=%s", + port, urlEncode(*project), urlEncode(*session)) + if *sinceSeq != "" { + u += "&sinceSeq=" + urlEncode(*sinceSeq) + } + if *worktree != "" { + u += "&worktree=" + urlEncode(*worktree) + } + + client := &http.Client{Timeout: 35 * time.Second} + resp, err := client.Get(u) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: could not contact server: %v\n", err) + os.Exit(1) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + bodyBytes, _ := io.ReadAll(resp.Body) + fmt.Fprintf(os.Stderr, "Error: server returned %d: %s\n", resp.StatusCode, strings.TrimSpace(string(bodyBytes))) + os.Exit(1) + } + + io.Copy(os.Stdout, resp.Body) + fmt.Fprintln(os.Stdout) +} + +// --- shared helpers --- + +// ensureServer checks for a running server, launching the app if needed. +// Returns the port of the running server. +// E-PENPAL-CLI: shared server startup logic for open and attach. +func ensureServer() int { port := config.ReadPortFile() - // Check if server is already running (desktop app is open) if port > 0 && isServerRunning(port) { - openPaths(port, paths) - return + return port } - // No running server — launch the desktop app, which starts its own sidecar openApp() - // Wait for the app's sidecar server to become ready. - // Re-read the port file each iteration so we pick up the fresh port the - // new server writes, instead of polling a stale value from a prior run. port = waitForServerStart(10 * time.Second) if port <= 0 { fmt.Fprintf(os.Stderr, "Error: server did not start within timeout\n") os.Exit(1) } - openPaths(port, paths) + return port +} + +// getPort reads the port file and returns the port. +// Exits if no server is running. +// E-PENPAL-CLI-AGENT-CMDS: used by agent commands that require a running server. +func getPort() int { + port := config.ReadPortFile() + if port <= 0 || !isServerRunning(port) { + fmt.Fprintf(os.Stderr, "Error: penpal server is not running\n") + os.Exit(1) + } + return port } // openPaths sends each path to the /api/open endpoint, then opens the desktop app. @@ -146,3 +555,36 @@ func openApp() { fmt.Fprintf(os.Stderr, "Is Penpal.app installed? Run: just install\n") } } + +// doGet performs an HTTP GET and prints the response body to stdout. +// E-PENPAL-CLI-AGENT-CMDS: shared GET helper for agent commands. +func doGet(url string) { + resp, err := http.Get(url) + if err != nil { + fmt.Fprintf(os.Stderr, "Error: could not contact server: %v\n", err) + os.Exit(1) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + bodyBytes, _ := io.ReadAll(resp.Body) + fmt.Fprintf(os.Stderr, "Error: server returned %d: %s\n", resp.StatusCode, strings.TrimSpace(string(bodyBytes))) + os.Exit(1) + } + + io.Copy(os.Stdout, resp.Body) + fmt.Fprintln(os.Stdout) +} + +// requireFlags checks that session and project flags are provided, exits with usage if not. +func requireFlags(command, session, project string) { + if session == "" || project == "" { + fmt.Fprintf(os.Stderr, "Error: --session and --project are required for %s\n", command) + os.Exit(1) + } +} + +// urlEncode encodes a string for use in a URL query parameter. +func urlEncode(s string) string { + return url.QueryEscape(s) +} diff --git a/apps/penpal/cmd/penpal-server/main.go b/apps/penpal/cmd/penpal-server/main.go index fd010911..2b4976bb 100644 --- a/apps/penpal/cmd/penpal-server/main.go +++ b/apps/penpal/cmd/penpal-server/main.go @@ -85,7 +85,7 @@ func runServe(port int, rootOverride string) { defer w.Stop() am := agents.New(c, cs, port) - mcpHandler := mcpserver.NewHandler(cs, c) + mcpHandler := mcpserver.NewHandler(cs, c, am.AgentName) srv := server.New(c, w, cs, mcpHandler, am, act, cfg, cfgPath) // E-PENPAL-LOCAL-ONLY: bind to loopback only — no network exposure. addr := fmt.Sprintf("127.0.0.1:%d", port) diff --git a/apps/penpal/internal/agents/manager.go b/apps/penpal/internal/agents/manager.go index 50945a4c..617eee67 100644 --- a/apps/penpal/internal/agents/manager.go +++ b/apps/penpal/internal/agents/manager.go @@ -39,6 +39,7 @@ type Manager struct { port int onChange func(projectName string) // called when agent starts or stops claudeBin func() string // returns resolved path to claude binary + sm *sessionManager // E-PENPAL-SESSION-MGMT: external CLI session tracking } func New(c *cache.Cache, cs *comments.Store, port int) *Manager { @@ -47,6 +48,7 @@ func New(c *cache.Cache, cs *comments.Store, port int) *Manager { cache: c, comments: cs, port: port, + sm: newSessionManager(), } } @@ -65,27 +67,71 @@ func (m *Manager) SetOnChange(fn func(projectName string)) { } // Start launches a Claude agent for the given project. -// Returns nil if an agent is already running for this project. -// E-PENPAL-AGENT-SPAWN: writes temp MCP config, builds prompt, runs claude. +// Returns nil, nil if a spawned agent is already running. +// Returns an error if a CLI session is active. +// E-PENPAL-AGENT-SPAWN: claims session, writes temp MCP config, builds prompt, runs claude. func (m *Manager) Start(projectName string) (*Agent, error) { - m.mu.Lock() - defer m.mu.Unlock() + proj := m.cache.FindProject(projectName) + if proj == nil { + return nil, fmt.Errorf("project %q not found", projectName) + } - if a, ok := m.agents[projectName]; ok { - select { - case <-a.done: - // Previous agent exited, clean up and proceed - delete(m.agents, projectName) - default: - return nil, nil // already running + // Claim a spawned session — this handles all contention. + // If a spawned session already exists (agent already running), claimSession + // returns an error; we map that to the idempotent nil,nil return. + // E-PENPAL-AGENT-SELF-ID: spawned agents are always claude. + sess, err := m.claimSession(projectName, "", "claude", false, SessionSpawned) + if err != nil { + // Check if a spawned agent is already running — return nil,nil (idempotent). + m.mu.Lock() + if a, ok := m.agents[projectName]; ok { + select { + case <-a.done: + // Agent exited but session not yet cleaned up — fall through to error. + delete(m.agents, projectName) + default: + m.mu.Unlock() + return nil, nil // already running + } } + m.mu.Unlock() + return nil, err } - proj := m.cache.FindProject(projectName) - if proj == nil { - return nil, fmt.Errorf("project %q not found", projectName) + // Build and launch the process outside the lock. + agent, err := m.launchAgent(projectName, proj.Path, sess.Token) + if err != nil { + // Claim failed at launch — release the session. + m.Detach(sess.Token) + return nil, err + } + + // Install the agent under the lock, verifying we still own the session. + m.mu.Lock() + currentToken, owned := m.sm.projectSession[projectName] + if !owned || currentToken != sess.Token { + // Someone else claimed the project while we were launching. + m.mu.Unlock() + agent.cmd.Process.Kill() + <-agent.done + return nil, fmt.Errorf("session was replaced during agent launch for %q", projectName) + } + m.agents[projectName] = agent + m.mu.Unlock() + + log.Printf("Agent started for %s (PID %d)", projectName, agent.PID) + + if m.onChange != nil { + go m.onChange(projectName) } + return agent, nil +} + +// launchAgent builds the temp MCP config and starts the claude process. +// Returns the Agent (with background goroutines running) or an error. +// The sessionToken is captured so the exit goroutine can clean up the right session. +func (m *Manager) launchAgent(projectName, projectPath, sessionToken string) (*Agent, error) { // Write temporary MCP config mcpConfigPath := filepath.Join(os.TempDir(), fmt.Sprintf("penpal-agent-%s.json", sanitize(projectName))) @@ -112,10 +158,10 @@ func (m *Manager) Start(projectName string) (*Agent, error) { "--max-budget-usd", "5", "--model", "opus", ) - cmd.Dir = proj.Path + cmd.Dir = projectPath // Log agent output to a file - logPath := filepath.Join(proj.Path, ".penpal", "agent.log") + logPath := filepath.Join(projectPath, ".penpal", "agent.log") os.MkdirAll(filepath.Dir(logPath), 0755) logFile, err := os.Create(logPath) if err != nil { @@ -135,7 +181,7 @@ func (m *Manager) Start(projectName string) (*Agent, error) { agent := &Agent{ Project: projectName, - ProjectPath: proj.Path, + ProjectPath: projectPath, PID: cmd.Process.Pid, StartedAt: time.Now(), cmd: cmd, @@ -144,46 +190,38 @@ func (m *Manager) Start(projectName string) (*Agent, error) { } // Parse NDJSON stream in background, writing through to log file. - // streamDone is closed when parseStream finishes so we can safely - // close logFile only after all writes complete. streamDone := make(chan struct{}) go func() { agent.parseStream(stdout, logFile) close(streamDone) }() - m.agents[projectName] = agent - - log.Printf("Agent started for %s (PID %d)", projectName, agent.PID) - // Monitor process exit in background + // Monitor process exit in background. go func() { agent.exitErr = cmd.Wait() - <-streamDone // wait for parseStream to finish before closing log + <-streamDone logFile.Close() os.Remove(mcpConfigPath) close(agent.done) log.Printf("Agent exited for %s (PID %d): %v", projectName, agent.PID, agent.exitErr) - m.comments.ClearProjectHeartbeats(projectName) m.comments.ClearProjectWorking(projectName) m.mu.Lock() - // Only delete if it's still the same agent (not replaced) if current, ok := m.agents[projectName]; ok && current == agent { delete(m.agents, projectName) } fn := m.onChange m.mu.Unlock() + // Clean up the session by token (safe even if already replaced). + m.Detach(sessionToken) + if fn != nil { fn(projectName) } }() - if m.onChange != nil { - go m.onChange(projectName) - } - return agent, nil } @@ -287,15 +325,48 @@ func (m *Manager) SimulateFinished(projectName string) { } } +// AgentName returns the agent name from the active session for a project. +// Returns "agent" if no active session is found. +// E-PENPAL-AGENT-SELF-ID: used by MCP tools to derive comment author from session. +func (m *Manager) AgentName(projectName string) string { + m.mu.Lock() + defer m.mu.Unlock() + + token, ok := m.sm.projectSession[projectName] + if !ok { + return "agent" + } + sess, exists := m.sm.sessions[token] + if !exists || sess.Evicted || sess.isExpired() { + return "agent" + } + if sess.AgentName == "" { + return "agent" + } + return sess.AgentName +} + // SimulateRunning inserts a synthetic agent entry that appears to be -// actively running. This is intended for testing the "agent running" status -// path without requiring an external binary. +// actively running, with an associated spawned session. func (m *Manager) SimulateRunning(projectName string, contextUsed, contextWindow int, totalCostUSD float64, numTurns int) { m.mu.Lock() defer m.mu.Unlock() + + now := time.Now() + sess := &Session{ + Token: generateToken(), + Project: projectName, + AgentName: "claude", + Kind: SessionSpawned, + CreatedAt: now, + LastHeartbeat: now, + } + m.sm.sessions[sess.Token] = sess + m.sm.projectSession[projectName] = sess.Token + m.agents[projectName] = &Agent{ Project: projectName, - StartedAt: time.Now(), + StartedAt: now, PID: 99999, done: make(chan struct{}), // not closed = still running contextWindow: contextWindow, diff --git a/apps/penpal/internal/agents/manager_test.go b/apps/penpal/internal/agents/manager_test.go index 6497cb72..8d90cef9 100644 --- a/apps/penpal/internal/agents/manager_test.go +++ b/apps/penpal/internal/agents/manager_test.go @@ -43,7 +43,7 @@ func newTestManager(t *testing.T) (*Manager, *comments.Store) { return m, cs } -// E-PENPAL-AGENT-CLEANUP: verifies that after agent finishes, heartbeats and working are cleared. +// E-PENPAL-AGENT-CLEANUP: verifies that after agent finishes, working indicators are cleared. // Uses a synthetic agent (via done channel) to simulate agent exit without spawning a real process. func TestAgentCleanupOnExit(t *testing.T) { _, cs := newTestManager(t) @@ -58,16 +58,11 @@ func TestAgentCleanupOnExit(t *testing.T) { m := New(c, cs, 0) - // Pre-populate heartbeats and working indicators - cs.RecordHeartbeat("testproj", "file1.md") - cs.RecordHeartbeat("testproj", "file2.md") + // Pre-populate working indicators cs.SetWorking("testproj", "file1.md", "thread-1", "") cs.SetWorking("testproj", "file2.md", "thread-2", "") // Verify they are active before cleanup - if !cs.IsAgentActive("testproj", "file1.md") { - t.Fatal("setup: expected file1.md heartbeat to be active") - } if !cs.IsWorking("testproj", "file1.md", "thread-1") { t.Fatal("setup: expected thread-1 to be working") } @@ -99,7 +94,6 @@ func TestAgentCleanupOnExit(t *testing.T) { // This mirrors the logic in manager.go Start() exit goroutine. go func() { <-done - cs.ClearProjectHeartbeats("testproj") cs.ClearProjectWorking("testproj") m.mu.Lock() @@ -130,14 +124,6 @@ func TestAgentCleanupOnExit(t *testing.T) { t.Fatal("timed out waiting for onChange callback") } - // Verify heartbeats are cleared - if cs.IsAgentActive("testproj", "file1.md") { - t.Error("expected file1.md heartbeat to be cleared after agent exit") - } - if cs.IsAgentActive("testproj", "file2.md") { - t.Error("expected file2.md heartbeat to be cleared after agent exit") - } - // Verify working indicators are cleared if cs.IsWorking("testproj", "file1.md", "thread-1") { t.Error("expected thread-1 working to be cleared after agent exit") diff --git a/apps/penpal/internal/agents/session.go b/apps/penpal/internal/agents/session.go new file mode 100644 index 00000000..220feaaa --- /dev/null +++ b/apps/penpal/internal/agents/session.go @@ -0,0 +1,273 @@ +package agents + +import ( + "crypto/rand" + "fmt" + "math/big" + "time" +) + +const sessionTimeout = 90 * time.Second + +// SessionKind distinguishes how a session was created. +type SessionKind int + +const ( + // SessionCLI is an external CLI-attached agent session with heartbeat expiry. + SessionCLI SessionKind = iota + // SessionSpawned is a session owned by a Manager-launched process (no heartbeat expiry). + SessionSpawned +) + +// Session represents an agent session (spawned or CLI-attached). +// E-PENPAL-SESSION-MGMT: tracks token, project, worktree, heartbeat, eviction, kind, and agent name. +type Session struct { + Token string + Project string // qualified project name + Worktree string // may be empty + AgentName string // self-reported agent name (e.g., "amp", "claude") + Kind SessionKind + CreatedAt time.Time + LastHeartbeat time.Time + Evicted bool +} + +// isExpired reports whether the session's last heartbeat is older than sessionTimeout. +// E-PENPAL-SESSION-MGMT: lazy expiration check on access. +func (s *Session) isExpired() bool { + if s.Kind == SessionSpawned { + return false // spawned sessions live until process exits + } + return time.Since(s.LastHeartbeat) > sessionTimeout +} + +// sessionManager holds external CLI-attached agent session state. +// All fields are protected by Manager.mu (no separate mutex — consistent +// lock ordering eliminates ABBA deadlock risk). +// E-PENPAL-CLI-ATTACH: embedded in Manager for session tracking. +type sessionManager struct { + sessions map[string]*Session // token -> session + projectSession map[string]string // project name -> token +} + +func newSessionManager() *sessionManager { + return &sessionManager{ + sessions: make(map[string]*Session), + projectSession: make(map[string]string), + } +} + +// generateToken produces a UUID-style random token using crypto/rand. +// E-PENPAL-SESSION-MGMT: secure token generation for session identity. +func generateToken() string { + const alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" + b := make([]byte, 32) + for i := range b { + n, _ := rand.Int(rand.Reader, big.NewInt(int64(len(alphabet)))) + b[i] = alphabet[n.Int64()] + } + return string(b) +} + +// Attach creates an external CLI agent session for the given project. +// If force is false and any agent (spawned or CLI) is active, an error is returned. +// If force is true, existing agents/sessions are evicted. +// E-PENPAL-CLI-ATTACH: creates session, checks contention, evicts if forced. +// E-PENPAL-AGENT-SELF-ID: stores the agent's self-reported name on the session. +func (m *Manager) Attach(projectName, worktree, agentName string, force bool) (*Session, error) { + return m.claimSession(projectName, worktree, agentName, force, SessionCLI) +} + +// claimSession is the unified contention + session creation path used by both +// Attach (CLI) and Start (spawned). It evicts any prior owner when force is +// true, then creates a new Session of the given kind. +// Caller must NOT hold m.mu. +// E-PENPAL-SESSION-MGMT: unified session claim for all agent types. +// E-PENPAL-AGENT-SELF-ID: stores agent name on the session. +func (m *Manager) claimSession(projectName, worktree, agentName string, force bool, kind SessionKind) (*Session, error) { + m.mu.Lock() + + // Check for spawned agent contention. + if agent, ok := m.agents[projectName]; ok { + running := true + select { + case <-agent.done: + running = false + delete(m.agents, projectName) + default: + } + if running { + if !force { + m.mu.Unlock() + return nil, fmt.Errorf("agent already running for %q (PID %d)", projectName, agent.PID) + } + // Force-evict: stop the spawned agent outside the lock + // (Stop acquires m.mu internally). + m.mu.Unlock() + m.Stop(projectName) + m.mu.Lock() + // Re-verify agent is gone after re-acquiring lock. + if agent, ok := m.agents[projectName]; ok { + select { + case <-agent.done: + delete(m.agents, projectName) + default: + m.mu.Unlock() + return nil, fmt.Errorf("failed to stop agent for %q", projectName) + } + } + } + } + // From here on, m.mu is held until the end of the function. + defer m.mu.Unlock() + + // Check for existing session contention. + if token, ok := m.sm.projectSession[projectName]; ok { + if sess, exists := m.sm.sessions[token]; exists { + if !sess.Evicted && !sess.isExpired() { + if !force { + return nil, fmt.Errorf("agent session already active for %q", projectName) + } + sess.Evicted = true + } + delete(m.sm.sessions, token) + delete(m.sm.projectSession, projectName) + } + } + + // Create new session. + now := time.Now() + sess := &Session{ + Token: generateToken(), + Project: projectName, + Worktree: worktree, + AgentName: agentName, + Kind: kind, + CreatedAt: now, + LastHeartbeat: now, + } + + m.sm.sessions[sess.Token] = sess + m.sm.projectSession[projectName] = sess.Token + + // Notify onChange for CLI sessions immediately. For spawned sessions, + // Start() fires onChange after the process is installed. + if kind == SessionCLI { + fn := m.onChange + if fn != nil { + go fn(projectName) + } + } + + return sess, nil +} + +// ValidateSession returns the session for the given token, or an error if the +// session is not found, evicted, or expired. +// E-PENPAL-SESSION-MGMT: validates token and performs lazy expiration cleanup. +func (m *Manager) ValidateSession(token string) (*Session, error) { + m.mu.Lock() + defer m.mu.Unlock() + + sess, ok := m.sm.sessions[token] + if !ok { + return nil, fmt.Errorf("session not found") + } + if sess.Evicted { + delete(m.sm.sessions, token) + delete(m.sm.projectSession, sess.Project) + return nil, fmt.Errorf("session was evicted") + } + if sess.isExpired() { + delete(m.sm.sessions, token) + delete(m.sm.projectSession, sess.Project) + return nil, fmt.Errorf("session expired") + } + return sess, nil +} + +// RecordSessionHeartbeat updates the LastHeartbeat for the given session token. +// E-PENPAL-SESSION-MGMT: heartbeat keeps session alive, prevents expiration. +func (m *Manager) RecordSessionHeartbeat(token string) { + m.mu.Lock() + defer m.mu.Unlock() + + if sess, ok := m.sm.sessions[token]; ok && !sess.Evicted { + sess.LastHeartbeat = time.Now() + } +} + +// HasActiveAgent reports whether any active (non-expired, non-evicted) session +// exists for the given project. +// E-PENPAL-AGENT-PARITY: unified check — session is the single source of truth. +func (m *Manager) HasActiveAgent(projectName string) bool { + m.mu.Lock() + defer m.mu.Unlock() + + token, ok := m.sm.projectSession[projectName] + if !ok { + return false + } + sess, exists := m.sm.sessions[token] + if !exists { + delete(m.sm.projectSession, projectName) + return false + } + if sess.Evicted || sess.isExpired() { + delete(m.sm.sessions, token) + delete(m.sm.projectSession, projectName) + return false + } + return true +} + +// StopAny stops any active agent for the project — spawned or CLI-attached. +// E-PENPAL-CLI-CONTENTION: unified stop for both agent types. +func (m *Manager) StopAny(projectName string) { + m.mu.Lock() + hasAgent := false + if _, ok := m.agents[projectName]; ok { + hasAgent = true + } + + // Evict any session (spawned or CLI). + if token, ok := m.sm.projectSession[projectName]; ok { + if sess, exists := m.sm.sessions[token]; exists { + sess.Evicted = true + } + delete(m.sm.sessions, token) + delete(m.sm.projectSession, projectName) + } + + fn := m.onChange + m.mu.Unlock() + + // Stop spawned agent outside m.mu (Stop acquires m.mu internally). + if hasAgent { + m.Stop(projectName) + } + + if fn != nil { + fn(projectName) + } +} + +// Detach removes a session cleanly (spawned or CLI). +// E-PENPAL-CLI-ATTACH: clean session teardown. +func (m *Manager) Detach(token string) { + m.mu.Lock() + sess, ok := m.sm.sessions[token] + if !ok { + m.mu.Unlock() + return + } + projectName := sess.Project + delete(m.sm.sessions, token) + delete(m.sm.projectSession, projectName) + fn := m.onChange + m.mu.Unlock() + + if fn != nil { + fn(projectName) + } +} diff --git a/apps/penpal/internal/agents/session_test.go b/apps/penpal/internal/agents/session_test.go new file mode 100644 index 00000000..e29d047f --- /dev/null +++ b/apps/penpal/internal/agents/session_test.go @@ -0,0 +1,304 @@ +package agents + +import ( + "testing" + "time" +) + +// E-PENPAL-CLI-ATTACH: verifies Attach creates a valid session. +func TestAttach_CreatesSession(t *testing.T) { + m, _ := newTestManager(t) + + sess, err := m.Attach("testproj", "", "claude", false) + if err != nil { + t.Fatalf("Attach: %v", err) + } + if sess.Token == "" { + t.Error("expected non-empty token") + } + if sess.Project != "testproj" { + t.Errorf("expected project=testproj, got %q", sess.Project) + } + if sess.Evicted { + t.Error("expected Evicted=false") + } +} + +// E-PENPAL-CLI-CONTENTION: verifies Attach fails when another session is active. +func TestAttach_Conflict_WithoutForce(t *testing.T) { + m, _ := newTestManager(t) + + _, err := m.Attach("testproj", "", "claude", false) + if err != nil { + t.Fatalf("first Attach: %v", err) + } + + _, err = m.Attach("testproj", "", "claude", false) + if err == nil { + t.Fatal("expected conflict error on second Attach without force") + } +} + +// E-PENPAL-CLI-CONTENTION: verifies Attach with force evicts existing session. +func TestAttach_Force_EvictsSession(t *testing.T) { + m, _ := newTestManager(t) + + sess1, err := m.Attach("testproj", "", "claude", false) + if err != nil { + t.Fatalf("first Attach: %v", err) + } + + sess2, err := m.Attach("testproj", "", "claude", true) + if err != nil { + t.Fatalf("forced Attach: %v", err) + } + + if sess2.Token == sess1.Token { + t.Error("expected different token after force-evict") + } + + // Validate the evicted session should fail. + if _, err := m.ValidateSession(sess1.Token); err == nil { + t.Error("expected evicted session to fail validation") + } +} + +// E-PENPAL-CLI-CONTENTION: verifies Attach fails when a spawned agent is running. +func TestAttach_ConflictWithSpawnedAgent(t *testing.T) { + m, _ := newTestManager(t) + + m.SimulateRunning("testproj", 1000, 200000, 0.5, 1) + + // Without force, Attach should fail because a spawned agent is running. + _, err := m.Attach("testproj", "", "claude", false) + if err == nil { + t.Fatal("expected error when spawned agent is running") + } +} + +// E-PENPAL-CLI-CONTENTION: verifies Attach with force succeeds after spawned agent exits. +func TestAttach_Force_AfterSpawnedAgentExits(t *testing.T) { + m, _ := newTestManager(t) + + // Insert a finished (exited) agent so force-eviction can clean it up. + m.SimulateFinished("testproj") + + sess, err := m.Attach("testproj", "", "claude", true) + if err != nil { + t.Fatalf("forced Attach after exited agent: %v", err) + } + if sess.Token == "" { + t.Error("expected valid session") + } +} + +// E-PENPAL-SESSION-MGMT: verifies ValidateSession returns session for valid token. +func TestValidateSession_Valid(t *testing.T) { + m, _ := newTestManager(t) + + sess, _ := m.Attach("testproj", "wt1", "claude", false) + + validated, err := m.ValidateSession(sess.Token) + if err != nil { + t.Fatalf("ValidateSession: %v", err) + } + if validated.Project != "testproj" { + t.Errorf("expected project=testproj, got %q", validated.Project) + } + if validated.Worktree != "wt1" { + t.Errorf("expected worktree=wt1, got %q", validated.Worktree) + } +} + +// E-PENPAL-SESSION-MGMT: verifies ValidateSession returns error for evicted session. +func TestValidateSession_Evicted(t *testing.T) { + m, _ := newTestManager(t) + + sess, _ := m.Attach("testproj", "", "claude", false) + + // Evict by forcing a new session. + m.Attach("testproj", "", "claude", true) + + _, err := m.ValidateSession(sess.Token) + if err == nil { + t.Fatal("expected error for evicted session") + } +} + +// E-PENPAL-SESSION-MGMT: verifies ValidateSession returns error for expired session. +func TestValidateSession_Expired(t *testing.T) { + m, _ := newTestManager(t) + + sess, _ := m.Attach("testproj", "", "claude", false) + + // Directly set LastHeartbeat to the past to simulate expiration. + m.mu.Lock() + sess.LastHeartbeat = time.Now().Add(-2 * sessionTimeout) + m.mu.Unlock() + + _, err := m.ValidateSession(sess.Token) + if err == nil { + t.Fatal("expected error for expired session") + } +} + +// E-PENPAL-SESSION-MGMT: verifies ValidateSession returns error for unknown token. +func TestValidateSession_NotFound(t *testing.T) { + m, _ := newTestManager(t) + + _, err := m.ValidateSession("nonexistent-token") + if err == nil { + t.Fatal("expected error for unknown token") + } +} + +// E-PENPAL-AGENT-ACTIVE-UNIFIED: verifies HasActiveAgent returns true for spawned agent. +func TestHasActiveAgent_SpawnedAgent(t *testing.T) { + m, _ := newTestManager(t) + + m.SimulateRunning("testproj", 1000, 200000, 0.5, 1) + + if !m.HasActiveAgent("testproj") { + t.Error("expected HasActiveAgent=true for spawned agent") + } +} + +// E-PENPAL-AGENT-ACTIVE-UNIFIED: verifies HasActiveAgent returns true for CLI session. +func TestHasActiveAgent_CLISession(t *testing.T) { + m, _ := newTestManager(t) + + m.Attach("testproj", "", "claude", false) + + if !m.HasActiveAgent("testproj") { + t.Error("expected HasActiveAgent=true for CLI session") + } +} + +// E-PENPAL-AGENT-ACTIVE-UNIFIED: verifies HasActiveAgent returns false when nothing active. +func TestHasActiveAgent_NoAgent(t *testing.T) { + m, _ := newTestManager(t) + + if m.HasActiveAgent("testproj") { + t.Error("expected HasActiveAgent=false when nothing active") + } +} + +// E-PENPAL-AGENT-ACTIVE-UNIFIED: verifies HasActiveAgent returns false for expired session. +func TestHasActiveAgent_ExpiredSession(t *testing.T) { + m, _ := newTestManager(t) + + sess, _ := m.Attach("testproj", "", "claude", false) + + // Expire the session. + m.mu.Lock() + sess.LastHeartbeat = time.Now().Add(-2 * sessionTimeout) + m.mu.Unlock() + + if m.HasActiveAgent("testproj") { + t.Error("expected HasActiveAgent=false for expired session") + } +} + +// E-PENPAL-CLI-CONTENTION: verifies StopAny evicts a CLI session. +func TestStopAny_EvictsCLISession(t *testing.T) { + m, _ := newTestManager(t) + + sess, _ := m.Attach("testproj", "", "claude", false) + + m.StopAny("testproj") + + _, err := m.ValidateSession(sess.Token) + if err == nil { + t.Fatal("expected session to be evicted after StopAny") + } +} + +// E-PENPAL-CLI-CONTENTION: verifies StopAny on empty project doesn't panic. +func TestStopAny_NoAgent(t *testing.T) { + m, _ := newTestManager(t) + + // Should not panic. + m.StopAny("testproj") +} + +// E-PENPAL-SESSION-MGMT: verifies RecordSessionHeartbeat updates LastHeartbeat. +func TestRecordSessionHeartbeat(t *testing.T) { + m, _ := newTestManager(t) + + sess, _ := m.Attach("testproj", "", "claude", false) + + before := sess.LastHeartbeat + time.Sleep(5 * time.Millisecond) + + m.RecordSessionHeartbeat(sess.Token) + + m.mu.Lock() + after := sess.LastHeartbeat + m.mu.Unlock() + + if !after.After(before) { + t.Error("expected LastHeartbeat to be updated after RecordSessionHeartbeat") + } +} + +// E-PENPAL-AGENT-SELF-ID: verifies Attach stores AgentName on the session. +func TestAttach_StoresAgentName(t *testing.T) { + m, _ := newTestManager(t) + + sess, err := m.Attach("testproj", "", "amp", false) + if err != nil { + t.Fatalf("Attach: %v", err) + } + if sess.AgentName != "amp" { + t.Errorf("expected AgentName=amp, got %q", sess.AgentName) + } +} + +// E-PENPAL-AGENT-SELF-ID: verifies AgentName returns the session's agent name. +func TestAgentName_ReturnsSessionName(t *testing.T) { + m, _ := newTestManager(t) + + m.Attach("testproj", "", "amp", false) + + if got := m.AgentName("testproj"); got != "amp" { + t.Errorf("AgentName = %q, want %q", got, "amp") + } +} + +// E-PENPAL-AGENT-SELF-ID: verifies AgentName returns "agent" when no session exists. +func TestAgentName_DefaultsToAgent(t *testing.T) { + m, _ := newTestManager(t) + + if got := m.AgentName("testproj"); got != "agent" { + t.Errorf("AgentName = %q, want %q", got, "agent") + } +} + +// E-PENPAL-AGENT-SELF-ID: verifies SimulateRunning sets AgentName to "claude". +func TestSimulateRunning_SetsAgentName(t *testing.T) { + m, _ := newTestManager(t) + + m.SimulateRunning("testproj", 1000, 200000, 0.5, 1) + + if got := m.AgentName("testproj"); got != "claude" { + t.Errorf("AgentName = %q, want %q", got, "claude") + } +} + +// E-PENPAL-CLI-ATTACH: verifies Detach removes the session cleanly. +func TestDetach_RemovesSession(t *testing.T) { + m, _ := newTestManager(t) + + sess, _ := m.Attach("testproj", "", "claude", false) + + m.Detach(sess.Token) + + if m.HasActiveAgent("testproj") { + t.Error("expected no active agent after Detach") + } + + _, err := m.ValidateSession(sess.Token) + if err == nil { + t.Fatal("expected session to be gone after Detach") + } +} diff --git a/apps/penpal/internal/comments/comments.go b/apps/penpal/internal/comments/comments.go index a508642e..cc66c9ca 100644 --- a/apps/penpal/internal/comments/comments.go +++ b/apps/penpal/internal/comments/comments.go @@ -10,15 +10,17 @@ import ( "github.com/loganj/penpal/internal/cache" ) +// NOTE: Heartbeat-based agent presence tracking has been removed. +// Agent presence is now determined solely by agents.Manager.HasActiveAgent(), +// which checks both spawned processes and CLI sessions. + // Store manages comment threads and reviews for project files. // It uses sidecar JSON files stored alongside the thoughts directory. type Store struct { cache *cache.Cache activity *activity.Tracker - mu sync.Mutex // serializes file writes per-project - heartbeats map[string]time.Time // key: "project:filePath" -> last agent poll time - heartMu sync.RWMutex - changed chan struct{} // closed on every Save, then replaced + mu sync.Mutex // serializes file writes per-project + changed chan struct{} // closed on every Save, then replaced changedMu sync.Mutex changeSeq uint64 // monotonic counter incremented on each change workingMu sync.RWMutex @@ -101,11 +103,10 @@ type FileInReview struct { // NewStore creates a new comment Store backed by the given cache. func NewStore(c *cache.Cache, act *activity.Tracker) *Store { return &Store{ - cache: c, - activity: act, - heartbeats: make(map[string]time.Time), - changed: make(chan struct{}), - working: make(map[string]workingEntry), + cache: c, + activity: act, + changed: make(chan struct{}), + working: make(map[string]workingEntry), } } @@ -155,60 +156,6 @@ func (s *Store) WaitForChangeSince(ctx context.Context, sinceSeq uint64) (uint64 } } -// RecordHeartbeat records the current time as the last agent poll for the -// given project and file path. -// E-PENPAL-HEARTBEAT: records agent activity in in-memory heartbeats map. -func (s *Store) RecordHeartbeat(projectName, filePath string) { - s.heartMu.Lock() - defer s.heartMu.Unlock() - if s.heartbeats == nil { - s.heartbeats = make(map[string]time.Time) - } - s.heartbeats[projectName+":"+filePath] = time.Now() -} - -// IsAgentActive returns true if an agent has polled for the given file -// within the last 60 seconds. -// E-PENPAL-HEARTBEAT: returns true if heartbeat is <60s old. -func (s *Store) IsAgentActive(projectName, filePath string) bool { - s.heartMu.RLock() - defer s.heartMu.RUnlock() - if s.heartbeats == nil { - return false - } - t, ok := s.heartbeats[projectName+":"+filePath] - if !ok { - return false - } - return time.Since(t) < 60*time.Second -} - -// ClearProjectHeartbeats removes all heartbeat entries for a project. -func (s *Store) ClearProjectHeartbeats(projectName string) { - prefix := projectName + ":" - s.heartMu.Lock() - defer s.heartMu.Unlock() - for key := range s.heartbeats { - if strings.HasPrefix(key, prefix) { - delete(s.heartbeats, key) - } - } -} - -// IsProjectActive returns true if any agent has polled for any file (or the -// project itself) within the last 60 seconds. -func (s *Store) IsProjectActive(projectName string) bool { - s.heartMu.RLock() - defer s.heartMu.RUnlock() - prefix := projectName + ":" - for key, t := range s.heartbeats { - if strings.HasPrefix(key, prefix) && time.Since(t) < 60*time.Second { - return true - } - } - return false -} - // SetOnWorking sets a callback invoked when working state changes. func (s *Store) SetOnWorking(fn func(project string)) { s.workingMu.Lock() diff --git a/apps/penpal/internal/comments/comments_test.go b/apps/penpal/internal/comments/comments_test.go index 4ce04015..9bfd80b0 100644 --- a/apps/penpal/internal/comments/comments_test.go +++ b/apps/penpal/internal/comments/comments_test.go @@ -707,6 +707,181 @@ func TestClearWorkingRemovesAfterCommentID(t *testing.T) { } } +// E-PENPAL-WORKING: verifies AddCommentForWorktree auto-sets InReplyTo and WorkingStartedAt +// from stored working entry for agent-role comments, then clears working. +func TestAddCommentForWorktreeAutoHandlesWorkingForAgent(t *testing.T) { + store := newTestStore(t) + + anchor := Anchor{SelectedText: "text"} + thread, err := store.CreateThread(testProject, "doc.md", anchor, Comment{ + Author: "alice", Role: "human", Body: "Please review", + }) + if err != nil { + t.Fatalf("CreateThread: %v", err) + } + + // Simulate agent reading the thread (sets working indicator) + firstCommentID := thread.Comments[0].ID + store.SetWorking(testProject, "doc.md", thread.ID, firstCommentID) + + // Agent replies — should auto-populate InReplyTo and WorkingStartedAt + agentComment := Comment{Author: "claude", Role: "agent", Body: "I can help."} + updated, err := store.AddComment(testProject, "doc.md", thread.ID, agentComment) + if err != nil { + t.Fatalf("AddComment: %v", err) + } + + reply := updated.Comments[1] + if reply.InReplyTo != firstCommentID { + t.Errorf("InReplyTo = %q, want %q (from working entry)", reply.InReplyTo, firstCommentID) + } + if reply.WorkingStartedAt == nil { + t.Error("expected WorkingStartedAt to be set from working entry") + } + + // Working indicator should be cleared + if store.IsWorking(testProject, "doc.md", thread.ID) { + t.Error("expected working indicator to be cleared after agent reply") + } + if got := store.WorkingAfterCommentID(testProject, "doc.md", thread.ID); got != "" { + t.Errorf("WorkingAfterCommentID = %q, want empty after clear", got) + } +} + +// E-PENPAL-WORKING: verifies AddCommentForWorktree does NOT touch working for human-role comments. +func TestAddCommentForWorktreeSkipsWorkingForHuman(t *testing.T) { + store := newTestStore(t) + + anchor := Anchor{SelectedText: "text"} + thread, err := store.CreateThread(testProject, "doc.md", anchor, Comment{ + Author: "claude", Role: "agent", Body: "Here's my review", + }) + if err != nil { + t.Fatalf("CreateThread: %v", err) + } + + // Human replies — should not set WorkingStartedAt or clear working + humanComment := Comment{Author: "alice", Role: "human", Body: "Thanks"} + updated, err := store.AddComment(testProject, "doc.md", thread.ID, humanComment) + if err != nil { + t.Fatalf("AddComment: %v", err) + } + + reply := updated.Comments[1] + if reply.WorkingStartedAt != nil { + t.Error("expected WorkingStartedAt to be nil for human comment") + } +} + +// E-PENPAL-WORKING: verifies MarkFileThreadsRead sets working for threads awaiting agent response. +func TestMarkFileThreadsRead(t *testing.T) { + store := newTestStore(t) + + // Thread with last comment from human — should trigger SetWorking + threads := []Thread{ + { + ID: "t1", + Status: "open", + Comments: []Comment{ + {ID: "c1", Author: "alice", Role: "human", Body: "Hello"}, + }, + }, + { + ID: "t2", + Status: "open", + Comments: []Comment{ + {ID: "c2", Author: "alice", Role: "human", Body: "Question"}, + {ID: "c3", Author: "claude", Role: "agent", Body: "Answer"}, + }, + }, + } + + store.MarkFileThreadsRead("proj", "file.md", threads) + + // t1: last comment is human → working should be set + if !store.IsWorking("proj", "file.md", "t1") { + t.Error("expected t1 to be working (last comment is human)") + } + if got := store.WorkingAfterCommentID("proj", "file.md", "t1"); got != "c1" { + t.Errorf("t1 WorkingAfterCommentID = %q, want %q", got, "c1") + } + + // t2: last comment is agent → working should NOT be set + if store.IsWorking("proj", "file.md", "t2") { + t.Error("expected t2 to not be working (last comment is agent)") + } +} + +// E-PENPAL-WORKING: verifies MarkThreadsRead sets working for ThreadWithFile entries. +func TestMarkThreadsRead(t *testing.T) { + store := newTestStore(t) + + threads := []ThreadWithFile{ + { + Thread: Thread{ + ID: "t1", + Status: "open", + Comments: []Comment{ + {ID: "c1", Author: "alice", Role: "human", Body: "Review this"}, + }, + }, + FilePath: "file1.md", + }, + { + Thread: Thread{ + ID: "t2", + Status: "resolved", + Comments: []Comment{ + {ID: "c2", Author: "alice", Role: "human", Body: "Done"}, + }, + }, + FilePath: "file2.md", + }, + } + + store.MarkThreadsRead("proj", threads) + + // t1 is open with human last → working set + if !store.IsWorking("proj", "file1.md", "t1") { + t.Error("expected t1 to be working") + } + + // t2 is resolved → working NOT set + if store.IsWorking("proj", "file2.md", "t2") { + t.Error("expected t2 to not be working (resolved)") + } +} + +// E-PENPAL-WORKING: verifies MarkFileThreadsRead refreshes existing entries instead of overwriting. +func TestMarkFileThreadsReadRefreshesExisting(t *testing.T) { + store := newTestStore(t) + + // Pre-set working with a specific afterCommentID + store.SetWorking("proj", "file.md", "t1", "c1") + originalStartedAt := store.WorkingStartedAt("proj", "file.md", "t1") + + threads := []Thread{ + { + ID: "t1", + Status: "open", + Comments: []Comment{ + {ID: "c1", Author: "alice", Role: "human", Body: "Hello"}, + }, + }, + } + + // MarkFileThreadsRead should refresh (same afterCommentID) not re-set + store.MarkFileThreadsRead("proj", "file.md", threads) + + // startedAt should be preserved (refresh, not re-set) + if got := store.WorkingStartedAt("proj", "file.md", "t1"); got != originalStartedAt { + t.Errorf("WorkingStartedAt changed after refresh: got %v, want %v", got, originalStartedAt) + } + if got := store.WorkingAfterCommentID("proj", "file.md", "t1"); got != "c1" { + t.Errorf("WorkingAfterCommentID = %q, want %q", got, "c1") + } +} + func TestLegacyJSONWithoutInReplyToLoads(t *testing.T) { store := newTestStore(t) diff --git a/apps/penpal/internal/comments/heartbeat_test.go b/apps/penpal/internal/comments/heartbeat_test.go deleted file mode 100644 index 8d1ba936..00000000 --- a/apps/penpal/internal/comments/heartbeat_test.go +++ /dev/null @@ -1,112 +0,0 @@ -package comments - -import ( - "testing" - - "github.com/loganj/penpal/internal/cache" -) - -// newMinimalStore creates a Store without a backing project on disk, -// suitable for testing in-memory-only functionality like heartbeats. -func newMinimalStore() *Store { - return NewStore(cache.New(), nil) -} - -// E-PENPAL-HEARTBEAT: RecordHeartbeat then IsAgentActive returns true. -func TestRecordHeartbeatThenActive(t *testing.T) { - s := newMinimalStore() - s.RecordHeartbeat("proj", "file.md") - - if !s.IsAgentActive("proj", "file.md") { - t.Error("expected IsAgentActive to return true after RecordHeartbeat") - } -} - -// E-PENPAL-HEARTBEAT: IsAgentActive returns false for unrecorded project. -func TestIsAgentActiveUnrecorded(t *testing.T) { - s := newMinimalStore() - - if s.IsAgentActive("unknown", "file.md") { - t.Error("expected IsAgentActive to return false for unrecorded project") - } -} - -// E-PENPAL-HEARTBEAT: IsAgentActive returns false for recorded project but different file. -func TestIsAgentActiveDifferentFile(t *testing.T) { - s := newMinimalStore() - s.RecordHeartbeat("proj", "file1.md") - - if s.IsAgentActive("proj", "file2.md") { - t.Error("expected IsAgentActive to return false for different file path") - } -} - -// E-PENPAL-HEARTBEAT: ClearProjectHeartbeats clears all heartbeats for a project. -func TestClearProjectHeartbeats(t *testing.T) { - s := newMinimalStore() - - // Record heartbeats for multiple files in the same project - s.RecordHeartbeat("proj", "file1.md") - s.RecordHeartbeat("proj", "file2.md") - s.RecordHeartbeat("proj", "sub/file3.md") - - // Also record a heartbeat for a different project - s.RecordHeartbeat("other", "doc.md") - - // Verify all are active - if !s.IsAgentActive("proj", "file1.md") { - t.Fatal("setup: expected proj/file1.md to be active") - } - if !s.IsAgentActive("proj", "file2.md") { - t.Fatal("setup: expected proj/file2.md to be active") - } - if !s.IsAgentActive("proj", "sub/file3.md") { - t.Fatal("setup: expected proj/sub/file3.md to be active") - } - - // Clear heartbeats for "proj" - s.ClearProjectHeartbeats("proj") - - // All "proj" heartbeats should be gone - if s.IsAgentActive("proj", "file1.md") { - t.Error("expected proj/file1.md to be inactive after ClearProjectHeartbeats") - } - if s.IsAgentActive("proj", "file2.md") { - t.Error("expected proj/file2.md to be inactive after ClearProjectHeartbeats") - } - if s.IsAgentActive("proj", "sub/file3.md") { - t.Error("expected proj/sub/file3.md to be inactive after ClearProjectHeartbeats") - } - - // The "other" project should still be active - if !s.IsAgentActive("other", "doc.md") { - t.Error("expected other/doc.md to remain active after clearing 'proj'") - } -} - -// E-PENPAL-HEARTBEAT: ClearProjectHeartbeats is a no-op when project has no heartbeats. -func TestClearProjectHeartbeatsNoop(t *testing.T) { - s := newMinimalStore() - s.RecordHeartbeat("keep", "file.md") - - // Clearing a nonexistent project should not panic or affect others - s.ClearProjectHeartbeats("nonexistent") - - if !s.IsAgentActive("keep", "file.md") { - t.Error("expected 'keep' heartbeat to survive clearing nonexistent project") - } -} - -// E-PENPAL-HEARTBEAT: IsProjectActive returns true when any file heartbeat exists. -func TestIsProjectActive(t *testing.T) { - s := newMinimalStore() - - if s.IsProjectActive("proj") { - t.Error("expected IsProjectActive to return false before any heartbeats") - } - - s.RecordHeartbeat("proj", "file.md") - if !s.IsProjectActive("proj") { - t.Error("expected IsProjectActive to return true after RecordHeartbeat") - } -} diff --git a/apps/penpal/internal/comments/operations.go b/apps/penpal/internal/comments/operations.go index 670374f0..698a5245 100644 --- a/apps/penpal/internal/comments/operations.go +++ b/apps/penpal/internal/comments/operations.go @@ -1,6 +1,7 @@ package comments import ( + "context" "encoding/json" "fmt" "os" @@ -62,11 +63,32 @@ func (s *Store) AddComment(projectName, filePath, threadID string, comment Comme } // AddCommentForWorktree appends a comment scoped to a specific worktree. +// For agent-role comments, automatically sets InReplyTo and WorkingStartedAt +// from the stored working entry, then clears the working indicator after writing. +// This consolidates working indicator logic so MCP tools and REST handlers +// don't need to manage it themselves. // E-PENPAL-THREAD-MUTEX: serializes comment addition via per-project sync.Mutex. +// E-PENPAL-WORKING: auto-handles working indicators for agent replies. func (s *Store) AddCommentForWorktree(projectName, filePath, worktree, threadID string, comment Comment) (*Thread, error) { s.mu.Lock() defer s.mu.Unlock() + // For agent replies, populate InReplyTo and WorkingStartedAt from stored + // working entry. Reads are inside mu.Lock() to prevent two concurrent + // agent replies from reading the same working state. + if comment.Role == "agent" { + if comment.InReplyTo == "" { + if afterID := s.WorkingAfterCommentID(projectName, filePath, threadID); afterID != "" { + comment.InReplyTo = afterID + } + } + if comment.WorkingStartedAt == nil { + if startedAt := s.WorkingStartedAt(projectName, filePath, threadID); !startedAt.IsZero() { + comment.WorkingStartedAt = &startedAt + } + } + } + fc, err := s.LoadForWorktree(projectName, filePath, worktree) if err != nil { return nil, err @@ -88,6 +110,13 @@ func (s *Store) AddCommentForWorktree(projectName, filePath, worktree, threadID s.activity.Record(activity.Comment, projectName, filePath) } t := fc.Threads[i] + + // Clear working AFTER writing so SSE broadcasts trigger a + // fetchThreads that reads the updated file. + if comment.Role == "agent" { + s.ClearWorking(projectName, filePath, threadID) + } + return &t, nil } } @@ -95,6 +124,41 @@ func (s *Store) AddCommentForWorktree(projectName, filePath, worktree, threadID return nil, fmt.Errorf("thread not found: %s", threadID) } +// markThreadWorking updates the working indicator for a single thread if it is +// open and the last comment is from a human. If a working entry already exists +// for the same comment, only the expiry timer is refreshed. +// E-PENPAL-WORKING: shared helper for MarkThreadsRead and MarkFileThreadsRead. +func (s *Store) markThreadWorking(project, filePath string, t Thread) { + if t.Status != "open" || len(t.Comments) == 0 || t.Comments[len(t.Comments)-1].Role != "human" { + return + } + lastCommentID := t.Comments[len(t.Comments)-1].ID + if s.WorkingAfterCommentID(project, filePath, t.ID) == lastCommentID { + s.RefreshWorkingTimestamp(project, filePath, t.ID) + } else { + s.SetWorking(project, filePath, t.ID, lastCommentID) + } +} + +// MarkThreadsRead sets working indicators for all open threads where the last +// comment is from a human. Call this when an agent reads thread listings so +// the UI shows the "working" pulsing dot. +// E-PENPAL-WORKING: consolidates set-working-on-read logic from MCP and REST layers. +func (s *Store) MarkThreadsRead(project string, threads []ThreadWithFile) { + for _, t := range threads { + s.markThreadWorking(project, t.FilePath, t.Thread) + } +} + +// MarkFileThreadsRead sets working indicators for open threads on a specific file +// where the last comment is from a human. +// E-PENPAL-WORKING: consolidates set-working-on-read logic for single-file operations. +func (s *Store) MarkFileThreadsRead(project, filePath string, threads []Thread) { + for _, t := range threads { + s.markThreadWorking(project, filePath, t) + } +} + // ResolveThread marks a thread as resolved. // E-PENPAL-THREAD-MODEL: transitions thread status to "resolved" with ResolvedAt/ResolvedBy. func (s *Store) ResolveThread(projectName, filePath, threadID, resolvedBy string) error { @@ -238,6 +302,71 @@ func (s *Store) ListThreadsByStatusForWorktree(projectName, status, worktree str return results, nil } +// WaitResult holds the outcome of a wait-for-changes operation. +// E-PENPAL-CHANGE-SEQ: result of WaitAndEnrich combining wait + enrich + refresh. +type WaitResult struct { + Changed bool `json:"changed"` + Seq uint64 `json:"seq"` + Files []WaitResultFile `json:"files"` +} + +// WaitResultFile describes a file in review with optional pending thread detail. +type WaitResultFile struct { + FilePath string `json:"filePath"` + OpenThreads int `json:"openThreads"` + Threads []Thread `json:"threads,omitempty"` +} + +// WaitAndEnrich blocks until comments change (or context cancels), then returns +// enriched files with pending threads and refreshed working indicators. +// This consolidates the duplicated wait-enrich-refresh logic from the MCP +// penpal_wait_for_changes tool and the REST handleAgentWait handler. +// E-PENPAL-CHANGE-SEQ: uses WaitForChangeSince to block until changeSeq advances. +// E-PENPAL-MCP-WORKING: refreshes working timestamps during wait cycles to prevent expiry. +func (s *Store) WaitAndEnrich(ctx context.Context, project, worktree string, sinceSeq uint64) (*WaitResult, error) { + seq, waitErr := s.WaitForChangeSince(ctx, sinceSeq) + changed := waitErr == nil + + files, err := s.ListFilesInReviewForWorktree(project, worktree) + if err != nil { + return nil, err + } + + // Single loop: refresh working indicators for all files, and collect + // pending threads when changes were detected. + var pendingByFile map[string][]Thread + if changed { + pendingByFile = make(map[string][]Thread) + } + for _, f := range files { + fc, loadErr := s.LoadForWorktree(project, f.FilePath, worktree) + if loadErr != nil { + continue + } + s.MarkFileThreadsRead(project, f.FilePath, fc.Threads) + if changed { + for _, t := range fc.Threads { + if t.Status == "open" && len(t.Comments) > 0 && t.Comments[len(t.Comments)-1].Role == "human" { + pendingByFile[f.FilePath] = append(pendingByFile[f.FilePath], t) + } + } + } + } + + resultFiles := make([]WaitResultFile, len(files)) + for i, f := range files { + resultFiles[i] = WaitResultFile{ + FilePath: f.FilePath, + OpenThreads: f.OpenThreads, + } + if changed { + resultFiles[i].Threads = pendingByFile[f.FilePath] + } + } + + return &WaitResult{Changed: changed, Seq: seq, Files: resultFiles}, nil +} + // HasPendingHumanComments returns true if any open thread in the project // has a human as the last commenter (i.e., awaiting agent response). func (s *Store) HasPendingHumanComments(projectName string) bool { diff --git a/apps/penpal/internal/mcpserver/mcpserver.go b/apps/penpal/internal/mcpserver/mcpserver.go index 647f65f9..96a35642 100644 --- a/apps/penpal/internal/mcpserver/mcpserver.go +++ b/apps/penpal/internal/mcpserver/mcpserver.go @@ -12,14 +12,15 @@ import ( // protocol. It exposes comment and review tools so AI agents can interact // with penpal programmatically. // E-PENPAL-MCP-TRANSPORT: Streamable HTTP transport via mcp.NewStreamableHTTPHandler. -func NewHandler(store *comments.Store, c *cache.Cache) http.Handler { +// E-PENPAL-AGENT-SELF-ID: agentNameFunc derives the comment author from the session. +func NewHandler(store *comments.Store, c *cache.Cache, agentNameFunc func(project string) string) http.Handler { server := mcp.NewServer(&mcp.Implementation{ Name: "penpal", Version: "1.0.0", }, &mcp.ServerOptions{ Instructions: "Penpal operates on markdown files for collaborative document review with humans. File paths are relative to the project root (e.g., thoughts/plans/foo.md). It is NOT for code review.\n\nWhen your reply asks for confirmation or presents options, include suggestedReplies (up to 3 short strings) so the human can respond with one click. Keep suggestions short (1-4 words). Only suggest replies that provide meaningful content the human would actually type — do NOT suggest generic responses like \"looks good\", \"yes\", \"no\", or \"needs changes\" that are redundant with the reply and resolve buttons.", }) - registerTools(server, store, c) + registerTools(server, store, c, agentNameFunc) return mcp.NewStreamableHTTPHandler(func(r *http.Request) *mcp.Server { return server }, nil) diff --git a/apps/penpal/internal/mcpserver/tools.go b/apps/penpal/internal/mcpserver/tools.go index 2e4806eb..d2fc9f6d 100644 --- a/apps/penpal/internal/mcpserver/tools.go +++ b/apps/penpal/internal/mcpserver/tools.go @@ -77,11 +77,11 @@ func textResult(v any) (*mcp.CallToolResult, error) { // registerTools adds all penpal MCP tools to the server. // E-PENPAL-MCP-TOOLS: registers penpal_find_project, penpal_list_threads, penpal_read_thread, penpal_reply, penpal_create_thread, penpal_files_in_review, penpal_wait_for_changes. -func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { +// E-PENPAL-AGENT-SELF-ID: agentNameFunc derives the comment author from the session. +func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache, agentNameFunc func(project string) string) { // penpal_list_threads // E-PENPAL-MCP-TOOLS: penpal_list_threads lists threads by file or project-wide. // E-PENPAL-MCP-WORKING: auto-sets working indicator for threads where last comment is from human. - // E-PENPAL-HEARTBEAT: records heartbeat on each tool call. mcp.AddTool(server, &mcp.Tool{ Name: "penpal_list_threads", Description: "List comment threads on documentation files. Paths are relative to the project root (e.g., thoughts/plans/foo.md). When path is omitted, returns all open threads across the project. Optionally filter by status (open/resolved).", @@ -92,7 +92,6 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { if input.Path == "" { // List threads across the entire project, filtered by status - store.RecordHeartbeat(input.Project, "") status := input.Status if status == "" { status = "open" @@ -101,23 +100,12 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { if err != nil { return nil, nil, err } - // Record heartbeat and set working for each thread awaiting response - seen := make(map[string]bool) - for _, t := range threads { - if !seen[t.FilePath] { - store.RecordHeartbeat(input.Project, t.FilePath) - seen[t.FilePath] = true - } - if t.Status == "open" && len(t.Comments) > 0 && t.Comments[len(t.Comments)-1].Role == "human" { - store.SetWorking(input.Project, t.FilePath, t.ID, t.Comments[len(t.Comments)-1].ID) - } - } + store.MarkThreadsRead(input.Project, threads) res, err := textResult(threads) return res, nil, err } // Load threads for a specific file - store.RecordHeartbeat(input.Project, input.Path) fc, err := store.LoadForWorktree(input.Project, input.Path, input.Worktree) if err != nil { return nil, nil, err @@ -128,10 +116,8 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { if input.Status == "" || t.Status == input.Status { filtered = append(filtered, t) } - if t.Status == "open" && len(t.Comments) > 0 && t.Comments[len(t.Comments)-1].Role == "human" { - store.SetWorking(input.Project, input.Path, t.ID, t.Comments[len(t.Comments)-1].ID) - } } + store.MarkFileThreadsRead(input.Project, input.Path, fc.Threads) res, err := textResult(filtered) return res, nil, err }) @@ -139,7 +125,6 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { // penpal_read_thread // E-PENPAL-MCP-TOOLS: penpal_read_thread returns full thread with all comments. // E-PENPAL-MCP-WORKING: auto-sets working indicator when last comment is from human. - // E-PENPAL-HEARTBEAT: records heartbeat on each tool call. mcp.AddTool(server, &mcp.Tool{ Name: "penpal_read_thread", Description: "Read a full comment thread on a document. Path is relative to project root (e.g., thoughts/plans/foo.md). Returns the complete thread JSON with all comments.", @@ -148,8 +133,6 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { return nil, nil, fmt.Errorf("project, path, and threadId are all required") } - store.RecordHeartbeat(input.Project, input.Path) - fc, err := store.LoadForWorktree(input.Project, input.Path, input.Worktree) if err != nil { return nil, nil, err @@ -157,10 +140,7 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { for _, t := range fc.Threads { if t.ID == input.ThreadID { - // Set working indicator if last comment is from a human - if len(t.Comments) > 0 && t.Comments[len(t.Comments)-1].Role == "human" { - store.SetWorking(input.Project, input.Path, input.ThreadID, t.Comments[len(t.Comments)-1].ID) - } + store.MarkFileThreadsRead(input.Project, input.Path, []comments.Thread{t}) res, err := textResult(t) return res, nil, err } @@ -179,28 +159,20 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { return nil, nil, fmt.Errorf("project, path, threadId, and body are all required") } - // E-PENPAL-MCP-WORKING: set InReplyTo and WorkingStartedAt from stored working entry. - afterID := store.WorkingAfterCommentID(input.Project, input.Path, input.ThreadID) - startedAt := store.WorkingStartedAt(input.Project, input.Path, input.ThreadID) - + // Working indicator handling (InReplyTo, WorkingStartedAt, ClearWorking) + // is done automatically by AddCommentForWorktree for agent-role comments. + // E-PENPAL-AGENT-SELF-ID: derive author from session via agentNameFunc. comment := comments.Comment{ - Author: "claude", + Author: agentNameFunc(input.Project), Role: "agent", Body: input.Body, SuggestedReplies: input.SuggestedReplies, - InReplyTo: afterID, - } - if !startedAt.IsZero() { - comment.WorkingStartedAt = &startedAt } thread, err := store.AddCommentForWorktree(input.Project, input.Path, input.Worktree, input.ThreadID, comment) if err != nil { return nil, nil, err } - // Clear working AFTER writing the comment so the SSE broadcast - // triggers a fetchThreads that reads the updated file. - store.ClearWorking(input.Project, input.Path, input.ThreadID) res, err := textResult(thread) return res, nil, err }) @@ -271,8 +243,9 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { anchor.StartLine = line } + // E-PENPAL-AGENT-SELF-ID: derive author from session via agentNameFunc. comment := comments.Comment{ - Author: "claude", + Author: agentNameFunc(input.Project), Role: "agent", Body: input.Body, SuggestedReplies: input.SuggestedReplies, @@ -289,7 +262,6 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { // penpal_files_in_review // E-PENPAL-MCP-TOOLS: penpal_files_in_review lists files with open threads, enriched with oldest pending. // E-PENPAL-MCP-WORKING: auto-sets working indicator for oldest pending thread. - // E-PENPAL-HEARTBEAT: records heartbeat for each file in review. mcp.AddTool(server, &mcp.Tool{ Name: "penpal_files_in_review", Description: "List all documentation files currently in review for a project. File paths are relative to the project root (e.g., thoughts/plans/foo.md). Records a heartbeat for each file to signal agent presence in the penpal UI. For each file, includes all open threads and the full content of the oldest pending thread (where the last comment is from a human). The working indicator is set for the oldest pending thread so the UI shows the agent is working on it.", @@ -312,8 +284,6 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { enrichedFiles := make([]fileWithThreads, 0, len(files)) for _, f := range files { - store.RecordHeartbeat(input.Project, f.FilePath) - ef := fileWithThreads{ FilePath: f.FilePath, OpenThreads: f.OpenThreads, @@ -339,8 +309,8 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { if oldestPending != nil { ef.OldestPending = oldestPending - store.SetWorking(input.Project, f.FilePath, oldestPending.ID, oldestPending.Comments[len(oldestPending.Comments)-1].ID) } + store.MarkFileThreadsRead(input.Project, f.FilePath, fc.Threads) } enrichedFiles = append(enrichedFiles, ef) @@ -352,9 +322,7 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { // penpal_wait_for_changes // E-PENPAL-MCP-TOOLS: penpal_wait_for_changes blocks via 30s long-poll for comment changes. - // E-PENPAL-CHANGE-SEQ: uses WaitForChangeSince to block until changeSeq advances. - // E-PENPAL-MCP-WORKING: refreshes working timestamps during 30s cycle to prevent expiry. - // E-PENPAL-HEARTBEAT: records heartbeat at start and after waking. + // E-PENPAL-CHANGE-SEQ: uses WaitAndEnrich to block, enrich, and refresh working timestamps. mcp.AddTool(server, &mcp.Tool{ Name: "penpal_wait_for_changes", Description: "Block until comment threads change for a project (new thread, reply, resolve, or reopen), or until timeout (30s). Returns the current files in review. Use this in a loop instead of polling penpal_files_in_review. Also records agent heartbeat. Pass the `seq` value from the previous response as `sinceSeq` to avoid missing changes between calls.", @@ -363,103 +331,14 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { return nil, nil, fmt.Errorf("project is required") } - // Record heartbeat at start of wait - store.RecordHeartbeat(input.Project, "") - waitCtx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() - seq, waitErr := store.WaitForChangeSince(waitCtx, input.SinceSeq) - changed := waitErr == nil - - // Record heartbeat after waking - store.RecordHeartbeat(input.Project, "") - - files, err := store.ListFilesInReviewForWorktree(input.Project, input.Worktree) + result, err := store.WaitAndEnrich(waitCtx, input.Project, input.Worktree, input.SinceSeq) if err != nil { return nil, nil, err } - for _, f := range files { - store.RecordHeartbeat(input.Project, f.FilePath) - } - - // When changed, enrich response with pending threads. - // Set working indicators first so the UI shows dots before the agent - // receives the response and potentially replies quickly. - if changed { - type fileWithThreads struct { - FilePath string `json:"filePath"` - OpenThreads int `json:"openThreads"` - Threads []comments.Thread `json:"threads,omitempty"` - } - - // First pass: set working indicators for all threads awaiting a response - type pendingThread struct { - filePath string - thread comments.Thread - } - var pending []pendingThread - for _, f := range files { - fc, loadErr := store.LoadForWorktree(input.Project, f.FilePath, input.Worktree) - if loadErr == nil { - for _, t := range fc.Threads { - if t.Status == "open" && len(t.Comments) > 0 && t.Comments[len(t.Comments)-1].Role == "human" { - lastCommentID := t.Comments[len(t.Comments)-1].ID - if store.WorkingAfterCommentID(input.Project, f.FilePath, t.ID) == lastCommentID { - store.RefreshWorkingTimestamp(input.Project, f.FilePath, t.ID) - } else { - store.SetWorking(input.Project, f.FilePath, t.ID, lastCommentID) - } - pending = append(pending, pendingThread{filePath: f.FilePath, thread: t}) - } - } - } - } - - // Second pass: build response with pending threads - pendingByFile := make(map[string][]comments.Thread) - for _, p := range pending { - pendingByFile[p.filePath] = append(pendingByFile[p.filePath], p.thread) - } - var enrichedFiles []fileWithThreads - for _, f := range files { - ef := fileWithThreads{ - FilePath: f.FilePath, - OpenThreads: f.OpenThreads, - Threads: pendingByFile[f.FilePath], - } - enrichedFiles = append(enrichedFiles, ef) - } - - result := map[string]any{ - "changed": true, - "seq": seq, - "files": enrichedFiles, - } - res, err := textResult(result) - return res, nil, err - } - - // Refresh working timestamps for threads still awaiting a response - // so they survive across 30s wait cycles. Use RefreshWorkingTimestamp - // to preserve the afterCommentID — the agent hasn't re-read these threads. - for _, f := range files { - fc, loadErr := store.LoadForWorktree(input.Project, f.FilePath, input.Worktree) - if loadErr == nil { - for _, t := range fc.Threads { - if t.Status == "open" && len(t.Comments) > 0 && t.Comments[len(t.Comments)-1].Role == "human" { - store.RefreshWorkingTimestamp(input.Project, f.FilePath, t.ID) - } - } - } - } - - result := map[string]any{ - "changed": false, - "seq": seq, - "files": files, - } res, err := textResult(result) return res, nil, err }) diff --git a/apps/penpal/internal/mcpserver/tools_test.go b/apps/penpal/internal/mcpserver/tools_test.go index 905ad53b..c756e1e4 100644 --- a/apps/penpal/internal/mcpserver/tools_test.go +++ b/apps/penpal/internal/mcpserver/tools_test.go @@ -40,7 +40,7 @@ func setup(t *testing.T) (*testEnv, func()) { Origin: "standalone", }}) - handler := NewHandler(cs, c) + handler := NewHandler(cs, c, func(project string) string { return "claude" }) ts := httptest.NewServer(handler) ctx := context.Background() diff --git a/apps/penpal/internal/server/agents.go b/apps/penpal/internal/server/agents.go index d8b1ce5b..42f4a429 100644 --- a/apps/penpal/internal/server/agents.go +++ b/apps/penpal/internal/server/agents.go @@ -1,11 +1,18 @@ package server import ( + "bytes" + "context" "encoding/json" "log" "net/http" + "os" + "path/filepath" + "strconv" + "time" "github.com/loganj/penpal/internal/agents" + "github.com/loganj/penpal/internal/comments" ) // agentStatusResponse wraps AgentStatus with server-level fields. @@ -16,6 +23,7 @@ type agentStatusResponse struct { // handleAgentStatus handles GET /api/agents?project=X. // E-PENPAL-API-ROUTES: GET /api/agents endpoint. +// E-PENPAL-AGENT-ACTIVE-UNIFIED: checks both spawned agents and CLI sessions. func (s *Server) handleAgentStatus(w http.ResponseWriter, r *http.Request) { projectName := r.URL.Query().Get("project") if projectName == "" { @@ -34,6 +42,11 @@ func (s *Server) handleAgentStatus(w http.ResponseWriter, r *http.Request) { } } + // If no spawned agent is running, check for an external CLI session. + if !status.Running && s.isAgentActive(projectName) { + status.Running = true + } + resp := agentStatusResponse{AgentStatus: status} if !status.Running && s.comments != nil && s.comments.HasPendingHumanComments(projectName) { resp.NeedsAgent = true @@ -98,10 +111,8 @@ func (s *Server) handleAgentStop(w http.ResponseWriter, r *http.Request) { return } - if err := s.agents.Stop(projectName); err != nil { - http.Error(w, err.Error(), http.StatusNotFound) - return - } + // E-PENPAL-CLI-CONTENTION: stop both spawned and CLI-attached agents. + s.agents.StopAny(projectName) w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(map[string]bool{"ok": true}) @@ -112,13 +123,263 @@ func (s *Server) handleAgentStop(w http.ResponseWriter, r *http.Request) { // 2. No agent is already running for this project // // E-PENPAL-AGENT-AUTOSTART: maybeStartAgent after handleCreateThread/handleAddComment. +// E-PENPAL-CLI-CONTENTION: skips auto-start when an external CLI agent is attached. func (s *Server) maybeStartAgent(projectName, role string) { if role != "human" || s.agents == nil { return } + if s.agents.HasActiveAgent(projectName) { + return + } go func() { if _, err := s.agents.Start(projectName); err != nil { log.Printf("Auto-start agent for %s: %v", projectName, err) } }() } + +// handleAgentAttach handles POST /api/agents/attach. +// E-PENPAL-CLI-ATTACH: resolves path to project and creates an external agent session. +func (s *Server) handleAgentAttach(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } + + var req struct { + Path string `json:"path"` + Force bool `json:"force"` + Agent string `json:"agent"` // self-reported agent name (e.g., "amp", "claude") + } + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + http.Error(w, "invalid JSON: "+err.Error(), http.StatusBadRequest) + return + } + + if req.Path == "" { + http.Error(w, "path is required", http.StatusBadRequest) + return + } + + absPath, err := filepath.Abs(req.Path) + if err != nil { + http.Error(w, "invalid path: "+err.Error(), http.StatusBadRequest) + return + } + + if _, err := os.Stat(absPath); err != nil { + http.Error(w, "path not found: "+absPath, http.StatusBadRequest) + return + } + + project, worktree := s.cache.FindProjectByPathWithWorktree(absPath) + if project == nil { + http.Error(w, "no project found for path: "+absPath, http.StatusNotFound) + return + } + + if s.agents == nil { + http.Error(w, "agent manager not available", http.StatusServiceUnavailable) + return + } + + projectQN := project.QualifiedName() + agentName := req.Agent + if agentName == "" { + agentName = "agent" + } + // E-PENPAL-AGENT-SELF-ID: pass agent name from request to session. + sess, err := s.agents.Attach(projectQN, worktree, agentName, req.Force) + if err != nil { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusConflict) + json.NewEncoder(w).Encode(map[string]string{"error": err.Error()}) + return + } + + // Trigger the open flow in the background so the file appears in the UI. + go func() { + openBody, _ := json.Marshal(map[string]string{"path": absPath}) + openReq, _ := http.NewRequest(http.MethodPost, "/api/open", bytes.NewReader(openBody)) + openReq.Header.Set("Content-Type", "application/json") + dw := &discardResponseWriter{} + s.handleAPIOpen(dw, openReq) + if dw.statusCode >= 400 { + log.Printf("Warning: open flow failed for %s (status %d)", absPath, dw.statusCode) + } + }() + + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]string{ + "project": projectQN, + "worktree": sess.Worktree, + "sessionToken": sess.Token, + "agentName": sess.AgentName, + }) +} + +// discardResponseWriter is a no-op http.ResponseWriter used when we need to +// call resolveOpenDirectory/resolveOpenFile for side effects only. +type discardResponseWriter struct { + header http.Header + statusCode int +} + +func (d *discardResponseWriter) Header() http.Header { + if d.header == nil { + d.header = make(http.Header) + } + return d.header +} +func (d *discardResponseWriter) Write(b []byte) (int, error) { return len(b), nil } +func (d *discardResponseWriter) WriteHeader(code int) { d.statusCode = code } + +// handleAgentWait handles GET /api/agents/wait?project=X&session=T&sinceSeq=N&worktree=W. +// E-PENPAL-CLI-AGENT-CMDS: long-poll endpoint for CLI agents, mirrors MCP penpal_wait_for_changes. +func (s *Server) handleAgentWait(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } + + projectName := r.URL.Query().Get("project") + sessionToken := r.URL.Query().Get("session") + worktree := r.URL.Query().Get("worktree") + + if projectName == "" || sessionToken == "" { + http.Error(w, "missing project or session parameter", http.StatusBadRequest) + return + } + + if s.agents == nil { + http.Error(w, "agent manager not available", http.StatusServiceUnavailable) + return + } + + // Validate session and enforce project ownership. + // E-PENPAL-SESSION-MGMT: validates token and project scoping before processing. + sess, err := s.agents.ValidateSession(sessionToken) + if err != nil { + http.Error(w, err.Error(), http.StatusUnauthorized) + return + } + if sess.Project != projectName { + http.Error(w, "session does not own this project", http.StatusForbidden) + return + } + + // Record heartbeat at start of wait. + s.agents.RecordSessionHeartbeat(sessionToken) + + // Use the session's worktree as the authoritative value. + worktree = sess.Worktree + + sinceSeq := uint64(0) + if v := r.URL.Query().Get("sinceSeq"); v != "" { + if n, err := strconv.ParseUint(v, 10, 64); err == nil { + sinceSeq = n + } + } + + // Long-poll: wait up to 30s for changes. + // E-PENPAL-CLI-AGENT-CMDS: uses WaitAndEnrich like MCP penpal_wait_for_changes. + waitCtx, cancel := context.WithTimeout(r.Context(), 30*time.Second) + defer cancel() + + var result *comments.WaitResult + result, err = s.comments.WaitAndEnrich(waitCtx, projectName, worktree, sinceSeq) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + + // Record heartbeat after waking. + s.agents.RecordSessionHeartbeat(sessionToken) + + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(result) +} + +// validateSessionParam checks the "session" query parameter if present. +// When a session token is provided, it validates the token and verifies that +// the session's project matches the "project" query parameter on the request. +// Returns true if the request should continue, false if an error was written. +// E-PENPAL-SESSION-MGMT: session validation helper for CLI agent requests. +func (s *Server) validateSessionParam(w http.ResponseWriter, r *http.Request) bool { + sessionToken := r.URL.Query().Get("session") + if sessionToken == "" { + return true // no session param — not a CLI agent request + } + if s.agents == nil { + http.Error(w, "agent manager not available", http.StatusServiceUnavailable) + return false + } + sess, err := s.agents.ValidateSession(sessionToken) + if err != nil { + http.Error(w, err.Error(), http.StatusUnauthorized) + return false + } + // Enforce project and worktree ownership: the session must own what's being accessed. + if projectName := r.URL.Query().Get("project"); projectName != "" && sess.Project != projectName { + http.Error(w, "session does not own this project", http.StatusForbidden) + return false + } + if wt := r.URL.Query().Get("worktree"); wt != "" && sess.Worktree != "" && wt != sess.Worktree { + http.Error(w, "session does not own this worktree", http.StatusForbidden) + return false + } + s.agents.RecordSessionHeartbeat(sessionToken) + return true +} + +// requireSessionForAgent checks that a valid session exists for agent-role +// writes via REST. Returns true if the request may proceed. If no session is +// provided or the session is invalid/mismatched, writes an HTTP error and +// returns false. +// E-PENPAL-CLI-CONTENTION: prevents session-less agent writes via REST. +func (s *Server) requireSessionForAgent(w http.ResponseWriter, r *http.Request, project string) bool { + sessionToken := r.URL.Query().Get("session") + if sessionToken == "" { + http.Error(w, "agent-role requests require a session token", http.StatusUnauthorized) + return false + } + if s.agents == nil { + http.Error(w, "agent manager not available", http.StatusServiceUnavailable) + return false + } + sess, err := s.agents.ValidateSession(sessionToken) + if err != nil { + http.Error(w, err.Error(), http.StatusUnauthorized) + return false + } + if sess.Project != project { + http.Error(w, "session does not own this project", http.StatusForbidden) + return false + } + return true +} + +// agentNameFromSession returns the agent name from the session identified by the +// "session" query parameter. Returns empty string if no session is present or invalid. +// E-PENPAL-AGENT-SELF-ID: derives comment author from session. +func (s *Server) agentNameFromSession(r *http.Request) string { + sessionToken := r.URL.Query().Get("session") + if sessionToken == "" || s.agents == nil { + return "" + } + sess, err := s.agents.ValidateSession(sessionToken) + if err != nil { + return "" + } + return sess.AgentName +} + +// isAgentActive returns true if any agent (spawned or CLI-attached) is active for the project. +// E-PENPAL-AGENT-ACTIVE-UNIFIED: unified agent presence check. +func (s *Server) isAgentActive(projectName string) bool { + if s.agents == nil { + return false + } + return s.agents.HasActiveAgent(projectName) +} + diff --git a/apps/penpal/internal/server/api_agents_test.go b/apps/penpal/internal/server/api_agents_test.go index 59f15d15..f600bd63 100644 --- a/apps/penpal/internal/server/api_agents_test.go +++ b/apps/penpal/internal/server/api_agents_test.go @@ -5,7 +5,10 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" + "time" "github.com/loganj/penpal/internal/agents" "github.com/loganj/penpal/internal/comments" @@ -79,6 +82,7 @@ func TestAPIAgentStatus_NoNeedsAgent_WhenAgentReplied(t *testing.T) { s, c, cs := testServer(t) dir := t.TempDir() seedProject(c, "test-proj", dir, nil) + token := attachSession(t, s, c, cs, "test-proj") // Create a thread with human comment directly anchor := comments.Anchor{SelectedText: "text"} @@ -96,7 +100,7 @@ func TestAPIAgentStatus_NoNeedsAgent_WhenAgentReplied(t *testing.T) { "role": "agent", "body": "Done", }) - req := httptest.NewRequest(http.MethodPost, "/api/threads/"+thread.ID+"/comments", bytes.NewReader(replyBody)) + req := httptest.NewRequest(http.MethodPost, "/api/threads/"+thread.ID+"/comments?session="+token, bytes.NewReader(replyBody)) req.Header.Set("Content-Type", "application/json") rec := httptest.NewRecorder() s.ServeHTTP(rec, req) @@ -273,3 +277,306 @@ func TestAPIAgentStatus_MissingProject(t *testing.T) { t.Errorf("expected 400, got %d", rec.Code) } } + +// E-PENPAL-CLI-ATTACH: verifies POST /api/agents/attach succeeds with valid path. +func TestAPIAgentAttach_Success(t *testing.T) { + s, c, cs := testServer(t) + dir := t.TempDir() + // Create a markdown file so the project path is valid. + os.MkdirAll(filepath.Join(dir, "thoughts"), 0755) + os.WriteFile(filepath.Join(dir, "thoughts", "plan.md"), []byte("# Plan"), 0644) + seedProject(c, "test-proj", dir, nil) + s.agents = agents.New(c, cs, 0) + + body, _ := json.Marshal(map[string]any{"path": dir}) + req := httptest.NewRequest(http.MethodPost, "/api/agents/attach", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + + var resp map[string]string + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("parse: %v", err) + } + if resp["project"] != "test-proj" { + t.Errorf("expected project=test-proj, got %q", resp["project"]) + } + if resp["sessionToken"] == "" { + t.Error("expected non-empty sessionToken") + } +} + +// E-PENPAL-CLI-CONTENTION: verifies double-attach without force returns 409. +func TestAPIAgentAttach_Conflict(t *testing.T) { + s, c, cs := testServer(t) + dir := t.TempDir() + os.MkdirAll(filepath.Join(dir, "thoughts"), 0755) + os.WriteFile(filepath.Join(dir, "thoughts", "plan.md"), []byte("# Plan"), 0644) + seedProject(c, "test-proj", dir, nil) + s.agents = agents.New(c, cs, 0) + + body, _ := json.Marshal(map[string]any{"path": dir}) + + // First attach. + req := httptest.NewRequest(http.MethodPost, "/api/agents/attach", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("first attach: expected 200, got %d", rec.Code) + } + + // Second attach without force should 409. + body, _ = json.Marshal(map[string]any{"path": dir}) + req = httptest.NewRequest(http.MethodPost, "/api/agents/attach", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec = httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusConflict { + t.Errorf("expected 409, got %d: %s", rec.Code, rec.Body.String()) + } +} + +// E-PENPAL-CLI-CONTENTION: verifies attach with force=true succeeds when agent active. +func TestAPIAgentAttach_ForceEvicts(t *testing.T) { + s, c, cs := testServer(t) + dir := t.TempDir() + os.MkdirAll(filepath.Join(dir, "thoughts"), 0755) + os.WriteFile(filepath.Join(dir, "thoughts", "plan.md"), []byte("# Plan"), 0644) + seedProject(c, "test-proj", dir, nil) + s.agents = agents.New(c, cs, 0) + + body, _ := json.Marshal(map[string]any{"path": dir}) + req := httptest.NewRequest(http.MethodPost, "/api/agents/attach", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("first attach: expected 200, got %d", rec.Code) + } + + // Force attach should succeed. + body, _ = json.Marshal(map[string]any{"path": dir, "force": true}) + req = httptest.NewRequest(http.MethodPost, "/api/agents/attach", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec = httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Errorf("expected 200 for forced attach, got %d: %s", rec.Code, rec.Body.String()) + } +} + +// E-PENPAL-CLI-ATTACH: verifies attach returns 400 when path is missing. +func TestAPIAgentAttach_MissingPath(t *testing.T) { + s, c, cs := testServer(t) + s.agents = agents.New(c, cs, 0) + + body, _ := json.Marshal(map[string]any{}) + req := httptest.NewRequest(http.MethodPost, "/api/agents/attach", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Errorf("expected 400, got %d", rec.Code) + } +} + +// E-PENPAL-CLI-ATTACH: verifies attach returns 503 when s.agents is nil. +func TestAPIAgentAttach_NoManager(t *testing.T) { + s, _, _ := testServer(t) + dir := t.TempDir() + + body, _ := json.Marshal(map[string]any{"path": dir}) + req := httptest.NewRequest(http.MethodPost, "/api/agents/attach", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + // Without a manager, the handler returns 503 (or 404 for no project). + // The path check happens before the manager check, so we need a valid project. + // Since agents is nil, if we get past path checks we should see 503. + if rec.Code != http.StatusNotFound && rec.Code != http.StatusServiceUnavailable && rec.Code != http.StatusBadRequest { + t.Errorf("expected 400/404/503, got %d", rec.Code) + } +} + +// E-PENPAL-SESSION-MGMT: verifies wait returns 401 for invalid session. +func TestAPIAgentWait_InvalidSession(t *testing.T) { + s, c, cs := testServer(t) + s.agents = agents.New(c, cs, 0) + + req := httptest.NewRequest(http.MethodGet, "/api/agents/wait?project=test-proj&session=bad-token", nil) + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusUnauthorized { + t.Errorf("expected 401, got %d: %s", rec.Code, rec.Body.String()) + } +} + +// E-PENPAL-CLI-AGENT-CMDS: verifies wait returns 400 when params are missing. +func TestAPIAgentWait_MissingParams(t *testing.T) { + s, c, cs := testServer(t) + s.agents = agents.New(c, cs, 0) + + req := httptest.NewRequest(http.MethodGet, "/api/agents/wait", nil) + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Errorf("expected 400, got %d", rec.Code) + } +} + +// E-PENPAL-CLI-CONTENTION: verifies POST /api/agents/stop returns 200 even when no agent running. +func TestAPIAgentStop_ReturnsOK_WhenNoAgent(t *testing.T) { + s, c, cs := testServer(t) + s.agents = agents.New(c, cs, 0) + dir := t.TempDir() + seedProject(c, "test-proj", dir, nil) + + req := httptest.NewRequest(http.MethodPost, "/api/agents/stop?project=test-proj", nil) + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Errorf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } +} + +// E-PENPAL-AGENT-SELF-ID: verifies attach returns agentName and uses it for comment author. +func TestAPIAgentAttach_AgentName(t *testing.T) { + s, c, cs := testServer(t) + dir := t.TempDir() + os.MkdirAll(filepath.Join(dir, "thoughts"), 0755) + os.WriteFile(filepath.Join(dir, "thoughts", "plan.md"), []byte("# Plan"), 0644) + seedProject(c, "test-proj", dir, nil) + s.agents = agents.New(c, cs, 0) + + // Attach with agent name "amp". + body, _ := json.Marshal(map[string]any{"path": dir, "agent": "amp"}) + req := httptest.NewRequest(http.MethodPost, "/api/agents/attach", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + + var resp map[string]string + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("parse: %v", err) + } + if resp["agentName"] != "amp" { + t.Errorf("expected agentName=amp, got %q", resp["agentName"]) + } + token := resp["sessionToken"] + + // Create a human thread first so we can post an agent reply. + anchor := comments.Anchor{SelectedText: "Plan"} + comment := comments.Comment{Author: "user", Role: "human", Body: "Review this"} + thread, err := cs.CreateThread("test-proj", "thoughts/plan.md", anchor, comment) + if err != nil { + t.Fatalf("create thread: %v", err) + } + + // Post an agent reply — server should override author to "amp". + replyBody, _ := json.Marshal(map[string]string{ + "project": "test-proj", + "path": "thoughts/plan.md", + "author": "ignored", + "role": "agent", + "body": "Looks good", + }) + req = httptest.NewRequest(http.MethodPost, "/api/threads/"+thread.ID+"/comments?session="+token, bytes.NewReader(replyBody)) + req.Header.Set("Content-Type", "application/json") + rec = httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("reply: expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + + var result comments.Thread + if err := json.Unmarshal(rec.Body.Bytes(), &result); err != nil { + t.Fatalf("parse reply: %v", err) + } + lastComment := result.Comments[len(result.Comments)-1] + if lastComment.Author != "amp" { + t.Errorf("expected author=amp, got %q", lastComment.Author) + } +} + +// E-PENPAL-AGENT-SELF-ID: verifies attach defaults agentName to "agent" when not provided. +func TestAPIAgentAttach_DefaultAgentName(t *testing.T) { + s, c, cs := testServer(t) + dir := t.TempDir() + os.MkdirAll(filepath.Join(dir, "thoughts"), 0755) + os.WriteFile(filepath.Join(dir, "thoughts", "plan.md"), []byte("# Plan"), 0644) + seedProject(c, "test-proj", dir, nil) + s.agents = agents.New(c, cs, 0) + + body, _ := json.Marshal(map[string]any{"path": dir}) + req := httptest.NewRequest(http.MethodPost, "/api/agents/attach", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + + var resp map[string]string + json.Unmarshal(rec.Body.Bytes(), &resp) + if resp["agentName"] != "agent" { + t.Errorf("expected agentName=agent, got %q", resp["agentName"]) + } +} + +// E-PENPAL-CLI-CONTENTION: verifies maybeStartAgent skips when CLI agent is attached. +func TestMaybeStartAgent_SkipsWhenCLIAgentAttached(t *testing.T) { + s, c, cs := testServer(t) + s.agents = agents.New(c, cs, 0) + dir := t.TempDir() + os.MkdirAll(filepath.Join(dir, "thoughts"), 0755) + os.WriteFile(filepath.Join(dir, "thoughts", "plan.md"), []byte("# Plan"), 0644) + seedProject(c, "test-proj", dir, nil) + + // Attach a CLI session. + s.agents.Attach("test-proj", "", "claude", false) + + // Create a human comment via POST /api/threads (triggers maybeStartAgent). + threadBody, _ := json.Marshal(map[string]any{ + "project": "test-proj", + "path": "thoughts/plan.md", + "author": "user", + "role": "human", + "body": "Please review this", + "anchor": map[string]any{"selectedText": "Plan"}, + }) + req := httptest.NewRequest(http.MethodPost, "/api/threads", bytes.NewReader(threadBody)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("create thread: expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + + // Give maybeStartAgent's goroutine a moment to run. + time.Sleep(50 * time.Millisecond) + + // Verify no spawned agent started — Status should be nil (no spawned agent). + status := s.agents.Status("test-proj") + if status != nil && status.PID != 0 { + t.Errorf("expected no spawned agent, but found PID %d", status.PID) + } +} diff --git a/apps/penpal/internal/server/api_threads_test.go b/apps/penpal/internal/server/api_threads_test.go index fab3471d..a8c4282f 100644 --- a/apps/penpal/internal/server/api_threads_test.go +++ b/apps/penpal/internal/server/api_threads_test.go @@ -69,10 +69,11 @@ func TestAPIThreads_CreateAndList(t *testing.T) { // E-PENPAL-API-ROUTES: verifies POST /api/threads/{id}/comments adds a reply. func TestAPIThreads_AddComment(t *testing.T) { - s, c, _ := testServer(t) + s, c, cs := testServer(t) dir := t.TempDir() seedProject(c, "test-proj", dir, nil) + token := attachSession(t, s, c, cs, "test-proj") // Create thread createBody, _ := json.Marshal(map[string]interface{}{ @@ -91,7 +92,7 @@ func TestAPIThreads_AddComment(t *testing.T) { var thread comments.Thread json.Unmarshal(rec.Body.Bytes(), &thread) - // Add a comment + // Add a comment (agent role requires session token) commentBody, _ := json.Marshal(map[string]string{ "project": "test-proj", "path": "thoughts/plan.md", @@ -99,7 +100,7 @@ func TestAPIThreads_AddComment(t *testing.T) { "role": "agent", "body": "Reply", }) - req = httptest.NewRequest(http.MethodPost, "/api/threads/"+thread.ID+"/comments", bytes.NewReader(commentBody)) + req = httptest.NewRequest(http.MethodPost, "/api/threads/"+thread.ID+"/comments?session="+token, bytes.NewReader(commentBody)) req.Header.Set("Content-Type", "application/json") rec = httptest.NewRecorder() s.ServeHTTP(rec, req) @@ -394,26 +395,6 @@ func TestAPIThreads_AgentWorkingFalseByDefault(t *testing.T) { } } -// E-PENPAL-HEARTBEAT: verifies heartbeat is recorded when agent=true query param is set on threads endpoint. -func TestAPIThreads_HeartbeatRecordedOnAgentPoll(t *testing.T) { - s, c, cs := testServer(t) - dir := t.TempDir() - seedProject(c, "test-proj", dir, nil) - - // Poll with agent=true - req := httptest.NewRequest(http.MethodGet, "/api/threads?project=test-proj&path=thoughts/plan.md&agent=true", nil) - rec := httptest.NewRecorder() - s.ServeHTTP(rec, req) - - if rec.Code != http.StatusOK { - t.Fatalf("expected 200, got %d", rec.Code) - } - - if !cs.IsAgentActive("test-proj", "thoughts/plan.md") { - t.Errorf("expected IsAgentActive=true after agent poll") - } -} - // E-PENPAL-API-ROUTES: verifies agentActive and workingThreads fields in reviews response. func TestAPIReviews_AgentActiveAndWorkingFields(t *testing.T) { s, c, cs := testServer(t) @@ -480,49 +461,4 @@ func TestAPIReviews_AgentActiveAndWorkingFields(t *testing.T) { } } -// E-PENPAL-HEARTBEAT: verifies heartbeat is recorded when agent=true is set on reviews endpoint. -func TestAPIReviews_HeartbeatRecordedOnAgentPoll(t *testing.T) { - s, c, cs := testServer(t) - dir := t.TempDir() - seedProject(c, "test-proj", dir, nil) - // Create the actual source file - if err := os.MkdirAll(filepath.Join(dir, "thoughts"), 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(dir, "thoughts", "plan.md"), []byte("text"), 0o644); err != nil { - t.Fatal(err) - } - - // Create a thread to put a file "in review" - createBody, _ := json.Marshal(map[string]interface{}{ - "project": "test-proj", - "path": "thoughts/plan.md", - "anchor": map[string]string{"selectedText": "text"}, - "author": "user", - "role": "human", - "body": "Review this", - }) - req := httptest.NewRequest(http.MethodPost, "/api/threads", bytes.NewReader(createBody)) - req.Header.Set("Content-Type", "application/json") - rec := httptest.NewRecorder() - s.ServeHTTP(rec, req) - - if rec.Code != http.StatusOK { - t.Fatalf("create thread: expected 200, got %d: %s", rec.Code, rec.Body.String()) - } - - // Poll reviews with agent=true - req = httptest.NewRequest(http.MethodGet, "/api/reviews?project=test-proj&agent=true", nil) - rec = httptest.NewRecorder() - s.ServeHTTP(rec, req) - - if rec.Code != http.StatusOK { - t.Fatalf("expected 200, got %d", rec.Code) - } - - // Verify heartbeat was recorded for the file in review - if !cs.IsAgentActive("test-proj", "thoughts/plan.md") { - t.Errorf("expected IsAgentActive=true after agent poll on reviews") - } -} diff --git a/apps/penpal/internal/server/comments.go b/apps/penpal/internal/server/comments.go index 20f7699c..0206c7ba 100644 --- a/apps/penpal/internal/server/comments.go +++ b/apps/penpal/internal/server/comments.go @@ -12,6 +12,10 @@ import ( // handleAPIThreads dispatches GET (list threads) and POST (create thread). // E-PENPAL-API-ROUTES: GET/POST /api/threads endpoint. func (s *Server) handleAPIThreads(w http.ResponseWriter, r *http.Request) { + // E-PENPAL-SESSION-MGMT: validate session token if present. + if !s.validateSessionParam(w, r) { + return + } switch r.Method { case http.MethodGet: s.handleListThreads(w, r) @@ -24,6 +28,10 @@ func (s *Server) handleAPIThreads(w http.ResponseWriter, r *http.Request) { // handleAPIThreadAction handles /api/threads/{id} and /api/threads/{id}/comments. func (s *Server) handleAPIThreadAction(w http.ResponseWriter, r *http.Request) { + // E-PENPAL-SESSION-MGMT: validate session token if present. + if !s.validateSessionParam(w, r) { + return + } // Parse the path: /api/threads/{id} or /api/threads/{id}/comments rest := strings.TrimPrefix(r.URL.Path, "/api/threads/") parts := strings.Split(rest, "/") @@ -69,6 +77,10 @@ type APIFileInReview struct { // handleAPIListReviews handles GET /api/reviews?project=X[&agent=true][&worktree=Z]. // E-PENPAL-REVIEW-COUNT: returns files with open threads for review count. func (s *Server) handleAPIListReviews(w http.ResponseWriter, r *http.Request) { + // E-PENPAL-SESSION-MGMT: validate session token if present. + if !s.validateSessionParam(w, r) { + return + } if r.Method != http.MethodGet { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) return @@ -81,7 +93,6 @@ func (s *Server) handleAPIListReviews(w http.ResponseWriter, r *http.Request) { } worktree := r.URL.Query().Get("worktree") - isAgent := r.URL.Query().Get("agent") == "true" files, err := s.comments.ListFilesInReviewForWorktree(projectName, worktree) if err != nil { @@ -89,14 +100,8 @@ func (s *Server) handleAPIListReviews(w http.ResponseWriter, r *http.Request) { return } - // Record heartbeat when an agent polls reviews - if isAgent { - for _, f := range files { - s.comments.RecordHeartbeat(projectName, f.FilePath) - } - } - - agentActive := s.agents != nil && s.agents.Status(projectName) != nil && s.agents.Status(projectName).Running + // E-PENPAL-AGENT-ACTIVE-UNIFIED: checks both spawned agents and CLI sessions. + agentActive := s.isAgentActive(projectName) result := make([]APIFileInReview, len(files)) for i, f := range files { workingThreads := s.comments.WorkingCount(projectName, f.FilePath) @@ -136,12 +141,6 @@ func (s *Server) handleListThreads(w http.ResponseWriter, r *http.Request) { filePath := r.URL.Query().Get("path") status := r.URL.Query().Get("status") worktree := r.URL.Query().Get("worktree") - isAgent := r.URL.Query().Get("agent") == "true" - - // Record heartbeat when an agent polls for threads - if isAgent && filePath != "" { - s.comments.RecordHeartbeat(projectName, filePath) - } // When path is omitted, return all open threads across the project if filePath == "" { @@ -188,7 +187,8 @@ func (s *Server) handleListThreads(w http.ResponseWriter, r *http.Request) { } } - agentRunning := s.agents != nil && s.agents.Status(projectName) != nil && s.agents.Status(projectName).Running + // E-PENPAL-AGENT-ACTIVE-UNIFIED: checks both spawned agents and CLI sessions. + agentRunning := s.isAgentActive(projectName) var result []threadResponse for _, t := range threads { tr := threadResponse{Thread: t} @@ -234,8 +234,21 @@ func (s *Server) handleCreateThread(w http.ResponseWriter, r *http.Request) { return } + // E-PENPAL-CLI-CONTENTION: agent-role writes require a valid session. + if req.Role == "agent" && !s.requireSessionForAgent(w, r, req.Project) { + return + } + + // E-PENPAL-AGENT-SELF-ID: override author from session when agent role. + author := req.Author + if req.Role == "agent" { + if name := s.agentNameFromSession(r); name != "" { + author = name + } + } + comment := comments.Comment{ - Author: req.Author, + Author: author, Role: req.Role, Body: req.Body, SuggestedReplies: req.SuggestedReplies, @@ -276,8 +289,23 @@ func (s *Server) handleAddComment(w http.ResponseWriter, r *http.Request, thread return } + // E-PENPAL-CLI-CONTENTION: agent-role writes require a valid session. + if req.Role == "agent" && !s.requireSessionForAgent(w, r, req.Project) { + return + } + + // E-PENPAL-AGENT-SELF-ID: override author from session when agent role. + author := req.Author + if req.Role == "agent" { + if name := s.agentNameFromSession(r); name != "" { + author = name + } + } + + // Working indicator handling (InReplyTo, WorkingStartedAt, ClearWorking) + // is done automatically by AddCommentForWorktree for agent-role comments. comment := comments.Comment{ - Author: req.Author, + Author: author, Role: req.Role, Body: req.Body, SuggestedReplies: req.SuggestedReplies, diff --git a/apps/penpal/internal/server/in_review.go b/apps/penpal/internal/server/in_review.go index 04b116f0..ccdef40c 100644 --- a/apps/penpal/internal/server/in_review.go +++ b/apps/penpal/internal/server/in_review.go @@ -66,7 +66,8 @@ func (s *Server) listAllReviewGroups() []ReviewGroup { continue } - agentActive := s.agents != nil && s.agents.Status(qn) != nil && s.agents.Status(qn).Running + // E-PENPAL-AGENT-ACTIVE-UNIFIED: checks both spawned agents and CLI sessions. + agentActive := s.isAgentActive(qn) // Build source name -> SourceTypeName lookup sourceTypeMap := make(map[string]string) diff --git a/apps/penpal/internal/server/server.go b/apps/penpal/internal/server/server.go index 6e22e28d..bfa1368b 100644 --- a/apps/penpal/internal/server/server.go +++ b/apps/penpal/internal/server/server.go @@ -287,6 +287,8 @@ func (s *Server) routes() { s.mux.HandleFunc("/api/agents", s.handleAgentStatus) s.mux.HandleFunc("/api/agents/start", s.handleAgentStart) s.mux.HandleFunc("/api/agents/stop", s.handleAgentStop) + s.mux.HandleFunc("/api/agents/attach", s.handleAgentAttach) + s.mux.HandleFunc("/api/agents/wait", s.handleAgentWait) // Raw file content s.mux.HandleFunc("/api/raw", s.handleRawFile) // Activity tracking diff --git a/apps/penpal/internal/server/testhelper_test.go b/apps/penpal/internal/server/testhelper_test.go index 9b42bfae..69a3171e 100644 --- a/apps/penpal/internal/server/testhelper_test.go +++ b/apps/penpal/internal/server/testhelper_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/loganj/penpal/internal/activity" + "github.com/loganj/penpal/internal/agents" "github.com/loganj/penpal/internal/cache" "github.com/loganj/penpal/internal/comments" "github.com/loganj/penpal/internal/config" @@ -34,6 +35,20 @@ func testServer(t *testing.T) (*Server, *cache.Cache, *comments.Store) { return s, c, cs } +// attachSession creates an agent manager on the server and attaches a session +// for the given project, returning the session token. Use this when tests need +// to post role="agent" comments via the REST API. +func attachSession(t *testing.T, s *Server, c *cache.Cache, cs *comments.Store, projectName string) string { + t.Helper() + mgr := agents.New(c, cs, 0) + s.agents = mgr + sess, err := mgr.Attach(projectName, "", "claude", false) + if err != nil { + t.Fatalf("attach session: %v", err) + } + return sess.Token +} + // seedProject adds a project with files to the cache for testing. func seedProject(c *cache.Cache, name, path string, files []cache.FileInfo) discovery.Project { project := discovery.Project{ diff --git a/apps/penpal/penpal-cli b/apps/penpal/penpal-cli deleted file mode 100755 index c2305d28..00000000 Binary files a/apps/penpal/penpal-cli and /dev/null differ