mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 12:33:05 +01:00
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - Agents and the board read task workspace files through the
`/api/issues/:issueId/file-resources/*` routes
> - Clients can identify a task by its UUID or by its human identifier,
for example `ABC-230`
> - The issues router changes identifiers to UUIDs, but the
file-resources router does not
> - Thus an identifier goes into a `uuid` comparison, Postgres rejects
the query, and the route returns 500
> - This pull request resolves identifiers in the file-resources router
and returns 404 for malformed ids
> - The benefit is that identifier links work, and bad ids give a
correct 404 and no server error
## Linked Issues or Issue Description
**What happened?**
A request to `GET /api/issues/ABC-230/file-resources/list` returned 500.
The server log showed `invalid input syntax for type uuid` from
`workspaceFileResourceService.getIssue`. Sentry recorded this error on
hosted staging.
**Steps to reproduce**
1. Create a task that has an identifier, for example `ABC-1`.
2. Send `GET /api/issues/ABC-1/file-resources/list?workspace=project`.
3. The server returns 500. The same request with the task UUID returns
200.
**Expected behavior**
The route accepts the identifier, as the other `/api/issues/:id` routes
do. An unknown or malformed id returns 404.
## What Changed
- Added `resolveIssueId` to `WorkspaceFileResourceService`. It changes a
task identifier to the task UUID. It does not check visibility.
`getIssue` still does the privacy-scoped read.
- Added a `router.param("issueId")` handler to the file-resources
router. It calls `resolveIssueId`, the same as the issues router does
for its routes.
- `getIssue` now returns 404 before the query when the id is not a UUID.
This stops the Postgres cast error for all callers.
- Added a regression test for identifier, lowercase identifier, private
task, unknown identifier, and malformed id on the `content`, `list`, and
`availability` routes.
- Updated the test service fakes with the new method.
## Verification
- `pnpm vitest run src/__tests__/file-resources.test.ts
src/__tests__/file-resources-git-scan-load.test.ts` in `server/`: 55
passed.
- Without the source change, the new test fails with `expected 200 "OK",
got 500 "Internal Server Error"`. This reproduces the production error.
- I did not run a full local `tsc` because the sandbox did not have
sufficient memory. CI runs the typecheck.
## Risks
- Low risk. The change adds one indexed lookup on `issues.identifier`
(unique index) only when the path value has the identifier format.
- The identifier lookup does not check visibility. `getIssue` then
applies the privacy-scoped read, so a private or unknown task gives the
same 404 as before. The route does not show if a task exists.
> 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
- Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used through Claude
Code with tool use (shell, file edit, test runs).
## 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: devinfoley <139239+devinfoley@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Paperclip <noreply@paperclip.ing>
272 lines
9.7 KiB
TypeScript
272 lines
9.7 KiB
TypeScript
import express from "express";
|
|
import fs from "node:fs/promises";
|
|
import os from "node:os";
|
|
import path from "node:path";
|
|
import request from "supertest";
|
|
import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";
|
|
import type { Db } from "@paperclipai/db";
|
|
import { errorHandler } from "../middleware/index.js";
|
|
import {
|
|
createFileResourceListLimiter,
|
|
fileResourceRoutes,
|
|
type WorkspaceFileResourceService,
|
|
} from "../routes/file-resources.js";
|
|
import {
|
|
createWorkspaceGitOperationScheduler,
|
|
WORKSPACE_GIT_SCAN_ERROR_CODES,
|
|
WorkspaceGitScanError,
|
|
type WorkspaceGitRunner,
|
|
} from "../services/workspace-git-operation-scheduler.js";
|
|
vi.mock("../services/activity-log.js", () => ({
|
|
logActivity: vi.fn(async () => undefined),
|
|
}));
|
|
|
|
function unavailableMethods(): Pick<WorkspaceFileResourceService, "availability" | "resolve" | "readContent" | "prepareDownload"> {
|
|
return {
|
|
availability: vi.fn(async () => { throw new Error("not used"); }),
|
|
resolve: vi.fn(async () => { throw new Error("not used"); }),
|
|
readContent: vi.fn(async () => { throw new Error("not used"); }),
|
|
prepareDownload: vi.fn(async () => { throw new Error("not used"); }),
|
|
};
|
|
}
|
|
|
|
function availableList(input: { mode?: "all" | "recent" | "changed" | null } = {}) {
|
|
return {
|
|
kind: "workspace_file_list" as const,
|
|
state: "available" as const,
|
|
workspace: {
|
|
provider: "local_fs" as const,
|
|
workspaceLabel: "Workspace",
|
|
workspaceKind: "project_workspace" as const,
|
|
workspaceId: "11111111-1111-4111-8111-111111111111",
|
|
},
|
|
query: {
|
|
workspace: "auto" as const,
|
|
mode: input.mode ?? "changed",
|
|
q: null,
|
|
limit: 25,
|
|
offset: 0,
|
|
},
|
|
items: [],
|
|
scannedCount: 0,
|
|
truncated: false,
|
|
};
|
|
}
|
|
|
|
function createLoadApp(db: Db, companyId: string, service: WorkspaceFileResourceService) {
|
|
const app = express();
|
|
app.get("/api/health", (_req, res) => res.json({ status: "ok" }));
|
|
app.use((req, _res, next) => {
|
|
req.actor = {
|
|
type: "board",
|
|
userId: String(req.headers["x-test-actor"] ?? "board-user"),
|
|
companyIds: [companyId],
|
|
source: "session",
|
|
isInstanceAdmin: false,
|
|
};
|
|
next();
|
|
});
|
|
app.use("/api", fileResourceRoutes(db, {
|
|
service,
|
|
listLimiter: createFileResourceListLimiter({
|
|
maxConcurrent: 2,
|
|
maxRequests: 10,
|
|
windowMs: 60_000,
|
|
}),
|
|
}));
|
|
app.use(errorHandler);
|
|
return app;
|
|
}
|
|
|
|
describe("workspace Git scan route load regression", () => {
|
|
const db = {} as Db;
|
|
const companyId = "11111111-1111-4111-8111-111111111111";
|
|
let tempRoot: string;
|
|
|
|
beforeAll(async () => {
|
|
tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-file-scan-load-"));
|
|
await Promise.all([
|
|
fs.mkdir(path.join(tempRoot, "repository-a")),
|
|
fs.mkdir(path.join(tempRoot, "repository-b")),
|
|
]);
|
|
}, 60_000);
|
|
|
|
afterAll(async () => {
|
|
await fs.rm(tempRoot, { recursive: true, force: true });
|
|
});
|
|
|
|
it("keeps 500 cross-actor/cross-issue requests globally bounded while health stays responsive", async () => {
|
|
let releaseScans!: () => void;
|
|
const scanGate = new Promise<void>((resolve) => {
|
|
releaseScans = resolve;
|
|
});
|
|
let active = 0;
|
|
let peakActive = 0;
|
|
let runnerCalls = 0;
|
|
const runner: WorkspaceGitRunner = async () => {
|
|
runnerCalls += 1;
|
|
active += 1;
|
|
peakActive = Math.max(peakActive, active);
|
|
await scanGate;
|
|
active -= 1;
|
|
return { stdout: "", stderr: "" };
|
|
};
|
|
const scheduler = createWorkspaceGitOperationScheduler({
|
|
concurrency: 2,
|
|
queueCapacity: 4,
|
|
runner,
|
|
defaultCacheTtlMs: 0,
|
|
});
|
|
const roots = [path.join(tempRoot, "repository-a"), path.join(tempRoot, "repository-b")];
|
|
const service: WorkspaceFileResourceService = {
|
|
resolveIssueId: vi.fn(async (issueId: string) => issueId),
|
|
getIssue: vi.fn(async () => ({ companyId })),
|
|
list: vi.fn(async (issueId, input, opts) => {
|
|
const numericId = Number(issueId.slice("issue-".length));
|
|
await scheduler.run({
|
|
workspacePath: roots[numericId % roots.length]!,
|
|
args: ["status", "--porcelain=v1", "-z", "--untracked-files=all"],
|
|
operation: "route_load.changed_files",
|
|
fairnessKeys: opts?.scanContext?.fairnessKeys,
|
|
signal: opts?.scanContext?.signal,
|
|
cacheTtlMs: 0,
|
|
});
|
|
return availableList(input);
|
|
}),
|
|
...unavailableMethods(),
|
|
};
|
|
const app = createLoadApp(db, companyId, service);
|
|
// Model one busy API server, not 500 independent ephemeral listeners whose
|
|
// accept/close races can reset clients before they reach the scheduler.
|
|
const server = app.listen(0, "127.0.0.1");
|
|
await new Promise<void>(resolve => server.once("listening", resolve));
|
|
const pendingResponses = Array.from({ length: 500 }, (_, index) => request(server)
|
|
.get(`/api/issues/issue-${index}/file-resources/list`)
|
|
.set("x-test-actor", `actor-${index % 73}`)
|
|
.query({ mode: "changed" })
|
|
.then((response) => response));
|
|
// Observe every client rejection immediately, including on assertion failure.
|
|
const settledResponses = Promise.allSettled(pendingResponses);
|
|
try {
|
|
await vi.waitFor(
|
|
() => expect(scheduler.snapshot().totals.singleFlightJoins).toBe(498),
|
|
{ timeout: 15_000, interval: 20 },
|
|
);
|
|
const loadedSnapshot = scheduler.snapshot();
|
|
expect(loadedSnapshot).toMatchObject({ activeCount: 2, queuedCount: 0, inFlightCount: 2 });
|
|
|
|
const healthLatencies: number[] = [];
|
|
for (let index = 0; index < 25; index += 1) {
|
|
const startedAt = performance.now();
|
|
const response = await request(server).get("/api/health");
|
|
healthLatencies.push(performance.now() - startedAt);
|
|
expect(response.status).toBe(200);
|
|
}
|
|
healthLatencies.sort((left, right) => left - right);
|
|
const healthP99Ms = healthLatencies[Math.ceil(healthLatencies.length * 0.99) - 1]!;
|
|
expect(healthP99Ms).toBeLessThan(250);
|
|
|
|
releaseScans();
|
|
const responses = (await settledResponses).map(result => {
|
|
if (result.status === "rejected") throw result.reason;
|
|
return result.value;
|
|
});
|
|
const outcomeCounts = responses.reduce<Record<number, number>>((counts, response) => {
|
|
counts[response.status] = (counts[response.status] ?? 0) + 1;
|
|
return counts;
|
|
}, {});
|
|
expect(outcomeCounts).toEqual({ 200: 500 });
|
|
expect({ runnerCalls, peakActive }).toEqual({ runnerCalls: 2, peakActive: 2 });
|
|
expect(scheduler.snapshot()).toMatchObject({ activeCount: 0, queuedCount: 0, inFlightCount: 0 });
|
|
|
|
console.info("workspace Git scan regression metrics", {
|
|
requests: responses.length,
|
|
repositories: roots.length,
|
|
underlyingScans: runnerCalls,
|
|
peakActive,
|
|
peakQueued: loadedSnapshot.queuedCount,
|
|
outcomes: outcomeCounts,
|
|
healthP99Ms: Math.round(healthP99Ms * 100) / 100,
|
|
unreapedChildCount: 0,
|
|
});
|
|
} finally {
|
|
releaseScans();
|
|
server.closeAllConnections();
|
|
await settledResponses;
|
|
await new Promise<void>((resolve, reject) => server.close(error => error ? reject(error) : resolve()));
|
|
}
|
|
}, 60_000);
|
|
|
|
it.each([
|
|
[WORKSPACE_GIT_SCAN_ERROR_CODES.saturated, 503],
|
|
[WORKSPACE_GIT_SCAN_ERROR_CODES.timeout, 504],
|
|
] as const)("returns stable retryable %s responses", async (code, status) => {
|
|
const service: WorkspaceFileResourceService = {
|
|
resolveIssueId: vi.fn(async (issueId: string) => issueId),
|
|
getIssue: vi.fn(async () => ({ companyId })),
|
|
list: vi.fn(async () => {
|
|
throw new WorkspaceGitScanError(code, "Changed files temporarily unavailable");
|
|
}),
|
|
...unavailableMethods(),
|
|
};
|
|
const app = createLoadApp(db, companyId, service);
|
|
|
|
const response = await request(app)
|
|
.get(`/api/issues/${code}/file-resources/list`)
|
|
.query({ mode: "changed" });
|
|
|
|
expect(response.status).toBe(status);
|
|
expect(response.headers["retry-after"]).toBe("1");
|
|
expect(response.body).toMatchObject({
|
|
code,
|
|
details: { code, retryable: true },
|
|
});
|
|
});
|
|
|
|
it("cancels the underlying scan and releases its slot when the HTTP client disconnects", async () => {
|
|
let runnerAborted = false;
|
|
const scheduler = createWorkspaceGitOperationScheduler({
|
|
concurrency: 1,
|
|
runner: ({ signal }) => new Promise((_resolve, reject) => {
|
|
signal.addEventListener("abort", () => {
|
|
runnerAborted = true;
|
|
reject(new WorkspaceGitScanError(
|
|
WORKSPACE_GIT_SCAN_ERROR_CODES.cancelled,
|
|
"cancelled",
|
|
));
|
|
}, { once: true });
|
|
}),
|
|
});
|
|
const service: WorkspaceFileResourceService = {
|
|
resolveIssueId: vi.fn(async (issueId: string) => issueId),
|
|
getIssue: vi.fn(async () => ({ companyId })),
|
|
list: vi.fn(async (_issueId, input, opts) => {
|
|
await scheduler.run({
|
|
workspacePath: path.join(tempRoot, "repository-a"),
|
|
args: ["status", "--porcelain=v1", "-z", "--untracked-files=all"],
|
|
operation: "route_disconnect.changed_files",
|
|
signal: opts?.scanContext?.signal,
|
|
cacheTtlMs: 0,
|
|
});
|
|
return availableList(input);
|
|
}),
|
|
...unavailableMethods(),
|
|
};
|
|
const app = createLoadApp(db, companyId, service);
|
|
const pending = request(app)
|
|
.get("/api/issues/disconnected/file-resources/list")
|
|
.query({ mode: "changed" });
|
|
const outcome = pending.then(
|
|
(response) => response,
|
|
(error: unknown) => error,
|
|
);
|
|
await vi.waitFor(() => expect(scheduler.snapshot().activeCount).toBe(1));
|
|
|
|
pending.abort();
|
|
await outcome;
|
|
|
|
await vi.waitFor(() => expect(scheduler.snapshot()).toMatchObject({ activeCount: 0, inFlightCount: 0 }));
|
|
expect(runnerAborted).toBe(true);
|
|
});
|
|
});
|