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." };