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'] }]) + }) +})