diff --git a/docs/adr/0017-follow-renamed-skills.md b/docs/adr/0017-follow-renamed-skills.md new file mode 100644 index 00000000..1bedddcd --- /dev/null +++ b/docs/adr/0017-follow-renamed-skills.md @@ -0,0 +1,75 @@ +# ADR-0017: Follow renamed Skills + +**Status:** Accepted +**Date:** 2026-10-13 + +Extends [ADR-0015](0015-follow-moved-repositories.md) from Repositories to the Skills inside one. + +## Context + +The registry names a Skill after its folder. When an author renames the folder, +sync saw one Skill disappear and another appear. The old page became a 410 +tombstone, its likes and collection entries stayed behind, and old run commands +failed. The new name started with no history. + +On 2026-10-13 `harlan-zw/brundlefly` renamed `im-not-a-fly` to `im-part-human` +and `im-a-fly` to `im-not-a-human`. The old pages answered 410 and the new +names answered 404. + +## Decision + +Sync detects a rename and follows it. + +- Detection runs in a sync's final sweep, once the tree's full name set is + known. A candidate pair is a name the tree no longer holds and a Skill first + seen in this sync. The removal and the addition can come from different + invocations of one sync, so both copies are read from D1. +- The pair must share at least 60% of the distinct body lines of their + `SKILL.md`, frontmatter excluded, and each must be the other's single best + match. Brundlefly's renamed Skills scored 0.97 and 0.99 against their old + copies and 0.00 against everything else. An ambiguous pair stays a removal + and an addition. +- A name the sweep already marked gone still pairs. A scheduled sync can sweep + the old name before an index request admits the new one. +- The old name's rows move to the new name in the sync's final batch, through + the same Skill table list a Repository move uses. A clash keeps the row under + the new name. The new name keeps the first-seen date of the old one. +- The old name stays as a `skill_aliases` row. `/gh///`{lang="html"}, + every path under it, and its `.md` copy answer 301 to the same path under the + new name, in one hop. A later rename rewrites older aliases. A rename back + deletes the alias. +- `skilld run //`{lang="html"} keeps working. The registry + answers the old name with the new identity. + +`layers/registry/server/utils/skill-rename.ts` holds detection and the rename. +`layers/registry/server/middleware/gh-renamed-skill-redirect.ts` answers old URLs. + +## Consequences + +A rename adds no indexable page and keeps the Skill's likes, collection +entries, revisions, and history. The new name still needs admission like any +new Skill. For a Repository that only a submission admits, the owner's index +request brings the new name in, and that sync records the rename. + +Every renamed Skill is queued in `skill_dirty` with reason `skill_renamed`. The +`embedding` marker stays under the old name, as in a Repository move. + +Only a folder rename inside one Repository is detected. A rename that also +rewrites more than about 40% of the body reads as a removal and an addition. A +move of a Skill between Repositories reads the same way. + +Cull path: there is none while old URLs receive traffic. Dropping +`skill_aliases` ends every redirect. + +## Rejected alternatives + +Redirecting a removed Skill to its Repository page would point an old URL at a +page that is not its equivalent. Search engines treat that as a soft 404, and +the site was suppressed once for scaled low-value pages. A removal without a +detected rename keeps its 410. + +Rename detection from GitHub's compare API needs a stored base commit and one +more request for each sync. The cached `SKILL.md` already answers the question. + +An author-declared previous name, for example in frontmatter `metadata`, is +exact, but no Agent Skills convention defines it today. diff --git a/layers/registry/server/middleware/gh-renamed-skill-redirect.ts b/layers/registry/server/middleware/gh-renamed-skill-redirect.ts new file mode 100644 index 00000000..d667dd17 --- /dev/null +++ b/layers/registry/server/middleware/gh-renamed-skill-redirect.ts @@ -0,0 +1,25 @@ +import { repositoryPathSegments } from '../utils/repository-aliases' +import { indexSkillAliases, loadSkillAliases, resolveRenamedSkillRoute } from '../utils/skill-aliases' + +/** + * Answer every URL under a renamed Skill with a 301 to its new name, so the + * old URLs pass their link equity on (ADR-0017). Paths without a Skill + * segment skip the cache read. + */ +export default defineEventHandler(async (event) => { + const url = getRequestURL(event) + if (!repositoryPathSegments(url.pathname)?.rest.length) + return + + const db = event.context.platform?.db + if (!db) + return + + const aliases = await loadSkillAliases(useStorage('cache'), db) + if (!aliases?.length) + return + + const decision = resolveRenamedSkillRoute(url.pathname, url.search, indexSkillAliases(aliases)) + if (decision._tag === 'redirect') + return sendRedirect(event, decision.location, 301) +}) diff --git a/layers/registry/server/utils/repository-move.ts b/layers/registry/server/utils/repository-move.ts index 107a11bf..37f87992 100644 --- a/layers/registry/server/utils/repository-move.ts +++ b/layers/registry/server/utils/repository-move.ts @@ -101,7 +101,7 @@ const ROOT_SKILL = `(SELECT name FROM skills WHERE owner = :from_owner AND repo const movedSkillName = (column: string) => `CASE WHEN ${column} = ${ROOT_SKILL} THEN :root_name ELSE ${column} END` const movedRowName = (row = '') => `CASE WHEN ${row}rendered_skill_path = 'SKILL.md' THEN :root_name ELSE ${row}name END` -interface KeyedTable { +export interface KeyedTable { table: string repo?: string /** The Skill name column, for a table that holds Skills. */ @@ -121,7 +121,7 @@ const skillTable = (table: string, options: Omit = {}): Key * nightly prune finds through that marker, and the missing marker under the * new name queues a fresh embedding. */ -const SKILL_TABLES: KeyedTable[] = [ +export const SKILL_TABLES: readonly KeyedTable[] = [ skillTable('skill_likes'), skillTable('skill_revisions'), skillTable('skill_description_history'), @@ -270,8 +270,16 @@ const TOKEN = /:([a-z_]+)/g * uses. D1 rejects a statement bound with more values than it names. */ export function repositoryMoveStatements(move: RepositoryMove): MoveStatement[] { - const values = moveValues(move) - return MOVE_SQL.map((template) => { + return namedStatements(MOVE_SQL, moveValues(move)) +} + +/** + * Bind `:name` tokens to positional `?N` parameters, giving each statement + * exactly the values it uses. D1 rejects a statement bound with more values + * than it names. + */ +export function namedStatements(templates: readonly string[], values: Record): MoveStatement[] { + return templates.map((template) => { const order: string[] = [] const sql = template.replace(TOKEN, (_, token: string) => { if (!order.includes(token)) diff --git a/layers/registry/server/utils/skill-aliases.ts b/layers/registry/server/utils/skill-aliases.ts new file mode 100644 index 00000000..c1b3b352 --- /dev/null +++ b/layers/registry/server/utils/skill-aliases.ts @@ -0,0 +1,128 @@ +/// + +import type { ReadThroughCache } from '#shared/server/cache' +import { readCache, writeCache } from '#shared/server/cache' +import { repositoryPathSegments } from './repository-aliases' + +/** An old Skill name and the name the Skill now lives under (ADR-0017). */ +export interface SkillAlias { + owner: string + repo: string + name: string + targetName: string +} + +interface SkillAliasRow { + owner: string + repo: string + name: string + target_name: string +} + +export type RenamedSkillRoute + = | { _tag: 'pass' } + | { _tag: 'redirect', location: string } + +export const SKILL_ALIASES_CACHE_KEY = 'skills:aliases:v1' +/** Seconds. A new alias waits at most this long before its URLs redirect. */ +export const SKILL_ALIASES_CACHE_TTL = 15 * 60 + +// An admitted row under the old name means the author brought the name back. +const ALIAS_COLUMNS = `a.owner, a.repo, a.name, a.target_name` +const LIVE_NAME = `EXISTS ( + SELECT 1 FROM skills s + WHERE s.owner = a.owner AND s.repo = a.repo AND s.name = a.name AND s.source_resolved = 1)` + +export const SKILL_ALIASES_SQL = ` + SELECT ${ALIAS_COLUMNS} + FROM skill_aliases a + WHERE NOT ${LIVE_NAME} +` + +function toAlias(row: SkillAliasRow): SkillAlias { + return { owner: row.owner, repo: row.repo, name: row.name, targetName: row.target_name } +} + +/** The alias an old Skill name answers with, matched without case, or null. */ +export async function findSkillAlias( + db: D1Database, + skill: { owner: string, repo: string, name: string }, +): Promise { + const row = await db + .prepare(`SELECT ${ALIAS_COLUMNS} FROM skill_aliases a WHERE a.owner = ? AND a.repo = ? AND a.name = ? AND NOT ${LIVE_NAME}`) + .bind(skill.owner, skill.repo, skill.name) + .first() + return row ? toAlias(row) : null +} + +export async function selectSkillAliases(db: D1Database): Promise { + const rows = await db.prepare(SKILL_ALIASES_SQL).bind().all() + return (rows.results ?? []).map(toAlias) +} + +/** + * Every live alias for this request, or null when neither store answers. + * + * This runs on every `/gh` Skill request, so the set comes from the cache. A + * failure costs one request the redirect: the old URL renders what it did + * before the rename. The wide event carries the reason. + */ +export async function loadSkillAliases( + cache: ReadThroughCache, + db: D1Database, +): Promise { + const cached = await readCache(cache, SKILL_ALIASES_CACHE_KEY) + if (Array.isArray(cached)) + return cached + + try { + const aliases = await selectSkillAliases(db) + await writeCache(cache, SKILL_ALIASES_CACHE_KEY, aliases, { ttl: SKILL_ALIASES_CACHE_TTL }) + return aliases + } + catch (error) { + emitOperationalEvent(createWideEvent({ + operation: 'skill-aliases', + outcome: 'failed', + reason: error instanceof Error ? error.message : String(error), + })) + return null + } +} + +const aliasKey = (owner: string, repo: string, name: string) => `${owner}/${repo}/${name}`.toLowerCase() + +export function indexSkillAliases(aliases: readonly SkillAlias[]): ReadonlyMap { + return new Map(aliases.map(alias => [aliasKey(alias.owner, alias.repo, alias.name), alias])) +} + +/** + * Send a URL under a renamed Skill to the same place under its new name, in + * one 301. The Skill page, its file deep links, and its agent `.md` copy all + * follow. The owner and Repository segments and the query string stay. + */ +export function resolveRenamedSkillRoute( + pathname: string, + search: string, + aliases: ReadonlyMap, +): RenamedSkillRoute { + const path = repositoryPathSegments(pathname) + const [segment, ...below] = path?.rest ?? [] + if (!path || !segment) + return { _tag: 'pass' } + + const exact = aliases.get(aliasKey(path.owner, path.repo, segment)) + // `/gh///.md` is the agent copy of the Skill page. + const markdown = !exact && below.length === 0 && segment.toLowerCase().endsWith('.md') + ? aliases.get(aliasKey(path.owner, path.repo, segment.slice(0, -'.md'.length))) + : undefined + const alias = exact ?? markdown + if (!alias) + return { _tag: 'pass' } + + const name = markdown ? `${alias.targetName}.md` : alias.targetName + return { + _tag: 'redirect', + location: `/gh/${[path.owner, path.repo, name, ...below].join('/')}${search}`, + } +} diff --git a/layers/registry/server/utils/skill-rename.ts b/layers/registry/server/utils/skill-rename.ts new file mode 100644 index 00000000..b9ca61b1 --- /dev/null +++ b/layers/registry/server/utils/skill-rename.ts @@ -0,0 +1,198 @@ +/// + +import type { KeyedTable, MoveStatement } from './repository-move' +import { namedStatements, SKILL_TABLES } from './repository-move' + +/** A Skill the registry holds, with the SKILL.md it last rendered. */ +export interface SkillCopy { + name: string + raw: string +} + +/** One Skill folder an author renamed inside a Repository (ADR-0017). */ +export interface SkillRename { + owner: string + repo: string + from: string + to: string + /** Unix seconds. */ + renamedAt: number +} + +/** + * Two SKILL.md bodies that share at least this share of their lines are the + * same Skill. Brundlefly's two renamed Skills scored 0.97 and 0.99 against + * their old copies, and 0.00 against each other and an unrelated new Skill. + */ +export const RENAME_SIMILARITY = 0.6 + +// Short lines, such as headings and list markers, recur across unrelated Skills. +const MIN_LINE_LENGTH = 20 +const FRONTMATTER = /^---\r?\n[\s\S]*?\r?\n---(?:\r?\n|$)/ + +function bodyLines(raw: string): Set { + return new Set( + raw.replace(FRONTMATTER, '') + .split(/\r?\n/) + .map(line => line.trim()) + .filter(line => line.length >= MIN_LINE_LENGTH), + ) +} + +/** + * The share of distinct body lines two SKILL.md files hold in common, from 0 + * to 1. Frontmatter does not count, because a rename edits the name in it. + */ +export function skillBodySimilarity(left: string, right: string): number { + const a = bodyLines(left) + const b = bodyLines(right) + const union = new Set([...a, ...b]).size + if (union === 0) + return 0 + let shared = 0 + for (const line of a) { + if (b.has(line)) + shared++ + } + return shared / union +} + +/** The index of the single best score at or above the threshold, or -1. */ +function singleBest(scores: readonly number[], threshold: number): number { + const top = Math.max(...scores) + if (!(top >= threshold) || scores.filter(score => score === top).length !== 1) + return -1 + return scores.indexOf(top) +} + +/** + * Pair each removed Skill with the added Skill it became. A pair needs a + * similarity at or above the threshold, and each side must be the other's + * single best match. An ambiguous case stays a removal and an addition. + */ +export function matchSkillRenames( + removed: readonly SkillCopy[], + added: readonly SkillCopy[], + threshold = RENAME_SIMILARITY, +): Array<{ from: string, to: string }> { + if (!removed.length || !added.length) + return [] + const scores = removed.map(old => added.map(fresh => skillBodySimilarity(old.raw, fresh.raw))) + const pairs: Array<{ from: string, to: string }> = [] + removed.forEach((old, row) => { + const column = singleBest(scores[row]!, threshold) + if (column < 0) + return + if (singleBest(scores.map(rowScores => rowScores[column]!), threshold) === row) + pairs.push({ from: old.name, to: added[column]!.name }) + }) + return pairs +} + +/** + * The renames one sync made, judged once it knows every name the tree still + * holds. `missing` are the names the tree no longer holds. A Skill first seen + * at `checkedAt` was added by this sync, in whichever invocation wrote it. + * + * A missing name can already be marked gone: a scheduled sync can sweep the + * old name before an index request admits the new one. Its cached copy still + * pairs it, so its tombstone becomes a redirect. + */ +export async function findSkillRenames( + db: D1Database, + input: { owner: string, repo: string, missing: readonly string[], checkedAt: number }, +): Promise { + if (!input.missing.length) + return [] + const added = await db.prepare( + `SELECT name, rendered_raw AS raw FROM skills + WHERE owner = ? AND repo = ? AND first_seen_at = ? + AND source_resolved = 1 AND rendered_raw IS NOT NULL`, + ).bind(input.owner, input.repo, input.checkedAt).all() + if (!added.results?.length) + return [] + const removed = await db.prepare( + `SELECT name, rendered_raw AS raw FROM skills + WHERE owner = ? AND repo = ? AND name IN (SELECT value FROM json_each(?)) + AND rendered_raw IS NOT NULL`, + ).bind(input.owner, input.repo, JSON.stringify(input.missing)).all() + return matchSkillRenames(removed.results ?? [], added.results).map(pair => ({ + owner: input.owner, + repo: input.repo, + ...pair, + renamedAt: input.checkedAt, + })) +} + +function renameRows({ table, repo = 'repo', name = 'name', keep }: KeyedTable): string[] { + const where = `owner = :owner AND ${repo} = :repo AND ${name} = :from_name${keep ? ` AND NOT (${keep})` : ''}` + return [ + `UPDATE OR IGNORE ${table} SET ${name} = :to_name WHERE ${where}`, + `DELETE FROM ${table} WHERE ${where}`, + ] +} + +const RENAME_SQL: string[] = [ + // The new name is live, so it is nobody's alias. This also ends a rename back, + // and runs first so the retarget below cannot point an alias at itself. + `DELETE FROM skill_aliases WHERE owner = :owner AND repo = :repo AND name = :to_name`, + // The old name becomes an alias. A repeat of the same rename keeps the first date. + `INSERT INTO skill_aliases (owner, repo, name, target_name, renamed_at) + VALUES (:owner, :repo, :from_name, :to_name, :renamed_at) + ON CONFLICT (owner, repo, name) DO UPDATE SET + target_name = excluded.target_name, + renamed_at = CASE + WHEN skill_aliases.target_name = excluded.target_name THEN skill_aliases.renamed_at + ELSE excluded.renamed_at + END`, + // An older alias points straight at the new name, so no redirect chains. + `UPDATE skill_aliases SET target_name = :to_name + WHERE owner = :owner AND repo = :repo AND target_name = :from_name`, + + // Social posts key a Skill by its `owner/name` slug. + `UPDATE OR IGNORE skill_social_posts SET skill_slug = :owner || '/' || :to_name + WHERE skill_slug = :owner || '/' || :from_name`, + `DELETE FROM skill_social_posts WHERE skill_slug = :owner || '/' || :from_name`, + + // A collection that already lists the new name keeps one entry. + `DELETE FROM collection_skills_v2 + WHERE owner = :owner AND repo = :repo AND name = :from_name + AND EXISTS ( + SELECT 1 FROM collection_skills_v2 kept + WHERE kept.collection_id = collection_skills_v2.collection_id + AND kept.owner = :owner AND kept.repo = :repo AND kept.name = :to_name)`, + ...SKILL_TABLES.flatMap(renameRows), + + // The new name keeps the date the registry first saw the Skill. + `UPDATE skills + SET first_seen_at = MIN( + COALESCE(first_seen_at, :renamed_at), + COALESCE((SELECT old.first_seen_at FROM skills old + WHERE old.owner = :owner AND old.repo = :repo AND old.name = :from_name), :renamed_at)) + WHERE owner = :owner AND repo = :repo AND name = :to_name`, + `DELETE FROM skills WHERE owner = :owner AND repo = :repo AND name = :from_name`, + // Counters, trust, and indexability read the merged rows. + `INSERT OR REPLACE INTO skill_dirty (owner, repo, name, reason, queued_at, attempts) + VALUES (:owner, :repo, :to_name, 'skill_renamed', :renamed_at, 0)`, + // The agent sitemap lists these routes, and the old ones now answer 301. + // '0' sorts right after '/', so the range is every route under the old name. + `DELETE FROM ai_ready_pages + WHERE route = :old_route OR (route >= :old_route || '/' AND route < :old_route || '0')`, +] + +/** + * The statements of one rename, in order. Run them in the sync's final batch, + * so the rename lands with the tree cursor or not at all. + */ +export function skillRenameStatements(rename: SkillRename): MoveStatement[] { + if (rename.from.toLowerCase() === rename.to.toLowerCase()) + throw new Error(`A Skill cannot be renamed to its own name: ${rename.owner}/${rename.repo}/${rename.from}`) + return namedStatements(RENAME_SQL, { + owner: rename.owner, + repo: rename.repo, + from_name: rename.from, + to_name: rename.to, + renamed_at: rename.renamedAt, + old_route: `/gh/${rename.owner}/${rename.repo}/${rename.from}`.toLowerCase(), + }) +} diff --git a/layers/registry/server/utils/skill-run-identity.ts b/layers/registry/server/utils/skill-run-identity.ts index 62dde761..6f8d2512 100644 --- a/layers/registry/server/utils/skill-run-identity.ts +++ b/layers/registry/server/utils/skill-run-identity.ts @@ -1,4 +1,5 @@ import { findRepositoryAlias } from './repository-aliases' +import { findSkillAlias } from './skill-aliases' export interface SkillRunIdentityQuery { owner: string @@ -36,6 +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). */ export async function findSkillRunIdentity( db: D1Database, @@ -44,6 +46,9 @@ 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 diff --git a/layers/registry/server/utils/sync-repo.ts b/layers/registry/server/utils/sync-repo.ts index 9ff2652a..2dd6f87c 100644 --- a/layers/registry/server/utils/sync-repo.ts +++ b/layers/registry/server/utils/sync-repo.ts @@ -15,6 +15,7 @@ import { skillContentSha256 } from './skill-content-hash' import { parseSkillFile, registrySkillName } from './skill-frontmatter' import { isOfficialSkillRepo, scoreSkillIndexability } from './skill-indexability' import { parseSkillMd } from './skill-md-render' +import { findSkillRenames, skillRenameStatements } from './skill-rename' import { resolveSkillTrust } from './skill-trust' export interface SyncRepoStats { @@ -24,6 +25,8 @@ export interface SyncRepoStats { reason?: string skillsSeen: number skillsUpserted: number + /** Skills this sync followed to a new folder name (ADR-0017). */ + skillsRenamed?: number /** Files present upstream that GitHub will not return as text. */ skillsUnreadable: number revisionsInserted: number @@ -1575,8 +1578,15 @@ export async function syncRepo( // judgement has to wait for every chunk. Rows acknowledged by an earlier // invocation carry this run's stable last_synced_at checkpoint. const finalWrites: D1PreparedStatement[] = [] - for (const [name, skill] of existing) { - if (!seenNames.has(name) && skill.last_synced_at !== now) { + const missing = [...existing] + .filter(([name, skill]) => !seenNames.has(name) && skill.last_synced_at !== now) + .map(([name]) => name) + // A missing Skill whose body reappears under a name this sync added was + // renamed, not removed. Its rows follow it, and its old URLs redirect. + const renames = await findSkillRenames(db, { owner, repo, missing, checkedAt: now }) + const renamedFrom = new Set(renames.map(rename => rename.from)) + for (const name of missing) { + if (!renamedFrom.has(name)) { finalWrites.push(db.prepare( `UPDATE skills SET source_resolved = 0, @@ -1594,6 +1604,9 @@ export async function syncRepo( ).bind(now, now, owner, repo, name)) } } + for (const rename of renames) + finalWrites.push(...skillRenameStatements(rename).map(statement => db.prepare(statement.sql).bind(...statement.params))) + stats.skillsRenamed = renames.length // The content cursor is written last and only once every slice has committed. // A run that fails partway leaves the tree unacknowledged, so the next run diff --git a/migrations/0157_skill_renames.sql b/migrations/0157_skill_renames.sql new file mode 100644 index 00000000..f9bd1ae9 --- /dev/null +++ b/migrations/0157_skill_renames.sql @@ -0,0 +1,25 @@ +-- A Skill whose folder an author renames follows the rename (ADR-0017). One +-- sync that loses a Skill and gains another with a near-identical SKILL.md +-- body moves the old Skill's rows to the new name, and the old name stays +-- behind as an alias that answers its URLs with a 301. +-- +-- One row per old name, inside one Repository. `name` is the old registry +-- Skill name and `target_name` the Skill it became. Registry names compare +-- without case, so these do too. +-- +-- Cull path: a later rename rewrites the target, and a rename back deletes +-- the row. Dropping the table ends every redirect, so do not drop it while +-- the old URLs still receive traffic. +CREATE TABLE IF NOT EXISTS skill_aliases ( + owner TEXT NOT NULL COLLATE NOCASE, + repo TEXT NOT NULL COLLATE NOCASE, + name TEXT NOT NULL COLLATE NOCASE, + target_name TEXT NOT NULL COLLATE NOCASE, + -- Unix seconds the registry first renamed the Skill to this target. + renamed_at INTEGER NOT NULL, + PRIMARY KEY (owner, repo, name), + CHECK (name <> target_name) +); + +-- A later rename of the target rewrites every alias that points at it. +CREATE INDEX IF NOT EXISTS idx_skill_aliases_target ON skill_aliases (owner, repo, target_name); diff --git a/test/unit/skill-rename.test.ts b/test/unit/skill-rename.test.ts new file mode 100644 index 00000000..6a56f9d3 --- /dev/null +++ b/test/unit/skill-rename.test.ts @@ -0,0 +1,257 @@ +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 { syncRepo } from '../../layers/registry/server/utils/sync-repo' +import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite' + +const github = vi.hoisted(() => ({ + getBlobsBatch: vi.fn(), + getCommitsBatch: vi.fn(), + getRepoSummary: vi.fn(), + getTree: vi.fn(), + logRateLimit: vi.fn(), +})) + +vi.mock('../../layers/registry/server/utils/github-client', () => github) + +const NOW = 1_790_000_000 + +const EDIT_BODY = [ + '# Edit prose for its reader', + 'Read the complete text, request, and supplied reader preferences first.', + 'Edit only where a change solves a reading problem for this reader.', + 'Preserve distinctive vocabulary, humor, rhythm, and deliberate voice.', + 'Verify material facts and relevant assumptions before rewriting them.', + 'Never invent an actor, experience, example, or supporting detail.', +].join('\n') + +const COMPRESS_BODY = [ + '# Compress text for agents', + 'Produce the shortest clear text that preserves the source meaning.', + 'Record each distinct claim or rule and its source section first.', + 'Merge duplicates only when meaning, scope, and exceptions all match.', + 'Preserve frontmatter, code, commands, and links exactly as written.', + 'Count tokens with the target tokenizer before claiming any savings.', +].join('\n') + +const NAMING_BODY = [ + '# Recover names from user tasks', + 'Find implementation names that hide the tasks users actually perform.', + 'Write one plain sentence naming the person, the thing, and the task.', + 'Rank proposals by user impact and confidence, not by occurrence count.', +].join('\n') + +const skillMd = (name: string, body: string) => `---\nname: ${name}\ndescription: ${name} description\n---\n${body}\n` + +describe('skill rename matching', () => { + it('scores a body that kept its lines under a new frontmatter name as the same Skill', () => { + expect(skillBodySimilarity(skillMd('im-not-a-fly', EDIT_BODY), skillMd('im-part-human', EDIT_BODY))).toBe(1) + expect(skillBodySimilarity(skillMd('im-not-a-fly', EDIT_BODY), skillMd('slop-naming-recovery', NAMING_BODY))).toBe(0) + }) + + it('pairs each renamed Skill with the one it became and leaves a new Skill unpaired', () => { + const removed = [ + { name: 'im-not-a-fly', raw: skillMd('im-not-a-fly', EDIT_BODY) }, + { name: 'im-a-fly', raw: skillMd('im-a-fly', COMPRESS_BODY) }, + ] + const added = [ + { name: 'im-part-human', raw: skillMd('im-part-human', `${EDIT_BODY}\nKeep explanations the reader needs to understand.`) }, + { name: 'im-not-a-human', raw: skillMd('im-not-a-human', COMPRESS_BODY) }, + { name: 'slop-naming-recovery', raw: skillMd('slop-naming-recovery', NAMING_BODY) }, + ] + + expect(matchSkillRenames(removed, added)).toEqual([ + { from: 'im-not-a-fly', to: 'im-part-human' }, + { from: 'im-a-fly', to: 'im-not-a-human' }, + ]) + }) + + it('leaves an ambiguous pair as a removal and two additions', () => { + const removed = [{ name: 'edit', raw: skillMd('edit', EDIT_BODY) }] + const added = [ + { name: 'edit-blog', raw: skillMd('edit-blog', EDIT_BODY) }, + { name: 'edit-docs', raw: skillMd('edit-docs', EDIT_BODY) }, + ] + + expect(matchSkillRenames(removed, added)).toEqual([]) + }) + + it('leaves a body rewritten past the threshold unpaired', () => { + const removed = [{ name: 'edit', raw: skillMd('edit', EDIT_BODY) }] + const added = [{ name: 'prose', raw: skillMd('prose', `${EDIT_BODY.split('\n').slice(0, 2).join('\n')}\n${COMPRESS_BODY}`) }] + + expect(matchSkillRenames(removed, added)).toEqual([]) + }) +}) + +describe('renamed skill route', () => { + const aliases = indexSkillAliases([{ owner: 'harlan-zw', repo: 'brundlefly', name: 'im-not-a-fly', targetName: 'im-part-human' }]) + + it('sends the Skill page, its deep links, and its agent copy to the new name', () => { + expect(resolveRenamedSkillRoute('/gh/harlan-zw/brundlefly/im-not-a-fly', '?tab=files', aliases)) + .toEqual({ _tag: 'redirect', location: '/gh/harlan-zw/brundlefly/im-part-human?tab=files' }) + expect(resolveRenamedSkillRoute('/gh/Harlan-ZW/Brundlefly/IM-NOT-A-FLY/references/storytelling.md', '', aliases)) + .toEqual({ _tag: 'redirect', location: '/gh/Harlan-ZW/Brundlefly/im-part-human/references/storytelling.md' }) + expect(resolveRenamedSkillRoute('/gh/harlan-zw/brundlefly/im-not-a-fly.md', '', aliases)) + .toEqual({ _tag: 'redirect', location: '/gh/harlan-zw/brundlefly/im-part-human.md' }) + }) + + it('leaves the Repository hub and other Skills alone', () => { + expect(resolveRenamedSkillRoute('/gh/harlan-zw/brundlefly', '', aliases)).toEqual({ _tag: 'pass' }) + expect(resolveRenamedSkillRoute('/gh/harlan-zw/brundlefly/readme', '', aliases)).toEqual({ _tag: 'pass' }) + expect(resolveRenamedSkillRoute('/gh/other/brundlefly/im-not-a-fly', '', aliases)).toEqual({ _tag: 'pass' }) + }) +}) + +describe('sync follows a renamed Skill', () => { + let d1: SqliteD1 + let blobs: Map + + function exec(sql: string): void { + d1.raw.exec(sql) + } + + function rows(sql: string): T[] { + return d1.raw.prepare(sql).all() as T[] + } + + function seedSkill(name: string, body: string, options: { firstSeenAt?: number, resolved?: boolean } = {}): void { + d1.raw.prepare(` + INSERT INTO skills (owner, repo, name, display_name, slug, description, current_sha, source_resolved, + sync_status, rendered_skill_path, rendered_status, rendered_raw, first_seen_at, seo_indexable) + VALUES ('acme', 'skills', ?, ?, ?, 'Does it.', ?, ?, ?, ?, 'ok', ?, ?, 1)`) + .run(name, name, `acme/${name}`, `blob-${name}`, options.resolved === false ? 0 : 1, options.resolved === false ? 'path_missing' : 'ok', `skills/${name}/SKILL.md`, skillMd(name, body), options.firstSeenAt ?? NOW - 5000) + } + + /** GitHub serves these Skill folders, each with the body given. */ + function serveTree(skills: Record, treeSha = 'tree-2'): void { + blobs = new Map(Object.entries(skills).map(([name, body]) => [`skills/${name}/SKILL.md`, skillMd(name, body)])) + github.getRepoSummary.mockResolvedValue({ + status: 200, + data: { + repositoryId: 7, + headTreeSha: treeSha, + meta: { + name: 'skills', + full_name: 'acme/skills', + html_url: 'https://github.com/acme/skills', + owner: { login: 'acme' }, + default_branch: 'main', + description: 'Skills', + stargazers_count: 3, + forks_count: 0, + pushed_at: '2026-10-01T00:00:00Z', + created_at: '2025-01-01T00:00:00Z', + }, + }, + rateLimit: null, + notModified: false, + }) + github.getTree.mockResolvedValue({ + status: 200, + data: { sha: treeSha, tree: Object.keys(skills).map(name => ({ path: `skills/${name}/SKILL.md`, sha: `blob-${name}`, type: 'blob' })) }, + rateLimit: null, + notModified: false, + }) + } + + beforeEach(() => { + vi.useFakeTimers() + vi.setSystemTime(NOW * 1000) + vi.clearAllMocks() + d1 = createSqliteD1(allMigrations()) + exec(` + INSERT INTO users (id, github_id, login, created_at, last_login_at) VALUES (101, 101, 'user-101', 1, 1); + INSERT INTO repos (owner, repo, default_branch, stars, last_tree_sha, pushed_at, repo_meta_synced_at, repo_skill_count, repository_id) + VALUES ('acme', 'skills', 'main', 3, 'tree-1', ${NOW - 100}, ${NOW - 100}, 2, 7); + `) + github.getBlobsBatch.mockImplementation(async (_owner: string, _repo: string, _ref: string, paths: string[]) => ({ + status: 200, + data: new Map(paths.flatMap(path => blobs.has(path) ? [[path, blobs.get(path)!]] : [])), + unreadable: new Set(), + rateLimit: null, + notModified: false, + })) + github.getCommitsBatch.mockImplementation(async (_owner: string, _repo: string, paths: string[]) => ({ + status: 200, + data: new Map(paths.map(path => [path, []])), + unreadable: new Set(), + rateLimit: null, + notModified: false, + })) + }) + + afterEach(() => { + vi.useRealTimers() + d1.close() + }) + + it('moves the old Skill to its new folder name and keeps the old name as an alias', async () => { + seedSkill('edit', EDIT_BODY, { firstSeenAt: NOW - 9000 }) + seedSkill('naming', NAMING_BODY) + exec(`INSERT INTO skill_likes (user_id, owner, repo, name, created_at) VALUES (101, 'acme', 'skills', 'edit', 1)`) + serveTree({ prose: EDIT_BODY, naming: NAMING_BODY, compress: COMPRESS_BODY }) + + const stats = await syncRepo('acme', 'skills', {}, d1.db, { submitted: true }) + + expect(stats).toMatchObject({ status: 'indexed', skillsRenamed: 1 }) + expect(rows(`SELECT name, source_resolved FROM skills ORDER BY name`)).toEqual([ + { name: 'compress', source_resolved: 1 }, + { name: 'naming', source_resolved: 1 }, + { name: 'prose', source_resolved: 1 }, + ]) + expect(rows(`SELECT name, target_name, renamed_at FROM skill_aliases`)).toEqual([{ name: 'edit', target_name: 'prose', renamed_at: NOW }]) + expect(rows(`SELECT name FROM skill_likes`)).toEqual([{ name: 'prose' }]) + expect(rows(`SELECT first_seen_at FROM skills WHERE name = 'prose'`)).toEqual([{ first_seen_at: NOW - 9000 }]) + 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 () => { + 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' }) + }) + + it('turns a tombstone into an alias when the new name is admitted after the old one was swept', async () => { + seedSkill('edit', EDIT_BODY, { resolved: false }) + serveTree({ prose: EDIT_BODY }) + + await syncRepo('acme', 'skills', {}, d1.db, { submitted: true }) + + expect(rows(`SELECT name FROM skills`)).toEqual([{ name: 'prose' }]) + expect(rows(`SELECT name, target_name FROM skill_aliases`)).toEqual([{ name: 'edit', target_name: 'prose' }]) + }) + + it('ends the alias and points the other way when the author renames the folder back', async () => { + seedSkill('edit', EDIT_BODY) + serveTree({ prose: EDIT_BODY }, 'tree-2') + await syncRepo('acme', 'skills', {}, d1.db, { submitted: true }) + vi.setSystemTime((NOW + 60) * 1000) + serveTree({ edit: EDIT_BODY }, 'tree-3') + + await syncRepo('acme', 'skills', {}, d1.db, { submitted: true }) + + expect(rows(`SELECT name FROM skills`)).toEqual([{ name: 'edit' }]) + expect(rows(`SELECT name, target_name FROM skill_aliases`)).toEqual([{ name: 'prose', target_name: 'edit' }]) + }) + + it('marks an unrelated removal gone instead of renaming it', async () => { + seedSkill('edit', EDIT_BODY) + serveTree({ compress: COMPRESS_BODY }) + + const stats = await syncRepo('acme', 'skills', {}, d1.db, { submitted: true }) + + expect(stats.skillsRenamed).toBe(0) + expect(rows(`SELECT name, sync_status FROM skills ORDER BY name`)).toEqual([ + { name: 'compress', sync_status: 'ok' }, + { name: 'edit', sync_status: 'path_missing' }, + ]) + expect(rows(`SELECT name FROM skill_aliases`)).toEqual([]) + }) +}) diff --git a/test/unit/skill-run-identity.test.ts b/test/unit/skill-run-identity.test.ts index 7ee05b02..c2d399d7 100644 --- a/test/unit/skill-run-identity.test.ts +++ b/test/unit/skill-run-identity.test.ts @@ -23,6 +23,7 @@ function db() { )`) harness.raw.exec('CREATE TABLE repos (owner TEXT, repo TEXT)') harness.raw.exec(readFileSync('migrations/0146_repository_moves.sql', 'utf8')) + harness.raw.exec(readFileSync('migrations/0157_skill_renames.sql', 'utf8')) } return harness }