diff --git a/scripts/check-changed-skills.sh b/scripts/check-changed-skills.sh index 0653fb5dea7..10daac3acfd 100755 --- a/scripts/check-changed-skills.sh +++ b/scripts/check-changed-skills.sh @@ -15,6 +15,10 @@ # the legacy WARN-only posture. # Any FAIL fails this gate. # +# Every run, whatever changed, also runs scripts/check-view-medium-lists.mjs +# over every view lane in lib/view-registry.mjs: a lane's skill text that lists +# two or more medium ids fails the gate. +# # (e.g. origin/main) is resolved to `git merge-base HEAD` # (#7014), and that merge base is BOTH the changed-file diff base and # CHECK_SKILL_BASE_REF. A branch behind is thereby judged on what it @@ -31,7 +35,8 @@ # without the package. Eval-schema validation (validate-evals) is a separate CI # step — the check-jsonschema composite action over every evals.json. # -# Exit 0 = every changed skill passes (or none changed); 1 = one or more failed; +# Exit 0 = every changed skill passes (or none changed) and no view lane lists +# media; 1 = one or more failed, or a view lane lists media; # 2 = usage / environment error (fail closed — never a silent skip), or no failure # but PLUGIN_TEST_DEADLINE, which scripts/check-affected.sh --tests sets and the # checker reads, left a skill's script tests unrun, each named on a `SKIP` line. @@ -106,6 +111,20 @@ if [[ ! -f "$CHECKER" ]]; then exit 2 fi +# Every run scans every view lane, changed or not, since a stale exemption row +# shows only over the whole tree. A tree with no view registry has no lanes. +# The check reads the registry as text, so the lane form executes nothing of +# the checked tree. +medium_lists=0 +if [[ -f lib/view-registry.mjs ]]; then + [[ -f "$SCRIPT_DIR/check-view-medium-lists.mjs" ]] || { + printf 'Error: view medium-list check not found: %s\n' "$SCRIPT_DIR/check-view-medium-lists.mjs" >&2 + exit 2 + } + node "$SCRIPT_DIR/check-view-medium-lists.mjs" --root "$PWD" || medium_lists=$? + ((medium_lists <= 1)) || exit 2 +fi + # Recorded evals-warrant skips. Missing file = no skips (tests, other repos). # A stale row is a gate failure so the list can only shrink. EXEMPTIONS="${EVALS_WARRANT_EXEMPTIONS:-scripts/evals-warrant-exemptions.txt}" @@ -257,11 +276,11 @@ done if ((checked == 0)); then echo "No changed skills under plugins/*/skills/ — nothing to gate." - gate_entry::finish 0 + gate_entry::finish "$medium_lists" fi printf '\n%d skill(s) checked, %d failed.\n' "$checked" "$failed" ((left == 0)) || printf '%d skill(s) left script tests to CI, past the deadline.\n' "$left" -((failed == 0)) || gate_entry::finish 1 +((failed == 0 && medium_lists == 0)) || gate_entry::finish 1 ((left == 0)) || gate_entry::finish 2 gate_entry::finish 0 diff --git a/scripts/check-changed-skills.test.sh b/scripts/check-changed-skills.test.sh index d9f713b54ee..dc8feccc0b9 100755 --- a/scripts/check-changed-skills.test.sh +++ b/scripts/check-changed-skills.test.sh @@ -93,6 +93,31 @@ else fi rm -rf "$r" +# --- every run scans the view lanes, with or without a changed skill -------- +fixture_tree::build r --sut "$SCRIPT" --sut "$SELF_DIR/check-view-medium-lists.mjs" --git +mkdir -p "$r/lib" "$r/plugins/p1/skills/view" +printf '%s\n' 'export const MEDIUMS = Object.freeze({' ' terminal: entry("terminal"),' ' file: entry("file"),' '});' \ + 'const lane = (laneId) => laneId;' 'export const LANES = [lane("p1:view")];' >"$r/lib/view-registry.mjs" +printf '%s\n' "Set \`file\`." >"$r/plugins/p1/skills/view/SKILL.md" +commit_all "$r" base >/dev/null +b="$(base_sha "$r")" +printf 'unrelated\n' >"$r/README.md" +if out="$(run "$r" "$b" 2>&1)" && grep -q "nothing to gate" <<<"$out"; then + ok "a view lane naming one medium passes" +else + fail "a view lane naming one medium should pass, got: $out" +fi +printf '%s\n' "Choose \`terminal\` or \`file\`." >"$r/plugins/p1/skills/view/context.md" +commit_all "$r" "list media" >/dev/null +rc=0 +out="$(run "$r" "$(base_sha "$r")" 2>&1)" || rc=$? +if ((rc == 1)) && grep -qF "plugins/p1/skills/view/context.md:1: \`terminal\`, \`file\`" <<<"$out"; then + ok "a view lane listing media fails the gate though no skill changed" +else + fail "a view lane listing media should exit 1 with no changed skill (rc=$rc): $out" +fi +rm -rf "$r" + # --- one changed skill, checker passes ------------------------------------- mk_repo r add_skill "$r" p1 alpha diff --git a/scripts/check-view-medium-lists.mjs b/scripts/check-view-medium-lists.mjs new file mode 100755 index 00000000000..d24e847935e --- /dev/null +++ b/scripts/check-view-medium-lists.mjs @@ -0,0 +1,179 @@ +#!/usr/bin/env node +// Gate: a view lane's skill text names no list of media. +// +// node scripts/check-view-medium-lists.mjs [--root ] +// +// The rendered-view medium ids belong to lib/view-registry.mjs, which skills +// reach through the shared resolver. A view lane's `*.md` or `evals.json` that +// names two or more distinct medium ids, each as a backticked code span, on one +// line (or, in `evals.json`, in one JSON string) restates that registry and goes +// stale when it changes. One id on a line, such as a fixture prompt's setup, +// passes. +// +// The medium ids are the keys of `MEDIUMS` and the lanes the `lane(...)` entries, +// both read from the registry as text, never imported, so a run over a tree it +// must not execute stays a read. A `plugin:skill` lane covers +// plugins//skills// and a bare lane id plugins//. CHANGELOG.md, +// README.md and every copy scripts/shared-copies.txt lists for +// lib/rendered-view.md, which names the ids on purpose, are skipped. +// +// scripts/view-medium-list-exemptions.txt names the lanes still waiting on their +// slice, one lane id per row with a `# #` citation. A row whose lane has +// no flagged line, or names no registered lane, fails too, so the list only +// shrinks. +// +// Exit 0 clean, 1 findings, 2 usage or an unreadable input. +import { existsSync, readdirSync, readFileSync, realpathSync } from "node:fs"; +import path from "node:path"; +import process from "node:process"; +import { fileURLToPath } from "node:url"; + +const REGISTRY = "lib/view-registry.mjs"; +const EXEMPTIONS = "scripts/view-medium-list-exemptions.txt"; +const SHARED = "scripts/shared-copies.txt"; +const SHARED_SOURCE = "lib/rendered-view.md"; +const SKIP_FILES = new Set(["CHANGELOG.md", "README.md"]); +const SKIP_DIRS = new Set([".git", "node_modules"]); + +// readRegistry(text) -> { mediums: string[], lanes: string[] } +export function readRegistry(text) { + const block = /^export const MEDIUMS = Object\.freeze\(\{\r?\n([\s\S]*?)^\}\);/m.exec(text); + const mediums = block ? [...block[1].matchAll(/^\s*([A-Za-z_$][\w$]*)\s*:/gm)].map((m) => m[1]) : []; + const lanes = [...text.matchAll(/\blane\(\s*"([^"]+)"/g)].map((m) => m[1]); + return { mediums, lanes }; +} + +export const laneDir = (laneId) => { + const [plugin, skill] = laneId.split(":"); + return skill ? `plugins/${plugin}/skills/${skill}` : `plugins/${plugin}`; +}; + +// mediaIn(text, mediums) -> the distinct medium ids `text` holds as backticked code spans. +export function mediaIn(text, mediums) { + const ids = new Set(); + for (const m of text.matchAll(/`([^`\n]+)`/g)) if (mediums.has(m[1])) ids.add(m[1]); + return [...ids]; +} + +// listings(rel, text, mediums) -> [{ line, ids }] for each line, or JSON string in +// evals.json, naming two or more medium ids. +export function listings(rel, text, mediums) { + const found = []; + text.split(/\r?\n/).forEach((raw, i) => { + const units = path.basename(rel) === "evals.json" ? (raw.match(/"(?:[^"\\]|\\.)*"/g) ?? []).map(decoded) : [raw]; + for (const unit of units) { + const ids = mediaIn(unit, mediums); + if (ids.length >= 2) found.push({ line: i + 1, ids }); + } + }); + return found; +} + +function decoded(literal) { + try { + return JSON.parse(literal); + } catch { + return literal; + } +} + +function walk(root, rel, skip) { + const files = []; + for (const ent of readdirSync(path.join(root, rel), { withFileTypes: true })) { + const r = `${rel}/${ent.name}`; + if (ent.isDirectory()) { + if (!SKIP_DIRS.has(ent.name)) files.push(...walk(root, r, skip)); + } else if (ent.isFile() && !skip.has(r) && !SKIP_FILES.has(ent.name) && (ent.name.endsWith(".md") || ent.name === "evals.json")) { + files.push(r); + } + } + return files; +} + +const listRows = (file) => + existsSync(file) + ? readFileSync(file, "utf8") + .split(/\r?\n/) + .map((l) => l.trim()) + .filter((l) => l && !l.startsWith("#")) + : []; + +// collect(root) -> { flagged: Map, lanes: string[] } +export function collect(root) { + const { mediums, lanes } = readRegistry(readFileSync(path.join(root, REGISTRY), "utf8")); + if (!mediums.length || !lanes.length) throw new Error(`${REGISTRY}: found no MEDIUMS keys or no lane(...) entries`); + const skip = new Set( + listRows(path.join(root, SHARED)) + .map((l) => l.split(/\s+/)) + .filter(([src]) => src === SHARED_SOURCE) + .map(([, dest]) => dest), + ); + const ids = new Set(mediums); + const flagged = new Map(); + for (const laneId of lanes) { + const dir = laneDir(laneId); + if (!existsSync(path.join(root, dir))) continue; + for (const rel of walk(root, dir, skip).sort()) { + for (const { line, ids: hit } of listings(rel, readFileSync(path.join(root, rel), "utf8"), ids)) { + flagged.set(laneId, [...(flagged.get(laneId) ?? []), `${rel}:${line}: ${hit.map((id) => `\`${id}\``).join(", ")}`]); + } + } + } + return { flagged, lanes }; +} + +// exemptions(root) -> [{ laneId, cites }] +function exemptions(root) { + return listRows(path.join(root, EXEMPTIONS)).map((row) => { + const hash = row.indexOf("#"); + return { laneId: (hash < 0 ? row : row.slice(0, hash)).trim(), cites: hash >= 0 && /#\d+/.test(row.slice(hash + 1)) }; + }); +} + +export function main(argv, out = { log: console.log, error: console.error }) { + let root = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); + for (let i = 0; i < argv.length; i++) { + if (argv[i] === "--root" && argv[i + 1]) root = path.resolve(argv[++i]); + else { + out.error("usage: check-view-medium-lists.mjs [--root ]"); + return 2; + } + } + let result; + let rows; + try { + result = collect(root); + rows = exemptions(root); + } catch (err) { + out.error(`check-view-medium-lists: ${err.message}`); + return 2; + } + const { flagged, lanes } = result; + const findings = []; + const exempt = new Set(); + for (const { laneId, cites } of rows) { + if (!lanes.includes(laneId)) findings.push(`${EXEMPTIONS}: "${laneId}" is not a lane in ${REGISTRY}; delete the row`); + else if (!flagged.has(laneId)) findings.push(`${EXEMPTIONS}: "${laneId}" lists no media any more; delete the row`); + else if (exempt.has(laneId)) findings.push(`${EXEMPTIONS}: "${laneId}" is listed twice`); + else if (!cites) findings.push(`${EXEMPTIONS}: "${laneId}" cites no issue; add "# #" naming its open slice`); + exempt.add(laneId); + } + for (const [laneId, hits] of flagged) { + if (exempt.has(laneId)) continue; + for (const hit of hits) findings.push(`${hit} (lane ${laneId} lists media; leave the ids to ${REGISTRY})`); + } + if (findings.length) { + for (const f of findings) out.error(f); + out.error(`${findings.length} finding(s). A view lane names at most one medium id per line.`); + return 1; + } + out.log(`view-medium-lists: no view lane lists media (${exempt.size} lane(s) exempt).`); + return 0; +} + +// Node loads the entry module from its real path, so a checkout reached through a +// symlink must compare real paths, or the gate never runs and exits 0. +const self = realpathSync(fileURLToPath(import.meta.url)); +if (process.argv[1] && existsSync(process.argv[1]) && realpathSync(process.argv[1]) === self) { + process.exitCode = main(process.argv.slice(2)); +} diff --git a/scripts/check-view-medium-lists.test.mjs b/scripts/check-view-medium-lists.test.mjs new file mode 100644 index 00000000000..da6b59eca72 --- /dev/null +++ b/scripts/check-view-medium-lists.test.mjs @@ -0,0 +1,156 @@ +import assert from "node:assert/strict"; +import { mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { after, describe, test } from "node:test"; +import { laneDir, listings, main, readRegistry } from "./check-view-medium-lists.mjs"; +import { LANES, MEDIUMS } from "../lib/view-registry.mjs"; +import { tempDir } from "../lib/test-tmp.mjs"; + +const REPO = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const IDS = new Set(["terminal", "file", "artifact", "hosted"]); +const hits = (rel, text) => listings(rel, text, IDS).map((h) => `${h.line}:${h.ids.join(",")}`); + +describe("readRegistry", () => { + test("reads the ids lib/view-registry.mjs exports, without importing it", () => { + const { mediums, lanes } = readRegistry(readFileSync(path.join(REPO, "lib/view-registry.mjs"), "utf8")); + assert.deepEqual(mediums, Object.keys(MEDIUMS)); + assert.deepEqual(lanes, Object.keys(LANES)); + }); + + test("maps a plugin:skill lane to its skill and a bare id to its plugin", () => { + assert.equal(laneDir("alpha:show"), "plugins/alpha/skills/show"); + assert.equal(laneDir("beta"), "plugins/beta"); + }); +}); + +describe("listings", () => { + test("flags two distinct backticked ids on one line", () => { + assert.deepEqual(hits("SKILL.md", "Pick `terminal`, `file` or `artifact`."), ["1:terminal,file,artifact"]); + }); + + test("passes one id, one id twice, and ids written as plain words", () => { + assert.deepEqual(hits("SKILL.md", "Set `file`.\nThe `file` medium writes a `file`.\nNo artifact surface, file writing useful."), []); + }); + + test("passes a span that only contains an id", () => { + assert.deepEqual(hits("SKILL.md", "Run `medium: file` or `terminal-ui`."), []); + }); + + test("reads evals.json one JSON string at a time", () => { + const json = [ + '{"prompt": "visualize it (Fixture: the userConfig medium option is set to `artifact`.)",', + ' "expected_output": "the team file\'s `file` wins over the configured `artifact`",', + ' "expectations": ["Uses `file`", "Never `artifact`"]}', + ].join("\n"); + assert.deepEqual(hits("evals/evals.json", json), ["2:file,artifact"]); + }); +}); + +const work = tempDir("view-medium-lists-"); +after(() => rmSync(work, { recursive: true, force: true })); +let n = 0; + +const REGISTRY = [ + "export const MEDIUMS = Object.freeze({", + ' terminal: entry("terminal", false),', + ' file: entry("file", false),', + "});", + "const lane = (laneId) => laneId;", + 'export const LANES = [lane("alpha:show"), lane("beta")];', + "", +].join("\n"); + +// repo({ files, exemptions, shared }) -> root +function repo({ files = {}, exemptions, shared = [] }) { + const root = path.join(work, `repo${++n}`); + mkdirSync(path.join(root, "lib"), { recursive: true }); + mkdirSync(path.join(root, "scripts"), { recursive: true }); + writeFileSync(path.join(root, "lib/view-registry.mjs"), REGISTRY); + writeFileSync(path.join(root, "scripts/shared-copies.txt"), `# header\n${shared.join("\n")}\n`); + if (exemptions) writeFileSync(path.join(root, "scripts/view-medium-list-exemptions.txt"), `# header\n${exemptions.join("\n")}\n`); + for (const [rel, text] of Object.entries(files)) { + mkdirSync(path.dirname(path.join(root, rel)), { recursive: true }); + writeFileSync(path.join(root, rel), text); + } + return root; +} + +function gate(root) { + const lines = { log: [], error: [] }; + const code = main(["--root", root], { log: (s) => lines.log.push(s), error: (s) => lines.error.push(s) }); + return { code, out: lines.log.join("\n"), err: lines.error.join("\n") }; +} + +const LIST = "Choose `terminal` or `file`.\n"; +const SETUP = '{"evals": [{"prompt": "render it (Fixture: the medium option is set to `file`.)"}]}\n'; + +describe("main", () => { + test("fails a lane's skill markdown that lists two medium ids", () => { + const r = gate(repo({ files: { "plugins/alpha/skills/show/context/x.md": LIST } })); + assert.equal(r.code, 1); + assert.match(r.err, /plugins\/alpha\/skills\/show\/context\/x\.md:1: `terminal`, `file` \(lane alpha:show/); + }); + + test("fails a list in a bare lane's plugin, outside its skills", () => { + const r = gate(repo({ files: { "plugins/beta/reference/a.md": LIST } })); + assert.equal(r.code, 1); + assert.match(r.err, /lane beta lists media/); + }); + + test("passes a fixture prompt that names one medium as setup", () => { + const r = gate(repo({ files: { "plugins/alpha/skills/show/evals/evals.json": SETUP } })); + assert.equal(r.code, 0, r.err); + }); + + test("skips CHANGELOG.md, README.md, shared copies, other files and other skills", () => { + const r = gate( + repo({ + files: { + "plugins/beta/CHANGELOG.md": LIST, + "plugins/beta/README.md": LIST, + "plugins/beta/reference/rendered-view.md": LIST, + "plugins/beta/scripts/x.sh": LIST, + "plugins/alpha/skills/other/SKILL.md": LIST, + }, + shared: ["lib/rendered-view.md plugins/beta/reference/rendered-view.md"], + }), + ); + assert.equal(r.code, 0, r.err); + }); + + test("passes an exempt lane that still lists media", () => { + const r = gate(repo({ files: { "plugins/beta/a.md": LIST }, exemptions: ["beta # #12 beta slice"] })); + assert.equal(r.code, 0, r.err); + assert.match(r.out, /1 lane\(s\) exempt/); + }); + + test("fails an exemption row for a clean lane", () => { + const r = gate(repo({ files: { "plugins/beta/a.md": "Set `file`.\n" }, exemptions: ["beta # #12"] })); + assert.equal(r.code, 1); + assert.match(r.err, /"beta" lists no media any more; delete the row/); + }); + + test("fails an exemption row for a lane the registry does not hold", () => { + const r = gate(repo({ exemptions: ["gamma # #12"] })); + assert.equal(r.code, 1); + assert.match(r.err, /"gamma" is not a lane/); + }); + + test("fails an exemption row that cites no issue", () => { + const r = gate(repo({ files: { "plugins/beta/a.md": LIST }, exemptions: ["beta # slice"] })); + assert.equal(r.code, 1); + assert.match(r.err, /"beta" cites no issue/); + }); + + test("exits 2 when the registry yields no medium ids", () => { + const root = repo({}); + writeFileSync(path.join(root, "lib/view-registry.mjs"), "export const LANES = {};\n"); + assert.equal(gate(root).code, 2); + }); + + test("passes the current tree with its exemption list", () => { + const r = gate(REPO); + assert.equal(r.code, 0, r.err); + }); +}); diff --git a/scripts/check-view-medium-lists.test.sh b/scripts/check-view-medium-lists.test.sh new file mode 100755 index 00000000000..c564b590e7f --- /dev/null +++ b/scripts/check-view-medium-lists.test.sh @@ -0,0 +1,5 @@ +#!/usr/bin/env bash +# Discovery wrapper: scripts/run-plugin-tests.sh finds scripts/**/*.test.sh, so this +# hands off to the Node suite. +set -euo pipefail +exec node --test "$(dirname "${BASH_SOURCE[0]}")/check-view-medium-lists.test.mjs" diff --git a/scripts/view-medium-list-exemptions.txt b/scripts/view-medium-list-exemptions.txt new file mode 100644 index 00000000000..3856226c1d2 --- /dev/null +++ b/scripts/view-medium-list-exemptions.txt @@ -0,0 +1,15 @@ +# View lanes scripts/check-view-medium-lists.mjs lets list medium ids for now, +# each still waiting on its open view-medium slice of #12646. +# +# Format: one lane id from lib/view-registry.mjs per row, then `# #` +# naming the open slice. The gate FAILS on a row whose lane lists no media any +# more, or that names no registered lane, so the list can only shrink: the slice +# that removes a lane's list deletes its row. + +architecture # #12672 architecture slice +education:illustrate # #12682 education slice +education:teach # #12682 education slice +overengineering:audit # #12679 overengineering slice +prototype:explore-directions # #12684 prototype slice +prototype:pressure-test # #12684 prototype slice +visualization:visualize # #12669 visualization slice