From 2336f914ab532f7a7b02de27a7f3b9dc7721d503 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Tue, 13 Oct 2026 13:01:44 +1100 Subject: [PATCH] fix(registry): never link a Skill whose source is gone A Skill whose SKILL.md is gone upstream answers 410. #389 kept the Skill page from linking such a sibling, but the file page render still read every Skill name in the repository. Related Skills and the featured sections on /skills read Skill cards without the source check too. The Skill page and the file page now share one read of linkable sibling names. The registry card reads share one linkable Skill predicate. The asset cache key moves to v6 so cached renders drop stale links. --- .../server/api/skill-asset/[...slug].get.ts | 18 +++---- .../server/api/skills/[...slug].get.ts | 14 ++---- .../server/utils/skill-stored-source.ts | 16 +++++++ .../registry/server/utils/skills-registry.ts | 29 +++++++----- test/unit/skill-asset-tree-outage.test.ts | 17 +++++++ .../skill-links-skip-gone-sources.test.ts | 47 +++++++++++++++++++ 6 files changed, 107 insertions(+), 34 deletions(-) create mode 100644 test/unit/skill-links-skip-gone-sources.test.ts diff --git a/layers/registry/server/api/skill-asset/[...slug].get.ts b/layers/registry/server/api/skill-asset/[...slug].get.ts index 9f84bb9e..814fd60b 100644 --- a/layers/registry/server/api/skill-asset/[...slug].get.ts +++ b/layers/registry/server/api/skill-asset/[...slug].get.ts @@ -4,7 +4,7 @@ import { normalizeSkillAssetFilePath } from '#shared/skill-asset-path' import { resolveRepoSourceIdentityFromRow } from '../../utils/repo-source-identity' import { skillImagePolicyForEvent } from '../../utils/skill-image-policy' import { parseSkillMd } from '../../utils/skill-md-render' -import { loadStoredSkillRow, parseStoredAssets, resolveReferencedFileTarget } from '../../utils/skill-stored-source' +import { loadLinkableRepoSkillNames, loadStoredSkillRow, parseStoredAssets, resolveReferencedFileTarget } from '../../utils/skill-stored-source' import { findSkill } from '../../utils/skills-registry' import { fetchUpstreamText } from '../../utils/upstream-text' @@ -22,10 +22,6 @@ interface AssetCache { skillPath: string | null } -interface RepoSkillNameRow { - name: string -} - interface RegisteredAsset { path: string size: number @@ -70,12 +66,9 @@ export default defineApiHandler({ if (!skill) throw createError({ statusCode: 404, message: 'Skill not found' }) - const [row, repoSkillRows] = await Promise.all([ + const [row, repoSkillNames] = await Promise.all([ loadStoredSkillRow(platform.db, skill), - platform.db - .prepare(`SELECT name FROM skills WHERE owner = ? AND repo = ? ORDER BY name`) - .bind(skill.owner, skill.repo) - .all(), + loadLinkableRepoSkillNames(platform.db, skill), ]) if (!row) @@ -107,7 +100,8 @@ export default defineApiHandler({ const branch = row.rendered_commit_sha! const skillMdPath = row.rendered_skill_path! const skillDir = skillMdPath.replace(/(?:^|\/)SKILL\.md$/i, '') - const cacheKey = `skills:asset:v5:${source.owner}/${source.repo}/${skill.name}:${filePath}:${branch}` + // v6: v5 renders could link a sibling Skill whose source is gone. + const cacheKey = `skills:asset:v6:${source.owner}/${source.repo}/${skill.name}:${filePath}:${branch}` const cached = await readCache(useStorage('edge-cache'), cacheKey) if (cached) { if (cached.status === 'missing') @@ -151,7 +145,7 @@ export default defineApiHandler({ branch, skillDir, filePath, - skillNames: (repoSkillRows.results ?? []).map(candidate => candidate.name), + skillNames: repoSkillNames, registryOwner: skill.owner, registryRepo: skill.repo, }, await skillImagePolicyForEvent(event)) diff --git a/layers/registry/server/api/skills/[...slug].get.ts b/layers/registry/server/api/skills/[...slug].get.ts index 4515edd7..61db4ffa 100644 --- a/layers/registry/server/api/skills/[...slug].get.ts +++ b/layers/registry/server/api/skills/[...slug].get.ts @@ -19,6 +19,7 @@ import { getGeneratedKinds } from '../../utils/skill-generated' import { skillImagePolicyForEvent } from '../../utils/skill-image-policy' import { readRepositoryLicense } from '../../utils/skill-license' import { parseSkillMd } from '../../utils/skill-md-render' +import { loadLinkableRepoSkillNames } from '../../utils/skill-stored-source' import { findDuplicateGroupForSkill, findSkillWithRow } from '../../utils/skills-registry' import { tagLinkPath } from '../../utils/tag-quality' import { isSkillIndexable, noteAdmissionFallback, SKILL_INDEX_INPUT_COLUMNS_SQL } from '../../utils/trending-admission' @@ -207,10 +208,6 @@ interface SkillDetailRow { latest_revision_sha: string | null } -interface RepoSkillNameRow { - name: string -} - const skillDetailHandler = defineApiHandler({ response: SkillDetailResponseSchema, handler: async ({ event, platform }) => { @@ -242,14 +239,10 @@ async function loadSkillDetail(event: H3Event, platform: Platform, slug: string) const { skill, row } = found const curators: CuratorEndorsement[] = [] - const [duplicateGroup, generated, repoSkillRows] = await Promise.all([ + const [duplicateGroup, generated, repoSkillNames] = await Promise.all([ findDuplicateGroupForSkill(event, `${skill.owner}/${skill.repo}/${skill.name}`), getGeneratedKinds(platform.db, { owner: skill.owner, repo: skill.repo, name: skill.name }, ['faq', 'tags', 'summary']), - // Dependency links must only target Skills whose source still exists. - platform.db - .prepare(`SELECT name FROM skills WHERE owner = ? AND repo = ? AND source_resolved = 1 ORDER BY name`) - .bind(skill.owner, skill.repo) - .all(), + loadLinkableRepoSkillNames(platform.db, skill), ]) const faqRow = generated.get('faq') as { payload: FaqPayload } | undefined const tagRow = generated.get('tags') as { payload: TagPayload } | undefined @@ -258,7 +251,6 @@ async function loadSkillDetail(event: H3Event, platform: Platform, slug: string) const source = resolveRepoSourceIdentityFromRow(skill, row) const githubUrl = `https://github.com/${source.owner}/${source.repo}` const branch = row.rendered_commit_sha || row.default_branch || 'main' - const repoSkillNames = (repoSkillRows.results ?? []).map(candidate => candidate.name) // Warm path: render is in D1. Cold path (no usable stored render): render // live so the page still has content. A page view never writes the render diff --git a/layers/registry/server/utils/skill-stored-source.ts b/layers/registry/server/utils/skill-stored-source.ts index 6b4e12f1..a766b51c 100644 --- a/layers/registry/server/utils/skill-stored-source.ts +++ b/layers/registry/server/utils/skill-stored-source.ts @@ -52,6 +52,22 @@ export async function loadStoredSkillRow( .first() } +/** + * The Skill names in one repository that a rendered document may link. + * + * A Markdown render turns a `/name` mention of a sibling Skill into a link. + * A Skill whose SKILL.md is gone upstream answers 410, so its name stays out. + * The Skill page and each file page share this read, so neither can link a + * sibling the other keeps as text. + */ +export async function loadLinkableRepoSkillNames(db: D1Database, skill: RepoIdentity): Promise { + const rows = await db + .prepare(`SELECT name FROM skills WHERE owner = ? AND repo = ? AND source_resolved = 1 ORDER BY name`) + .bind(skill.owner, skill.repo) + .all<{ name: string }>() + return (rows.results ?? []).map(row => row.name) +} + export type StoredSkillMd = | { _tag: 'ok', body: string, source: string } | { _tag: 'gone' } diff --git a/layers/registry/server/utils/skills-registry.ts b/layers/registry/server/utils/skills-registry.ts index ad19563f..dd8c86e4 100644 --- a/layers/registry/server/utils/skills-registry.ts +++ b/layers/registry/server/utils/skills-registry.ts @@ -19,6 +19,13 @@ import { SUPPORTED_SKILL_SQL } from './supported-sources' import { noteAdmissionFallback, SKILL_ADMISSIONS_POPULATED_SQL, SKILL_INDEXABLE_SQL } from './trending-admission' const NOT_BROKEN_SQL = notBrokenSql('r') +/** + * A Skill that a page may link. A Skill whose source is gone upstream + * (`source_resolved = 0`) answers 410 (server/middleware/skill-source-gone.ts), + * so a link to it is a broken internal link. Listings, related Skills, and + * featured sections read through this one predicate. + */ +const LINKABLE_SKILL_SQL = `(${NOT_BROKEN_SQL} AND s.source_resolved = 1)` const NOT_AGGREGATOR_SQL = notAggregatorSql('r') const FROM_SKILLS_JOIN_REPOS = `FROM skills s ${JOIN_REPOS_SQL}` const SELECT_SKILL_ROW_BASE = ` @@ -250,13 +257,10 @@ export async function querySkills(event: H3Event, opts: SkillsQuery): Promise l.packageName))] const placeholders = uniqueNames.map(() => '?').join(',') - const brokenClause = opts.includeBroken ? '' : ` AND ${NOT_BROKEN_SQL}` + const brokenClause = opts.includeBroken ? '' : ` AND ${LINKABLE_SKILL_SQL}` const supportedClause = opts.supportedOnly ? ` AND (${SUPPORTED_SKILL_SQL})` : '' const rows = await db .prepare(`SELECT ${SELECT_SKILL_ROW} ${FROM_SKILLS_JOIN_REPOS} WHERE s.name IN (${placeholders})${brokenClause}${supportedClause}`) @@ -703,7 +710,7 @@ export async function getFeaturedOfficialSections( ROW_NUMBER() OVER (PARTITION BY s.owner, s.repo ORDER BY s.modified_at DESC, s.name ASC) AS rn, COUNT(*) OVER (PARTITION BY s.owner, s.repo) AS repo_total ${FROM_SKILLS_JOIN_REPOS} - WHERE (${filter.sql}) AND ${NOT_BROKEN_SQL} + WHERE (${filter.sql}) AND ${LINKABLE_SKILL_SQL} ) ranked WHERE rn <= ?`, ) .bind(...filter.params, perOrg) @@ -964,12 +971,12 @@ export async function findRelatedSkills( AND repo_skills.repo = ?2 AND repo_skills.source_resolved = 1 ) AS repo_skill_count FROM ( - SELECT ${SELECT_SKILL_ROW_BASE} ${FROM_SKILLS_JOIN_REPOS} WHERE s.owner = ?1 AND s.repo = ?2 AND s.name != ?3 AND s.source_resolved = 1 AND ${NOT_BROKEN_SQL} ORDER BY s.modified_at DESC, s.name ASC LIMIT ?4 + SELECT ${SELECT_SKILL_ROW_BASE} ${FROM_SKILLS_JOIN_REPOS} WHERE s.owner = ?1 AND s.repo = ?2 AND s.name != ?3 AND ${LINKABLE_SKILL_SQL} ORDER BY s.modified_at DESC, s.name ASC LIMIT ?4 ) paged ORDER BY modified_at DESC, name ASC`) .bind(owner, repo, excludeName, limit), db .prepare(`SELECT paged.*, ${repoSkillCountSql('paged')} FROM ( - SELECT ${SELECT_SKILL_ROW_BASE} ${FROM_SKILLS_JOIN_REPOS} WHERE s.owner = ? AND NOT (s.repo = ?) AND s.name != ? AND s.source_resolved = 1 AND ${NOT_BROKEN_SQL} ORDER BY r.stars DESC, s.modified_at DESC, s.name ASC LIMIT ? + SELECT ${SELECT_SKILL_ROW_BASE} ${FROM_SKILLS_JOIN_REPOS} WHERE s.owner = ? AND NOT (s.repo = ?) AND s.name != ? AND ${LINKABLE_SKILL_SQL} ORDER BY r.stars DESC, s.modified_at DESC, s.name ASC LIMIT ? ) paged ORDER BY stars DESC, modified_at DESC, name ASC`) .bind(owner, repo, excludeName, limit), ]) diff --git a/test/unit/skill-asset-tree-outage.test.ts b/test/unit/skill-asset-tree-outage.test.ts index e0a6a42c..6b0a3e28 100644 --- a/test/unit/skill-asset-tree-outage.test.ts +++ b/test/unit/skill-asset-tree-outage.test.ts @@ -98,6 +98,23 @@ describe('skill-asset source gone', () => { }) }) +describe('skill-asset Skill references', () => { + it('links a live sibling Skill and keeps a removed one as text', async () => { + fixture.raw.prepare( + `INSERT INTO skills (owner, repo, name, slug, display_name, source_resolved) + VALUES ('owner', 'repo', 'live', 'owner/live', 'Live', 1), + ('owner', 'repo', 'removed', 'owner/removed', 'Removed', 0)`, + ).run() + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(new Response('Run `/live`, then `/removed`.'))) + const handler = (await import('../../layers/registry/server/api/skill-asset/[...slug].get')).default + + const body = await handler(event()) as { html: string } + + expect([...body.html.matchAll(/href="([^"]+)"/g)].map(match => match[1])).toEqual(['/gh/owner/repo/live']) + expect(body.html).toContain('/removed') + }) +}) + function event(): H3Event { return { method: 'GET', diff --git a/test/unit/skill-links-skip-gone-sources.test.ts b/test/unit/skill-links-skip-gone-sources.test.ts new file mode 100644 index 00000000..c210254e --- /dev/null +++ b/test/unit/skill-links-skip-gone-sources.test.ts @@ -0,0 +1,47 @@ +import type { H3Event } from 'h3' +import type { SqliteD1 } from './helpers/d1-sqlite' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { findSkillsByLookups, getFeaturedOfficialSections } from '../../layers/registry/server/utils/skills-registry' +import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite' + +// A Skill whose SKILL.md is gone upstream answers 410. Every page that links +// it carries a broken internal link, so a Skill card read must skip it. + +let fixture: SqliteD1 + +beforeEach(() => { + fixture = createSqliteD1(allMigrations()) + fixture.raw.exec(` + INSERT INTO repos (owner, repo, stars) VALUES ('acme', 'skills', 10); + INSERT INTO skills (owner, repo, name, slug, display_name, source_resolved, modified_at) VALUES + ('acme', 'skills', 'live', 'acme/live', 'Live', 1, 1), + ('acme', 'skills', 'removed', 'acme/removed', 'Removed', 0, 2); + `) +}) + +afterEach(() => fixture.close()) + +function event(): H3Event { + return { context: { platform: { db: fixture.db } } } as unknown as H3Event +} + +describe('skill card reads skip a Skill whose source is gone', () => { + it('resolves related Skill names to live Skills only', async () => { + const found = await findSkillsByLookups(event(), [{ packageName: 'live' }, { packageName: 'removed' }]) + + expect([...found.keys()]).toEqual(['live']) + }) + + it('still resolves a removed Skill for the CLI name lookup', async () => { + const found = await findSkillsByLookups(event(), [{ packageName: 'removed' }], { includeBroken: true }) + + expect(found.get('removed')?.repo).toBe('skills') + }) + + it('features live Skills only and counts only those', async () => { + const sections = await getFeaturedOfficialSections(event(), [{ owner: 'acme', repo: 'skills' }], 5) + + expect(sections.map(section => ({ total: section.totalSkills, names: section.skills.map(skill => skill.name) }))) + .toEqual([{ total: 1, names: ['live'] }]) + }) +})