mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 12:33:05 +01:00
Devin FoleyandClaude Opus 5.5
3c7af35103
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents call connected tools through the tool gateway. Some tool calls need a human review before they run. > - A user approves a tool review from a task card. The approve route calls the gateway. > - The gateway can reject the approval with `ToolGatewayHttpError(409, "approved_tool_target_changed")`. This is an expected rejection: the tool or connection changed after the review. > - `ToolGatewayHttpError` does not extend `HttpError`. The shared error handler only keeps the status of an `HttpError`. Thus the route answers 500 "Internal server error" and sends the rejection to Sentry as a crash. > - The user does not see the reason. The user clicks Approve again, and each click makes one more crash report. One user made 69 reports on staging this way. > - This pull request makes the shared error handler keep the status and reason code of client (4xx) gateway errors. > - The benefit is a clear 409 with a reason for the user, and no false crash reports. ## Linked Issues or Issue Description Refs #12775 (related community PR. It changes `ToolGatewayHttpError` to extend `HttpError`, which affects every `instanceof HttpError` check. This PR is smaller and keeps gateway server failures on the crash path.) **What happened?** A user approves a tool review on a task card. The tool definition or connection changed after the review. The API answers 500 "Internal server error". Sentry records the expected rejection "Tool definition or connection changed; request a new review" as a server crash. Each retry records one more event. **Expected behavior** The API answers 409 with `code: "approved_tool_target_changed"` and the message "Tool definition or connection changed; request a new review". Sentry does not record a crash. **Steps to reproduce** 1. Make a connected tool that needs approval, and start a tool call so that a review card appears. 2. Change the tool connection (for example, its config) before you approve. 3. Click Approve on the task card (`POST /api/issues/:id/interactions/:interactionId/accept`). 4. See 500 "Internal server error" and a new Sentry event from `approveActionRequest`. **Paperclip version or commit** `master` at `482ec0aea9ab6f2684baaa1487f2c7a2eedf009e`. ## What Changed - `server/src/middleware/error-handler.ts`: the handler converts a `ToolGatewayHttpError` with a 4xx status to an `HttpError`. The new error keeps the status, the message, the gateway details, and the reason code as `code`. The original stack stays. - Gateway errors with a 5xx status do not change. They still go on the crash path, answer 500 "Internal server error", and go to Sentry. Thus provider text in those messages stays hidden. - `server/src/__tests__/error-handler-tool-gateway.test.ts`: new tests for the 409 mapping, for the unchanged 5xx path, and for an Express route that lets the gateway error escape. ## Verification - `cd server && npx vitest run src/__tests__/error-handler-tool-gateway.test.ts src/__tests__/error-handler.test.ts` passes: 21 tests. - Without the fix, the new tests fail with `expected 500 to be 409`. With the fix, they pass. - `src/__tests__/issue-thread-interaction-routes.test.ts` and `src/__tests__/json-parse-error-handler.test.ts` also pass: 175 tests in 4 suites with the two error handler suites. - I checked the runtime import graph of `services/tool-gateway.ts` (685 modules). It does not import `middleware/error-handler.ts`, so the new import makes no cycle. - I did not run the full server typecheck locally, because it ran out of memory in an 8 GB sandbox. CI must do this check. ## Risks - Low risk. Only gateway errors with a 4xx status change. Before, they became 500. Now they keep their own status. - Some routes outside the gateway router can now return gateway 4xx details in the `details` field. The gateway routes already return the same details through `sendGatewayError`. - `middleware/error-handler.ts` now imports `services/tool-gateway.ts`. Unit tests that import the error handler load more modules. The app already loads this module. - This PR does not fix the cause of some of these rejections. A routine OAuth token refresh changes values in the signed approval snapshot (token secret versions, the order of grant secret refs, and token expiry times). This can make a pending review fail. That fix changes `services/tool-gateway.ts` and comes in a separate PR. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used as a coding agent with tool use (shell, file edits, test runs, and GitHub API). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes (no documentation change is necessary) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>