feat(core): extract feedback handlers (reply, comment, ack, close)#166
feat(core): extract feedback handlers (reply, comment, ack, close)#166galligan wants to merge 1 commit into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Add four transport-agnostic feedback handlers: - replyHandler: resolve comment ID, find thread, post reply, optionally resolve - commentHandler: resolve PR node ID, post issue comment - ackHandler: batch-resolve IDs, store acks locally, optionally add reactions - closeHandler: polymorphic — resolve thread or close PR All use detectAuth → lazy GitHubClient import, return Result<T, Error>. No console.*, process.exit, or transport-specific code. 🤘🏻 In-collaboration-with: [Claude Code](https://claude.com/claude-code)
galligan
force-pushed
the
fire-5/read-handlers
branch
from
289ae13 to
e353d4e
Compare
galligan
force-pushed
the
fire-6/feedback-handlers
branch
from
07f873a to
c806561
Compare
galligan
changed the title
![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: c806561355
ℹ️ 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 Result.ok({ | ||
| id: shortId, | ||
| ...(reply.url && { url: reply.url }), | ||
| ...(input.resolve && { resolved: true }), |
There was a problem hiding this comment.
resolved only after thread resolution succeeds
When input.resolve is true, the handler logs and ignores resolveReviewThread failures but still returns resolved: true, which can mislead CLI/MCP callers into treating unresolved threads as closed. This occurs whenever GitHub rejects resolution (permissions, transient API failure, invalid thread state), so downstream automation may skip retries even though the thread is still open.
Useful? React with 👍 / 👎.
| if (reactionResult.isErr()) { | ||
| ctx.logger.debug("Reaction already exists or failed", { commentId }); | ||
| } | ||
| return Result.ok({ type: "thread", ok: true }); |
There was a problem hiding this comment.
In the issue-comment branch, addReaction errors are swallowed and the function always returns { ok: true }, but there is no local ack fallback in this handler. If the reaction call fails for reasons other than “already reacted” (e.g., auth/network/API errors), the operation performs no acknowledgement while reporting success, causing callers to believe the feedback was handled when it was not.
Useful? React with 👍 / 👎.
1 participant
Summary
replyHandler,commentHandler,ackHandler,closeHandlerto coreidentifyUnaddressedFeedbackfrom both CLI and MCP into shared handler@abc123) shared across all feedback handlers (FIRE-6)Test plan
bun run checkpassesbun testpasses (288 tests)🤘🏻 In-collaboration-with: Claude Code
Greptile Summary
Extracts feedback handlers (
replyHandler,commentHandler,ackHandler,closeHandler) from CLI topackages/core/src/handlers/, following the same architectural pattern established in recent commits forstatusHandlerand read handlers. The extraction consolidates short ID resolution (@abc123) and batch operations into shared handler implementations, enabling code reuse across CLI and MCP interfaces.Handler<TInput, TOutput>contract withHandlerContextResulttype for consistent error handling@abc123) is implemented consistently across all feedback handlersresolveBatchIds,batchAddReactions,deduplicateByCommentId) are properly utilizedcloseHandlerhandles both PR closure and thread resolution)Confidence Score: 5/5
Important Files Changed
Last reviewed commit: c806561