mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-11 05:31:46 +02:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Claude local adapter supports subscription login through a sandbox > - The new-agent page must show login before the user creates an agent > - Test results must not expose raw sandbox diagnostics or secret values > - This pull request adds the login UI to both Test lanes and closes the diagnostic boundary > - The branch also adds durable cleanup recovery for failed sandbox teardown > - Reusable sandboxes must retain both their recorded teardown configuration and a valid lifecycle path until destruction succeeds > - The benefit is a usable login flow with fixed public checks, redacted server logs, and recoverable sandbox cleanup ## Linked Issues or Issue Description Related public work: [#9488](https://github.com/paperclipai/paperclip/pull/9488) adds first-class recognition for `CLAUDE_CODE_OAUTH_TOKEN` in headless and remote runs. Related public issue: [#2681](https://github.com/paperclipai/paperclip/issues/2681) requests Claude Code subscription support. This pull request adds the login transport and new-agent UI flow that those changes do not provide. **Subsystem affected:** Claude local adapter, server login probes, sandbox provider setup, cleanup recovery, and the new-agent UI. **Problem or motivation:** The Test lanes did not show the sandbox login panel in all supported cases. Test results also exposed raw probe diagnostics, and JSON escapes could end secret redaction early. **Proposed solution:** Surface the login capability through the bundled provider manifest. Prepare the same probe runtime in the ACP lane. Send diagnostics only to redacted server logs. Keep Test checks on fixed public messages. Normalize login URL hints to allowlisted HTTPS Claude and Anthropic hosts. Consume JSON escapes during redaction. Preserve failed sandbox cleanup state across retries and restarts, and prevent deletion from severing the lifecycle context of a live reusable sandbox. **Alternatives considered:** Keep raw diagnostics in Test checks or trust login URL text from the sandbox. Both choices increase information exposure. Keep separate probe behavior in the ACP lane. That choice would leave the two Test lanes inconsistent. ## What Changed - Surface the sandbox login panel on both Test lanes. - Reconcile the bundled Daytona plugin manifest so `supportsSetupTokenLogin` reaches the UI capability gate. - Prepare the ACP Test lane with the same probe runtime as the CLI Test lane. - Add the `claude_acp_login_probe_unavailable` warning when the ACP probe cannot run. - Send raw sandbox diagnostics only to redacted server logs. - Keep Test checks on fixed public messages in the ACP, managed-config, and CLI paths. - Normalize login URL hints to allowlisted HTTPS Claude and Anthropic hosts. - Redact JSON and escaped-JSON secret values, including escaped quotes and backslashes. - Preserve orphan cleanup records across provider failures, restarts, and unavailable plugins. - Atomically block environment deletion while a live reusable sandbox lease still depends on it. - Verify pending cleanup destroys plugin sandboxes with the provider configuration recorded on the lease, even after the current environment configuration changes. ## Verification - Head under review: `506b7fa2d83c36bfa5fd722ee9d95b0c7431c241`. - Focused environment route/service/runtime coverage passes: 196 tests across 3 files. - `pnpm -r typecheck` passes. - `pnpm build` passes. - The full Vitest run completed with 4,754 passing and 28 failing tests. All 23 source-test failures reproduce unchanged on parent head `58cfe61a33191ce03d965d65085d26064b4888ba`; the other 5 are duplicate executions from stale `server/dist` output. The failures are unrelated macOS path/listener and scheduler-fixture failures, so there is no new bad commit for bisect to localize. - All required CI checks pass for the current head, including build, typecheck/release registry, all server and workspace shards, serialized server suites, canary, and e2e. - A fresh Greptile review for `506b7fa2d83c36bfa5fd722ee9d95b0c7431c241` reports 5/5, “safe to merge,” with no blocking failure remaining. ## Risks - A probe or redaction change could hide useful server diagnostics. - An allowlist change could reject a valid Claude login URL. - Cleanup recovery changes could affect provider teardown ordering. - An environment with a live reusable sandbox can no longer be deleted until the owning issue or execution workspace completes teardown. - The implementation keeps public Test messages fixed and sends detail to redacted server logs. ## Model Used OpenAI GPT-5 via Codex — exact model ID: GPT-5; tool use and code execution enabled; extended reasoning enabled. The implementation author used AI-assisted development. ## 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 documented the result - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation or confirmed no separate documentation change is needed - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
320 lines
10 KiB
TypeScript
320 lines
10 KiB
TypeScript
import { EventEmitter } from "node:events";
|
|
import { describe, expect, it, vi } from "vitest";
|
|
import {
|
|
coordinateHeartbeatSchedulerShutdown,
|
|
finalizeServerShutdown,
|
|
loadWithoutCoordinatedShutdownSignalHooks,
|
|
} from "./shutdown.js";
|
|
|
|
function deferred<T = void>() {
|
|
let resolve!: (value: T) => void;
|
|
let reject!: (reason?: unknown) => void;
|
|
const promise = new Promise<T>((res, rej) => {
|
|
resolve = res;
|
|
reject = rej;
|
|
});
|
|
return { promise, resolve, reject };
|
|
}
|
|
|
|
function stubLogger() {
|
|
return { info: vi.fn(), error: vi.fn() };
|
|
}
|
|
|
|
describe("finalizeServerShutdown", () => {
|
|
it("awaits the setup-token cleanup before the database stop and the process exit", async () => {
|
|
const order: string[] = [];
|
|
// The held promise models the setup-token session cancellation and its
|
|
// sandbox lease release. The teardown must not continue while it is pending.
|
|
const release = deferred();
|
|
const shutdownAppServices = vi.fn(async () => {
|
|
order.push("appServices:start");
|
|
await release.promise;
|
|
order.push("appServices:settled");
|
|
});
|
|
const stopEmbeddedPostgres = vi.fn(async () => {
|
|
order.push("postgres:stop");
|
|
});
|
|
const shutdownInstrumentation = vi.fn(async () => {
|
|
order.push("instrumentation:flush");
|
|
});
|
|
|
|
let exited = false;
|
|
const finalize = finalizeServerShutdown({
|
|
signal: "SIGTERM",
|
|
shutdownAppServices,
|
|
stopEmbeddedPostgres,
|
|
shutdownInstrumentation,
|
|
log: stubLogger(),
|
|
}).then(() => {
|
|
// This models the caller's `process.exit(0)` continuation.
|
|
exited = true;
|
|
order.push("exit");
|
|
});
|
|
|
|
// The cleanup is in flight. The database stop, the instrumentation flush,
|
|
// and the process exit continuation must all wait for it to settle.
|
|
await vi.waitFor(() => expect(shutdownAppServices).toHaveBeenCalledOnce());
|
|
expect(stopEmbeddedPostgres).not.toHaveBeenCalled();
|
|
expect(shutdownInstrumentation).not.toHaveBeenCalled();
|
|
expect(exited).toBe(false);
|
|
|
|
release.resolve();
|
|
await finalize;
|
|
|
|
expect(exited).toBe(true);
|
|
expect(order).toEqual([
|
|
"appServices:start",
|
|
"appServices:settled",
|
|
"postgres:stop",
|
|
"instrumentation:flush",
|
|
"exit",
|
|
]);
|
|
});
|
|
|
|
it("keeps the teardown durable and still exits when the setup-token release fails", async () => {
|
|
const order: string[] = [];
|
|
// The held promise rejects, which models a lease release that failed. The
|
|
// reaper owns the durable retry, so the teardown must log the failure and
|
|
// continue rather than swallow it or block the exit.
|
|
const release = deferred();
|
|
const releaseError = new Error("lease release failed");
|
|
const shutdownAppServices = vi.fn(async () => {
|
|
await release.promise;
|
|
});
|
|
const stopEmbeddedPostgres = vi.fn(async () => {
|
|
order.push("postgres:stop");
|
|
});
|
|
const shutdownInstrumentation = vi.fn(async () => {
|
|
order.push("instrumentation:flush");
|
|
});
|
|
const log = stubLogger();
|
|
|
|
let exited = false;
|
|
const finalize = finalizeServerShutdown({
|
|
signal: "SIGTERM",
|
|
shutdownAppServices,
|
|
stopEmbeddedPostgres,
|
|
shutdownInstrumentation,
|
|
log,
|
|
}).then(() => {
|
|
exited = true;
|
|
});
|
|
|
|
await vi.waitFor(() => expect(shutdownAppServices).toHaveBeenCalledOnce());
|
|
expect(stopEmbeddedPostgres).not.toHaveBeenCalled();
|
|
expect(exited).toBe(false);
|
|
|
|
release.reject(releaseError);
|
|
await finalize;
|
|
|
|
// The teardown surfaced the failure in the log, then finished the ordered
|
|
// teardown and reached the exit continuation.
|
|
expect(log.error).toHaveBeenCalledWith(
|
|
expect.objectContaining({ err: releaseError, signal: "SIGTERM" }),
|
|
expect.any(String),
|
|
);
|
|
expect(order).toEqual(["postgres:stop", "instrumentation:flush"]);
|
|
expect(exited).toBe(true);
|
|
});
|
|
|
|
it("skips the database stop when no embedded PostgreSQL runs in this process", async () => {
|
|
const shutdownAppServices = vi.fn(async () => undefined);
|
|
const shutdownInstrumentation = vi.fn(async () => undefined);
|
|
const log = stubLogger();
|
|
|
|
await finalizeServerShutdown({
|
|
signal: "SIGINT",
|
|
shutdownAppServices,
|
|
stopEmbeddedPostgres: null,
|
|
shutdownInstrumentation,
|
|
log,
|
|
});
|
|
|
|
expect(shutdownAppServices).toHaveBeenCalledOnce();
|
|
expect(shutdownInstrumentation).toHaveBeenCalledOnce();
|
|
expect(log.info).not.toHaveBeenCalled();
|
|
});
|
|
});
|
|
|
|
describe("loadWithoutCoordinatedShutdownSignalHooks", () => {
|
|
it("removes the eager signal handlers from the real embedded-postgres import", async () => {
|
|
const before = {
|
|
SIGINT: process.rawListeners("SIGINT"),
|
|
SIGTERM: process.rawListeners("SIGTERM"),
|
|
};
|
|
const moduleName = "embedded-postgres";
|
|
|
|
await loadWithoutCoordinatedShutdownSignalHooks(() => import(moduleName));
|
|
|
|
expect(process.rawListeners("SIGINT")).toEqual(before.SIGINT);
|
|
expect(process.rawListeners("SIGTERM")).toEqual(before.SIGTERM);
|
|
});
|
|
|
|
it("keeps the database available for a marker-backed SIGTERM snapshot", async () => {
|
|
const signalTarget = new EventEmitter();
|
|
const preexistingSignalListener = vi.fn();
|
|
signalTarget.on("SIGTERM", preexistingSignalListener);
|
|
|
|
let databaseAvailable = true;
|
|
const embeddedPostgresExitHook = vi.fn(() => {
|
|
databaseAvailable = false;
|
|
});
|
|
await loadWithoutCoordinatedShutdownSignalHooks(
|
|
async () => {
|
|
signalTarget.on("SIGINT", embeddedPostgresExitHook);
|
|
signalTarget.on("SIGTERM", embeddedPostgresExitHook);
|
|
return { default: class EmbeddedPostgres {} };
|
|
},
|
|
signalTarget,
|
|
);
|
|
|
|
let shutdown: Promise<unknown> | null = null;
|
|
let snapshotCaptured = false;
|
|
signalTarget.once("SIGTERM", () => {
|
|
shutdown = coordinateHeartbeatSchedulerShutdown({
|
|
signal: "SIGTERM",
|
|
prepareHotRestartShutdown: async () => {
|
|
// This models the real failure path: a valid intent exists, and the
|
|
// snapshot must query embedded PostgreSQL after SIGTERM is delivered.
|
|
expect(databaseAvailable).toBe(true);
|
|
snapshotCaptured = true;
|
|
return { mode: "hot_restart" as const, skipDrain: true };
|
|
},
|
|
waitForHeartbeatSchedulerIdle: vi.fn(async () => undefined),
|
|
});
|
|
});
|
|
|
|
signalTarget.emit("SIGTERM");
|
|
await shutdown;
|
|
|
|
expect(preexistingSignalListener).toHaveBeenCalledOnce();
|
|
expect(embeddedPostgresExitHook).not.toHaveBeenCalled();
|
|
expect(snapshotCaptured).toBe(true);
|
|
});
|
|
});
|
|
|
|
describe("coordinateHeartbeatSchedulerShutdown", () => {
|
|
it("quiesces active scheduler work before capturing a hot-restart snapshot", async () => {
|
|
let snapshotCaptured = false;
|
|
let releaseScheduler!: () => void;
|
|
const schedulerIdle = new Promise<void>((resolve) => {
|
|
releaseScheduler = resolve;
|
|
});
|
|
const waitForHeartbeatSchedulerIdle = vi.fn(() => schedulerIdle);
|
|
|
|
const shutdown = coordinateHeartbeatSchedulerShutdown({
|
|
signal: "SIGTERM",
|
|
prepareHotRestartShutdown: vi.fn(async () => {
|
|
snapshotCaptured = true;
|
|
return { mode: "prepared" as const, skipDrain: true };
|
|
}),
|
|
waitForHeartbeatSchedulerIdle,
|
|
});
|
|
|
|
await vi.waitFor(() => expect(waitForHeartbeatSchedulerIdle).toHaveBeenCalledOnce());
|
|
expect(snapshotCaptured).toBe(false);
|
|
releaseScheduler();
|
|
|
|
const result = await shutdown;
|
|
expect(snapshotCaptured).toBe(true);
|
|
expect(result).toEqual({
|
|
hotRestart: { mode: "prepared", skipDrain: true },
|
|
preparationError: null,
|
|
waitedForSchedulerIdle: true,
|
|
});
|
|
});
|
|
|
|
it("quiesces scheduler work before selecting server-stdio runs to drain", async () => {
|
|
const waitForHeartbeatSchedulerIdle = vi.fn(async () => undefined);
|
|
|
|
const result = await coordinateHeartbeatSchedulerShutdown({
|
|
signal: "SIGTERM",
|
|
prepareHotRestartShutdown: vi.fn(async () => ({
|
|
mode: "acp_drain_required" as const,
|
|
skipDrain: false,
|
|
drainRunIds: ["acp-run"],
|
|
})),
|
|
waitForHeartbeatSchedulerIdle,
|
|
});
|
|
|
|
expect(waitForHeartbeatSchedulerIdle).toHaveBeenCalledOnce();
|
|
expect(result).toEqual({
|
|
hotRestart: {
|
|
mode: "acp_drain_required",
|
|
skipDrain: false,
|
|
drainRunIds: ["acp-run"],
|
|
},
|
|
preparationError: null,
|
|
waitedForSchedulerIdle: true,
|
|
});
|
|
});
|
|
|
|
it("preserves the scheduler idle wait for normal graceful shutdown", async () => {
|
|
let releaseScheduler!: () => void;
|
|
const schedulerIdle = new Promise<void>((resolve) => {
|
|
releaseScheduler = resolve;
|
|
});
|
|
const waitForHeartbeatSchedulerIdle = vi.fn(() => schedulerIdle);
|
|
let settled = false;
|
|
|
|
const shutdown = coordinateHeartbeatSchedulerShutdown({
|
|
signal: "SIGTERM",
|
|
prepareHotRestartShutdown: vi.fn(async () => ({
|
|
mode: "not_requested" as const,
|
|
skipDrain: false,
|
|
})),
|
|
waitForHeartbeatSchedulerIdle,
|
|
}).finally(() => {
|
|
settled = true;
|
|
});
|
|
|
|
await vi.waitFor(() => expect(waitForHeartbeatSchedulerIdle).toHaveBeenCalledOnce());
|
|
expect(settled).toBe(false);
|
|
|
|
releaseScheduler();
|
|
|
|
await expect(shutdown).resolves.toEqual({
|
|
hotRestart: { mode: "not_requested", skipDrain: false },
|
|
preparationError: null,
|
|
waitedForSchedulerIdle: true,
|
|
});
|
|
});
|
|
|
|
it("waits for scheduler idle when hot-restart preparation is unavailable", async () => {
|
|
const waitForHeartbeatSchedulerIdle = vi.fn(async () => undefined);
|
|
|
|
const result = await coordinateHeartbeatSchedulerShutdown({
|
|
signal: "SIGTERM",
|
|
prepareHotRestartShutdown: null,
|
|
waitForHeartbeatSchedulerIdle,
|
|
});
|
|
|
|
expect(waitForHeartbeatSchedulerIdle).toHaveBeenCalledOnce();
|
|
expect(result).toEqual({
|
|
hotRestart: null,
|
|
preparationError: null,
|
|
waitedForSchedulerIdle: true,
|
|
});
|
|
});
|
|
|
|
it("falls back to the scheduler idle wait when hot-restart preparation fails", async () => {
|
|
const preparationError = new Error("snapshot failed");
|
|
const waitForHeartbeatSchedulerIdle = vi.fn(async () => undefined);
|
|
|
|
const result = await coordinateHeartbeatSchedulerShutdown({
|
|
signal: "SIGTERM",
|
|
prepareHotRestartShutdown: vi.fn(async () => {
|
|
throw preparationError;
|
|
}),
|
|
waitForHeartbeatSchedulerIdle,
|
|
});
|
|
|
|
expect(waitForHeartbeatSchedulerIdle).toHaveBeenCalledOnce();
|
|
expect(result).toEqual({
|
|
hotRestart: null,
|
|
preparationError,
|
|
waitedForSchedulerIdle: true,
|
|
});
|
|
});
|
|
});
|