fix(penpal): address Codex review findings#304
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
![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: cada418a88
ℹ️ 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
fix/penpal-codex-followups
branch
from
cada418 to
6998c73
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6998c734b2
ℹ️ 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
fix/penpal-codex-followups
branch
from
6998c73 to
9bbcc3e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bbcc3edca
ℹ️ 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".
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 <noreply@anthropic.com>
loganj
force-pushed
the
fix/penpal-codex-followups
branch
from
9bbcc3e to
9c9e182
Compare
1 participant
Summary
Follow-up to #302 — addresses remaining findings from Codex code review of the penpal Go backend:
isSubpath—isSubpath(parent, child)returned true whenchild == parent, sopath=.passed validation and could reachos.Removeon the project directory itself. Now requires strict subpath.penpal_list_threads— When called without apath, the MCP tool unconditionally returned only open threads, ignoring thestatusparameter. ExtractListThreadsByStatusand use it sostatus=resolvedworks project-wide.StopPollingagainst double-close — Closing the stop channel without a guard panics if called twice during shutdown. Add mutex protection and nil the channel after close.parseStream/logFilerace —cmd.Wait()could return beforeparseStreamfinished writing to the log file, and the exit goroutine would close it mid-write. Synchronize with a channel.Test plan
go test ./...passes (pre-existingTestAPIProjects_ListsProjectsflake unrelated)TestIsSubpathupdated —parent == childnow returns falseListOpenThreadsdelegates to newListThreadsByStatus, existing tests pass🤖 Generated with Claude Code