From 6ce0909db4e8a388b13b48c0295f187120bad35c Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Wed, 28 Oct 2026 08:25:27 -0500 Subject: [PATCH] fix(agents): skip runtime probes for unchanged connections (#18016) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip manages AI agents and their runtime settings. > - Operators save identity, model, connection, and run-policy settings from the agent page. > - PostgreSQL stores connection bindings as JSONB and can return keys in a different order. > - The save route compared JSON strings and treated an unchanged binding as a new connection. > - That false change started a remote runtime and credential probe during ordinary saves. > - This pull request compares binding values and checks real execution selection changes separately. > - Ordinary edits save without a probe. New connections and execution selections retain validation. ## Linked Issues or Issue Description Fixes #14518. Related work: #17760 also addresses prospective environment resolution. #14027 also adds explicit connection removal. This change is limited to the save validation gate and its regressions. **What happened?** A title-only save took 9.8–16.0 seconds in a hosted reproduction. JSONB key order made the unchanged AI connection look different. The route then waited for a remote runtime probe. **Expected behavior** Identity, thinking-effort, and run-policy edits skip the probe when the connection and execution selection stay the same. A changed connection, harness, model, provider, engine, or environment still validates adoption. **Steps to reproduce** 1. Save an agent with a managed AI connection in PostgreSQL. 2. Edit its title or thinking effort without changing the connection or execution selection. 3. Observe that the old route starts the adapter environment test. The fixed route saves without that test. ## What Changed - Use value equality for the stored and validated AI connection bindings. - Compare the effective execution selection separately. Preserve runtime probes for real selection changes and existing Copilot checks. - Add PostgreSQL route regressions for shared, delegated, default-user, and pooled bindings. Cover failed connection, model, engine, environment, and harness changes. - Keep the existing credential rejection and reconnect tests. Document the save behavior. ## Verification - `pnpm -r typecheck` passed on the rebased implementation. - Focused PostgreSQL 17 route selection: 21 passed, 58 excluded by the name filter. This covers ordinary saves, selection-change rejection, and credential health. - Full PostgreSQL 17 connection suite: 79 passed, zero skipped. Final `pnpm build` passed at `8fde75fa1a8fe63c71b05a3db149edf289295672`. - Before the fix, the PostgreSQL regression reached the unintended probe and failed for unchanged shared, delegated, and default-user bindings. The pool control passed. - An early controlled HTTP comparison of the binding fix used a synthetic 500 ms provider probe: five measured saves had a median of 798 ms before and 56 ms after. Probe count fell from five to zero. This is local supporting evidence, not a hosted latency result. - Earlier adjacent-route run: 159 of 160 tests passed. One existing roster-edit case exceeded its 15-second timeout. That case passed alone with the normal timeout. The cause of the earlier timeout remains unproven. - Local full-repository tests were not run. Native PostgreSQL startup fails because the host has exhausted System V semaphores. The focused tests use an isolated PostgreSQL 17 Docker fixture. - `git diff --check` passed. Hosted improvement remains unverified until deployment. - Greptile and Ponytail Paperclip Review both rated the exact head `8fde75fa1a8fe63c71b05a3db149edf289295672` 5/5, with no actionable defects. - CI attempt 1 passed 51 checks. Three self-hosted runners lost communication, interrupting the chat shard, serialized server shard, and canary dry run. GitHub annotations identify the runner communication failures; the serialized shard log also records a shutdown signal. Only those failed jobs were retried, once, on the unchanged commit. The [full CI workflow](https://github.com/paperclipai/paperclip/actions/runs/40692641103/attempts/2) then passed on attempt 2 at the same commit. All 55 passing checks and the legacy Snyk status are green; no checks remain pending. ## Risks This changes when connection adoption validation runs. Separate selection checks preserve validation for real runtime changes. Regression tests verify that rejected selections do not replace the saved configuration. Company access, connection authorization, activity records, and Copilot checks retain their existing paths. There is no schema or migration change. ## Model Used OpenAI Codex, configured model `gpt-6.1-sol` with ultra reasoning. Work used source inspection, shell tools, browser network timing, and PostgreSQL route tests. The context window size is not exposed by this session. ## 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/DEVELOPING.md | 7 ++ .../agent-hire-ai-connections.test.ts | 74 ++++++++++++++++++- server/src/routes/agents.ts | 12 ++- 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index f828a2c85e0..f06bb5c943b 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -2146,6 +2146,13 @@ Lifecycle, timeout, and interrupt grace settings live under **Advanced Run Policy**. Permission selectors with a single valid mode are hidden; a saved unsupported mode still exposes remediation. +Saving an existing agent compares AI connection bindings by value, independent +of PostgreSQL JSONB key order. Identity, thinking-effort and run-policy edits +avoid the connection-adoption runtime probe when the connection and execution +selection stay the same. Connection, harness, model, provider, engine and +environment changes still validate adoption. Explicit runtime tests and +required Copilot metadata checks keep their existing behavior. + Model catalogs and refresh follow the selected provider. ACPX Claude uses the normal Claude catalog and accepts custom model IDs; the exact ID is sent to Claude, which can reject unavailable models. Package/version verification is diff --git a/server/src/__tests__/agent-hire-ai-connections.test.ts b/server/src/__tests__/agent-hire-ai-connections.test.ts index cba65a86d5a..2b397429aa8 100644 --- a/server/src/__tests__/agent-hire-ai-connections.test.ts +++ b/server/src/__tests__/agent-hire-ai-connections.test.ts @@ -9,7 +9,7 @@ import express from "express"; import request from "supertest"; import { and, eq } from "drizzle-orm"; import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; -import { activityLog, agents, authUsers, companies, companyMemberships, createDb, closeRegisteredClients, heartbeatRuns, issues, plugins, principalPermissionGrants, toolConnectionInstalls } from "@paperclipai/db"; +import { activityLog, agents, authUsers, companies, companyMemberships, createDb, closeRegisteredClients, environments, heartbeatRuns, issues, plugins, principalPermissionGrants, toolConnectionInstalls } from "@paperclipai/db"; import { type AiConnectionBinding, type AiConnectionPoolMember, type PaperclipPluginManifestV1 } from "@paperclipai/shared"; import { startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; import { agentRoutes } from "../routes/agents.js"; @@ -209,6 +209,78 @@ describe("agent-created hires use managed AI connections", () => { expect(saved.runtimeConfig.aiConnection).toEqual(f.binding); }); + it.each(["shared", "delegated", "responsible_user", "router"] as const)("saves unrelated edits without probing an unchanged %s connection read from PostgreSQL", async (mode) => { + const f = await fixture("openai"); + await db.insert(principalPermissionGrants).values({ companyId: f.companyId, principalType: "user", principalId: f.userId, permissionKey: "agents:configure" }); + const account = mode === "shared" + ? await aiConnectionService(db).save(f.companyId, f.userId, { + provider: "openai", method: "api_key", ownership: "shared", name: "Shared save fixture", + apiKey: "fixture", agentIds: [f.agentId], allAgents: false, + }, "fixture") + : f.account; + const binding = mode === "router" ? (await poolFixture(f)).binding + : mode === "responsible_user" ? f.binding + : { provider: "openai", method: "api_key", mode, ...account }; + const adapterType = mode === "shared" ? "paperclip_runner" : f.adapterType; + await db.update(agents).set({ adapterType, + ...(adapterType === "paperclip_runner" ? { adapterConfig: { provider: "codex", model: "gpt-5.6-sol" } } : {}), + runtimeConfig: { aiConnection: binding }, + }).where(eq(agents.id, f.agentId)); + const [before] = await db.select().from(agents).where(eq(agents.id, f.agentId)); + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.actor = { type: "board", userId: f.userId, companyIds: [f.companyId], source: "session" }; + next(); + }); + app.use("/api", agentRoutes(db)); + app.use(errorHandler); + const original = getServerAdapter(adapterType); + const probe = vi.fn(async () => { throw new Error("The runtime is unavailable; unrelated edits must still save"); }); + registerServerAdapter({ ...original, testEnvironment: probe }); + try { + for (const patch of [ + { title: "Updated title" }, + { adapterConfig: { reasoningEffort: "high" } }, + { defaultEnvironmentId: null }, + { runtimeConfig: { ...before.runtimeConfig, heartbeat: { enabled: false } } }, + ]) { + const response = await request(app).patch(`/api/agents/${f.agentId}`).send(patch); + expect(response.status, JSON.stringify(response.body)).toBe(200); + expect(response.body.runtimeConfig.aiConnection).toEqual(binding); + } + expect(probe).not.toHaveBeenCalled(); + const [saved] = await db.select().from(agents).where(eq(agents.id, f.agentId)); + expect(saved).toMatchObject({ title: "Updated title", adapterConfig: { reasoningEffort: "high" }, + runtimeConfig: { aiConnection: binding, heartbeat: { enabled: false } } }); + expect(await db.select().from(activityLog).where(and(eq(activityLog.entityId, f.agentId), eq(activityLog.action, "agent.updated")))).toHaveLength(4); + } finally { unregisterServerAdapter(adapterType); } + }); + + it.each(["model", "engine", "environment", "harness", "connection"] as const)("probes a changed %s selection and preserves the saved configuration on failure", async (selection) => { + const f = await fixture("openai"); + await db.insert(principalPermissionGrants).values({ companyId: f.companyId, principalType: "user", principalId: f.userId, permissionKey: "agents:configure" }); + const [environment] = await db.select().from(environments).where(eq(environments.driver, "local")); + const adapterType = selection === "harness" ? "paperclip_runner" : f.adapterType; + const original = getServerAdapter(adapterType); + const probe = vi.fn(async () => ({ adapterType, status: "fail" as const, testedAt: new Date().toISOString(), + checks: [{ code: "fixture_runtime_unavailable", level: "error" as const, message: "The selected runtime is unavailable" }], + })); + registerServerAdapter({ ...original, testEnvironment: probe }); + const patch = selection === "environment" ? { defaultEnvironmentId: environment.id } + : selection === "harness" ? { adapterType, adapterConfig: { provider: "codex", model: "gpt-5.6-sol" } } + : selection === "connection" ? { runtimeConfig: { aiConnection: { ...f.binding, mode: "delegated", ...f.account } } } + : { adapterConfig: { [selection]: selection === "model" ? "changed-model" : "cli" } }; + try { + const response = await request(f.app).patch(`/api/agents/${f.agentId}`).send(patch); + expect(response.status, JSON.stringify(response.body)).toBe(422); + expect(probe).toHaveBeenCalledOnce(); + const [saved] = await db.select().from(agents).where(eq(agents.id, f.agentId)); + expect(saved).toMatchObject({ adapterType: f.adapterType, adapterConfig: {}, defaultEnvironmentId: null, + runtimeConfig: { aiConnection: f.binding } }); + } finally { unregisterServerAdapter(adapterType); } + }); + it("does not install a restricted shared connection through pool membership", async () => { const f = await fixture("openai"); const otherOwner = `other-${f.companyId}`; diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index e05270bc240..d81f90d21cb 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -50,6 +50,7 @@ import { executionProjectionForRun, executionProjectionsForRuns } from "../servi import { selectDashboardRunIds } from "../services/dashboard-run-selection.js"; import { Router, type NextFunction, type Request, type Response } from "express"; import { generateKeyPairSync, randomUUID } from "node:crypto"; +import { isDeepStrictEqual } from "node:util"; import { readFile, rm } from "node:fs/promises"; import path from "node:path"; import type { Db } from "@paperclipai/db"; @@ -5599,7 +5600,7 @@ export function agentRoutes( const nextAiBinding = aiRuntimeConnectionBindingSchema.safeParse(requestedRuntimeConfig ? requestedRuntimeConfig.aiConnection : existing.runtimeConfig.aiConnection).data; if (nextAiBinding) { await assertCanConfigureAgentSetup(req, existing); - const changed = JSON.stringify(nextAiBinding) !== JSON.stringify(existing.runtimeConfig.aiConnection); + const changed = !isDeepStrictEqual(nextAiBinding, existing.runtimeConfig.aiConnection); if (changed && req.actor.type === "agent") { await connectionManagementService(db).authorize(req.actor, "connections:profiles", { type: "connection_management", companyId: existing.companyId, purpose: "ai", targetAgentId: existing.id, @@ -5608,10 +5609,17 @@ export function agentRoutes( }); } const aiConfig = (patchData.adapterConfig ?? existing.adapterConfig) as Record; + const nextEnvironmentId = Object.prototype.hasOwnProperty.call(patchData, "defaultEnvironmentId") + ? patchData.defaultEnvironmentId : existing.defaultEnvironmentId; + const runtimeSelectionChanged = requestedAdapterType !== existing.adapterType + || nextEnvironmentId !== existing.defaultEnvironmentId + || (nextAiBinding.mode !== "router" && ["model", "provider", "acpxAgent", "engine"].some( + key => aiConfig[key] !== existing.adapterConfig[key], + )); if (nextAiBinding.mode !== "router" && !isAiConnectionCompatible(nextAiBinding, requestedAdapterType, aiConfig.model, aiConfig.provider, aiConfig.acpxAgent)) throw unprocessable("Select an AI connection compatible with the new harness and model"); const copilotConfigurationChanged = requestedAdapterType === "paperclip_runner" && aiConfig.provider === "acpx" && aiConfig.acpxAgent === "copilot" && (touchesAdapterConfiguration || Object.prototype.hasOwnProperty.call(patchData, "defaultEnvironmentId")); - if (runtimeSettings || changed || copilotConfigurationChanged || (nextAiBinding.mode === "router" && requestedAdapterType !== existing.adapterType)) { + if (runtimeSettings || changed || runtimeSelectionChanged || copilotConfigurationChanged) { const configuringSetup = isAgentSetupDraft(existing); await validateManagedAgentBinding(req, existing.companyId, existing.id, requestedAdapterType, aiConfig, nextAiBinding, (patchData.defaultEnvironmentId !== undefined ? patchData.defaultEnvironmentId : existing.defaultEnvironmentId) as string | null,