mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-24 10:13:11 +02:00
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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
ec2e0d6407
commit
5a7e740d82
3 files changed
+139
No files matched your search
@@ -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
|
||||
|
||||
@@ -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<ReturnType<typeof startEmbeddedPostgresTestDatabase>>;
|
||||
let db: ReturnType<typeof createDb>;
|
||||
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<void>>();
|
||||
const allocate = vi.fn<ReturnType<typeof realEnvironmentTests>["resolveAdapterTestExecutionContext"]>();
|
||||
const probe = vi.fn<ReturnType<typeof realEnvironmentTests>["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");
|
||||
});
|
||||
});
|
||||
@@ -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 {
|
||||
|
||||
Reference in new issue
Block a user