mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 08:22:20 +02:00
fix(server): keep local authorization failures from disabling chat endpoints (#16138)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Chat endpoints publish agent replies to providers such as GitHub, and a durable outbox classifies each send failure > - A GitHub review run that fails posts a "review failed" reply. When the run has no request binding, Paperclip throws its own `forbidden()` before any GitHub request > - The classifier reads that local 403 as a provider credential failure. The endpoint goes to `attention`, its tool connection is disabled, and new `pull_request` webhooks are dropped > - One failed review can stop all reviews on that endpoint until an operator reconnects it > - This pull request classifies Paperclip's own 401/403 `HttpError` without a provider status as a failure of that publication only > - The benefit is that only real provider credential failures lock an endpoint ## Linked Issues or Issue Description Fixes: #16137 ## What Changed - `server/src/services/chat-publication-errors.ts`: an `HttpError` (Paperclip's own class) with status 401 or 403 and no numeric `details.providerStatus` returns `{ kind: "failed" }`. The check runs before the credential branch. - Provider errors do not change. Adapter `AuthenticationError`/`PermissionError`, Octokit errors (named `HttpError`, but not this class) and Paperclip-wrapped provider rejections that carry `providerStatus` still return `endpoint_attention`. - `server/src/services/chat-publication-errors.test.ts`: a new test covers a local `forbidden()`, a local `unauthorized()` and a wrapped provider 403. ## Verification - `pnpm --filter @paperclipai/server vitest run src/services/chat-publication-errors.test.ts` - I ran the classifier source with type stripping on Node 22: local `forbidden("GitHub failure has no request binding")` → `failed`; local `unauthorized()` → `failed`; `new HttpError(403, …, { providerStatus: 403 })` → `endpoint_attention`; an Octokit-shaped 403 → `endpoint_attention`. - I applied the same change to the built `dist` of a self-hosted install and called its classifier with the same four errors. The results were the same. ## Risks - Low risk. The change applies only to Paperclip's own `HttpError` with status 401/403 and no provider status. Such an error now fails one publication and does not disable the endpoint. Code that throws a local 401/403 to report a provider credential problem must attach `providerStatus`. I found no such caller in `server/src/services`. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`, Anthropic), in Claude Code with tool use (shell, file edits, test runs). It helped to find the cause, write the change and the test, and run the checks. ## 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 - [ ] I have run tests locally and they pass (classifier checked with type stripping; the full vitest suite runs in CI) - [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
This commit is contained in:
1 parent
56e09cd19f
commit
0ed760ce9a
2 files changed
+33
No files matched your search
@@ -1,4 +1,5 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { forbidden, HttpError, unauthorized } from "../errors.js";
|
||||
import { classifyChatPublicationError } from "./chat-publication-errors.js";
|
||||
|
||||
function providerError(
|
||||
@@ -155,6 +156,27 @@ describe("chat publication error classification", () => {
|
||||
).toMatchObject({ kind: "endpoint_attention" });
|
||||
});
|
||||
|
||||
it("keeps Paperclip's own authorization failures scoped to the publication", () => {
|
||||
expect(
|
||||
classifyChatPublicationError(
|
||||
forbidden("GitHub failure has no request binding"),
|
||||
1,
|
||||
),
|
||||
).toMatchObject({ kind: "failed" });
|
||||
expect(classifyChatPublicationError(unauthorized(), 1)).toMatchObject({
|
||||
kind: "failed",
|
||||
});
|
||||
expect(
|
||||
classifyChatPublicationError(
|
||||
new HttpError(403, "GitHub rejected this operation", {
|
||||
code: "github_bot_operation_failed",
|
||||
providerStatus: 403,
|
||||
}),
|
||||
1,
|
||||
),
|
||||
).toMatchObject({ kind: "endpoint_attention" });
|
||||
});
|
||||
|
||||
it("does not call explicit GitHub 4xx rejections ambiguous delivery", () => {
|
||||
for (const status of [400, 409, 422]) {
|
||||
expect(
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
import { HttpError } from "../errors.js";
|
||||
|
||||
export type ChatPublicationErrorDisposition =
|
||||
| {
|
||||
kind: "retry";
|
||||
@@ -317,6 +319,15 @@ export function classifyChatPublicationError(
|
||||
return { kind: "resource_unavailable", reason };
|
||||
}
|
||||
|
||||
if (
|
||||
error instanceof HttpError &&
|
||||
(error.status === 401 || error.status === 403) &&
|
||||
typeof (error.details as { providerStatus?: unknown } | undefined)
|
||||
?.providerStatus !== "number"
|
||||
) {
|
||||
return { kind: "failed", reason };
|
||||
}
|
||||
|
||||
if (
|
||||
names.includes("AuthenticationError") ||
|
||||
names.includes("PermissionError") ||
|
||||
|
||||
Reference in new issue
Block a user