From 1132b61e453a1d1792308b58d343cf94ca31a926 Mon Sep 17 00:00:00 2001 From: Logan Johnson Date: Thu, 26 Feb 2026 15:45:59 -0500 Subject: [PATCH 1/4] fix(penpal): reject project-root path in isSubpath check isSubpath(parent, child) previously returned true when child == parent, allowing path=. to pass validation and reach os.Remove in the delete handler. This could delete the project directory itself on empty projects. Make the check strictly "inside parent" so equal paths are rejected. Addresses Codex review comment on PR #302. Co-Authored-By: Claude Opus 4.6 --- apps/penpal/internal/server/pathutil.go | 12 ++++++++---- apps/penpal/internal/server/pathutil_test.go | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/apps/penpal/internal/server/pathutil.go b/apps/penpal/internal/server/pathutil.go index dfd74dcd..7afbf38c 100644 --- a/apps/penpal/internal/server/pathutil.go +++ b/apps/penpal/internal/server/pathutil.go @@ -5,11 +5,15 @@ import ( "strings" ) -// isSubpath reports whether child is inside (or equal to) parent after +// isSubpath reports whether child is strictly inside parent after // cleaning both paths. It prevents path-traversal attacks by ensuring // the resolved child starts with the parent directory prefix. +// Returns false when child equals parent (e.g. path="."). func isSubpath(parent, child string) bool { - parent = filepath.Clean(parent) + string(filepath.Separator) - child = filepath.Clean(child) + string(filepath.Separator) - return strings.HasPrefix(child, parent) + parent = filepath.Clean(parent) + child = filepath.Clean(child) + if parent == child { + return false + } + return strings.HasPrefix(child, parent+string(filepath.Separator)) } diff --git a/apps/penpal/internal/server/pathutil_test.go b/apps/penpal/internal/server/pathutil_test.go index 56856097..ba416d0a 100644 --- a/apps/penpal/internal/server/pathutil_test.go +++ b/apps/penpal/internal/server/pathutil_test.go @@ -10,7 +10,7 @@ func TestIsSubpath(t *testing.T) { }{ {"/a/b", "/a/b/c", true}, {"/a/b", "/a/b/c/d", true}, - {"/a/b", "/a/b", true}, + {"/a/b", "/a/b", false}, {"/a/b", "/a/bc", false}, {"/a/b", "/a", false}, {"/a/b", "/a/b/../c", false}, From 0a5cbf105069098e02b3ff14ff5fec5747af8312 Mon Sep 17 00:00:00 2001 From: Logan Johnson Date: Thu, 26 Feb 2026 15:46:40 -0500 Subject: [PATCH 2/4] fix(penpal): honor status filter when listing threads project-wide penpal_list_threads with no path unconditionally called ListOpenThreads, ignoring the status parameter. Callers requesting status=resolved got only open threads. Extract ListThreadsByStatus and use it in the MCP tool so the status filter is respected for project-wide queries. Co-Authored-By: Claude Opus 4.6 --- apps/penpal/internal/comments/operations.go | 9 ++++++++- apps/penpal/internal/mcpserver/tools.go | 8 ++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/apps/penpal/internal/comments/operations.go b/apps/penpal/internal/comments/operations.go index 5e7724a7..d46d8adc 100644 --- a/apps/penpal/internal/comments/operations.go +++ b/apps/penpal/internal/comments/operations.go @@ -135,6 +135,13 @@ func (s *Store) ReopenThread(projectName, filePath, threadID string) error { // project and returns all threads with status "open" across all files. // Returned file paths are relative to the project root (e.g., "thoughts/shared/plans/foo.md"). func (s *Store) ListOpenThreads(projectName string) ([]ThreadWithFile, error) { + return s.ListThreadsByStatus(projectName, "open") +} + +// ListThreadsByStatus walks the .penpal/comments/ directory for the given +// project and returns threads matching the given status filter. +// An empty status returns all threads regardless of status. +func (s *Store) ListThreadsByStatus(projectName, status string) ([]ThreadWithFile, error) { project := s.cache.FindProject(projectName) if project == nil { return nil, fmt.Errorf("project not found: %s", projectName) @@ -172,7 +179,7 @@ func (s *Store) ListOpenThreads(projectName string) ([]ThreadWithFile, error) { filePath := strings.TrimSuffix(rel, ".json") for _, t := range fc.Threads { - if t.Status == "open" { + if status == "" || t.Status == status { results = append(results, ThreadWithFile{ Thread: t, FilePath: filePath, diff --git a/apps/penpal/internal/mcpserver/tools.go b/apps/penpal/internal/mcpserver/tools.go index 1839d610..39615d23 100644 --- a/apps/penpal/internal/mcpserver/tools.go +++ b/apps/penpal/internal/mcpserver/tools.go @@ -81,9 +81,13 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { } if input.Path == "" { - // List open threads across the entire project + // List threads across the entire project, filtered by status store.RecordHeartbeat(input.Project, "") - threads, err := store.ListOpenThreads(input.Project) + status := input.Status + if status == "" { + status = "open" + } + threads, err := store.ListThreadsByStatus(input.Project, status) if err != nil { return nil, nil, err } From fc301c612f23adf40b8debdf803014ba3830a6a6 Mon Sep 17 00:00:00 2001 From: Logan Johnson Date: Thu, 26 Feb 2026 15:47:00 -0500 Subject: [PATCH 3/4] fix(penpal): guard StopPolling against double-close panic StopPolling closed the stopCh channel without guarding against repeated calls, which would panic. Protect with a mutex and nil the channel after closing so subsequent calls are safe. Co-Authored-By: Claude Opus 4.6 --- apps/penpal/internal/agents/agents.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/apps/penpal/internal/agents/agents.go b/apps/penpal/internal/agents/agents.go index 2bb897b3..875f6c23 100644 --- a/apps/penpal/internal/agents/agents.go +++ b/apps/penpal/internal/agents/agents.go @@ -17,11 +17,16 @@ type Info struct { var ( mu sync.RWMutex cached map[string][]Info + + pollMu sync.Mutex stopCh chan struct{} ) // StartPolling begins background polling for active agents every 5 seconds. func StartPolling() { + pollMu.Lock() + defer pollMu.Unlock() + stopCh = make(chan struct{}) // Do an initial poll synchronously so the first read has data. result := poll() @@ -48,8 +53,12 @@ func StartPolling() { // StopPolling stops the background polling goroutine. func StopPolling() { + pollMu.Lock() + defer pollMu.Unlock() + if stopCh != nil { close(stopCh) + stopCh = nil } } From 9c9e18208cdd0f381ae3f02ca22790ce033ec0a7 Mon Sep 17 00:00:00 2001 From: Logan Johnson Date: Thu, 26 Feb 2026 15:47:23 -0500 Subject: [PATCH 4/4] fix(penpal): avoid race between parseStream and logFile.Close cmd.Wait() can return before parseStream finishes its last write to logFile. The exit goroutine then closes logFile while parseStream may still be writing. Synchronize with a channel so logFile is only closed after parseStream returns. Co-Authored-By: Claude Opus 4.6 --- apps/penpal/internal/agents/agents.go | 6 +++++- apps/penpal/internal/agents/manager.go | 11 +++++++++-- apps/penpal/internal/mcpserver/tools.go | 2 +- apps/penpal/internal/server/manage.go | 2 +- 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/apps/penpal/internal/agents/agents.go b/apps/penpal/internal/agents/agents.go index 875f6c23..ad0025d2 100644 --- a/apps/penpal/internal/agents/agents.go +++ b/apps/penpal/internal/agents/agents.go @@ -27,7 +27,11 @@ func StartPolling() { pollMu.Lock() defer pollMu.Unlock() + if stopCh != nil { + close(stopCh) + } stopCh = make(chan struct{}) + ch := stopCh // Do an initial poll synchronously so the first read has data. result := poll() mu.Lock() @@ -39,7 +43,7 @@ func StartPolling() { defer ticker.Stop() for { select { - case <-stopCh: + case <-ch: return case <-ticker.C: result := poll() diff --git a/apps/penpal/internal/agents/manager.go b/apps/penpal/internal/agents/manager.go index d258a04c..e387db7f 100644 --- a/apps/penpal/internal/agents/manager.go +++ b/apps/penpal/internal/agents/manager.go @@ -127,8 +127,14 @@ func (m *Manager) Start(projectName string) (*Agent, error) { contextWindow: 200000, // default for opus, refined when result arrives } - // Parse NDJSON stream in background, writing through to log file - go agent.parseStream(stdout, logFile) + // 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) @@ -136,6 +142,7 @@ func (m *Manager) Start(projectName string) (*Agent, error) { // Monitor process exit in background go func() { agent.exitErr = cmd.Wait() + <-streamDone // wait for parseStream to finish before closing log logFile.Close() os.Remove(mcpConfigPath) close(agent.done) diff --git a/apps/penpal/internal/mcpserver/tools.go b/apps/penpal/internal/mcpserver/tools.go index 39615d23..c17e34ab 100644 --- a/apps/penpal/internal/mcpserver/tools.go +++ b/apps/penpal/internal/mcpserver/tools.go @@ -98,7 +98,7 @@ func registerTools(server *mcp.Server, store *comments.Store, c *cache.Cache) { store.RecordHeartbeat(input.Project, t.FilePath) seen[t.FilePath] = true } - if len(t.Comments) > 0 && t.Comments[len(t.Comments)-1].Role == "human" { + if t.Status == "open" && len(t.Comments) > 0 && t.Comments[len(t.Comments)-1].Role == "human" { store.SetTyping(input.Project, t.FilePath, t.ID) } } diff --git a/apps/penpal/internal/server/manage.go b/apps/penpal/internal/server/manage.go index 70fc9918..bffcef06 100644 --- a/apps/penpal/internal/server/manage.go +++ b/apps/penpal/internal/server/manage.go @@ -276,7 +276,7 @@ func (s *Server) handleAddSource(w http.ResponseWriter, r *http.Request) { // Check what the path points to absPath := filepath.Join(project.Path, req.Path) resolved, err := filepath.Abs(absPath) - if err != nil || !isSubpath(project.Path, resolved) { + if err != nil || (resolved != filepath.Clean(project.Path) && !isSubpath(project.Path, resolved)) { http.Error(w, "invalid path", http.StatusBadRequest) return }