From 3248a60ce3af671c2c4f027c682c973fdf81bd77 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Tue, 13 Oct 2026 23:07:51 +1100 Subject: [PATCH] fix(delivery): answer a renamed Skill run with its new name Delivering a renamed Skill under its old name made skilld refuse it with SOURCE_MISMATCH, because the CLI checks that the Skill it receives carries the name it asked for. An old name now answers 404 with the command for the new name, which released CLIs show as written (ADR-0018). --- ...er-renamed-skill-runs-with-the-new-name.md | 44 +++++++++++++++++++ .../server/utils/admitted-identity.ts | 29 +++++++++--- .../server/utils/request-resolution.ts | 6 ++- .../server/api/skills/run-identity.get.ts | 13 ++++-- .../server/presenters/skill-run-identity.ts | 20 +++++++-- .../server/utils/skill-run-identity.ts | 21 +++++++-- test/unit/skill-rename.test.ts | 10 ++--- test/unit/skill-run-identity.test.ts | 17 +++++-- 8 files changed, 133 insertions(+), 27 deletions(-) create mode 100644 docs/adr/0018-answer-renamed-skill-runs-with-the-new-name.md 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' } }