diff --git a/docs/adr/0018-answer-renamed-skill-runs-with-the-new-name.md b/docs/adr/0018-answer-renamed-skill-runs-with-the-new-name.md new file mode 100644 index 00000000..83348341 --- /dev/null +++ b/docs/adr/0018-answer-renamed-skill-runs-with-the-new-name.md @@ -0,0 +1,44 @@ +# ADR-0018: Answer renamed Skill runs with the new name + +**Status:** Accepted +**Date:** 2026-10-13 + +Amends the run-command decision in [ADR-0017](0017-follow-renamed-skills.md). + +## Context + +ADR-0017 let `skilld run //`{lang="html"} resolve the renamed +Skill's identity. `skilld` then refused the Artifact: it checks that the Skill it +receives carries the name it asked for, and answered `SOURCE_MISMATCH` with +"skilld.dev delivered the Skill im-part-human, not the Skill this ref names". + +That check stops a registry from substituting one Skill for another. A rename +should not weaken it. + +## Decision + +Delivery does not build a renamed Skill under its old name. A request that names +an old Skill name answers 404 with code `SOURCE_NOT_FOUND`, and the problem +detail names the new name and the command to run it: + +```text +The Skill im-not-a-fly was renamed to im-part-human. Run skilld run harlan-zw/brundlefly/im-part-human. +``` + +`findSkillRunIdentity` no longer follows a Skill alias. `findRenamedSkill` reads +the alias, and `/api/skills/run-identity` reports it as `renamedTo`. + +## Consequences + +Released CLIs show the problem detail as written, so this needs no CLI release. +The reader, or the agent acting for them, runs the new name on purpose. The +CLI's name check stays as strict as before. + +A Repository move still resolves old run commands to the new identity, as +ADR-0015 decided. + +## Rejected alternatives + +A rename field in the Resolution answer, accepted by a new CLI, would keep old +commands running. It needs a protocol release and a CLI release, and it relaxes +the substitution check for every renamed Skill. diff --git a/layers/artifact-delivery/server/utils/admitted-identity.ts b/layers/artifact-delivery/server/utils/admitted-identity.ts index 08dbe4a2..1ceb5876 100644 --- a/layers/artifact-delivery/server/utils/admitted-identity.ts +++ b/layers/artifact-delivery/server/utils/admitted-identity.ts @@ -1,11 +1,16 @@ import type { SourceRequest } from '../schemas/contracts' -export interface AdmittedSkillIdentity { - /** The admitted Skill folder inside the Repository, or `.` for the root. */ - skillPath: string - /** The commit the Skill page names, or null when the registry has none. */ - commitSha: string | null -} +/** What the registry holds under one Skill name. */ +export type AdmittedSkillIdentity + = | { + _tag: 'admitted' + /** The admitted Skill folder inside the Repository, or `.` for the root. */ + skillPath: string + /** The commit the Skill page names, or null when the registry has none. */ + commitSha: string | null + } + /** The Skill was renamed, and this is its new name (ADR-0017). */ + | { _tag: 'renamed', name: string } /** Ask the registry for the identity it admitted under one name; null when it holds none. */ export type FetchAdmittedSkillIdentity = (skill: { @@ -26,6 +31,10 @@ export type FetchAdmittedSkillIdentity = (skill: { * A reference the request names wins over the admitted commit, because the * person asked for it. A name the registry does not hold, such as a private * Skill, keeps the tree search by name. + * + * A renamed Skill's old name answers 404 with the command for its new name. + * `skilld run` refuses a Skill whose name differs from the one it asked for, + * and the reader should choose to run the renamed Skill. */ export async function admittedSourceRequest( source: SourceRequest, @@ -40,6 +49,14 @@ export async function admittedSourceRequest( }) if (!identity) return source + if (identity._tag === 'renamed') { + const renamed = `${source.owner}/${source.repository}/${identity.name}` + throw createError({ + statusCode: 404, + message: `The Skill ${source.selector.name} was renamed to ${identity.name}. Run skilld run ${renamed}.`, + data: { code: 'SOURCE_NOT_FOUND' }, + }) + } const ref = source.ref ?? (identity.commitSha ? { type: 'commit' as const, value: identity.commitSha } : undefined) return { provider: source.provider, diff --git a/layers/artifact-delivery/server/utils/request-resolution.ts b/layers/artifact-delivery/server/utils/request-resolution.ts index 0bb6ada3..92fbcda6 100644 --- a/layers/artifact-delivery/server/utils/request-resolution.ts +++ b/layers/artifact-delivery/server/utils/request-resolution.ts @@ -41,8 +41,10 @@ export function fetchAdmittedSkillIdentity(context?: H3EventContext): FetchAdmit return async ({ owner, repository, name }) => { // A named object passes `context`, which NitroFetchOptions does not declare. const options = { query: { owner, repo: repository, name }, ...(context ? { context } : {}) } - const answer = await $fetch<{ skillPath: string | null, commitSha: string | null }>('/api/skills/run-identity', options) - return answer.skillPath ? { skillPath: answer.skillPath, commitSha: answer.commitSha } : null + const answer = await $fetch<{ skillPath: string | null, commitSha: string | null, renamedTo: string | null }>('/api/skills/run-identity', options) + if (answer.skillPath) + return { _tag: 'admitted', skillPath: answer.skillPath, commitSha: answer.commitSha } + return answer.renamedTo ? { _tag: 'renamed', name: answer.renamedTo } : null } } diff --git a/layers/registry/server/api/skills/run-identity.get.ts b/layers/registry/server/api/skills/run-identity.get.ts index 54f0e05c..84451154 100644 --- a/layers/registry/server/api/skills/run-identity.get.ts +++ b/layers/registry/server/api/skills/run-identity.get.ts @@ -2,7 +2,7 @@ import { setHeader } from 'h3' import { z } from 'zod' import { defineApiHandler } from '#shared/server/handler' import { presentSkillRunIdentity } from '../../presenters/skill-run-identity' -import { findSkillRunIdentity } from '../../utils/skill-run-identity' +import { findRenamedSkill, findSkillRunIdentity } from '../../utils/skill-run-identity' const RunIdentityQuery = z.object({ owner: z.string().min(1).max(39).regex(/^[a-z0-9-]+$/i), @@ -13,10 +13,12 @@ const RunIdentityQuery = z.object({ const RunIdentityResponse = z.object({ skillPath: z.string().min(1).nullable(), commitSha: z.string().regex(/^[a-f0-9]{40}$/).nullable(), + renamedTo: z.string().min(1).max(64).nullable(), }).strict() /** - * The admitted folder and source commit of one registry Skill, or nulls. + * The admitted folder and source commit of one registry Skill, the new name + * of a renamed one, or nulls. * * Artifact delivery reads this over HTTP, so `skilld run OWNER/REPO/NAME` * resolves the identity the registry admitted and the Skill page shows. Not @@ -28,7 +30,12 @@ export default defineApiHandler({ response: RunIdentityResponse, handler: async ({ event, body, platform }) => { setHeader(event, 'cache-control', 'private, no-store') - return await findSkillRunIdentity(platform.db, { owner: body.owner, repository: body.repo, name: body.name }) + const query = { owner: body.owner, repository: body.repo, name: body.name } + const identity = await findSkillRunIdentity(platform.db, query) + if (identity) + return { _tag: 'admitted' as const, identity } + const renamed = await findRenamedSkill(platform.db, query) + return renamed ? { _tag: 'renamed' as const, name: renamed } : { _tag: 'unknown' as const } }, presenter: presentSkillRunIdentity, }) diff --git a/layers/registry/server/presenters/skill-run-identity.ts b/layers/registry/server/presenters/skill-run-identity.ts index 038afc03..37bf6c17 100644 --- a/layers/registry/server/presenters/skill-run-identity.ts +++ b/layers/registry/server/presenters/skill-run-identity.ts @@ -3,11 +3,23 @@ import type { SkillRunIdentity } from '../utils/skill-run-identity' export interface SkillRunIdentityDto { skillPath: string | null commitSha: string | null + /** The new name of a renamed Skill, set only when `skillPath` is null. */ + renamedTo: string | null } -export function presentSkillRunIdentity(identity: SkillRunIdentity | null): SkillRunIdentityDto { - return { - skillPath: identity?.skillPath ?? null, - commitSha: identity?.commitSha ?? null, +/** What one Skill name resolves to: an admitted identity, a new name, or nothing. */ +export type SkillRunAnswer + = | { _tag: 'admitted', identity: SkillRunIdentity } + | { _tag: 'renamed', name: string } + | { _tag: 'unknown' } + +export function presentSkillRunIdentity(answer: SkillRunAnswer): SkillRunIdentityDto { + switch (answer._tag) { + case 'admitted': + return { skillPath: answer.identity.skillPath, commitSha: answer.identity.commitSha, renamedTo: null } + case 'renamed': + return { skillPath: null, commitSha: null, renamedTo: answer.name } + case 'unknown': + return { skillPath: null, commitSha: null, renamedTo: null } } } diff --git a/layers/registry/server/utils/skill-run-identity.ts b/layers/registry/server/utils/skill-run-identity.ts index 6f8d2512..8d599520 100644 --- a/layers/registry/server/utils/skill-run-identity.ts +++ b/layers/registry/server/utils/skill-run-identity.ts @@ -37,7 +37,7 @@ interface IdentityRow { * * A Repository that moved keeps answering under its old name, so a run * command printed before the move resolves the identity the page shows now. - * A renamed Skill does the same under its old Skill name (ADR-0017). + * A renamed Skill does not: `findRenamedSkill` names its new name instead. */ export async function findSkillRunIdentity( db: D1Database, @@ -46,9 +46,6 @@ export async function findSkillRunIdentity( const identity = await findAdmittedIdentity(db, query) if (identity) return identity - const renamed = await findSkillAlias(db, { owner: query.owner, repo: query.repository, name: query.name }) - if (renamed) - return await findAdmittedIdentity(db, { owner: query.owner, repository: query.repository, name: renamed.targetName }) const alias = await findRepositoryAlias(db, { owner: query.owner, repo: query.repository }) if (!alias) return null @@ -59,6 +56,22 @@ export async function findSkillRunIdentity( }) } +/** + * The name a renamed Skill now lives under, or null (ADR-0017). + * + * `skilld run` checks that the Skill it receives carries the name it asked + * for, so delivering the renamed Skill under its old name fails that check. + * Delivery answers the old name with the new one instead, and the reader + * runs it. + */ +export async function findRenamedSkill( + db: D1Database, + query: SkillRunIdentityQuery, +): Promise { + const alias = await findSkillAlias(db, { owner: query.owner, repo: query.repository, name: query.name }) + return alias?.targetName ?? null +} + async function findAdmittedIdentity( db: D1Database, query: SkillRunIdentityQuery, diff --git a/test/unit/skill-rename.test.ts b/test/unit/skill-rename.test.ts index 6a56f9d3..f1623f17 100644 --- a/test/unit/skill-rename.test.ts +++ b/test/unit/skill-rename.test.ts @@ -2,7 +2,7 @@ import type { SqliteD1 } from './helpers/d1-sqlite' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { indexSkillAliases, resolveRenamedSkillRoute } from '../../layers/registry/server/utils/skill-aliases' import { matchSkillRenames, skillBodySimilarity } from '../../layers/registry/server/utils/skill-rename' -import { findSkillRunIdentity } from '../../layers/registry/server/utils/skill-run-identity' +import { findRenamedSkill, findSkillRunIdentity } from '../../layers/registry/server/utils/skill-run-identity' import { syncRepo } from '../../layers/registry/server/utils/sync-repo' import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite' @@ -208,14 +208,14 @@ describe('sync follows a renamed Skill', () => { expect(rows(`SELECT name FROM skill_dirty WHERE reason = 'skill_renamed'`)).toEqual([{ name: 'prose' }]) }) - it('keeps an old run command working through the alias', async () => { + it('answers an old run command with the new name and delivers nothing under the old one', async () => { seedSkill('edit', EDIT_BODY) serveTree({ prose: EDIT_BODY }) await syncRepo('acme', 'skills', {}, d1.db, { submitted: true }) - const identity = await findSkillRunIdentity(d1.db, { owner: 'acme', repository: 'skills', name: 'edit' }) - - expect(identity).toMatchObject({ skillPath: 'skills/prose' }) + expect(await findSkillRunIdentity(d1.db, { owner: 'acme', repository: 'skills', name: 'edit' })).toBeNull() + expect(await findRenamedSkill(d1.db, { owner: 'acme', repository: 'skills', name: 'edit' })).toBe('prose') + expect(await findRenamedSkill(d1.db, { owner: 'acme', repository: 'skills', name: 'prose' })).toBeNull() }) it('turns a tombstone into an alias when the new name is admitted after the old one was swept', async () => { diff --git a/test/unit/skill-run-identity.test.ts b/test/unit/skill-run-identity.test.ts index c2d399d7..803c6e9c 100644 --- a/test/unit/skill-run-identity.test.ts +++ b/test/unit/skill-run-identity.test.ts @@ -82,7 +82,7 @@ describe('admittedSourceRequest', () => { } it('resolves an admitted name by its folder at the commit the Skill page names', async () => { - const lookup = vi.fn(async () => ({ skillPath: 'skills/taste-skill', commitSha: NEWER })) + const lookup = vi.fn(async () => ({ _tag: 'admitted' as const, skillPath: 'skills/taste-skill', commitSha: NEWER })) expect(await admittedSourceRequest(named, lookup)).toEqual({ provider: 'github', @@ -95,7 +95,7 @@ describe('admittedSourceRequest', () => { }) it('keeps a reference the request names', async () => { - const lookup = vi.fn(async () => ({ skillPath: 'skills/taste-skill', commitSha: NEWER })) + const lookup = vi.fn(async () => ({ _tag: 'admitted' as const, skillPath: 'skills/taste-skill', commitSha: NEWER })) const request: SourceRequest = { ...named, ref: { type: 'branch', value: 'next' } } expect(await admittedSourceRequest(request, lookup)).toMatchObject({ @@ -105,12 +105,23 @@ describe('admittedSourceRequest', () => { }) it('resolves the default branch when the registry knows no commit', async () => { - const resolved = await admittedSourceRequest(named, async () => ({ skillPath: '.', commitSha: null })) + const resolved = await admittedSourceRequest(named, async () => ({ _tag: 'admitted' as const, skillPath: '.', commitSha: null })) expect(resolved).toEqual({ ...named, selector: { type: 'path', path: '.' } }) expect(resolved).not.toHaveProperty('ref') }) + it('answers an old Skill name with the command for its new name instead of delivering another Skill', async () => { + const lookup = vi.fn(async () => ({ _tag: 'renamed' as const, name: 'im-part-human' })) + const old: SourceRequest = { ...named, owner: 'harlan-zw', repository: 'brundlefly', selector: { type: 'named-skill', name: 'im-not-a-fly' } } + + await expect(admittedSourceRequest(old, lookup)).rejects.toMatchObject({ + statusCode: 404, + data: { code: 'SOURCE_NOT_FOUND' }, + message: 'The Skill im-not-a-fly was renamed to im-part-human. Run skilld run harlan-zw/brundlefly/im-part-human.', + }) + }) + it('leaves a name the registry does not hold, and a path, as requested', async () => { const lookup = vi.fn(async () => null) const path: SourceRequest = { ...named, selector: { type: 'path', path: 'skills/taste-skill' } }