From 4970d5ed3d184b8b3ddfb2c1185be6cb6dc8c2ea Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Mon, 19 Oct 2026 16:33:02 -0500 Subject: [PATCH] fix(skills): unblock agent attachment and preserve Latest selections (#16899) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Operators attach company skills to agents from the skill library. > - Attachment first reads the agent's skill snapshot to preserve other assignments. > - That read materialized and validated repository files for the entire library, even in read-only mode. > - The response also treated resolved runtime versions as stored pins. Saving could then fail with beta pins disabled. > - This pull request makes version-cache inspection lightweight and returns the stored assignment choices. > - The benefit is a responsive attachment flow that preserves Latest and explicit pins. ## Linked Issues or Issue Description **What happened?** Adding a repository skill to an agent could stall before the save request. The prerequisite snapshot read projected or checked every file in every installed repository skill. Once it returned, an existing Latest assignment could become a version pin in the save payload. The server then rejected that save when beta skill pins were disabled. **Expected behavior** Reading assignments must not prepare all repository snapshots for execution. Adding a skill must preserve the other assignments and their Latest or pinned choices. **Steps to reproduce** 1. Install repository-backed skills with stored immutable versions and cold disk caches. 2. Assign one repository skill to a Claude agent with Latest selected. Leave beta skill pins disabled. 3. Open another skill, select Add to agent, choose the same agent, and save. 4. Observe the expensive prerequisite read and the beta-pin rejection after that read completes. **Paperclip version or commit** Reproduced from master at `ac9459bae3329395f10bc81c638a374eef152baa`. **Deployment mode** Observed in hosted staging. Reproduced with a disposable local instance, embedded PostgreSQL, and the Claude local adapter on macOS arm64 with Node 26.4.0. Related: https://github.com/paperclipai/paperclip/pull/9995 adds a separate read-only assignment endpoint for peer-agent access. This fix keeps the existing configuration-gated snapshot and its permissions. ## What Changed - Respect read-only listing for current and pinned version caches. Check directory and entrypoint safety without projecting repository contents, reading all support files, or updating cache access records. - Keep complete snapshot restoration and byte validation at run preparation. Missing caches are restored from stored data. Corrupted files are repaired there. - Return stored assignment version choices in both list and sync responses. Keep runtime version details in the snapshot entries. - Add database-backed regressions for cold and warm reads, pinned versions, execution repair, and unsafe entrypoints. Add HTTP regressions for save round trips and explicit pins. - Update developer documentation and verification recipes. ## Verification - Red: both latest and pinned read-only cache regressions failed on the original code. The original code created caches during listing. - Red: the HTTP save-roundtrip regression returned 400 with the beta-pin error. The explicit-pin regression also failed. - Green: the updated API and cache suites passed all 145 tests. The three related company-skill service suites also passed. - Real browser: attached a repository skill to a paused test agent with another repository skill already set to Latest and beta pins disabled. The success toast appeared. The skill's Agents tab retained the assignment after reload. API reads confirmed both assignments remained unpinned and no repository version caches were created. - Local synthetic fixture: six skills share a 606-file repository snapshot. A cold HTTP skill-list request completed in 241 ms. This is local evidence, not a staging latency measurement. - Real service comparison on that fixture: the baseline cold list wrote 3,648 version/cache files. A warm baseline list read 3,642 version files and wrote six access records (457 ms). The fixed warm list read and wrote zero version/cache files (61 ms). Both services returned the same 16 library entries. - `pnpm -r typecheck` passed. The final API edit also passed the server typecheck. - `pnpm build` passed. - `node scripts/check-maps.mjs` and `git diff --check` passed. - The broad `pnpm test:run` invocation reported six failures in the untouched `tool-gateway-service.test.ts` suite. An isolated rerun passed all 59 tests. The broad run was stopped after recording those failures, so it is not a green full-local result. The PR CI matrix separately passed those suites. Greptile completed on the current commit with 5/5, zero comments, and no open threads. - One CI shard was interrupted by runner shutdown, with no failed assertion in its log. A single failed-job retry passed on the same commit. The aggregate CI gate passed. All current-head PR checks are complete with no failing or pending checks. - This PR does not claim that the fix has been deployed to staging. ## Risks - Settings reads check cache locations and entrypoints. They defer full support-file validation to execution. Regression tests verify that execution still repairs corrupt bytes and restores missing versions. - The response now distinguishes stored pins from resolved runtime versions. Explicit historical pins remain intact. Company, configuration, connector, and beta-pin authorization gates keep their existing behavior. - No schema migration or dependency change. ## Model Used - OpenAI GPT-6 through Codex, with reasoning, code execution, and browser tools. The exact runtime build ID and context-window size are not exposed in 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 | 10 ++++ maps/agent-access-map/skills.md | 10 ++++ maps/feature-map/skills.md | 10 ++++ .../src/__tests__/agent-skills-routes.test.ts | 42 ++++++++++++++ .../src/__tests__/skill-version-cache.test.ts | 56 +++++++++++++++++++ server/src/routes/agents.ts | 15 ++++- server/src/services/company-skills.ts | 14 +++++ 7 files changed, 155 insertions(+), 2 deletions(-) diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index 43b6b8e4875..52765631ffb 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -2219,6 +2219,16 @@ writes this directory from the stored files of the `company_skill_versions` row. The directory is a cache. When the server removes it and the row stays, the next run that selects the version writes it again. +Agent skill settings read version-cache locations without projecting repository +snapshots, checking every cached file, or updating cache access records. An absent +cache is reported as awaiting materialization on the next run. Run preparation +still restores and validates the complete selected snapshot, including supporting +files, before delivery. `snapshotLatest` retains this execution behavior even +when upstream materialization is disabled. +Skill-list and skill-sync responses report stored assignment pins in +`desiredSkillEntries`. A resolved runtime `versionId` does not turn a selection +that follows Latest into a pin when the UI saves another assignment. + The server starts scanning one minute after startup, then hourly, in batches of 50 inspected directories with one-second continuations. Current versions, explicit agent pins, active runs, and unresolved recovery pins stay. A company diff --git a/maps/agent-access-map/skills.md b/maps/agent-access-map/skills.md index 4aa6298bf3b..9c7dff92df7 100644 --- a/maps/agent-access-map/skills.md +++ b/maps/agent-access-map/skills.md @@ -139,6 +139,16 @@ remaining operations before qualifying the whole capability. Gaps: Policy administration has an explicit agent grant branch; assigning agent skills also checks the target's config authority. +The configuration-gated `GET /api/agents/{id}/skills` reads repository cache +locations without materializing versions or validating their support files. +Assignment mutations and run preparation retain their existing authorization. +The [database-backed cache tests](../../server/src/__tests__/skill-version-cache.test.ts) +cover latest/pinned reads and full validation during execution; they do not prove +live agent access. +Both skill-list and skill-sync responses preserve stored version pins rather than +returning a resolved runtime version as a pin. Latest remains unpinned across an +assignment round trip. + ## `testing` Review: `baseline`. diff --git a/maps/feature-map/skills.md b/maps/feature-map/skills.md index 1b5a206d2bc..532f101202e 100644 --- a/maps/feature-map/skills.md +++ b/maps/feature-map/skills.md @@ -55,6 +55,16 @@ Manual: Create or fork a disposable skill, edit instructions, save, and reload. Automated: [policy service](../../server/src/__tests__/company-skill-policy-service.test.ts) and [agent skills state](../../ui/src/pages/agent-skills/AgentSkillsTab.test.ts) cover policy/state contracts. +The [version-cache regression](../../server/src/__tests__/skill-version-cache.test.ts) +checks that listing assignments does not create repository caches, validate every +support file, or extend cache use. Run preparation still restores and repairs the +selected version. To check attachment, use a repository skill with a cold cache, +add it to an agent, then reload the skill's Agents tab and the agent's Skills tab. +Keep another repository skill set to Latest during this check. With beta version +pins disabled, adding the new skill must preserve that Latest selection and save +without a beta-pin error. The [HTTP regression](../../server/src/__tests__/agent-skills-routes.test.ts) +checks this round trip and retention of explicit historical pins. + Manual: Enable the skill for one test agent and leave another without access. Run a small task on each, inspect the available skill context, then change the release/policy and verify the next eligible run uses the intended version. A Paperclip Runner run with no selected skills must proceed while another run diff --git a/server/src/__tests__/agent-skills-routes.test.ts b/server/src/__tests__/agent-skills-routes.test.ts index 98551e35f7e..dce83a29d4c 100644 --- a/server/src/__tests__/agent-skills-routes.test.ts +++ b/server/src/__tests__/agent-skills-routes.test.ts @@ -417,6 +417,48 @@ describe("agent skill routes", () => { ); }, 10_000); + it("round-trips latest assignments without inventing beta version pins", async () => { + const existing = "github/fixture/existing"; + const added = "github/fixture/added"; + const resolvedVersionId = "22222222-2222-4222-8222-222222222222"; + mockAgentService.getById.mockResolvedValue({ + ...makeAgent("claude_local"), + adapterConfig: { paperclipSkillSync: { desiredSkills: [existing] } }, + }); + // Adapters describe the resolved runtime version. It is not an assignment pin. + mockAdapter.listSkills.mockResolvedValue({ + ...skillSnapshot(), desiredSkills: [existing], + desiredSkillEntries: [{ key: existing, versionId: resolvedVersionId }], + }); + mockAdapter.syncSkills.mockResolvedValue({ + ...skillSnapshot(), desiredSkills: [existing, added], + desiredSkillEntries: [{ key: existing, versionId: resolvedVersionId }, { key: added, versionId: resolvedVersionId }], + }); + const app = await createApp(); + const snapshot = await requestApp(app, baseUrl => request(baseUrl) + .get("/api/agents/11111111-1111-4111-8111-111111111111/skills?companyId=company-1")); + expect(snapshot.status).toBe(200); + const saved = await requestApp(app, baseUrl => request(baseUrl) + .post("/api/agents/11111111-1111-4111-8111-111111111111/skills/sync?companyId=company-1") + .send({ desiredSkills: [...snapshot.body.desiredSkillEntries, { key: added, versionId: null }], mode: "replace" })); + expect(saved.status, JSON.stringify(saved.body)).toBe(200); + expect(saved.body.desiredSkillEntries).toEqual([{ key: existing, versionId: null }, { key: added, versionId: null }]); + expect(mockAgentService.update.mock.calls.at(-1)?.[1].adapterConfig.paperclipSkillSync.desiredSkills).toEqual([existing, added]); + }); + + it("keeps explicit historical assignment pins in skill snapshots", async () => { + const key = "github/fixture/pinned"; + const pinnedVersionId = "22222222-2222-4222-8222-222222222222"; + mockInstanceSettingsService.getExperimental.mockResolvedValue({ enableBetaSkills: true }); + mockAgentService.getById.mockResolvedValue({ ...makeAgent("claude_local"), + adapterConfig: { paperclipSkillSync: { desiredSkills: [{ key, versionId: pinnedVersionId }] } } }); + mockAdapter.listSkills.mockResolvedValue({ ...skillSnapshot(), desiredSkills: [key], desiredSkillEntries: [{ key, versionId: null }] }); + const snapshot = await requestApp(await createApp(), baseUrl => request(baseUrl) + .get("/api/agents/11111111-1111-4111-8111-111111111111/skills?companyId=company-1")); + expect(snapshot.status).toBe(200); + expect(snapshot.body.desiredSkillEntries).toEqual([{ key, versionId: pinnedVersionId }]); + }); + it("lists skills without resolving required user-secret env bindings", async () => { const adapterConfig = { env: { diff --git a/server/src/__tests__/skill-version-cache.test.ts b/server/src/__tests__/skill-version-cache.test.ts index 725006cc4a5..4eca01f0a7b 100644 --- a/server/src/__tests__/skill-version-cache.test.ts +++ b/server/src/__tests__/skill-version-cache.test.ts @@ -89,6 +89,62 @@ describePostgres("immutable skill version cache", () => { expect(upstream).not.toHaveBeenCalled(); }); + it.each(["latest", "pinned"] as const)("does not materialize %s repository versions while listing agent skills", async selection => { + const f = await fixture(); + const version = f.versions[selection === "pinned" ? 0 : 1]!; + const options = { + onlyKeys: new Set([f.key]), + materializeMissing: false, + ...(selection === "pinned" ? { versionSelections: new Map([[f.key, version.id]]) } : {}), + }; + const paths = skillVersionCachePaths(f.companyId, f.skillId, version.id); + const service = companySkillService(db); + const upstream = vi.fn(() => Promise.reject(new Error("Upstream is unavailable"))); + vi.stubGlobal("fetch", upstream); + + const cold = await service.listRuntimeSkillEntries(f.companyId, options); + expect(cold).toMatchObject([{ key: f.key, versionId: version.id, sourceStatus: "missing" }]); + await expect(fs.stat(paths.directory)).rejects.toMatchObject({ code: "ENOENT" }); + await expect(fs.stat(paths.access)).rejects.toMatchObject({ code: "ENOENT" }); + + const prepare = async () => { + const entries = await service.listRuntimeSkillEntries(f.companyId, { ...options, snapshotLatest: true }); + expect(entries).toMatchObject([{ key: f.key, versionId: version.id, sourceStatus: "available" }]); + return entries[0]!; + }; + const prepared = await prepare(); + const accessed = await fs.readFile(paths.access, "utf8"); + const read = vi.spyOn(fs, "readFile"); + const warm = await service.listRuntimeSkillEntries(f.companyId, options); + expect(warm).toMatchObject([{ key: f.key, versionId: version.id, sourceStatus: "available", source: prepared.source }]); + // Browsing must not validate every repository file or extend cache use. + expect(read.mock.calls.some(([filename]) => String(filename).startsWith(paths.directory + path.sep))).toBe(false); + read.mockRestore(); + expect(await fs.readFile(paths.access, "utf8")).toBe(accessed); + + // Full validation remains at the execution boundary, including repair. + const marker = path.join(prepared.source, ".paperclip-repository/marker.bin"); + await fs.chmod(marker, 0o644); + await fs.writeFile(marker, "corrupt"); + await prepare(); + expect(await fs.readFile(marker)).toEqual(Buffer.from([0, selection === "pinned" ? 1 : 2, 255])); + expect(upstream).not.toHaveBeenCalled(); + }); + + it("rejects a symlinked cache entrypoint during read-only listing", async () => { + const f = await fixture(); + const version = f.versions[1]!; + const prepared = await f.resolve(version.id); + const entrypoint = path.join(prepared.source, "SKILL.md"); + await fs.unlink(entrypoint); + await fs.symlink(path.join(prepared.source, ".paperclip-repository/scripts/example.sh"), entrypoint); + const entries = await companySkillService(db).listRuntimeSkillEntries(f.companyId, { + onlyKeys: new Set([f.key]), materializeMissing: false, + }); + expect(entries).toMatchObject([{ key: f.key, sourceStatus: "missing", missingDetail: expect.stringContaining("Unsafe skill cache entrypoint") }]); + expect((await fs.lstat(entrypoint)).isSymbolicLink()).toBe(true); + }); + it("expires only idle old versions, reports dry-run candidates, and restores the evicted version", async () => { const f = await fixture(); const old = await f.resolve(f.versions[0]!.id); diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index fc796b3b7b4..2edfea26c0e 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -2677,6 +2677,17 @@ export function agentRoutes( }; } + function withAssignedSkillVersions(snapshot: AgentSkillSnapshot, adapterConfig: Record): AgentSkillSnapshot { + const assignments = new Map(readPaperclipSkillSyncPreference(adapterConfig).desiredSkillEntries + .map(entry => [entry.key, entry.versionId])); + return { + ...snapshot, + // A resolved runtime version is not a stored assignment pin. Returning + // it here makes a read/save round trip silently pin Latest selections. + desiredSkillEntries: snapshot.desiredSkills.map(key => ({ key, versionId: assignments.get(key) ?? null })), + }; + } + // CEO and board-created onboarding chief-of-staff instructions assume the // core paperclip skills (board coordination, planning, hiring, memory). // Union them into these skills-capable hires/creates so their desired skills @@ -3667,7 +3678,7 @@ export function agentRoutes( adapterType: agent.adapterType, config: connectorConfig, }); - res.json(annotateConnectorSkills(snapshot, connectorAssignments)); + res.json(annotateConnectorSkills(withAssignedSkillVersions(snapshot, agent.adapterConfig as Record), connectorAssignments)); }); router.post( @@ -3767,7 +3778,7 @@ export function agentRoutes( }, }); - res.json(annotateConnectorSkills(snapshot, connectorAssignments)); + res.json(annotateConnectorSkills(withAssignedSkillVersions(snapshot, nextAdapterConfig), connectorAssignments)); }, ); diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts index 37d3b14d785..ac114ee692f 100644 --- a/server/src/services/company-skills.ts +++ b/server/src/services/company-skills.ts @@ -6089,6 +6089,20 @@ export function companySkillService(db: Db, options: { actor?: AuthorizationActo // with the real cause — a silent drop makes the skill vanish from the // runtime while the library still shows it installed. try { + if (options.materializeMissing === false && !options.snapshotLatest) { + // Agent settings enumerate the library, including unselected skills. + // Execution, rather than browsing, validates the repository bytes. + try { + await ensureSkillCacheDirectory(versionPath); + const entrypoint = await fs.lstat(path.join(versionPath, "SKILL.md")); + if (!entrypoint.isFile() || entrypoint.isSymbolicLink()) throw unprocessable("Unsafe skill cache entrypoint."); + return { status: "available", source: versionPath }; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; + return { status: "missing", source: versionPath, + detail: "The selected skill version will be materialized on the next run." }; + } + } const versionSource = await materializeVersionSnapshot(companyId, skill, version); if (versionSource) return { status: "available", source: versionSource }; return { status: "missing", source: versionPath, detail: "The selected skill version produced no files." };