diff --git a/.github/scripts/check-public-refs.mjs b/.github/scripts/check-public-refs.mjs new file mode 100644 index 0000000..7ef2e58 --- /dev/null +++ b/.github/scripts/check-public-refs.mjs @@ -0,0 +1,85 @@ +#!/usr/bin/env node +// +// Every repository named in this repo must be fetchable by an anonymous reader. +// +// This repo is public. Naming a repository here discloses that it exists, who owns it and +// roughly what is in it — and a prohibition discloses exactly as much as a recommendation: +// "do not re-host to acme/secret-notes, it is private" publishes the name either way. So the +// rule is about the mention, not the sentiment attached to it. +// +// The check is a request, not a list. An owner allowlist looked cheaper and was wrong on its +// first run: it cleared nothing useful and flagged `nock/nock` and `phishfort/phishfort-lists`, +// because "is this owner well known" is not the property that matters. The property is whether +// a reader who is not you can open the link — which an unauthenticated request answers exactly. +// 404 means private or absent; both are unresolvable for a public reader, and both are defects. +// +// Deliberately unauthenticated: a token would see private repos and pass them, which is the +// failure this exists to prevent. +// +// 0 every referenced repository resolves anonymously +// 1 one or more do not +// 2 could not run (offline) — reported, not silently passed +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const ROOT = process.env.SKILLS_LINT_ROOT + ? path.resolve(process.env.SKILLS_LINT_ROOT) + : path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); + +// `orgs/`, `sponsors/` and friends are github.com paths that are not repositories. +const NOT_A_REPO = new Set(['orgs', 'sponsors', 'users', 'settings', 'apps', 'topics', 'features', 'pricing']); +// Org-internal repos are private to the public but readable by colleagues, and naming them is a +// deliberate call: they are load-bearing context for the audience this repo is written for. The +// rule being enforced is about *personal* repos, which are unreachable by colleagues too. +const INTERNAL_OWNERS = new Set(['MetaMask', 'Consensys']); +// Template placeholders in contributor docs are meant to be substituted, not resolved. +const PLACEHOLDER = /^(YOUR|MY|<|\$\{)/u; +const REPO_REF = /https?:\/\/(?:www\.)?github\.com\/([A-Za-z0-9][\w.-]*)\/([A-Za-z0-9][\w.-]*)/gu; +const TEXT = /\.(md|sh|py|mjs|js|ya?ml|json|tsx?)$/u; + +function walk(dir, out = []) { + for (const e of readdirSync(dir, { withFileTypes: true })) { + if (e.name === '.git' || e.name === 'node_modules') continue; + const full = path.join(dir, e.name); + if (e.isDirectory()) walk(full, out); + else if (TEXT.test(e.name)) out.push(full); + } + return out; +} + +const refs = new Map(); // "owner/repo" -> Set of relative paths +for (const file of walk(ROOT)) { + let text; + try { text = readFileSync(file, 'utf8'); } catch { continue; } + for (const [, owner, repo] of text.matchAll(REPO_REF)) { + if (NOT_A_REPO.has(owner) || INTERNAL_OWNERS.has(owner) || PLACEHOLDER.test(owner)) continue; + const key = `${owner}/${repo.replace(/\.git$/u, '')}`; + if (!refs.has(key)) refs.set(key, new Set()); + refs.get(key).add(path.relative(ROOT, file)); + } +} + +if (refs.size === 0) { console.log('check-public-refs: no repository references found'); process.exit(0); } + +let bad = 0, unknown = 0; +for (const [key, files] of [...refs].sort()) { + let status; + try { + const res = await fetch(`https://github.com/${key}`, { method: 'HEAD', redirect: 'follow' }); + status = res.status; + } catch { + console.error(` ???? ${key} — request failed; cannot conclude`); + unknown += 1; + continue; + } + if (status === 200) continue; + bad += 1; + console.error(` FAIL ${key} — HTTP ${status} anonymously; a public reader cannot open this`); + for (const f of files) console.error(` ${f}`); +} + +console.log(`\ncheck-public-refs: ${refs.size} repository reference(s) checked`); +if (unknown > 0 && bad === 0) { console.error(`${unknown} could not be checked — offline?`); process.exit(2); } +if (bad > 0) { console.error(`${bad} unresolvable. Cite an org-owned location, or state the rule without the example.`); process.exit(1); } +console.log('every referenced repository resolves anonymously'); diff --git a/.github/scripts/lint-skill-entry.mjs b/.github/scripts/lint-skill-entry.mjs index 8de11ba..c9564d4 100644 --- a/.github/scripts/lint-skill-entry.mjs +++ b/.github/scripts/lint-skill-entry.mjs @@ -9,7 +9,7 @@ // Run against the repo: node .github/scripts/lint-skill-entry.mjs // Run against another tree: SKILLS_LINT_ROOT=/path node .github/scripts/lint-skill-entry.mjs -import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { existsSync, readFileSync, readdirSync, statSync } from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -40,8 +40,10 @@ export function lintSkill(skill) { const dirName = skill.id.slice(skill.domain.length + 1); let raw; + let source = ''; try { - raw = parseFrontmatter(readFileSync(path.join(skill.path, 'skill.md'), 'utf8')); + source = readFileSync(path.join(skill.path, 'skill.md'), 'utf8'); + raw = parseFrontmatter(source); } catch (error) { return { errors: [`could not read skill.md: ${error.message}`], warnings }; } @@ -121,9 +123,84 @@ export function lintSkill(skill) { } } + crossReferenceChecks(skill, raw, source, errors, warnings); + return { errors, warnings }; } +// Lane IDs (`B7`, `C4`) are addresses into evidence-catalog.md, not names. They carry no +// meaning to a reader who has not opened the catalog, and a `description` cannot link out +// to it — frontmatter is plain text. So: never in a description, and in the body only on a +// line that also links the catalog. The catalog's own skill is exempt: it defines them. +const LANE_ID = /(? skill.name)); + return nameCache; +} + // Restrict to skills touched by the given file paths (the CI gate passes the // PR's changed files, so pre-existing drift in untouched skills never blocks an // unrelated change). With no paths, every skill is linted (a full audit). diff --git a/.github/workflows/lint-skill-entry.yml b/.github/workflows/lint-skill-entry.yml index b774aa7..86b637d 100644 --- a/.github/workflows/lint-skill-entry.yml +++ b/.github/workflows/lint-skill-entry.yml @@ -6,7 +6,11 @@ on: - 'domains/**' - 'tools/**' - '.github/scripts/lint-skill-entry.mjs' + - '.github/scripts/check-public-refs.mjs' - '.github/workflows/lint-skill-entry.yml' + - 'test/**' + - 'CONTRIBUTING.md' + - 'README.md' permissions: contents: read @@ -64,3 +68,13 @@ jobs: run: | mapfile -d '' -t files < changed-skill-files.bin node .github/scripts/lint-skill-entry.mjs "${files[@]}" + + # Runs on the WHOLE tree, not the changed files: a private-repo reference is a + # property of what this repository publishes, and a PR that touches nothing can + # still be the moment someone notices one. Unauthenticated by construction — a + # token would see private repos and pass them, which is the failure it prevents. + - name: Every referenced repository resolves anonymously + env: + GH_TOKEN: '' + GITHUB_TOKEN: '' + run: node .github/scripts/check-public-refs.mjs diff --git a/test/lint-skill-entry.test.mjs b/test/lint-skill-entry.test.mjs index 744096c..70cd24f 100644 --- a/test/lint-skill-entry.test.mjs +++ b/test/lint-skill-entry.test.mjs @@ -268,3 +268,108 @@ describe('changed-files mode', () => { assert.match(output, /over the \d+-char budget/u); }); }); + +describe('cross-reference checks', () => { + const FM = 'name: probe\ndescription: A probe skill'; + const SECTIONS = '## When To Use\n\n- always\n\n## Workflow\n\n1. do the thing\n'; + + test('a lane id in the description fails', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'probe', 'name: probe\ndescription: Runs the B7 lane'); + const { code, output } = lint(root); + assert.equal(code, 1, output); + assert.match(output, /cites a bare lane id \(B7\)/u); + }); + + test('a lane id in the body warns, naming the line the reader sees', () => { + const root = makeRoot(); + const dir = writeSkill(root, 'testing', 'probe', FM, `${SECTIONS}\nSee B7 for details.\n`); + + // Derived by scanning the written file, not by repeating the linter's arithmetic. The + // frontmatter it strips is exactly what shifts the numbers, so a test that recomputed + // the offset the same way would agree with the defect it exists to catch. + const lines = readFileSync(path.join(dir, 'skill.md'), 'utf8').split('\n'); + const expected = lines.findIndex((line) => line.includes('See B7')) + 1; + assert.ok(expected > 1, 'the fixture must place the citation below the frontmatter'); + + const { code, output } = lint(root); + assert.equal(code, 0, output); + assert.match(output, new RegExp(`line ${expected} cites lane B7`, 'u')); + }); + + test('a lane id on a line that links the catalog is accepted', () => { + const root = makeRoot(); + const body = '## When To Use\n\n- always\n\n## Workflow\n\n1. Run B7, per [the catalog](references/evidence-catalog.md).\n'; + writeSkill(root, 'testing', 'probe', FM, body); + const { code, output } = lint(root); + assert.equal(code, 0, output); + assert.doesNotMatch(output, /cites lane/u); + }); + + test('the skill that owns the catalog may use lane ids freely', () => { + const root = makeRoot(); + const dir = writeSkill(root, 'testing', 'probe', 'name: probe\ndescription: Runs the B7 lane', `${SECTIONS}\nRun B7.\n`); + mkdirSync(path.join(dir, 'references'), { recursive: true }); + writeFileSync(path.join(dir, 'references', 'evidence-catalog.md'), '# catalog\n'); + const { code, output } = lint(root); + assert.equal(code, 0, output); + assert.doesNotMatch(output, /lane id|cites lane/u, 'the skill that defines the ids is exempt'); + }); + + test('a private-vault wiki link fails', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'probe', FM, `${SECTIONS}\nSee [[some_note]].\n`); + const { code, output } = lint(root); + assert.equal(code, 1, output); + assert.match(output, /`\[\[some_note\]\]` is a private-vault wiki link/u); + }); + + test('a nested JS array literal is not a wiki link', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'probe', FM, `${SECTIONS}\nPass \`[[signer1.address, signer2.address]]\`.\n`); + const { code, output } = lint(root); + assert.equal(code, 0, output); + assert.doesNotMatch(output, /wiki link/u, 'workflow snippets must not trip the vault-link rule'); + }); + + test('`## Related` naming a skill that does not exist warns without failing', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'probe', FM, `${SECTIONS}\n## Related\n\n- \`no-such-skill\`\n`); + const { code, output } = lint(root); + assert.equal(code, 0, output, 'a forward reference to a concurrent PR must not block the branch'); + assert.match(output, /links `no-such-skill`, which is not a skill on this branch/u); + }); + + test('`## Related` naming an existing sibling is accepted', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'sibling-skill', 'name: sibling-skill\ndescription: A sibling'); + writeSkill(root, 'testing', 'probe', FM, `${SECTIONS}\n## Related\n\n- \`sibling-skill\`\n`); + const { code, output } = lint(root); + assert.equal(code, 0, output); + assert.doesNotMatch(output, /is not a skill on this branch/u); + }); + + test('`## Related` may name the skill itself', () => { + const root = makeRoot(); + writeSkill(root, 'testing', 'probe', FM, `${SECTIONS}\n## Related\n\n- \`probe\`\n`); + const { output } = lint(root); + assert.doesNotMatch(output, /is not a skill on this branch/u); + // Guards the behaviour, not the `name !== skill.name` clause that appears to deliver + // it: the linted skill is collected from the same tree as the known-name set, so its + // own name is always in that set and the clause never decides this case. Deleting the + // clause leaves this test green — which is how it was found. + }); + + test('`## Related` ends at the next heading', () => { + const root = makeRoot(); + const body = `${SECTIONS}\n## Related\n\n- \`inside-the-section\`\n\n## Notes\n\n- \`outside-the-section\`\n`; + writeSkill(root, 'testing', 'probe', FM, body); + const { output } = lint(root); + assert.match(output, /links `inside-the-section`/u); + assert.doesNotMatch( + output, + /outside-the-section/u, + 'a backticked token after the next heading is not a Related entry', + ); + }); +}); diff --git a/tools/skill-audit.mjs b/tools/skill-audit.mjs new file mode 100644 index 0000000..8c52fd3 --- /dev/null +++ b/tools/skill-audit.mjs @@ -0,0 +1,106 @@ +#!/usr/bin/env node +// +// What actually loaded, and what published without its gate. +// +// Skill loading is not deterministic. A description is matched by a model, so "did the right +// skill load" is a question about a probabilistic event, and the only honest way to answer it +// is to look at what happened rather than at what the description says should happen. +// +// Two reports: +// loaded every skill that entered context, by route. Three routes exist and they leave +// different traces, which is why counting only one of them reads as silence. +// ungated every outward-facing publish with no gate run before it in the same session. +// This one is deterministic and is the reason the script exists: whether a gate +// ran before a write is a fact about the transcript, not a judgement. +// +// Usage: node tools/skill-audit.mjs [--json] +import { createReadStream } from 'node:fs'; +import { createInterface } from 'node:readline'; + +const [file, ...flags] = process.argv.slice(2); +if (!file) { + console.error('usage: skill-audit.mjs [--json]'); + process.exit(2); +} + +const ROUTES = [ + // Skill tool call — the explicit path. + [/"skill"\s*:\s*"([a-z0-9-]+)"/g, 'skill-tool'], + // Slash command injected into the turn. + [/\/?([a-z0-9-]+)<\/command-name>/g, 'slash-command'], + // Description match / directory scope: the loader announces where it read the file from. + [/Base directory for this skill:[^\n"]*?skills\/([a-z0-9-]+)/g, 'auto-load'], +]; + +// An outward-facing write. Deliberately broader than the porcelain: `gh api` with a body +// field is the path that bypasses `gh pr comment`, and it is the one that got used. +const PUBLISH = /gh\s+(?:pr|issue)\s+(?:comment|edit|create)\b|gh\s+api\b[^"']*(?:-F|-f|--field|--raw-field)\s+body=/; +const GATE = /attest-gate\.sh|pr-evidence-gate\.py/; + +const loaded = new Map(); +const events = []; +let line = 0; + +const rl = createInterface({ input: createReadStream(file), crlfDelay: Infinity }); +for await (const raw of rl) { + line += 1; + for (const [re, route] of ROUTES) { + re.lastIndex = 0; + let m; + while ((m = re.exec(raw)) !== null) { + const key = `${m[1]} (${route})`; + loaded.set(key, (loaded.get(key) ?? 0) + 1); + } + } + const isGate = GATE.test(raw); + const isPublish = PUBLISH.test(raw); + if (isGate) events.push({ line, kind: 'gate' }); + // Chained means the gate and the write are one command, so the shell enforces the + // ordering. A gate that merely ran EARLIER proves nothing: the verdict can be read after + // the write, or not read at all, which is how a blocked artifact reached a public PR in + // the session this script was written from. + if (isPublish) events.push({ line, kind: 'publish', chained: isGate }); +} + +// A publish is gated if a gate invocation appears earlier in the transcript. This is +// deliberately generous — same session, any distance — because the failure it looks for is +// "no gate at all", and a stricter window would produce arguments about proximity rather +// than findings. +let lastGate = -1; +const ungated = []; +const unchained = []; +for (const e of events) { + if (e.kind === 'gate') { lastGate = e.line; continue; } + if (lastGate < 0) ungated.push(e.line); + if (!e.chained) unchained.push(e.line); +} + +const report = { + transcript: file, + loaded: Object.fromEntries([...loaded].sort((a, b) => b[1] - a[1])), + publishes: events.filter((e) => e.kind === 'publish').length, + gateRuns: events.filter((e) => e.kind === 'gate').length, + ungatedPublishLines: ungated, + unchainedPublishLines: unchained, +}; + +if (flags.includes('--json')) { + console.log(JSON.stringify(report, null, 2)); +} else { + console.log(`skill-audit: ${file}\n`); + console.log('loaded:'); + for (const [k, v] of Object.entries(report.loaded)) console.log(` ${String(v).padStart(4)} ${k}`); + if (!Object.keys(report.loaded).length) console.log(' (none)'); + console.log(`\npublishes: ${report.publishes} gate runs: ${report.gateRuns}`); + console.log(`unchained publishes: ${unchained.length} of ${report.publishes}`); + if (ungated.length) { + console.log(`\nUNGATED (${ungated.length}) — no gate ran at all before these:`); + for (const l of ungated.slice(0, 10)) console.log(` line ${l}`); + } + if (unchained.length) { + console.log(`\nUNCHAINED (${unchained.length}) — a gate ran earlier, but not as the same`); + console.log('command, so nothing forced the write to depend on its verdict:'); + for (const l of unchained.slice(0, 10)) console.log(` line ${l}`); + } +} +process.exit(ungated.length || unchained.length ? 1 : 0);