Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions .github/scripts/check-public-refs.mjs
Original file line number Diff line number Diff line change
@@ -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');
81 changes: 79 additions & 2 deletions .github/scripts/lint-skill-entry.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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 };
}
Expand Down Expand Up @@ -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 = /(?<![\w-])[A-G]\d(?![\w-])/u;
const CATALOG = 'evidence-catalog';

// `## Related` is by convention a list of sibling skills, so every backticked kebab-case
// token in it must name one. This is what catches a rename that swept the owning branch
// and left every branch that referenced it pointing at a name that no longer resolves.
// `$(?![\s\S])`, not `\z` — JS has no absolute-end anchor, and under `m` a bare `$` would
// stop the section at its own first line break.
const RELATED_SECTION = /^#{1,4}\s+Related\s*$([\s\S]*?)(?=^#{1,4}\s|$(?![\s\S]))/imu;
const BACKTICKED = /`([a-z][a-z0-9]*(?:-[a-z0-9]+)+)`/gu;

function crossReferenceChecks(skill, raw, source, errors, warnings) {
const ownsCatalog = existsSync(path.join(skill.path, 'references', `${CATALOG}.md`));

if (!ownsCatalog) {
if (raw.description && LANE_ID.test(raw.description)) {
errors.push(
`\`description\` cites a bare lane id (${raw.description.match(LANE_ID)[0]}); frontmatter cannot link ${CATALOG}.md, so name the category instead of indexing it`,
);
}
// `skill.body` is frontmatter-stripped, so its indices are not the line numbers a reader
// sees when they open the file. Offset by the stripped prefix — a lint message that
// points at the wrong line is the same unresolvable-reference defect this rule exists
// to catch.
const bodyLines = skill.body.split('\n');
// Locate the body in the source rather than subtracting line counts: the two differ by
// trailing-newline handling, which silently shifts every reported line by one.
const start = source.indexOf(skill.body);
const offset = start < 0 ? 0 : source.slice(0, start).split('\n').length - 1;
for (const [index, line] of bodyLines.entries()) {
const hit = line.match(LANE_ID);
if (hit && !line.includes(CATALOG)) {
warnings.push(
`line ${index + 1 + offset} cites lane ${hit[0]} without linking ${CATALOG}.md; an unresolvable index reads as noise`,
);
}
}
}

// `[[snake_case]]` is wiki-link syntax from a private authoring vault. It renders as
// literal brackets on GitHub and resolves nowhere for any reader here. Underscores are
// what separate it from JS array literals (`[[signer1.address, …]]`), which are common
// in workflow snippets and must not trip this.
for (const [, link] of skill.body.matchAll(/\[\[([a-z][a-z0-9]*(?:_[a-z0-9]+)+)\]\]/gu)) {
errors.push(`\`[[${link}]]\` is a private-vault wiki link; it resolves for no reader here — use a real path or URL`);
}

const related = skill.body.match(RELATED_SECTION);
if (related) {
const known = knownSkillNames();
for (const [, name] of related[1].matchAll(BACKTICKED)) {
if (!known.has(name) && name !== skill.name) {
// Warning, not error: the gate runs on the PR's own branch, where a sibling skill
// that ships in a concurrent PR does not exist yet. Blocking would fail a PR for a
// forward reference that resolves on merge.
warnings.push(`\`## Related\` links \`${name}\`, which is not a skill on this branch (renamed, removed, or still in an open PR?)`);
}
}
}
}

let nameCache;
function knownSkillNames() {
// `sources` is an array of roots; passing a bare string iterates its characters and
// silently yields zero skills, which would make every check below vacuously pass.
nameCache ??= new Set(collectSkills([ROOT]).map((skill) => 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).
Expand Down
14 changes: 14 additions & 0 deletions .github/workflows/lint-skill-entry.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
105 changes: 105 additions & 0 deletions test/lint-skill-entry.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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',
);
});
});
Loading