diff --git a/IMPLEMENTATION.md b/IMPLEMENTATION.md index 5de1ca9773..afa5ec390e 100644 --- a/IMPLEMENTATION.md +++ b/IMPLEMENTATION.md @@ -19,8 +19,7 @@ and [PRODUCT.md](PRODUCT.md)'s surface. database, reached through `DATABASE_URL`. - **[Drizzle](https://orm.drizzle.team)** — the ORM/query layer, both in vendored `@intx/db` and in workbench-owned packages that need their own - tables (e.g. `packages/chat`'s `chat` schema, `packages/skills`'s - `skills.skill_access`). + tables (e.g. `packages/chat`'s `chat` schema). - Package-owned migrations run through each package's own migration runner; see [docs/package-migrations.md](docs/package-migrations.md). diff --git a/apps/hub/src/skills-mount.ts b/apps/hub/src/skills-mount.ts index ecb57d57ab..9095427d95 100644 --- a/apps/hub/src/skills-mount.ts +++ b/apps/hub/src/skills-mount.ts @@ -16,7 +16,6 @@ import { } from "@corbits/agent-directory"; import { reportError } from "@corbits/error-sink"; import { - createDrizzleSkillAccessStore, createHubSkillAssetStore, createSkillRegistry, SkillRegistryError, @@ -72,7 +71,6 @@ export function mountSkills(deps: { assetService: deps.assetService, repoStore: deps.repoStore, }), - access: createDrizzleSkillAccessStore(deps.db), }); const pinnedBy: PinnedByResolver = { diff --git a/packages/skills/README.md b/packages/skills/README.md index e88ebc3726..c92aea63ce 100644 --- a/packages/skills/README.md +++ b/packages/skills/README.md @@ -7,22 +7,19 @@ and scope access to per-principal. ## Composition with @intx/* -A skill's storage, version history, and body are entirely the platform's -own asset/git machinery — this package writes no versions table and no -content cache, only reading and committing through the asset store. Routes -are built on `@intx/hub-api`'s `TenantEnv`/`requireGrant` convention, with -`@intx/hub-sessions` for session-authenticated calls; persistence for the -one table this package does own goes through `@intx/db`. +A skill's storage, version history, body, and visibility are entirely +the platform's own asset/git machinery — this package owns no tables +and ships no migrations, only reading and committing through the asset +store. Routes are built on `@intx/hub-api`'s `TenantEnv`/`requireGrant` +convention, with `@intx/hub-sessions` for session-authenticated calls. ## Key modules - `src/registry.ts` — `SkillRegistry`: create/list/get/versions/restore over the `kind:"skill"` asset, with no intermediate pending/draft state. -- `src/access.ts` / `src/access-store.ts` — the `skill_access` table and - visibility rules (`private` to the author, `tenant`-wide, or scoped), - the one thing this package persists beyond the asset itself. -- `src/skill-md.ts` — `SKILL.md` frontmatter parsing/building and the - `name`/`description` validation schemas. +- `src/skill-md.ts` — the SKILL.md grammar: builds, parses, and validates + the `name`/`description`/`scope` frontmatter and the markdown body. This + is the only place visibility is decided — there is no side table. - `src/hub-asset-store.ts` — the production `SkillAssetStore` binding against the platform's native asset kind handler. - `src/routes.ts` — `createSkillRoutes`: tenant-session routes under @@ -33,8 +30,6 @@ one table this package does own goes through `@intx/db`. - `src/prompt.ts` — `withAvailableSkills`: appends/replaces the `` system-prompt stanza for a definition's pinned skills. -- `src/migrations.ts` — this package's own `skills_migrations` ledger, - covering only the `skill_access` table. ## Running tests @@ -42,6 +37,5 @@ one table this package does own goes through `@intx/db`. cd packages/skills && bun test ``` -No drizzle suite in this package's `test/` directory; no `DATABASE_URL` -needed for `bun test` here (`src/migrations.ts` applies against a real -Postgres only when a host runs it). +No drizzle suite in this package's `test/` directory and no migrations +to apply; no `DATABASE_URL` needed for `bun test` here. diff --git a/packages/skills/package.json b/packages/skills/package.json index 6d8acf44ec..4b549a3e00 100644 --- a/packages/skills/package.json +++ b/packages/skills/package.json @@ -6,8 +6,7 @@ "license": "LGPL-2.1-or-later", "type": "module", "exports": { - ".": "./src/index.ts", - "./migrations": "./src/migrations.ts" + ".": "./src/index.ts" }, "scripts": { "typecheck": "tsc --noEmit", diff --git a/packages/skills/src/access-store.ts b/packages/skills/src/access-store.ts deleted file mode 100644 index 5d193fd677..0000000000 --- a/packages/skills/src/access-store.ts +++ /dev/null @@ -1,96 +0,0 @@ -// Drizzle-backed `SkillAccessStore` over `skills.skill_access`. -import { eq } from "drizzle-orm"; -import { type } from "arktype"; -import { getAncestorChain, type DB } from "@intx/db"; - -import { - skillAccessScopeSchema, - type SkillAccessRow, - type SkillAccessStore, -} from "./access"; -import { skillAccess } from "./schema"; - -/** - * `scope` is stored as plain text so the schema stays portable; parse it - * back through arktype on the way out rather than asserting, so a row - * hand-edited to an unknown scope fails loudly here instead of silently - * widening a private skill. - */ -function rowToAccess(row: typeof skillAccess.$inferSelect): SkillAccessRow { - const scope = skillAccessScopeSchema(row.scope); - if (scope instanceof type.errors) { - throw new Error( - `skill_access row ${row.assetId} carries an unknown scope ${JSON.stringify(row.scope)}`, - ); - } - return { - assetId: row.assetId, - tenantId: row.tenantId, - skillName: row.skillName, - creatorPrincipalId: row.creatorPrincipalId, - scope, - }; -} - -export function createDrizzleSkillAccessStore(db: DB["db"]): SkillAccessStore { - return { - async upsert(row) { - const now = new Date(); - await db - .insert(skillAccess) - .values({ - assetId: row.assetId, - tenantId: row.tenantId, - skillName: row.skillName, - creatorPrincipalId: row.creatorPrincipalId, - scope: row.scope, - createdAt: now, - updatedAt: now, - }) - .onConflictDoUpdate({ - target: skillAccess.assetId, - set: { - tenantId: row.tenantId, - skillName: row.skillName, - creatorPrincipalId: row.creatorPrincipalId, - scope: row.scope, - updatedAt: now, - }, - }); - }, - async get(assetId) { - const rows = await db - .select() - .from(skillAccess) - .where(eq(skillAccess.assetId, assetId)) - .limit(1); - const found = rows[0]; - return found === undefined ? null : rowToAccess(found); - }, - async listForTenant(tenantId) { - // Access rows aren't native assets, but the skills they gate are — - // an inherited skill's access row lives in the ancestor tenant that - // created it, so visibility has to walk the same chain - // `resolveAssetByName`/`listAssetsForTenant` walk. Leaf-to-root order - // means a tenant's own row is seen before any ancestor's, so it wins - // the per-name shadow below exactly like the native asset resolver. - const chain = await getAncestorChain(db, tenantId); - const bySkillName = new Map(); - for (const tid of chain) { - const rows = await db - .select() - .from(skillAccess) - .where(eq(skillAccess.tenantId, tid)); - for (const row of rows) { - const parsed = rowToAccess(row); - if (bySkillName.has(parsed.skillName)) continue; - bySkillName.set(parsed.skillName, parsed); - } - } - return [...bySkillName.values()]; - }, - async remove(assetId) { - await db.delete(skillAccess).where(eq(skillAccess.assetId, assetId)); - }, - }; -} diff --git a/packages/skills/src/access.test.ts b/packages/skills/src/access.test.ts deleted file mode 100644 index 5c7269a429..0000000000 --- a/packages/skills/src/access.test.ts +++ /dev/null @@ -1,106 +0,0 @@ -import { describe, expect, test } from "bun:test"; - -import { - canAdministerSkill, - isSkillVisibleTo, - type SkillAccessRow, -} from "./access"; - -function row(overrides: Partial = {}): SkillAccessRow { - return { - assetId: "asset_1", - tenantId: "tenant_1", - skillName: "triage", - creatorPrincipalId: "principal_author", - scope: "private", - ...overrides, - }; -} - -describe("isSkillVisibleTo", () => { - test("a private skill is visible to its author", () => { - expect( - isSkillVisibleTo(row(), { - tenantId: "tenant_1", - principalId: "principal_author", - }), - ).toBe(true); - }); - - test("a private skill is hidden from another principal in the same tenant", () => { - expect( - isSkillVisibleTo(row(), { - tenantId: "tenant_1", - principalId: "principal_other", - }), - ).toBe(false); - }); - - test("a tenant-scoped skill is visible to any principal in its tenant", () => { - expect( - isSkillVisibleTo(row({ scope: "tenant" }), { - tenantId: "tenant_1", - principalId: "principal_other", - }), - ).toBe(true); - }); - - // Whether a row from another tenant tree ever reaches this predicate is - // the resolution layer's job (`SkillAssetStore.findByName` / - // `SkillAccessStore.listForTenant`, both chain-bounded — see - // registry.test.ts's "tenant inheritance" suite for that boundary - // exercised end to end); once a row is inherited from an ancestor, - // scope is the only remaining gate, which is what these cases cover. - test("an inherited tenant-scoped skill is visible even though its row's tenant differs from the caller's", () => { - expect( - isSkillVisibleTo(row({ scope: "tenant", tenantId: "tenant_parent" }), { - tenantId: "tenant_child", - principalId: "principal_other", - }), - ).toBe(true); - }); - - test("an inherited private skill stays hidden from everyone but its creator", () => { - const inherited = row({ scope: "private", tenantId: "tenant_parent" }); - expect( - isSkillVisibleTo(inherited, { - tenantId: "tenant_child", - principalId: "principal_other", - }), - ).toBe(false); - expect( - isSkillVisibleTo(inherited, { - tenantId: "tenant_child", - principalId: "principal_author", - }), - ).toBe(true); - }); -}); - -describe("canAdministerSkill", () => { - test("only the author may administer, even when the skill is tenant-scoped", () => { - const shared = row({ scope: "tenant" }); - expect( - canAdministerSkill(shared, { - tenantId: "tenant_1", - principalId: "principal_author", - }), - ).toBe(true); - expect( - canAdministerSkill(shared, { - tenantId: "tenant_1", - principalId: "principal_other", - }), - ).toBe(false); - }); - - test("the skill's own creator cannot administer it from a child tenant it was only inherited into", () => { - const inherited = row({ scope: "tenant", tenantId: "tenant_parent" }); - expect( - canAdministerSkill(inherited, { - tenantId: "tenant_child", - principalId: "principal_author", - }), - ).toBe(false); - }); -}); diff --git a/packages/skills/src/access.ts b/packages/skills/src/access.ts deleted file mode 100644 index d6301a2418..0000000000 --- a/packages/skills/src/access.ts +++ /dev/null @@ -1,69 +0,0 @@ -// Visibility scoping for registry skills. The verdict is a pure -// function of the access row and the calling principal so it is -// provable without a database; the drizzle-backed row store that feeds -// it lives in `./access-store.ts`. -import { type } from "arktype"; - -export const skillAccessScopeSchema = type("'private' | 'tenant'"); - -export type SkillAccessScope = typeof skillAccessScopeSchema.infer; - -export type SkillAccessRow = { - readonly assetId: string; - readonly tenantId: string; - readonly skillName: string; - readonly creatorPrincipalId: string; - readonly scope: SkillAccessScope; -}; - -export type SkillCaller = { - readonly tenantId: string; - readonly principalId: string; -}; - -/** - * A `tenant`-scoped skill is visible to every principal in the tenant - * that can already reach the row; a `private` one only to the principal - * who created it, tenant notwithstanding — a private skill inherited - * from a parent stays invisible to everyone in the child but its author. - * - * This predicate only ever runs on rows `SkillAssetStore.findByName` and - * `SkillAccessStore.listForTenant` already resolved, both of which bound - * their results to the caller's own tenant plus its ancestors (the same - * chain-walk the native asset resolver uses, see `resolveAssetByName` / - * `listAssetsForTenant`) — so the tenant boundary is enforced once, at - * the resolution layer, not duplicated here. This function's whole job - * is the scope check. - */ -export function isSkillVisibleTo( - row: SkillAccessRow, - caller: SkillCaller, -): boolean { - if (row.scope === "tenant") return true; - return row.creatorPrincipalId === caller.principalId; -} - -/** - * Only the creating principal, calling from the tenant that owns the - * skill, may republish, restore, or change its scope. A row inherited - * from an ancestor tenant is never administerable from a descendant — - * even by its own author — so writes never touch an ancestor's asset; - * the registry refuses those explicitly rather than silently forking a - * copy (see `registry.ts`'s `requireOwnTenant`). - */ -export function canAdministerSkill( - row: SkillAccessRow, - caller: SkillCaller, -): boolean { - return ( - row.tenantId === caller.tenantId && - row.creatorPrincipalId === caller.principalId - ); -} - -export type SkillAccessStore = { - upsert(row: SkillAccessRow): Promise; - get(assetId: string): Promise; - listForTenant(tenantId: string): Promise; - remove(assetId: string): Promise; -}; diff --git a/packages/skills/src/index.ts b/packages/skills/src/index.ts index 4a36fa071f..bf087856f2 100644 --- a/packages/skills/src/index.ts +++ b/packages/skills/src/index.ts @@ -1,13 +1,14 @@ export { - canAdministerSkill, - isSkillVisibleTo, - skillAccessScopeSchema, - type SkillAccessRow, - type SkillAccessScope, - type SkillAccessStore, + SkillRegistryError, + createSkillRegistry, + type CreateSkillRegistryDeps, type SkillCaller, -} from "./access"; -export { createDrizzleSkillAccessStore } from "./access-store"; + type SkillDetail, + type SkillRegistry, + type SkillRegistryErrorReason, + type SkillSummary, + type SkillVersion, +} from "./registry"; export { readAssetCommitHistory, isAssetGenesisCommit } from "./asset-history"; export { skillMdPath, @@ -28,22 +29,11 @@ export { withAvailableSkills, type PinnedSkillIndexEntry, } from "./prompt"; -export { - SkillRegistryError, - createSkillRegistry, - type CreateSkillRegistryDeps, - type SkillDetail, - type SkillRegistry, - type SkillRegistryErrorReason, - type SkillSummary, - type SkillVersion, -} from "./registry"; export { createSkillRoutes, type CreateSkillRoutesDeps, type PinnedByResolver, } from "./routes"; -export { skillAccess, skillsSchema } from "./schema"; export { SKILL_MD_FILENAME, SkillContentError, @@ -53,8 +43,10 @@ export { skillDescriptionSchema, skillFrontmatterSchema, skillNameSchema, + skillScopeSchema, type ParsedSkillMd, type SkillFrontmatter, + type SkillScope, } from "./skill-md"; export { createWorkflowSkillRoutes, diff --git a/packages/skills/src/migrations.ts b/packages/skills/src/migrations.ts deleted file mode 100644 index 8964554069..0000000000 --- a/packages/skills/src/migrations.ts +++ /dev/null @@ -1,100 +0,0 @@ -// Package-owned migrations for @corbits/skills. Literal SQL, a ledger -// table the package owns, and one transaction per migration — see -// docs/package-migrations.md. Every table lives in the package's own -// `skills` Postgres schema, never `public`. -import postgres from "postgres"; - -export interface SkillsMigration { - name: string; - sql: string; -} - -const SCHEMA = "skills"; - -export const skillsMigrations: readonly SkillsMigration[] = [ - { - name: "0001_skill_access", - sql: ` - CREATE TABLE IF NOT EXISTS "skills"."skill_access" ( - "asset_id" text PRIMARY KEY, - "tenant_id" text NOT NULL, - "skill_name" text NOT NULL, - "creator_principal_id" text NOT NULL, - "scope" text NOT NULL, - "created_at" timestamptz NOT NULL DEFAULT now(), - "updated_at" timestamptz NOT NULL DEFAULT now(), - CONSTRAINT "skill_access_scope_check" - CHECK ("scope" IN ('private', 'tenant')) - ); - CREATE INDEX IF NOT EXISTS "skill_access_tenant_idx" - ON "skills"."skill_access" ("tenant_id"); - CREATE UNIQUE INDEX IF NOT EXISTS "skill_access_tenant_name_uidx" - ON "skills"."skill_access" ("tenant_id", "skill_name"); - `, - }, -]; - -const LEDGER_TABLE = "skills_migrations"; - -function quoteIdentifier(name: string): string { - return `"${name.replace(/"/g, '""')}"`; -} - -function quoteQualified(schema: string, name: string): string { - return `${quoteIdentifier(schema)}.${quoteIdentifier(name)}`; -} - -export interface ApplySkillsMigrationsReport { - applied: string[]; - alreadyApplied: string[]; -} - -export async function applySkillsMigrations( - databaseUrl: string, -): Promise { - const sql = postgres(databaseUrl, { max: 1, onnotice: () => undefined }); - try { - await sql.unsafe(`CREATE SCHEMA IF NOT EXISTS ${quoteIdentifier(SCHEMA)}`); - - await sql.unsafe( - `CREATE TABLE IF NOT EXISTS ${quoteQualified(SCHEMA, LEDGER_TABLE)} (` + - `name text PRIMARY KEY, applied_at timestamptz NOT NULL DEFAULT now())`, - ); - - const applied: string[] = []; - const alreadyApplied: string[] = []; - - for (const migration of skillsMigrations) { - const existing = await sql.unsafe( - `SELECT 1 FROM ${quoteQualified(SCHEMA, LEDGER_TABLE)} WHERE name = $1`, - [migration.name], - ); - if (existing.length > 0) { - alreadyApplied.push(migration.name); - continue; - } - try { - await sql.begin(async (tx) => { - await tx.unsafe(migration.sql); - await tx.unsafe( - `INSERT INTO ${quoteQualified(SCHEMA, LEDGER_TABLE)} (name) VALUES ($1)`, - [migration.name], - ); - }); - applied.push(migration.name); - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - throw new Error( - `skills migration ${migration.name} failed: ${message}`, - { - cause: err, - }, - ); - } - } - - return { applied, alreadyApplied }; - } finally { - await sql.end({ timeout: 5 }); - } -} diff --git a/packages/skills/src/registry.ts b/packages/skills/src/registry.ts index 8f6e13eef1..ae7eb3c4ce 100644 --- a/packages/skills/src/registry.ts +++ b/packages/skills/src/registry.ts @@ -1,23 +1,23 @@ // The skill registry. One surface, one store: every skill is a native // `kind:"skill"` hub asset carrying a single `/SKILL.md`, and every -// version of it is a commit on that asset's default ref. Creating a skill -// creates that asset directly — there is no intermediate pending state. -import { - canAdministerSkill, - isSkillVisibleTo, - skillAccessScopeSchema, - type SkillAccessRow, - type SkillAccessScope, - type SkillAccessStore, - type SkillCaller, -} from "./access"; -import type { SkillAssetStore, SkillCommit } from "./asset-store"; +// version of it is a commit on that asset's default ref. Visibility lives +// in the SKILL.md frontmatter — the asset's own bytes — so there is no +// side table to drift from the content it gates. Creating a skill creates +// that asset directly — there is no intermediate pending state. +import type { + SkillAssetRow, + SkillAssetStore, + SkillCommit, +} from "./asset-store"; import { buildSkillMd, parseSkillMd, skillDescriptionSchema, skillNameSchema, + skillScopeSchema, SkillContentError, + type ParsedSkillMd, + type SkillScope, } from "./skill-md"; import { type } from "arktype"; @@ -42,11 +42,25 @@ export class SkillRegistryError extends Error { } } +export type SkillCaller = { + readonly tenantId: string; + readonly principalId: string; +}; + +/** A skill as the registry resolved it: the parsed tip SKILL.md plus the + * asset it was read from. Scope and authorship both come from these two — + * nothing else is consulted. */ +type ResolvedSkill = { + readonly asset: SkillAssetRow; + readonly parsed: ParsedSkillMd; + readonly updatedAtIso: string; +}; + export type SkillSummary = { readonly assetId: string; readonly name: string; readonly description: string; - readonly scope: SkillAccessScope; + readonly scope: SkillScope; readonly creatorPrincipalId: string; readonly updatedAtIso: string; }; @@ -73,10 +87,13 @@ export type SkillRegistry = { name: string, commitSha: string, ): Promise; + /** Republishes the skill's current content under a new scope as its own + * commit, so the previous scope stays restorable like any other version. + * Only the skill's own creator may rescope it. */ setScope( caller: SkillCaller, name: string, - scope: SkillAccessScope, + scope: SkillScope, ): Promise; create( caller: SkillCaller, @@ -84,14 +101,14 @@ export type SkillRegistry = { readonly name: string; readonly description: string; readonly body: string; - readonly scope: SkillAccessScope; + readonly scope: SkillScope; }, ): Promise; /** Republishes an existing skill's body/description as a new commit on - * its same asset, scope untouched — the "republish" `canAdministerSkill` - * documents. Distinct from `create`, which always 409s on a name a + * its same asset, scope untouched — only the skill's own creator may + * `update` it. Distinct from `create`, which always 409s on a name a * fully-formed skill already owns (see `create`'s own conflict - * handling) — only the skill's own creator may `update` it. */ + * handling). */ update( caller: SkillCaller, name: string, @@ -108,7 +125,6 @@ export type SkillRegistry = { export type CreateSkillRegistryDeps = { assets: SkillAssetStore; - access: SkillAccessStore; }; /** True when an arktype failure includes a regex `pattern` check — the @@ -146,8 +162,8 @@ function assertDescription(raw: string): string { return parsed; } -function assertScope(raw: string): SkillAccessScope { - const parsed = skillAccessScopeSchema(raw); +function assertScope(raw: string): SkillScope { + const parsed = skillScopeSchema(raw); if (parsed instanceof type.errors) { throw new SkillRegistryError( "invalid", @@ -164,27 +180,62 @@ function contentErrorToRegistryError(cause: unknown): never { throw cause; } +/** + * A `tenant`-scoped skill is visible to every principal in the tenant + * that can already reach the asset; a `private` one only to the principal + * who created it, tenant notwithstanding — a private skill inherited from + * a parent stays invisible to everyone in the child but its author. + * + * This predicate only ever runs on assets `SkillAssetStore.findByName` + * and `SkillAssetStore.listForTenant` already resolved, both of which + * bound their results to the caller's own tenant plus its ancestors (the + * same chain-walk the native asset resolver uses) — so the tenant + * boundary is enforced once, at the resolution layer, not duplicated + * here. This function's whole job is the scope check. + */ +function isSkillVisibleTo(skill: ResolvedSkill, caller: SkillCaller): boolean { + if (skill.parsed.scope === "tenant") return true; + return skill.asset.creatorPrincipalId === caller.principalId; +} + +/** + * Only the creating principal, calling from the tenant that owns the + * skill, may republish, restore, or change its scope. An asset inherited + * from an ancestor tenant is never administerable from a descendant — + * even by its own author — so writes never touch an ancestor's asset; + * the registry refuses those explicitly rather than silently forking a + * copy (see `requireOwnTenant`). + */ +function canAdministerSkill( + skill: ResolvedSkill, + caller: SkillCaller, +): boolean { + return ( + skill.asset.tenantId === caller.tenantId && + skill.asset.creatorPrincipalId === caller.principalId + ); +} + /** * Guards every write path (`update`, `restore`, `setScope`). A skill * inherited from an ancestor tenant is refused loudly and specifically — * never silently forked into a same-named copy in the caller's own * tenant — before falling through to the ordinary "only the author" - * check `canAdministerSkill` already makes for skills the caller's own - * tenant does own. + * check for skills the caller's own tenant does own. */ function requireOwnTenant( - row: SkillAccessRow, + skill: ResolvedSkill, caller: SkillCaller, name: string, action: string, ): void { - if (row.tenantId !== caller.tenantId) { + if (skill.asset.tenantId !== caller.tenantId) { throw new SkillRegistryError( "forbidden", `"${name}" is inherited from a parent workbench — ${action} it from the workbench that owns it, not from a child.`, ); } - if (!canAdministerSkill(row, caller)) { + if (!canAdministerSkill(skill, caller)) { throw new SkillRegistryError( "forbidden", `only the author of "${name}" may ${action} it`, @@ -192,116 +243,97 @@ function requireOwnTenant( } } +function summarize(skill: ResolvedSkill): SkillSummary { + return { + assetId: skill.asset.id, + name: skill.parsed.name, + description: skill.parsed.description, + scope: skill.parsed.scope, + creatorPrincipalId: skill.asset.creatorPrincipalId ?? "unknown", + updatedAtIso: skill.updatedAtIso, + }; +} + +function detailOf(skill: ResolvedSkill): SkillDetail { + return { ...summarize(skill), body: skill.parsed.body }; +} + export function createSkillRegistry( deps: CreateSkillRegistryDeps, ): SkillRegistry { - const { assets, access } = deps; + const { assets } = deps; + + /** Reads a skill's tip SKILL.md and parses it. Returns null when the + * asset carries no SKILL.md yet — the half-written state a crashed + * `create` leaves behind — so callers treat it as invisible rather + * than as a skill with empty content. */ + async function readTip(asset: SkillAssetRow): Promise { + const contents = await assets.readSkillMd({ + assetId: asset.id, + skillName: asset.name, + }); + if (contents === null) return null; + return { + asset, + parsed: parseSkillMd(contents), + updatedAtIso: asset.updatedAt.toISOString(), + }; + } async function resolveVisible( caller: SkillCaller, name: string, - ): Promise<{ row: SkillAccessRow; assetId: string }> { + ): Promise { const asset = await assets.findByName(caller.tenantId, name); if (asset === null) { throw new SkillRegistryError("not_found", `no skill named "${name}"`); } - const row = await access.get(asset.id); - if (row === null || !isSkillVisibleTo(row, caller)) { + const skill = await readTip(asset); + if (skill === null || !isSkillVisibleTo(skill, caller)) { throw new SkillRegistryError("not_found", `no skill named "${name}"`); } - return { row, assetId: asset.id }; + return skill; } - async function readDetail( - row: SkillAccessRow, - updatedAtIso: string, - ): Promise { - const contents = await assets.readSkillMd({ - assetId: row.assetId, - skillName: row.skillName, - }); - if (contents === null) { - throw new SkillRegistryError( - "not_found", - `skill "${row.skillName}" has no SKILL.md on its default ref`, - ); - } - const parsed = parseSkillMd(contents); - return { - assetId: row.assetId, - name: parsed.name, - description: parsed.description, - body: parsed.body, - scope: row.scope, - creatorPrincipalId: row.creatorPrincipalId, - updatedAtIso, - }; - } - - async function summarize( + async function visibleSkills( caller: SkillCaller, - rows: readonly SkillAccessRow[], - ): Promise { + ): Promise { const assetRows = await assets.listForTenant(caller.tenantId); - const updatedById = new Map( - assetRows.map((row) => [row.id, row.updatedAt.toISOString()]), - ); - const out: SkillSummary[] = []; - for (const row of rows) { - const contents = await assets.readSkillMd({ - assetId: row.assetId, - skillName: row.skillName, - }); - if (contents === null) continue; - const parsed = parseSkillMd(contents); - out.push({ - assetId: row.assetId, - name: parsed.name, - description: parsed.description, - scope: row.scope, - creatorPrincipalId: row.creatorPrincipalId, - updatedAtIso: updatedById.get(row.assetId) ?? new Date(0).toISOString(), - }); + const out: ResolvedSkill[] = []; + for (const asset of assetRows) { + const skill = await readTip(asset); + if (skill === null) continue; + if (!isSkillVisibleTo(skill, caller)) continue; + out.push(skill); } - return out.sort((a, b) => a.name.localeCompare(b.name)); - } - - async function visibleRows( - caller: SkillCaller, - ): Promise { - const rows = await access.listForTenant(caller.tenantId); - return rows.filter((row) => isSkillVisibleTo(row, caller)); + return out.sort((a, b) => a.parsed.name.localeCompare(b.parsed.name)); } return { async list(caller) { - return summarize(caller, await visibleRows(caller)); + return (await visibleSkills(caller)).map(summarize); }, async search(caller, query) { const needle = query.trim().toLowerCase(); - const summaries = await summarize(caller, await visibleRows(caller)); - if (needle === "") return summaries; - return summaries.filter( - (skill) => - skill.name.toLowerCase().includes(needle) || - skill.description.toLowerCase().includes(needle), - ); + const skills = await visibleSkills(caller); + if (needle === "") return skills.map(summarize); + return skills + .filter( + (skill) => + skill.parsed.name.toLowerCase().includes(needle) || + skill.parsed.description.toLowerCase().includes(needle), + ) + .map(summarize); }, async load(caller, name) { - const { row } = await resolveVisible(caller, name); - const assetRows = await assets.listForTenant(caller.tenantId); - const assetRow = assetRows.find((entry) => entry.id === row.assetId); - return readDetail( - row, - (assetRow?.updatedAt ?? new Date(0)).toISOString(), - ); + return detailOf(await resolveVisible(caller, name)); }, async versions(caller, name) { - const { row } = await resolveVisible(caller, name); - const commits = await assets.history(row.assetId); + const skill = await resolveVisible(caller, name); + const commits = await assets.history(skill.asset.id); return commits.map((commit, index) => ({ ...commit, current: index === 0, @@ -309,10 +341,10 @@ export function createSkillRegistry( }, async versionContent(caller, name, commitSha) { - const { row } = await resolveVisible(caller, name); + const skill = await resolveVisible(caller, name); const contents = await assets.readSkillMd({ - assetId: row.assetId, - skillName: row.skillName, + assetId: skill.asset.id, + skillName: skill.asset.name, commitSha, }); if (contents === null) { @@ -321,8 +353,7 @@ export function createSkillRegistry( `skill "${name}" has no SKILL.md at commit ${commitSha}`, ); } - const parsed = parseSkillMd(contents); - const commit = (await assets.history(row.assetId)).find( + const commit = (await assets.history(skill.asset.id)).find( (entry) => entry.commitSha === commitSha, ); if (commit === undefined) { @@ -331,23 +362,24 @@ export function createSkillRegistry( `commit ${commitSha} is not in "${name}"'s history`, ); } + const parsed = parseSkillMd(contents); return { - assetId: row.assetId, + assetId: skill.asset.id, name: parsed.name, description: parsed.description, body: parsed.body, - scope: row.scope, - creatorPrincipalId: row.creatorPrincipalId, + scope: parsed.scope, + creatorPrincipalId: skill.asset.creatorPrincipalId ?? "unknown", updatedAtIso: commit.committedAtIso, }; }, async restore(caller, name, commitSha) { - const { row } = await resolveVisible(caller, name); - requireOwnTenant(row, caller, name, "restore"); + const skill = await resolveVisible(caller, name); + requireOwnTenant(skill, caller, name, "restore"); const contents = await assets.readSkillMd({ - assetId: row.assetId, - skillName: row.skillName, + assetId: skill.asset.id, + skillName: skill.asset.name, commitSha, }); if (contents === null) { @@ -357,35 +389,45 @@ export function createSkillRegistry( ); } await assets.writeSkillMd({ - assetId: row.assetId, - skillName: row.skillName, + assetId: skill.asset.id, + skillName: skill.asset.name, contents, message: `Restore ${name} to ${commitSha.slice(0, 8)}`, }); - return readDetail(row, new Date().toISOString()); + const restored = await readTip(skill.asset); + if (restored === null) { + throw new SkillRegistryError( + "not_found", + `skill "${name}" has no SKILL.md on its default ref`, + ); + } + return detailOf(restored); }, async setScope(caller, name, scope) { const parsedScope = assertScope(scope); - const { row } = await resolveVisible(caller, name); - requireOwnTenant(row, caller, name, "change who can see"); - const next: SkillAccessRow = { - assetId: row.assetId, - tenantId: row.tenantId, - skillName: row.skillName, - creatorPrincipalId: row.creatorPrincipalId, + const skill = await resolveVisible(caller, name); + requireOwnTenant(skill, caller, name, "change who can see"); + const rescoped = buildSkillMd({ + name: skill.parsed.name, + description: skill.parsed.description, scope: parsedScope, - }; - await access.upsert(next); - const summaries = await summarize(caller, [next]); - const summary = summaries[0]; - if (summary === undefined) { + body: skill.parsed.body, + }); + await assets.writeSkillMd({ + assetId: skill.asset.id, + skillName: skill.asset.name, + contents: rescoped, + message: `Set scope of ${name} to ${parsedScope}`, + }); + const updated = await readTip(skill.asset); + if (updated === null) { throw new SkillRegistryError( "not_found", `skill "${name}" has no SKILL.md on its default ref`, ); } - return summary; + return summarize(updated); }, async create(caller, input) { @@ -394,29 +436,25 @@ export function createSkillRegistry( const parsedScope = assertScope(input.scope); let contents: string; try { - contents = buildSkillMd({ name, description, body: input.body }); + contents = buildSkillMd({ + name, + description, + scope: parsedScope, + body: input.body, + }); } catch (cause) { contentErrorToRegistryError(cause); } - async function finish(assetId: string): Promise { - const row: SkillAccessRow = { - assetId, - tenantId: caller.tenantId, - skillName: name, - creatorPrincipalId: caller.principalId, - scope: parsedScope, - }; - await access.upsert(row); - const summaries = await summarize(caller, [row]); - const summary = summaries[0]; - if (summary === undefined) { + async function finish(asset: SkillAssetRow): Promise { + const skill = await readTip(asset); + if (skill === null) { throw new SkillRegistryError( "not_found", `created skill "${name}" is not readable back`, ); } - return summary; + return summarize(skill); } // Own-tenant only, deliberately not inheritance-aware: a name @@ -425,9 +463,9 @@ export function createSkillRegistry( // not a conflict. const existing = await assets.findOwnByName(caller.tenantId, name); if (existing !== null) { - const existingRow = await access.get(existing.id); + const existingSkill = await readTip(existing); if ( - existingRow !== null || + existingSkill !== null || existing.creatorPrincipalId !== caller.principalId ) { // Either a fully-formed skill already owns this name, or the @@ -438,25 +476,18 @@ export function createSkillRegistry( `a skill named "${name}" already exists in this workbench`, ); } - // The asset exists but has no access row: a prior create for this - // exact name got as far as `assets.create` (and maybe - // `writeSkillMd`) and then failed or timed out before finishing. - // Finish it — write the SKILL.md commit if it's still missing, - // then the access row — rather than 409ing on a name this same - // caller can never use again. Never re-create the asset itself. - const existingContents = await assets.readSkillMd({ + // The asset exists but carries no SKILL.md yet: a prior create + // for this exact name got as far as `assets.create` and then + // failed or timed out before the SKILL.md commit landed. Finish + // it — write the commit, never re-create the asset — rather than + // 409ing on a name this same caller can never use again. + await assets.writeSkillMd({ assetId: existing.id, skillName: name, + contents, + message: `Create ${name}`, }); - if (existingContents === null) { - await assets.writeSkillMd({ - assetId: existing.id, - skillName: name, - contents, - message: `Create ${name}`, - }); - } - return finish(existing.id); + return finish(existing); } const created = await assets.create({ @@ -471,49 +502,48 @@ export function createSkillRegistry( contents, message: `Create ${name}`, }); - return finish(created.id); + return finish(created); }, async update(caller, name, input) { const parsedName = assertSkillName(name); const description = assertDescription(input.description); + const skill = await resolveVisible(caller, parsedName); + requireOwnTenant(skill, caller, parsedName, "update"); + if (input.expectedHeadSha !== undefined) { + const head = (await assets.history(skill.asset.id))[0]; + if (head?.commitSha !== input.expectedHeadSha) { + throw new SkillRegistryError( + "conflict", + `"${parsedName}" changed since this edit started — review the current version before saving.`, + ); + } + } let contents: string; try { contents = buildSkillMd({ name: parsedName, description, + scope: skill.parsed.scope, body: input.body, }); } catch (cause) { contentErrorToRegistryError(cause); } - - const { row } = await resolveVisible(caller, parsedName); - requireOwnTenant(row, caller, parsedName, "update"); - if (input.expectedHeadSha !== undefined) { - const head = (await assets.history(row.assetId))[0]; - if (head?.commitSha !== input.expectedHeadSha) { - throw new SkillRegistryError( - "conflict", - `"${parsedName}" changed since this edit started — review the current version before saving.`, - ); - } - } await assets.writeSkillMd({ - assetId: row.assetId, + assetId: skill.asset.id, skillName: parsedName, contents, message: `Update ${parsedName}`, }); - const summaries = await summarize(caller, [row]); - const summary = summaries[0]; - if (summary === undefined) { + const updated = await readTip(skill.asset); + if (updated === null) { throw new SkillRegistryError( "not_found", `updated skill "${parsedName}" is not readable back`, ); } - return summary; + return summarize(updated); }, }; } diff --git a/packages/skills/src/routes.ts b/packages/skills/src/routes.ts index ed6fd8cbe9..9740d3710c 100644 --- a/packages/skills/src/routes.ts +++ b/packages/skills/src/routes.ts @@ -8,13 +8,12 @@ import { Hono } from "hono"; import type { TenantEnv, RequireGrant } from "@intx/hub-api"; -import { skillAccessScopeSchema } from "./access"; import { SkillRegistryError, type SkillRegistry, type SkillRegistryErrorReason, } from "./registry"; -import { parseSkillMd, SkillContentError } from "./skill-md"; +import { parseSkillMd, skillScopeSchema, SkillContentError } from "./skill-md"; import { makeErrorEnvelope } from "@corbits/error-sink"; /** Which workflow definitions pin a given skill. */ @@ -32,7 +31,7 @@ const CreateSkillFieldsBody = type({ name: "string", description: "string", body: "string", - scope: skillAccessScopeSchema, + scope: skillScopeSchema, }); /** The upload path: a whole SKILL.md, parsed at this boundary with the @@ -41,7 +40,7 @@ const CreateSkillFieldsBody = type({ * content would have. */ const CreateSkillFileBody = type({ source: "string", - scope: skillAccessScopeSchema, + scope: skillScopeSchema, }); const CreateSkillBody = CreateSkillFieldsBody.or(CreateSkillFileBody); @@ -63,7 +62,7 @@ const UpdateSkillBody = type({ const RestoreBody = type({ commitSha: commitShaSchema }); -const ScopeBody = type({ scope: skillAccessScopeSchema }); +const ScopeBody = type({ scope: skillScopeSchema }); const STATUS_BY_REASON: Record< SkillRegistryErrorReason, diff --git a/packages/skills/src/schema.ts b/packages/skills/src/schema.ts deleted file mode 100644 index 6e03bbaa0a..0000000000 --- a/packages/skills/src/schema.ts +++ /dev/null @@ -1,36 +0,0 @@ -// Who may see which skill. The skill itself — its SKILL.md, its body, -// its whole version history — lives in the native `kind:"skill"` asset's -// git repo, never here; this table carries only the visibility verdict -// the registry applies when listing or searching on behalf of a caller. -// -// Lives in its own `skills` Postgres schema, never `public` (see -// docs/package-migrations.md). `tenantId`, `assetId`, and -// `creatorPrincipalId` are plain text identifiers, not foreign keys, so -// the reference to a platform asset row works identically from a -// package-owned schema. -import { pgSchema, text, timestamp } from "drizzle-orm/pg-core"; - -export const skillsSchema = pgSchema("skills"); - -/** - * One row per registry-visible skill asset, keyed by the asset id. - * - * `scope` is the whole gate: `private` means only `creatorPrincipalId` - * sees the skill, `tenant` means every principal in `tenantId` does. A - * skill asset with no row here is invisible to everyone — a skill is - * published into the registry by writing this row, so an asset that - * exists without one can never leak. - */ -export const skillAccess = skillsSchema.table("skill_access", { - assetId: text("asset_id").primaryKey(), - tenantId: text("tenant_id").notNull(), - skillName: text("skill_name").notNull(), - creatorPrincipalId: text("creator_principal_id").notNull(), - scope: text("scope").notNull(), - createdAt: timestamp("created_at", { withTimezone: true }) - .notNull() - .defaultNow(), - updatedAt: timestamp("updated_at", { withTimezone: true }) - .notNull() - .defaultNow(), -}); diff --git a/packages/skills/src/skill-md.test.ts b/packages/skills/src/skill-md.test.ts index d105ea1987..ec32d322e1 100644 --- a/packages/skills/src/skill-md.test.ts +++ b/packages/skills/src/skill-md.test.ts @@ -8,27 +8,51 @@ import { } from "./skill-md"; describe("buildSkillMd", () => { - test("round-trips name, description, and body", () => { + test("round-trips name, description, scope, and body", () => { const md = buildSkillMd({ name: "summarize-transcript", description: "Condenses a meeting transcript into decisions and owners.", body: "1. Read the transcript.\n2. List every decision.", + scope: "tenant", }); const parsed = parseSkillMd(md); expect(parsed.name).toBe("summarize-transcript"); expect(parsed.description).toBe( "Condenses a meeting transcript into decisions and owners.", ); + expect(parsed.scope).toBe("tenant"); expect(parsed.body).toBe( "1. Read the transcript.\n2. List every decision.", ); }); + test("a private scope survives the round trip", () => { + const md = buildSkillMd({ + name: "triage", + description: "Sorts issues.", + body: "Sort them.", + scope: "private", + }); + expect(parseSkillMd(md).scope).toBe("private"); + }); + + test("rejects a scope outside the private/tenant vocabulary", () => { + expect(() => + buildSkillMd({ + name: "triage", + description: "Sorts issues.", + body: "Sort them.", + scope: "everyone", + }), + ).toThrow(SkillContentError); + }); + test("quotes a description containing a colon so the frontmatter stays one mapping", () => { const md = buildSkillMd({ name: "triage", description: "Sorts issues: bug, question, or feature.", body: "Sort them.", + scope: "private", }); expect(parseSkillMd(md).description).toBe( "Sorts issues: bug, question, or feature.", @@ -40,6 +64,7 @@ describe("buildSkillMd", () => { name: "triage", description: "Reads the reporter's own words first.", body: "Sort them.", + scope: "private", }); expect(parseSkillMd(md).description).toBe( "Reads the reporter's own words first.", @@ -52,13 +77,19 @@ describe("buildSkillMd", () => { name: "Summarize Transcript", description: "Anything.", body: "Body.", + scope: "private", }), ).toThrow(SkillContentError); }); test("rejects the reserved vendor names", () => { expect(() => - buildSkillMd({ name: "claude", description: "Anything.", body: "Body." }), + buildSkillMd({ + name: "claude", + description: "Anything.", + body: "Body.", + scope: "private", + }), ).toThrow(SkillContentError); }); @@ -68,13 +99,19 @@ describe("buildSkillMd", () => { name: "triage", description: "Sorts issues.", body: "Body.", + scope: "private", }), ).toThrow(SkillContentError); }); test("rejects an empty body", () => { expect(() => - buildSkillMd({ name: "triage", description: "Sorts.", body: " " }), + buildSkillMd({ + name: "triage", + description: "Sorts.", + body: " ", + scope: "private", + }), ).toThrow(SkillContentError); }); }); @@ -96,6 +133,13 @@ describe("parseSkillMd", () => { ); }); + test("a SKILL.md written before scope moved into frontmatter reads as private", () => { + const parsed = parseSkillMd( + "---\nname: triage\ndescription: Sorts issues.\n---\nBody", + ); + expect(parsed.scope).toBe("private"); + }); + test("ignores optional frontmatter keys the Claude Code superset allows", () => { const parsed = parseSkillMd( "---\nname: triage\ndescription: Sorts issues.\nmodel: any\n---\nBody", diff --git a/packages/skills/src/skill-md.ts b/packages/skills/src/skill-md.ts index bf37240abe..6916b37481 100644 --- a/packages/skills/src/skill-md.ts +++ b/packages/skills/src/skill-md.ts @@ -3,7 +3,9 @@ // walks each top-level directory of a skill asset, requires a SKILL.md // there, and rejects the push unless the YAML frontmatter carries a // kebab-case `name` matching the directory and a 1..1024-char -// `description` free of HTML tags. +// `description` free of HTML tags. This package always writes a `scope` +// key as well; the handler ignores undeclared keys, so SKILL.md files +// written before scope moved into the frontmatter still validate. // // Both halves of that contract are enforced here, at this package's own // boundary, so an author sees a plain-language rejection in the create @@ -33,9 +35,18 @@ export const skillDescriptionSchema = type("1 <= string <= 1024").and( type(/^(?!.*<[^>]+>).*$/s), ); +/** Who may see a skill: `private` (only its creator) or `tenant` (every + * principal in the owning workbench). Lives in the SKILL.md frontmatter — + * the asset's own bytes — so visibility travels with the content it gates + * instead of in a side table that can drift from it. */ +export const skillScopeSchema = type("'private' | 'tenant'"); + +export type SkillScope = typeof skillScopeSchema.infer; + export const skillFrontmatterSchema = type({ name: skillNameSchema, description: skillDescriptionSchema, + "scope?": skillScopeSchema, }).onUndeclaredKey("ignore"); export type SkillFrontmatter = typeof skillFrontmatterSchema.infer; @@ -43,6 +54,7 @@ export type SkillFrontmatter = typeof skillFrontmatterSchema.infer; export type ParsedSkillMd = { readonly name: string; readonly description: string; + readonly scope: SkillScope; readonly body: string; }; @@ -57,16 +69,20 @@ export class SkillContentError extends Error { * Renders a SKILL.md the hub's skill kind handler accepts. The * description is emitted as a single-quoted YAML scalar so a colon or a * leading `#` in an author's prose cannot re-open the frontmatter as a - * different mapping. + * different mapping. Scope is required — a skill is never written without + * saying who may see it — and validated against the same vocabulary the + * parser accepts. */ export function buildSkillMd(input: { readonly name: string; readonly description: string; + readonly scope: string; readonly body: string; }): string { const frontmatter = skillFrontmatterSchema({ name: input.name, description: input.description, + scope: input.scope, }); if (frontmatter instanceof type.errors) { throw new SkillContentError( @@ -82,6 +98,7 @@ export function buildSkillMd(input: { FRONTMATTER_DELIMITER, `name: ${frontmatter.name}`, `description: ${quotedDescription}`, + `scope: ${frontmatter.scope}`, FRONTMATTER_DELIMITER, "", body, @@ -90,10 +107,20 @@ export function buildSkillMd(input: { } /** - * Parses a SKILL.md back into the three fields the registry surfaces. + * Parses a SKILL.md back into the fields the registry surfaces. * Fails closed: a missing or malformed frontmatter block is an error, - * never a skill that silently reads as nameless. - */ + * never a skill that silently reads as nameless. A SKILL.md written + * before scope moved into the frontmatter carries no `scope` key and + * reads as `private` — the pre-cutover store's own default for a skill + * with no row — so old bytes stay invisible to everyone but their author + * instead of failing to parse. + * + * No backfill for the dropped `skill_access` table: every pre-cutover + * SKILL.md lacks `scope`, and no seed, fixture, template, or + * provisioning path plants a tenant-visible row (verified CL-7583), so + * there is nothing that would silently narrow. Pre-GA cutovers are not + * migrated — dev databases reseed via `bun run reset` (CL-7445 ruling; + * see docs/package-migrations.md). */ export function parseSkillMd(text_: string): ParsedSkillMd { const lines = text_.split(/\r?\n/); if (lines[0] !== FRONTMATTER_DELIMITER) { @@ -133,6 +160,7 @@ export function parseSkillMd(text_: string): ParsedSkillMd { return { name: frontmatter.name, description: frontmatter.description, + scope: frontmatter.scope ?? "private", body: lines .slice(endIdx + 1) .join("\n") diff --git a/packages/skills/test/fakes.ts b/packages/skills/test/fakes.ts index 5d784e6219..bb8790f695 100644 --- a/packages/skills/test/fakes.ts +++ b/packages/skills/test/fakes.ts @@ -1,9 +1,8 @@ -// In-memory doubles for the two ports the registry stands on. They keep -// the registry suite honest without a live hub or a database: the asset +// In-memory double for the one port the registry stands on. It keeps the +// registry suite honest without a live hub or a database: the asset // double keeps a real per-asset commit list so version history and // restore are exercised against an append-only log, exactly the shape // the git-backed store serves. -import type { SkillAccessRow, SkillAccessStore } from "../src/access"; import type { SkillAssetRow, SkillAssetStore, @@ -19,6 +18,9 @@ type StoredCommit = { export type FakeSkillAssets = SkillAssetStore & { readonly assets: Map; + /** Makes the next writeSkillMd call throw, simulating a crash between + * the asset-row write and the SKILL.md write. */ + failNextWriteSkillMd: () => void; }; /** A tenant's own id, then its parent, then its grandparent, and so on to @@ -51,6 +53,7 @@ export function createFakeSkillAssets( const commits = new Map(); let nextId = 1; let nextSha = 1; + let failNextWrite = false; function tip(assetId: string): StoredCommit | undefined { const log = commits.get(assetId); @@ -66,6 +69,9 @@ export function createFakeSkillAssets( return { assets, + failNextWriteSkillMd() { + failNextWrite = true; + }, async create(input) { const key = `${input.tenantId}${input.name}`; for (const existing of assets.values()) { @@ -121,6 +127,10 @@ export function createFakeSkillAssets( return [...byName.values()]; }, async writeSkillMd(input) { + if (failNextWrite) { + failNextWrite = false; + throw new Error("simulated SKILL.md write failure"); + } const log = commits.get(input.assetId); if (log === undefined) { throw new Error(`no such asset ${input.assetId}`); @@ -167,31 +177,3 @@ export function createFakeSkillAssets( }, }; } - -export function createFakeSkillAccess( - tenantParents: TenantParents = {}, -): SkillAccessStore { - const rows = new Map(); - return { - async upsert(row) { - rows.set(row.assetId, row); - }, - async get(assetId) { - return rows.get(assetId) ?? null; - }, - async listForTenant(tenantId) { - const bySkillName = new Map(); - for (const tid of ancestorChain(tenantParents, tenantId)) { - for (const row of rows.values()) { - if (row.tenantId !== tid) continue; - if (bySkillName.has(row.skillName)) continue; - bySkillName.set(row.skillName, row); - } - } - return [...bySkillName.values()]; - }, - async remove(assetId) { - rows.delete(assetId); - }, - }; -} diff --git a/packages/skills/test/registry.test.ts b/packages/skills/test/registry.test.ts index 1c66a8cb18..76c99e0fec 100644 --- a/packages/skills/test/registry.test.ts +++ b/packages/skills/test/registry.test.ts @@ -5,9 +5,8 @@ import { SkillRegistryError, type SkillRegistry, } from "../src/registry"; -import { createFakeSkillAccess, createFakeSkillAssets } from "./fakes"; +import { createFakeSkillAssets } from "./fakes"; import type { FakeSkillAssets } from "./fakes"; -import type { SkillAccessStore } from "../src/access"; const AUTHOR = { tenantId: "tenant_1", principalId: "principal_author" }; const TEAMMATE = { tenantId: "tenant_1", principalId: "principal_teammate" }; @@ -20,13 +19,11 @@ const CREATE_INPUT = { }; let assets: FakeSkillAssets; -let access: SkillAccessStore; let registry: SkillRegistry; beforeEach(() => { assets = createFakeSkillAssets(); - access = createFakeSkillAccess(); - registry = createSkillRegistry({ assets, access }); + registry = createSkillRegistry({ assets }); }); async function publish(scope: "private" | "tenant") { @@ -100,68 +97,45 @@ describe("create", () => { ).rejects.toThrow("Description can't contain HTML tags."); }); - test("retrying create after a failure between the asset write and the access-row write completes it, rather than 409ing forever", async () => { + test("retrying create after a crash between the asset write and the SKILL.md write completes it, rather than 409ing forever", async () => { // Simulates the exact crash window `create` cannot make transactional: - // the asset and its SKILL.md commit succeed, but the access-row write - // — the last step — fails once (a db timeout, say). - let failNext = true; - const flakyAccess: SkillAccessStore = { - ...access, - async upsert(row) { - if (failNext) { - failNext = false; - throw new Error("simulated write failure (e.g. db timeout)"); - } - return access.upsert(row); - }, - }; - const flakyRegistry = createSkillRegistry({ assets, access: flakyAccess }); + // the asset row exists, but its SKILL.md commit never landed. + assets.failNextWriteSkillMd(); await expect( - flakyRegistry.create(AUTHOR, { ...CREATE_INPUT, scope: "private" }), - ).rejects.toThrow(/simulated write failure/); + registry.create(AUTHOR, { ...CREATE_INPUT, scope: "private" }), + ).rejects.toThrow(/simulated SKILL.md write failure/); - // The asset was created and left behind, but is invisible: no access - // row backs it yet. + // The asset was created and left behind, but is invisible: it carries + // no SKILL.md yet. expect(assets.assets.size).toBe(1); - expect(await flakyRegistry.list(AUTHOR)).toHaveLength(0); + expect(await registry.list(AUTHOR)).toHaveLength(0); // Retrying the exact same create — the natural recovery a user or // client would attempt — finishes the interrupted write instead of // 409ing on a name this same caller can never use again. - const completed = await flakyRegistry.create(AUTHOR, { + const completed = await registry.create(AUTHOR, { ...CREATE_INPUT, scope: "private", }); expect(completed.name).toBe("triage"); expect(assets.assets.size).toBe(1); - expect((await flakyRegistry.list(AUTHOR)).map((s) => s.name)).toEqual([ + expect((await registry.list(AUTHOR)).map((s) => s.name)).toEqual([ "triage", ]); // A third attempt now hits a fully-formed skill and is a genuine // conflict. await expect( - flakyRegistry.create(AUTHOR, { ...CREATE_INPUT, scope: "private" }), + registry.create(AUTHOR, { ...CREATE_INPUT, scope: "private" }), ).rejects.toThrow(/already exists/); }); test("a caller can never complete another principal's half-written create", async () => { - let failNext = true; - const flakyAccess: SkillAccessStore = { - ...access, - async upsert(row) { - if (failNext) { - failNext = false; - throw new Error("simulated write failure"); - } - return access.upsert(row); - }, - }; - const flakyRegistry = createSkillRegistry({ assets, access: flakyAccess }); + assets.failNextWriteSkillMd(); await expect( - flakyRegistry.create(AUTHOR, { ...CREATE_INPUT, scope: "private" }), - ).rejects.toThrow(/simulated write failure/); + registry.create(AUTHOR, { ...CREATE_INPUT, scope: "private" }), + ).rejects.toThrow(/simulated SKILL.md write failure/); // A different principal retrying the same name hits a conflict, not // a takeover of the first caller's orphaned asset. @@ -216,6 +190,25 @@ describe("access scoping", () => { expect(await registry.list(TEAMMATE)).toHaveLength(1); }); + test("setScope re-commits the same content under the new scope", async () => { + await publish("private"); + const before = await registry.versions(AUTHOR, "triage"); + expect(before).toHaveLength(1); + + const shared = await registry.setScope(AUTHOR, "triage", "tenant"); + expect(shared.scope).toBe("tenant"); + + // The rescope is itself a commit: the content is unchanged, but the + // history grew, so the previous scope stays restorable. + const loaded = await registry.load(AUTHOR, "triage"); + expect(loaded.body).toBe(CREATE_INPUT.body); + expect(loaded.description).toBe(CREATE_INPUT.description); + const after = await registry.versions(AUTHOR, "triage"); + expect(after).toHaveLength(2); + expect(after[0]?.current).toBe(true); + expect(await registry.list(TEAMMATE)).toHaveLength(1); + }); + test("setScope unshares a tenant skill back to its author", async () => { await publish("tenant"); await registry.setScope(AUTHOR, "triage", "private"); @@ -254,7 +247,6 @@ describe("versions", () => { await publish("tenant"); const first = (await registry.versions(AUTHOR, "triage"))[0]; expect(first).toBeDefined(); - await registry.setScope(AUTHOR, "triage", "tenant"); // Author a second version by restoring, then confirm the rewind is // itself a new commit rather than a rewrite of the history. @@ -307,12 +299,10 @@ describe("tenant inheritance", () => { const inheritingAssets = createFakeSkillAssets({ tenantParents: PARENT_HIERARCHY, }); - const inheritingAccess = createFakeSkillAccess(PARENT_HIERARCHY); return { assets: inheritingAssets, registry: createSkillRegistry({ assets: inheritingAssets, - access: inheritingAccess, }), }; } diff --git a/packages/skills/test/routes.test.ts b/packages/skills/test/routes.test.ts index 3468d86c48..f3b01a9c8a 100644 --- a/packages/skills/test/routes.test.ts +++ b/packages/skills/test/routes.test.ts @@ -10,7 +10,7 @@ import type { TenantEnv } from "@intx/hub-api"; import { createSkillRegistry } from "../src/registry"; import { createSkillRoutes } from "../src/routes"; -import { createFakeSkillAccess, createFakeSkillAssets } from "./fakes"; +import { createFakeSkillAssets } from "./fakes"; const TENANT = { id: "tnt_1" }; const PRINCIPAL = { id: "prn_author" }; @@ -18,7 +18,6 @@ const PRINCIPAL = { id: "prn_author" }; function buildApp(): Hono { const registry = createSkillRegistry({ assets: createFakeSkillAssets(), - access: createFakeSkillAccess(), }); const routes = createSkillRoutes({ registry, diff --git a/packages/skills/test/workflow-routes.test.ts b/packages/skills/test/workflow-routes.test.ts index 433f4deb45..1dd8aedfd6 100644 --- a/packages/skills/test/workflow-routes.test.ts +++ b/packages/skills/test/workflow-routes.test.ts @@ -7,7 +7,7 @@ import { type WorkflowRunAuthenticator, type WorkflowSkillsEnv, } from "../src/workflow-routes"; -import { createFakeSkillAccess, createFakeSkillAssets } from "./fakes"; +import { createFakeSkillAssets } from "./fakes"; const AUTHOR_TOKEN = "sidecar-token"; const AUTHOR_ADDRESS = "run-author@runs.example"; @@ -57,7 +57,6 @@ async function request( beforeEach(async () => { const registry = createSkillRegistry({ assets: createFakeSkillAssets(), - access: createFakeSkillAccess(), }); const author = { tenantId: "tenant_1", principalId: "principal_author" }; await registry.create(author, { diff --git a/scripts/checks/no-product-tenancy.ts b/scripts/checks/no-product-tenancy.ts index a29adcdf52..0722010273 100644 --- a/scripts/checks/no-product-tenancy.ts +++ b/scripts/checks/no-product-tenancy.ts @@ -154,11 +154,6 @@ const ALLOWLIST: readonly { maxOccurrences: 1, tables: ["slack_channel_binding"], }, - { - relPath: "packages/skills/src/schema.ts", - maxOccurrences: 1, - tables: ["skill_access"], - }, { relPath: "packages/preferences/src/schema.ts", maxOccurrences: 1, diff --git a/scripts/db-setup.ts b/scripts/db-setup.ts index 6a46e7254d..f4010752cc 100644 --- a/scripts/db-setup.ts +++ b/scripts/db-setup.ts @@ -39,7 +39,6 @@ import { import { applyInsightsMigrations } from "../packages/insights/src/migrations"; import { applyPreferencesMigrations } from "../packages/preferences/src/migrations"; import { applyBenchMigrations } from "../packages/bench/src/migrations"; -import { applySkillsMigrations } from "../packages/skills/src/migrations"; import { applyAgentDirectoryMigrations } from "../packages/agent-directory/src/migrations"; import { applyOnboardingMigrations } from "../packages/onboarding/src/migrations"; import { applyAccessPolicyMigrations } from "../packages/access-policy/src/migrations"; @@ -71,7 +70,6 @@ const INSTALLED_PACKAGE_MIGRATIONS: readonly { { name: "@corbits/insights", apply: applyInsightsMigrations }, { name: "@corbits/preferences", apply: applyPreferencesMigrations }, { name: "@corbits/bench", apply: applyBenchMigrations }, - { name: "@corbits/skills", apply: applySkillsMigrations }, { name: "@corbits/agent-directory", apply: applyAgentDirectoryMigrations }, { name: "@workbench/onboarding", apply: applyOnboardingMigrations }, { name: "@workbench/access-policy", apply: applyAccessPolicyMigrations },