mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 01:57:31 +01:00
Devin FoleyandClaude Opus 5.5
c56aa689ac
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - The server sends unexpected 5xx errors to Sentry through the shared
Express error handler
> - Some routes carry credentials: chat setup, provider webhooks, MCP,
company observability settings, Muse, avatars, and similar routes
> - On these routes the handler replaces the error with a generic
`Secret-sensitive request failed` error, because the original name and
message can contain secrets
> - That replacement also removes every useful fact. The Sentry event
does not show the route, the status, the error type, or the throw site,
so a maintainer cannot find the cause
> - This pull request adds a small fixed-shape record to that generic
error. The record says where the failure came from, and it never copies
request data or error text
> - The benefit is that the next event on these routes points to a route
and a code location, while the privacy rule stays the same
## Linked Issues or Issue Description
**What happened?**
A hosted staging server reported one event with the message
`Secret-sensitive request failed`. The event had only the stack of the
sanitizer itself. It had no route, no status, no error type, and no
frame from the original error. The trace had no spans and no logs. A
maintainer cannot find the root cause from this event.
**Expected behavior**
The event keeps the generic message, so no secret leaves the process.
The event also tells the maintainer which secret-sensitive route family
failed, the HTTP method and status, the error type, a safe error code
when one exists, the matched route definition, and the original throw
site.
**Steps to reproduce**
1. Make a route handler throw on a secret-sensitive path, for example
`PUT /api/companies/:companyId/observability`.
2. Enable the backend Sentry DSN.
3. Look at the captured event. It shows only `Secret-sensitive request
failed` and the sanitizer stack.
**Paperclip version or commit**
`master` at e107a75755.
I searched open and closed pull requests for `Secret-sensitive`. I did
not find a pull request that changes these diagnostics.
## What Changed
- `server/src/middleware/http-log-policy.ts`: add
`secretSensitiveRouteFamily()`. It returns a fixed label for the policy
branch that matched, or `null`. `isSecretSensitiveHttpRequest()` now
calls it. The matching rules do not change; the combined `/mcp/runner`
and avatar pattern is split into two branches with the same rules.
- `server/src/middleware/secret-sensitive-request-error.ts` (new):
`SecretSensitiveRequestError`. It keeps the generic message, `name:
"Error"`, its own stack, and default grouping. It adds frozen
diagnostics:
- `routeFamily`: from the closed list above.
- `method`: one of the standard HTTP methods, or `other`.
- `status`: the 5xx `HttpError` status, or 500.
- `errorKind`: `http_error`, `type_error`, `range_error`,
`reference_error`, `syntax_error`, `error`, or `non_error`.
- `errnoCode`: only a key of `os.constants.errno`. `databaseErrorCode`:
only a five-character SQLSTATE. Both are read from own data properties
of the error and at most three causes. Getters are not called.
- `routePattern`: the matched Express route definition
(`req.route.path`), only when it is a string in a narrow character set.
A RegExp route becomes `null`.
- `originFrames`: up to eight complete V8 frames from the original
stack. The extractor first removes the exact `name: message` header that
V8 writes. It rebuilds the header like `Error.prototype.toString` from
data properties only and requires `stack` to start with it, so a
multiline message can never pass as a frame. If the header cannot be
identified, no frame is kept. Each remaining line must match a strict
frame pattern: a code-shaped function name and a location that may
contain spaces, Windows drive or UNC paths, or a percent-encoded
`file:///` URL.
- `server/src/middleware/error-handler.ts`: the sanitizer now returns
this error. The HTTP response, the request log context, and Telemetry do
not change.
- `server/src/sentry.ts`: `captureException` sends these diagnostics as
`secret_route_family`, `http_method`, `http_status`, `error_kind`,
optional `errno_code` and `db_error_code` tags, and a
`secret_sensitive_request` context. Run tags for agent requests stay.
The fingerprint stays `{{ default }}`. The module header documents the
new event fields next to the other Sentry privacy notes.
- `server/src/__tests__/secret-sensitive-request-error.test.ts` (new):
unit tests for each bounded field, the header-first frame extraction
(multiline-message canary, unidentifiable header, paths with spaces and
Windows paths), the route-family policy, and an end-to-end test through
a real Express app, the error handler, and the Sentry gate.
## Verification
- `cd server && pnpm vitest run
src/__tests__/secret-sensitive-request-error.test.ts
src/__tests__/http-log-policy.test.ts src/__tests__/sentry.test.ts
src/__tests__/error-handler.test.ts
src/__tests__/agent-commentary.integration.test.ts
src/__tests__/agent-profile-avatar.test.ts`: 6 files, 108 tests passed,
5 skipped.
- Red before green: with the old `sentry.ts` and the new test, the
end-to-end test fails because the event has no diagnostics tags. With
this change, it passes. The three frame-extraction tests added for
review feedback fail against the previous extractor and pass now.
- The end-to-end test puts a canary credential in the query string, the
JSON body, the error name, the error message, a cause message, and
`HttpError.details`. It checks that no Sentry argument and no reported
stack contains the canary. It also checks that the HTTP response stays
`{ "error": "Internal server error" }` with the original status.
- The existing tests in `error-handler.test.ts` for chat setup and
observability credentials still pass without change. They check that the
reported error, the error context, and Telemetry carry no canary.
- Server typecheck with `PAPERCLIP_TYPECHECK_LOW_MEMORY=1 node
scripts/typecheck-layers.mjs`: no error in server files. My sandbox had
only the server dependency subset installed, so the layers also reported
missing-module errors in `cli/`, `ui/`, and `packages/`. These errors
are not related to this change. CI runs the full check.
- Note for local runs: an empty `ANTHROPIC_BASE_URL` makes
`sentry.test.ts` fail at import on `master` too. Unset it before you run
the suite.
## Risks
- Low risk. Sentry stays opt-in, and the change affects only events from
secret-sensitive routes.
- Privacy: the new values are closed labels, bounded numbers,
source-code route definitions, Node errno names, SQLSTATE codes, and V8
frames that match a strict pattern after the message header is removed.
A frame contains a function name and a file location. In theory, code
can give a function a name from data. The frame pattern accepts only
identifier-like names of at most 120 characters, and the event never
includes the error name or message.
- If code changes an error's `message` or `name` after its stack was
formatted, the header no longer matches and the event has no origin
frames. This fails closed.
- `req.route` holds the last route that the router dispatched. If a
later middleware fails after a route calls `next()`, `routePattern` can
name that earlier route. The route family is still correct because it
comes from the URL policy.
- Grouping does not change. The reported error keeps the same type,
message, and sanitizer stack, so existing Sentry issues keep collecting
events.
> For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and
discuss it in `#dev` before opening the PR. Feature PRs that overlap
with planned core work may need to be redirected — check the roadmap
first. See `CONTRIBUTING.md`.
## Model Used
- Provider: Anthropic. Model: Claude Opus 5.5 (`claude-opus-5-5`), used
through Claude Code with tool use (shell, file edits, test runs).
Extended reasoning was not explicitly enabled.
## 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
- [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>