feat(penpal): CLI-based agent interface for external agent participation#607
feat(penpal): CLI-based agent interface for external agent participation#607
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd02124f41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
loganj
force-pushed
the
penpal-agent-feedback-loop
branch
2 times, most recently
from
289052c to
e27f840
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e27f840887
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
loganj
force-pushed
the
penpal-agent-feedback-loop
branch
from
e27f840 to
c50be1c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c50be1ccfe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
loganj
force-pushed
the
penpal-agent-feedback-loop
branch
2 times, most recently
from
95d36e5 to
18ddfe6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18ddfe61b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 { |
There was a problem hiding this comment.
validateSessionParam only compares the session token against project/worktree query params, but routes like PATCH /api/threads/{id} take project from JSON body instead; if the caller omits ?project=..., the ownership check is skipped and a valid session for project A can resolve/reopen threads in project B by sending B in the body. This breaks the session scoping model for thread mutation endpoints that don’t carry project identity in the URL query.
Useful? React with 👍 / 👎.
| if sess.Project != project { | ||
| http.Error(w, "session does not own this project", http.StatusForbidden) |
There was a problem hiding this comment.
requireSessionForAgent validates only sess.Project and never checks the target worktree, but agent write APIs (POST /api/threads and POST /api/threads/{id}/comments) accept worktree in the request body. A session attached to one worktree can therefore post/create comments in another worktree of the same project by changing worktree in JSON, violating the per-session worktree isolation expected by attach/wait flows.
Useful? React with 👍 / 👎.
loganj
force-pushed
the
penpal-agent-feedback-loop
branch
from
18ddfe6 to
79237d2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79237d294f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 { |
There was a problem hiding this comment.
validateSessionParam only enforces ownership against the project query parameter, but PATCH /api/threads/{id} (via handleUpdateThread) gets project from the JSON body. If a caller has a valid session for project A, they can omit ?project=... and submit a body for project B to resolve/reopen B’s threads while still passing validation. Session ownership should be checked against the effective project from the request body for these mutation routes.
Useful? React with 👍 / 👎.
| if sess.Project != project { | ||
| http.Error(w, "session does not own this project", http.StatusForbidden) |
There was a problem hiding this comment.
requireSessionForAgent validates only sess.Project, but agent write endpoints (POST /api/threads and POST /api/threads/{id}/comments) accept worktree in the request body. A session attached to one worktree can therefore create/reply in a different worktree of the same project by changing worktree, which breaks the session/worktree scoping enforced elsewhere (for example in /api/agents/wait).
Useful? React with 👍 / 👎.
loganj
force-pushed
the
penpal-agent-feedback-loop
branch
from
79237d2 to
8639d57
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8639d57bee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 { |
There was a problem hiding this comment.
This guard only checks worktree ownership when the worktree query parameter is present. For a session bound to a non-default worktree, calls like GET /api/threads?project=P&session=T or GET /api/reviews?... skip this check, then downstream handlers use the empty worktree ("", main worktree), which allows cross-worktree reads with the same session token. The session worktree should be enforced even when the request omits worktree.
Useful? React with 👍 / 👎.
| func runAttach(args []string) { | ||
| fs := flag.NewFlagSet("attach", flag.ExitOnError) | ||
| force := fs.Bool("force", false, "Evict existing agent and take over") | ||
| fs.Parse(args) |
There was a problem hiding this comment.
runAttach uses Go's flag parser directly, and go doc flag states: “Flag parsing stops just before the first non-flag argument.” Because usage documents penpal attach <path> [--force], users who pass the path first (attach /repo --force) will have --force treated as positional text, so force-eviction silently stays disabled and attach can fail with a conflict.
Useful? React with 👍 / 👎.
loganj
force-pushed
the
penpal-agent-feedback-loop
branch
from
8639d57 to
a7e3488
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7e3488697
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 { |
There was a problem hiding this comment.
Session worktree ownership is only checked when sess.Worktree != "", so a token attached to the main worktree (sess.Worktree == "") can still request any named worktree by passing ?worktree=... and read data outside its scope (for example via /api/threads and /api/reviews). Fresh evidence: the guard at this line skips validation entirely for empty-session worktrees, which leaves cross-worktree access open for main-bound sessions.
Useful? React with 👍 / 👎.
Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019d82bb-0087-761b-ab29-1acd2391048d
loganj
force-pushed
the
penpal-agent-feedback-loop
branch
from
a7e3488 to
c557d4d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c557d4d449
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| delete(m.sm.sessions, token) | ||
| delete(m.sm.projectSession, projectName) |
There was a problem hiding this comment.
When StopAny evicts a CLI session it deletes only session ownership, but it never clears the project's working entries. Because CLI wait calls populate working state, stopping an attached agent can leave threads showing as working for up to the expiry window, and those stale entries can reappear as active when a later session is attached (the UI paths use HasWorkingEntry/WorkingCountNoExpiry while an agent is active). This makes agent state inaccurate and can carry stale InReplyTo context into later replies.
Useful? React with 👍 / 👎.
1 participant
Summary
Built a CLI-based agent interface for Penpal — 7 new subcommands (
attach,files-in-review,list-threads,read-thread,reply,create-thread,wait) that let external AI agents participate in document reviews without MCP, avoiding stale port issues by discovering the server at runtime.Consolidated agent session management — Introduced session-based ownership via
penpal attachwith heartbeats, enforced a single-agent policy (preventing dual-agent contention), and made the UI stop button correctly evict CLI-attached agents (they get a 401 on nextwait).Refactored internals —
agents.Managerbecame the single source of truth for agent presence (both spawned and CLI-attached), heartbeat logic was removed fromcomments.Store, and the comment operations were cleaned up into a separateoperations.gofile.Consolidated working indicators — Moved
InReplyTo/WorkingStartedAt/ClearWorkinglogic into the Store layer (operations.go), removing duplication across MCP and REST handlers.