From da072f9a9b53146440a1bd61b8c756dbde082141 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Thu, 8 Oct 2026 06:57:52 -0400 Subject: [PATCH] Q34: built-in skill updates reach existing orgs and their bindings Co-Authored-By: Claude Opus 5.5 --- src/main/index.ts | 4 +-- src/main/services/agent-skill-tools.ts | 6 ++-- src/main/services/builtin-skills.ts | 23 +++++++----- src/main/services/skill-service.ts | 35 +++++++++++++++++- tests/unit/builtin-skills.test.ts | 50 +++++++++++++++++++++++++- 5 files changed, 102 insertions(+), 16 deletions(-) diff --git a/src/main/index.ts b/src/main/index.ts index ba3e603f..52e5772e 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -102,8 +102,8 @@ function syncAllOrganizationSkills(connection: DatabaseConnection, artifactRoot: const orgs = connection.db.prepare("select id from organizations").all() as Array<{ id: string }>; for (const org of orgs) { const report = connection.transaction(() => importStagedSkills(skills, org.id, staged)); - if (report.imported > 0 || report.retired > 0 || report.failures.length > 0) { - console.info(`[skills] org ${org.id}: imported ${report.imported}, skipped ${report.skipped}, retired ${report.retired}, failures ${report.failures.length}`); + if (report.imported > 0 || report.upgraded > 0 || report.retired > 0 || report.failures.length > 0) { + console.info(`[skills] org ${org.id}: imported ${report.imported}, upgraded ${report.upgraded}, skipped ${report.skipped}, retired ${report.retired}, failures ${report.failures.length}`); for (const failure of report.failures.slice(0, 5)) console.warn(`[skills] ${failure}`); } } diff --git a/src/main/services/agent-skill-tools.ts b/src/main/services/agent-skill-tools.ts index e982bca5..1cf55c4b 100644 --- a/src/main/services/agent-skill-tools.ts +++ b/src/main/services/agent-skill-tools.ts @@ -10,7 +10,7 @@ export function listSkillsTool(deps: AgentToolDeps, ctx: AgentToolContext): stri select sd.id, sd.name, sd.slug, sr.manifest_json from skill_definitions sd left join skill_revisions sr on sr.id = ( - select id from skill_revisions where skill_id = sd.id order by published_at desc, id desc limit 1 + select id from skill_revisions where skill_id = sd.id order by published_at desc, rowid desc limit 1 ) where sd.org_id = ? and sd.status = 'active' order by sd.slug asc @@ -69,7 +69,7 @@ export function grantSkillTool(deps: AgentToolDeps, ctx: AgentToolContext, args: const skillService = new SkillService(deps.db, ""); if (mode === "grant") { const revision = deps.db.prepare( - "select id from skill_revisions where skill_id = ? order by published_at desc, id desc limit 1", + "select id from skill_revisions where skill_id = ? order by published_at desc, rowid desc limit 1", ).get(skill.id) as { id: string } | undefined; if (!revision) return `ERROR: The "${skillSlug}" skill has no published revision.`; skillService.bind({ orgId: ctx.orgId, agentId: target.id, skillId: skill.id, revisionId: revision.id }); @@ -94,7 +94,7 @@ export function bindSkillSlugs(deps: AgentToolDeps, orgId: string, agentId: stri select sd.id as skill_id, sd.name, sr.id as revision_id from skill_definitions sd join skill_revisions sr on sr.id = ( - select id from skill_revisions where skill_id = sd.id order by published_at desc, id desc limit 1 + select id from skill_revisions where skill_id = sd.id order by published_at desc, rowid desc limit 1 ) where sd.org_id = ? and sd.slug = ? and sd.status = 'active' `).get(orgId, slug) as { skill_id: string; name: string; revision_id: string } | undefined; diff --git a/src/main/services/builtin-skills.ts b/src/main/services/builtin-skills.ts index 0d258cd7..abba893d 100644 --- a/src/main/services/builtin-skills.ts +++ b/src/main/services/builtin-skills.ts @@ -1,4 +1,4 @@ -import { cpSync, existsSync, mkdirSync, readdirSync, readFileSync, writeFileSync } from "node:fs"; +import { cpSync, existsSync, mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { RISK_LEVEL } from "../../shared/domain"; import { loadSkillManifest, type SkillManifest } from "./skill-manifest"; @@ -17,7 +17,7 @@ import type { SkillService } from "./skill-service"; * Staging normalizes both into one library layout the import machinery * understands — `//SKILL.md` — a real copy on disk, never a * reference back into the bundle. Import then publishes every staged skill the - * org does not carry yet into the immutable artifact store. + * org does not carry yet, or whose files changed, into the immutable artifact store. */ export interface StagedBuiltinSkill { @@ -28,6 +28,8 @@ export interface StagedBuiltinSkill { export interface BuiltinSkillSyncReport { imported: number; + /** Existing skills that got a new revision because the bundle changed them. */ + upgraded: number; skipped: number; /** Active skills retired because the bundle no longer carries their slug. */ retired: number; @@ -73,17 +75,18 @@ export function stageBuiltinSkillsLibrary(sourceRoot: string, libraryRoot: strin /** * Makes the org catalogue mirror the bundle: publishes every staged skill - * the org does not carry yet, then retires active skills whose slug the - * bundle no longer ships (legacy ~/.agents imports, removed skills). - * Existing bindings pin revisions directly and keep working; retired skills - * simply leave list_skills and stop being grantable. + * the org does not carry yet, upgrades skills whose files changed (moving + * bindings onto the new revision), then retires active skills whose slug + * the bundle no longer ships. Retired skills leave list_skills and stop + * being grantable. */ export function importStagedSkills(skills: SkillService, orgId: string, staged: StagedBuiltinSkill[]): BuiltinSkillSyncReport { - const report: BuiltinSkillSyncReport = { imported: 0, skipped: 0, retired: 0, failures: [] }; + const report: BuiltinSkillSyncReport = { imported: 0, upgraded: 0, skipped: 0, retired: 0, failures: [] }; for (const unit of staged) { try { if (skills.hasSkill(orgId, unit.slug)) { - report.skipped += 1; + if (skills.upgradeIfChanged(orgId, unit.directory, unit.manifest, unit.slug)) report.upgraded += 1; + else report.skipped += 1; continue; } skills.importManifest(orgId, unit.directory, unit.manifest, unit.slug); @@ -99,7 +102,8 @@ export function importStagedSkills(skills: SkillService, orgId: string, staged: function stageDirectorySkill(sourceDir: string, dirName: string, libraryRoot: string, seen: Set): StagedBuiltinSkill { const slug = uniqueSlug(slugify(dirName), seen, sourceDir); const target = join(libraryRoot, slug); - mkdirSync(target, { recursive: true }); + // Clear first so files the bundle dropped do not linger in the library. + rmSync(target, { recursive: true, force: true }); cpSync(sourceDir, target, { recursive: true }); const manifest = existsSync(join(target, "skill.manifest.json")) ? loadSkillManifest(target) @@ -119,6 +123,7 @@ function stageCategorySkills(categoryDir: string, libraryRoot: string, seen: Set const stem = entry.name.replace(/\.md$/i, ""); const slug = uniqueSlug(slugify(stem), seen, join(categoryDir, entry.name)); const target = join(libraryRoot, slug); + rmSync(target, { recursive: true, force: true }); mkdirSync(target, { recursive: true }); writeFileSync(join(target, "SKILL.md"), body, "utf8"); staged.push({ diff --git a/src/main/services/skill-service.ts b/src/main/services/skill-service.ts index bd22d900..0a93a66a 100644 --- a/src/main/services/skill-service.ts +++ b/src/main/services/skill-service.ts @@ -83,6 +83,39 @@ export class SkillService { return retired; } + /** Publishes a new revision when the files changed and moves bindings on the latest revision onto it. */ + upgradeIfChanged(orgId: string, directoryPath: string, manifest: SkillManifest, slug: string): boolean { + const latest = this.db.prepare(` + select sr.id, sr.immutable_checksum from skill_definitions sd + join skill_revisions sr on sr.skill_id = sd.id + where sd.org_id = ? and sd.slug = ? + order by sr.published_at desc, sr.rowid desc limit 1 + `).get(orgId, slug) as { id: string; immutable_checksum: string } | undefined; + if (!latest) throw new Error(`Skill "${slug}" has no revision to upgrade.`); + const checksum = recursiveSkillChecksum(directoryPath); + if (latest.immutable_checksum === checksum) return false; + const revisions = this.db.prepare(` + select sr.version_semver from skill_revisions sr join skill_definitions sd on sd.id = sr.skill_id + where sd.org_id = ? and sd.slug = ? + `).all(orgId, slug) as Array<{ version_semver: string }>; + // Same declared version, new content: tag the build so revisions stay distinct. + const versionTaken = revisions.some((row) => row.version_semver === manifest.version); + const published = versionTaken ? { ...manifest, version: `${manifest.version}+rev${revisions.length + 1}` } : manifest; + const revisionId = this.importManifest(orgId, directoryPath, published, slug); + const moved = this.db.prepare("update skill_bindings set pinned_revision_id = ? where org_id = ? and pinned_revision_id = ?") + .run(revisionId, orgId, latest.id); + this.audit.record({ + orgId, + entityType: "skill_revision", + entityId: revisionId, + actorType: "system", + actorId: null, + action: "upgraded", + payload: { slug, fromRevisionId: latest.id, version: published.version, bindingsMoved: Number(moved.changes) }, + }); + return true; + } + importManifest(orgId: string, directoryPath: string, manifest: SkillManifest, explicitSlug?: string): string { const slug = explicitSlug ?? slugify(manifest.name); if (!slug) throw new Error("Skill name could not be derived."); @@ -216,7 +249,7 @@ export class SkillService { sa.object_key from skill_definitions sd join skill_revisions sr on sr.id = ( - select id from skill_revisions where skill_id = sd.id order by published_at desc, id desc limit 1 + select id from skill_revisions where skill_id = sd.id order by published_at desc, rowid desc limit 1 ) join skill_artifacts sa on sa.revision_id = sr.id and sa.artifact_type in ('entrypoint', 'markdown') where sd.org_id = ? and sd.status = 'active' and sd.slug in (${candidates.map(() => "?").join(", ")}) diff --git a/tests/unit/builtin-skills.test.ts b/tests/unit/builtin-skills.test.ts index a5599e97..dfad3a09 100644 --- a/tests/unit/builtin-skills.test.ts +++ b/tests/unit/builtin-skills.test.ts @@ -1,5 +1,5 @@ import { randomUUID } from "node:crypto"; -import { existsSync, mkdirSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { describe, expect, test } from "vitest"; @@ -106,6 +106,54 @@ describe("built-in skills", () => { h.connection.close(); }); + test("an app update that changes a built-in skill reaches existing orgs and their bindings", () => { + const h = harness(); + h.connection.transaction(() => importStagedSkills(h.skills, h.orgId, stageBuiltinSkillsLibrary(h.sourceRoot, h.libraryRoot))); + const skillId = (h.connection.db.prepare("select id from skill_definitions where org_id = ? and slug = 'engineering-backend-architect'").get(h.orgId) as { id: string }).id; + const oldRevision = (h.connection.db.prepare("select id from skill_revisions where skill_id = ?").get(skillId) as { id: string }).id; + let agentId = ""; + h.connection.transaction(() => { + agentId = h.organization.createAgent({ + orgId: h.orgId, name: "Ada", role: "Engineer", department: "Engineering", + modelProviderId: null, modelId: null, budgetCents: 1000, managerAgentId: null, + }); + h.skills.bind({ orgId: h.orgId, agentId, skillId, revisionId: oldRevision }); + }); + + writeFileSync(join(h.sourceRoot, "engineering", "engineering-backend-architect.md"), ARCHITECT_MD.replace("hold everything up", "scale to millions"), "utf8"); + rmSync(join(h.sourceRoot, "humanizer", "README.md")); + const report = h.connection.transaction(() => importStagedSkills(h.skills, h.orgId, stageBuiltinSkillsLibrary(h.sourceRoot, h.libraryRoot))); + + expect(report).toMatchObject({ imported: 0, upgraded: 2, skipped: 1, retired: 0, failures: [] }); + const bound = h.connection.db.prepare(` + select sa.object_key from skill_bindings sb join skill_artifacts sa on sa.revision_id = sb.pinned_revision_id and sa.artifact_type = 'entrypoint' + where sb.agent_id = ? + `).get(agentId) as { object_key: string }; + expect(readFileSync(bound.object_key, "utf8")).toContain("scale to millions"); + const humanizerFiles = h.connection.db.prepare(` + select sa.object_key from skill_definitions sd + join skill_revisions sr on sr.id = (select id from skill_revisions where skill_id = sd.id order by published_at desc, id desc limit 1) + join skill_artifacts sa on sa.revision_id = sr.id + where sd.org_id = ? and sd.slug = 'humanizer' + `).all(h.orgId) as Array<{ object_key: string }>; + const architectPath = join(h.sourceRoot, "engineering", "engineering-backend-architect.md"); + for (const body of [ARCHITECT_MD.replace("hold everything up", "stay up"), ARCHITECT_MD.replace("hold everything up", "scale to millions")]) { + writeFileSync(architectPath, body, "utf8"); + const again = h.connection.transaction(() => importStagedSkills(h.skills, h.orgId, stageBuiltinSkillsLibrary(h.sourceRoot, h.libraryRoot))); + expect(again).toMatchObject({ upgraded: 1, failures: [] }); + } + const versions = (h.connection.db.prepare("select version_semver from skill_revisions where skill_id = ? order by published_at, rowid").all(skillId) as Array<{ version_semver: string }>) + .map((row) => row.version_semver); + expect(versions).toEqual(["1.0.0", "1.0.0+rev2", "1.0.0+rev3", "1.0.0+rev4"]); + const rebound = h.connection.db.prepare(` + select sa.object_key from skill_bindings sb join skill_artifacts sa on sa.revision_id = sb.pinned_revision_id and sa.artifact_type = 'entrypoint' + where sb.agent_id = ? + `).get(agentId) as { object_key: string }; + expect(readFileSync(rebound.object_key, "utf8")).toContain("scale to millions"); + expect(humanizerFiles.map((row) => row.object_key.replaceAll("\\", "/").split("/").at(-1))).toEqual(["SKILL.md"]); + h.connection.close(); + }); + test("legacy catalogue entries retire so every install mirrors the bundle, while their bindings keep working", () => { const h = harness(); const staged = stageBuiltinSkillsLibrary(h.sourceRoot, h.libraryRoot);