From 5a7e740d820a5c53c0bb0d31b3193918ee63cd22 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 23 Oct 2026 16:31:18 -0700 Subject: [PATCH] fix: validate managed AI accounts before verification allocation (#17384) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A saved agent must pass execution configuration checks before it can run. > - Managed AI account selection belongs to the existing company-scoped account service. > - Verification allocated its execution environment before it checked that account. > - A missing default or revoked account could create a sandbox on each retry. > - This pull request checks account selection before allocation and keeps the final check. > - Invalid account settings now fail before they cause avoidable provider work. ## Linked Issues or Issue Description **What happened?** Saved-agent lifecycle verification allocates a test execution target before `withManagedAiProbe` selects the managed AI account. An unavailable account can cause sandbox creation and cleanup on every retry. **Expected behavior** Check account compatibility, responsible-user access, default selection, grant health and agent eligibility before target allocation. Check current account access again before the real probe. **Steps to reproduce** 1. Configure a saved OpenCode agent with an OpenRouter personal-default binding. 2. Leave that user's OpenRouter default empty, or revoke the selected grant. 3. Let lifecycle verification use a sandbox execution environment. 4. Before this change, verification can allocate the target and then reject the account. After this change, account selection rejects the configuration before allocation. **Paperclip version or commit** Source baseline `b5bf86ea09042a864b60910f80c8fdf7e64e9ef0`. **Deployment mode** Server with Postgres and a remote execution environment. Tests use disposable local Postgres and a controlled target allocation boundary. Searched public PRs and issues for verification, connection preflight, probes and lifecycle work. No duplicate fix found. Related public issue #16128 concerns credential probing and reauthorization. This change does not alter reauthorization or credential refresh. ## What Changed - Call the existing managed account selector with the resolved adapter and model before allocating a verification execution target. - Keep router resolution, secret handling, final account selection and target release with their existing owners. - Add six Postgres tests and document the saved-agent ordering contract. ## Verification - Focused verification suites: 30 tests passed across two files. - `pnpm build`: passed. - `pnpm -r typecheck`: passed. - Full PR CI passed on `ddc0879d46b60310c533e6d3c4b8acdd178d979c`: 54 successful checks and two expected skips. [Workflow evidence](https://github.com/paperclipai/paperclip/actions/runs/40033261909). This includes typecheck, tests, build and browser checks. - Apex review: 5/5 on that exact head, clean check, no comments or unresolved threads. Merge state is clean. - Additional local `pnpm test:run` is still running. The complete local suite is not yet a pass claim. - The first focused run passed the missing-default case but failed five setup hooks because fixtures reused a unique company prefix. Each fixture now has a distinct prefix. The complete focused rerun passed. - Tests use real account selection, memberships, grants, personal defaults, encrypted fixture credentials and managed runtime preparation. Only execution-target allocation and the provider probe are controlled boundaries. No live provider request is required. - Tests prove that missing defaults, revoked grants and incompatible models do not allocate a target. A valid account still reaches the managed probe. Revocation during allocation still fails the final account check and releases the target. Probe failure also releases it. - `git diff --check`: passed. Reviewed the diff for secrets and private account data before push. - No hosted login or production deployment is claimed for this follow-up. ## Risks - Adds one read-only account selection to each managed saved-agent verification attempt. The selector can read company accounts and grants. It makes no provider request and creates no execution target. - Final selection stays fresh. A successful preflight does not reserve an account or cache authorization. - Missing configuration still follows the existing durable lifecycle retry schedule. This change prevents needless allocation; it does not remove those database retries. - No schema, scheduler, provider-account choice, capacity or public API changes. ## Model Used OpenAI Codex, GPT-6 family. The exact deployment model ID and context window are not exposed in this session. Used reasoning, repository tools and code execution. ## 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 - [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 --- doc/connections/AI-CONNECTIONS.md | 5 + ...ent-harness-verification-preflight.test.ts | 128 ++++++++++++++++++ .../services/agent-harness-verification.ts | 6 + 3 files changed, 139 insertions(+) create mode 100644 server/src/__tests__/agent-harness-verification-preflight.test.ts diff --git a/doc/connections/AI-CONNECTIONS.md b/doc/connections/AI-CONNECTIONS.md index affb9ac7a39..1bd9d7a81da 100644 --- a/doc/connections/AI-CONNECTIONS.md +++ b/doc/connections/AI-CONNECTIONS.md @@ -116,6 +116,11 @@ runtime test accepts the form’s prospective adapter selection before it is sav For a saved-agent test, omitting `environmentId` uses the agent’s saved override. Sending `environmentId: null` tests a change back to the instance default. +Saved-agent lifecycle verification checks managed account access before it +allocates a test execution environment. Missing defaults, revoked accounts, and +incompatible selections fail without creating a sandbox. The managed probe +checks account access again after allocation so changes during setup still apply. + ## On-demand usage limits The AI account detail has **Check usage**. It reads the selected account only diff --git a/server/src/__tests__/agent-harness-verification-preflight.test.ts b/server/src/__tests__/agent-harness-verification-preflight.test.ts new file mode 100644 index 00000000000..b03d955e428 --- /dev/null +++ b/server/src/__tests__/agent-harness-verification-preflight.test.ts @@ -0,0 +1,128 @@ +import { randomUUID } from "node:crypto"; +import { mkdtemp, realpath, rm } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { eq } from "drizzle-orm"; +import { agents, aiProviderDefaults, companies, companyMemberships, connectionGrants, createDb } from "@paperclipai/db"; +import type { AdapterEnvironmentTestResult } from "@paperclipai/adapter-utils"; +import { agentHarnessVerificationService } from "../services/agent-harness-verification.js"; +import * as environmentTests from "../services/agent-environment-test.js"; +import { aiConnectionService } from "../services/ai-connections.js"; +import { readAiConnectionConfigurationFailure } from "../services/ai-connection-configuration-failure.js"; +import type { PluginWorkerManager } from "../services/plugin-worker-manager.js"; +import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; + +const support = await getEmbeddedPostgresTestSupport(); +const describePostgres = support.supported ? describe : describe.skip; +const realEnvironmentTests = environmentTests.agentEnvironmentTestService; + +describePostgres("saved harness account preflight", () => { + let database: Awaited>; + let db: ReturnType; + let home: string; + let agent: typeof agents.$inferSelect; + let userId: string; + let account: { connectionId: string; grantId: string }; + let fixtureNumber = 0; + const binding = { provider: "openrouter", method: "api_key", mode: "responsible_user" } as const; + const passed: AdapterEnvironmentTestResult = { + adapterType: "opencode_local", status: "pass", testedAt: new Date().toISOString(), + checks: [{ code: "opencode_hello_probe_passed", level: "info", message: "Fixture probe passed" }], + }; + const release = vi.fn<(status?: "released" | "failed") => Promise>(); + const allocate = vi.fn["resolveAdapterTestExecutionContext"]>(); + const probe = vi.fn["testManagedEnvironment"]>(); + const verify = () => agentHarnessVerificationService(db, {} as PluginWorkerManager).verify(agent); + + beforeAll(async () => { + home = await realpath(await mkdtemp(path.join(os.tmpdir(), "paperclip-harness-preflight-"))); + vi.stubEnv("PAPERCLIP_HOME", home); + vi.stubEnv("PAPERCLIP_INSTANCE_ID", "harness-preflight-fixture"); + database = await startEmbeddedPostgresTestDatabase("paperclip-harness-preflight-db-"); + db = createDb(database.connectionString); + }, 90_000); + + beforeEach(async () => { + release.mockReset().mockResolvedValue(undefined); + allocate.mockReset().mockResolvedValue({ + executionTarget: null, environmentName: "Fixture execution target", fallbackChecks: [], release, + }); + probe.mockReset().mockResolvedValue(passed); + vi.spyOn(environmentTests, "agentEnvironmentTestService").mockImplementation((database, manager) => ({ + ...realEnvironmentTests(database, manager), + resolveAdapterTestEnvironmentId: vi.fn().mockResolvedValue(null), + resolveAdapterTestExecutionContext: allocate, + testManagedEnvironment: probe, + })); + vi.spyOn(globalThis, "fetch").mockRejectedValue(new Error("Unexpected provider request")); + const companyId = randomUUID(); + userId = randomUUID(); + await db.insert(companies).values({ id: companyId, name: "Harness preflight", issuePrefix: `HP${++fixtureNumber}`, defaultResponsibleUserId: userId }); + await db.insert(companyMemberships).values({ companyId, principalId: userId, principalType: "user", status: "active", membershipRole: "member" }); + [agent] = await db.insert(agents).values({ + companyId, name: "Fixture agent", adapterType: "opencode_local", lifecycleState: "verifying", + adapterConfig: { model: "openrouter/anthropic/claude-sonnet-4.5" }, runtimeConfig: { aiConnection: binding }, + }).returning(); + account = await aiConnectionService(db).save(companyId, userId, { + provider: "openrouter", method: "api_key", ownership: "personal", name: "Fixture account", + apiKey: "fixture-preflight-key", allAgents: true, agentIds: [], + }, "fixture-preflight-key"); + await aiConnectionService(db).setDefault(companyId, userId, account.grantId); + }); + + afterEach(() => { vi.restoreAllMocks(); }); + afterAll(async () => { + await database?.cleanup(); + vi.unstubAllEnvs(); + if (home) await rm(home, { recursive: true, force: true }); + }); + + it("rejects a missing default before allocating a target", async () => { + await db.delete(aiProviderDefaults).where(eq(aiProviderDefaults.companyId, agent.companyId)); + const failure = await verify().catch(error => error); + expect(readAiConnectionConfigurationFailure(failure)).toBe("ai_connection_default_missing"); + expect(allocate).not.toHaveBeenCalled(); + expect(probe).not.toHaveBeenCalled(); + expect(release).not.toHaveBeenCalled(); + }); + + it("rejects a revoked account before allocating a target", async () => { + await db.update(connectionGrants).set({ status: "revoked" }).where(eq(connectionGrants.id, account.grantId)); + await expect(verify()).rejects.toThrow("Reconnect or validate the selected AI account"); + expect(allocate).not.toHaveBeenCalled(); + expect(probe).not.toHaveBeenCalled(); + }); + + it("rejects incompatible model routing before allocating a target", async () => { + await db.update(agents).set({ adapterConfig: { model: "openai/gpt-5" } }).where(eq(agents.id, agent.id)); + await expect(verify()).rejects.toThrow("Select an AI connection compatible with this harness and model"); + expect(allocate).not.toHaveBeenCalled(); + }); + + it("runs a valid managed probe and releases its target", async () => { + await expect(verify()).resolves.toBe("complete"); + expect(allocate).toHaveBeenCalledOnce(); + expect(probe).toHaveBeenCalledOnce(); + expect(probe.mock.calls[0][1].config.env).toMatchObject({ OPENROUTER_API_KEY: "fixture-preflight-key" }); + expect(release).toHaveBeenCalledExactlyOnceWith("released"); + expect(globalThis.fetch).not.toHaveBeenCalled(); + }); + + it("checks account access again after allocation and releases on revocation", async () => { + allocate.mockImplementationOnce(async () => { + await db.update(connectionGrants).set({ status: "revoked" }).where(eq(connectionGrants.id, account.grantId)); + return { executionTarget: null, environmentName: "Fixture execution target", fallbackChecks: [], release }; + }); + await expect(verify()).rejects.toThrow("Reconnect or validate the selected AI account"); + expect(allocate).toHaveBeenCalledOnce(); + expect(probe).not.toHaveBeenCalled(); + expect(release).toHaveBeenCalledExactlyOnceWith("failed"); + }); + + it("releases the target when the managed probe fails", async () => { + probe.mockRejectedValueOnce(new Error("Fixture probe failure")); + await expect(verify()).rejects.toThrow("Fixture probe failure"); + expect(release).toHaveBeenCalledExactlyOnceWith("failed"); + }); +}); diff --git a/server/src/services/agent-harness-verification.ts b/server/src/services/agent-harness-verification.ts index 20f64c64390..c84194a6c78 100644 --- a/server/src/services/agent-harness-verification.ts +++ b/server/src/services/agent-harness-verification.ts @@ -6,6 +6,7 @@ import { isForbiddenConfigEnvKey, parseObject } from "@paperclipai/adapter-utils import { requireServerAdapter } from "../adapters/index.js"; import { agentEnvironmentTestService } from "./agent-environment-test.js"; import { aiConnectionRouterService } from "./ai-connection-router.js"; +import { aiConnectionService } from "./ai-connections.js"; import { withManagedAiProbe, stripAiAuthBindings } from "./ai-connection-runtime.js"; import { environmentService } from "./environments.js"; import { secretService } from "./secrets.js"; @@ -73,6 +74,11 @@ export function agentHarnessVerificationService(db: Db, manager: PluginWorkerMan { ...context, consumerType: "environment", consumerId: environmentId }); config = { ...config, env: { ...resolved.env, ...parseObject(config.env) } }; } + if (binding) await aiConnectionService(db).select({ + companyId: agent.companyId, userId: responsibleUserId, agentId: agent.id, + adapterType: agent.adapterType, model: config.model, runnerProvider: config.provider, + acpxAgent: config.acpxAgent, binding, + }); const target = await tests.resolveAdapterTestExecutionContext({ agentId: agent.id, companyId: agent.companyId, adapterType: agent.adapterType, environmentId }); let status: "released" | "failed" = "failed"; try {