diff --git a/package.json b/package.json index 58eadab09..f04770584 100644 --- a/package.json +++ b/package.json @@ -122,7 +122,7 @@ "lint:frontmatter-scalar-broad-grep": "node scripts/lint-frontmatter-scalar-broad-grep.cjs", "lint:removed-but-needed": "node scripts/lint-removed-but-needed.cjs", "lint:response-language": "node scripts/lint-response-language-coverage.cjs", - "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-portable-timeout.cjs && node scripts/lint-portable-grep.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-tests.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-unreachable-guard-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-slug-derivation-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs && node scripts/lint-vendored-deps.cjs && node scripts/lint-docs-guard-registration.cjs && node scripts/lint-source-test-name-collision.cjs && npm run lint:hooks-runtime-build-seam && node scripts/check-contract-drift.cjs && node scripts/lint-mutation-test-derivation-drift.cjs && node scripts/lint-seam-enforcement.cjs && node scripts/lint-workflow-shellcheck.cjs && npm run lint:response-language", + "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-portable-timeout.cjs && node scripts/lint-portable-grep.cjs && node scripts/lint-allowed-tools-parity.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-tests.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-unreachable-guard-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-slug-derivation-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs && node scripts/lint-vendored-deps.cjs && node scripts/lint-docs-guard-registration.cjs && node scripts/lint-source-test-name-collision.cjs && npm run lint:hooks-runtime-build-seam && node scripts/check-contract-drift.cjs && node scripts/lint-mutation-test-derivation-drift.cjs && node scripts/lint-seam-enforcement.cjs && node scripts/lint-workflow-shellcheck.cjs && npm run lint:response-language", "lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs", "lint:regression-names": "node scripts/lint-regression-test-names.cjs", "lint:descriptions": "node scripts/lint-descriptions.cjs", diff --git a/scripts/lint-allowed-tools-parity.cjs b/scripts/lint-allowed-tools-parity.cjs new file mode 100644 index 000000000..a554e0310 --- /dev/null +++ b/scripts/lint-allowed-tools-parity.cjs @@ -0,0 +1,221 @@ +#!/usr/bin/env node +'use strict'; + +/** + * lint-allowed-tools-parity.cjs — catch `allowed-tools` frontmatter that + * declares `Bash` but omits `Grep` (#4394, follow-up to #3085). + * + * ## Why + * + * `gen-plugin-skills.cjs --check` already guarantees `skills/*` /SKILL.md` + * matches byte-for-byte what `commands/gsd/*.md` generates, so the two trees + * cannot silently diverge FROM EACH OTHER. Nothing guarded the shape #3085 + * actually found: a command shipping `Bash` without `Grep` purely by + * omission, identical in both trees and therefore invisible to a parity + * check that only compares them to each other. + * + * That drift ran long enough for 29 of 71 skills to lack a tool most of + * their siblings already declared, and it surfaced through a manual audit + * rather than any gate. A command that can shell out but cannot Grep does + * not fail loudly — it quietly reaches for `Bash` + `grep` instead, which is + * slower, less structured, and (per `lint-portable-grep.cjs`) a portability + * hazard of its own on hosts without GNU grep. + * + * ## The rule + * + * A command whose `allowed-tools` includes `Bash` must also include `Grep`, + * unless it is on the exemption list below. + * + * Detection only. This lint never edits a command's `allowed-tools`. + * + * ## Why an exemption list rather than a heuristic + * + * The alternative — inferring from a command's body whether it "really" + * needs Grep — would make the rule's verdict depend on prose that changes + * constantly, and produce a lint whose failures nobody can predict. A short + * literal list keeps every exemption a reviewable one-line diff, and the + * staleness check below stops it becoming a dumping ground: an entry that no + * longer needs to be there fails just as loudly as a missing tool. + */ + +const fs = require('fs'); +const path = require('path'); +const { ExitError, runMain } = require('./lib/cli-exit.cjs'); + +const ROOT = path.resolve(__dirname, '..'); +const DEFAULT_ROOT = 'commands/gsd'; + +/** + * Commands allowed to declare `Bash` without `Grep`. + * + * Keyed by command stem (the filename without `.md`), with the reason inline + * so changing the set is a one-line, reviewable diff. + * + * #4394 named eight candidates from the #3085 review: the six `gsd-ns-*` + * namespace dispatchers, `gsd-help`, and `gsd-surface`. Seven of those turn + * out not to need an entry at all — they do not declare `Bash` in the first + * place, so the rule never reaches them: + * + * ns-context/ns-ideate/ns-manage/ns-project/ns-review/ns-workflow Read + Skill + * help Read + * + * Listing them anyway would be seven pre-forgiven commands: the day one of + * them gained `Bash`, the omission it was granted an exemption for would + * pass silently. The staleness check in `scan()` below is what surfaced + * this, and it is why the list stays minimal — an exemption is a real + * suppression, not documentation. + */ +const EXEMPT = new Map([ + // Toggles which skills are surfaced. Mutates the install tree through the + // installer's own seams rather than by searching project files, so `Bash` + // here is not standing in for a search it cannot perform. + ['surface', 'mutates the install surface via installer seams, not by searching the project'], +]); + +/** + * Parse an `allowed-tools` frontmatter value. + * + * Handles both shapes the corpus uses: a YAML block sequence + * + * allowed-tools: + * - Read + * - Bash + * + * and an inline scalar or flow sequence (`allowed-tools: Read, Bash` / + * `allowed-tools: [Read, Bash]`). Returns `null` when the key is absent, + * which is NOT a violation — a command with no `allowed-tools` at all + * declares no `Bash` either, so this rule has nothing to say about it. + * + * Deliberately not a YAML parser: the frontmatter here is a fixed, shallow + * shape, and pulling in a parser to read one list would be a dependency + * bought for a single key. + * + * @param {string} text full file contents + * @returns {string[] | null} declared tool names, or null when the key is absent + */ +function parseAllowedTools(text) { + const lines = String(text).split(/\r?\n/); + const keyIndex = lines.findIndex((l) => /^allowed-tools:/.test(l)); + if (keyIndex === -1) return null; + + const inline = lines[keyIndex].replace(/^allowed-tools:/, '').trim(); + if (inline) { + return inline + .replace(/^\[/, '') + .replace(/\]$/, '') + .split(',') + .map((s) => s.trim().replace(/^['"]|['"]$/g, '')) + .filter(Boolean); + } + + const tools = []; + for (let i = keyIndex + 1; i < lines.length; i += 1) { + const line = lines[i]; + const item = line.match(/^\s*-\s+(.+?)\s*$/); + if (item) { + tools.push(item[1].replace(/^['"]|['"]$/g, '')); + continue; + } + // The first line that is neither a list item nor blank ends the block — + // the next frontmatter key, or the closing `---`. + if (line.trim() === '') continue; + break; + } + return tools; +} + +/** + * List command stems under `dir`, sorted. + * + * @param {string} dir absolute path to the commands directory + * @returns {{ stem: string, file: string }[]} + */ +function listCommands(dir) { + let entries; + try { + entries = fs.readdirSync(dir, { withFileTypes: true }); + } catch { + return []; + } + return entries + .filter((e) => e.isFile() && e.name.endsWith('.md')) + .map((e) => ({ stem: e.name.replace(/\.md$/, ''), file: path.join(dir, e.name) })) + .sort((a, b) => a.stem.localeCompare(b.stem)); +} + +/** + * Evaluate the corpus. + * + * Reports two independent failure classes, because an exemption list that + * can only ever grow rots into a list of things nobody re-examined: + * + * - `violations`: declares Bash, omits Grep, not exempt. + * - `staleExemptions`: on the list but no longer needs to be — either the + * command is gone, or it no longer declares Bash without Grep. Removing + * the entry is then a one-line diff, and the next omission in that + * command is caught rather than silently pre-forgiven. + * + * @param {string} [root] repo-relative commands directory + * @returns {{ scanned: number, violations: {stem: string, tools: string[]}[], staleExemptions: {stem: string, reason: string}[] }} + */ +function scan(root = DEFAULT_ROOT) { + const abs = path.isAbsolute(root) ? root : path.join(ROOT, root); + const commands = listCommands(abs); + const violations = []; + const seen = new Set(); + + for (const { stem, file } of commands) { + const tools = parseAllowedTools(fs.readFileSync(file, 'utf8')); + if (!tools) continue; + if (!tools.includes('Bash') || tools.includes('Grep')) continue; + seen.add(stem); + if (EXEMPT.has(stem)) continue; + violations.push({ stem, tools }); + } + + const present = new Set(commands.map((c) => c.stem)); + const staleExemptions = []; + for (const [stem, reason] of EXEMPT) { + if (!present.has(stem)) { + staleExemptions.push({ stem, reason: `no such command (${reason})` }); + } else if (!seen.has(stem)) { + staleExemptions.push({ stem, reason: `no longer declares Bash without Grep (${reason})` }); + } + } + + return { scanned: commands.length, violations, staleExemptions }; +} + +function main() { + const root = process.env.GSD_LINT_ALLOWED_TOOLS_ROOT || DEFAULT_ROOT; + const { scanned, violations, staleExemptions } = scan(root); + + if (violations.length > 0 || staleExemptions.length > 0) { + const parts = []; + if (violations.length > 0) { + parts.push( + 'lint-allowed-tools-parity: these commands declare `Bash` but not `Grep` (#4394).\n' + + 'A command that can shell out but cannot Grep reaches for `Bash` + `grep` instead —\n' + + 'slower, unstructured, and a portability hazard on hosts without GNU grep. Add `Grep`\n' + + 'to the frontmatter, or add the command to EXEMPT in this script with a reason:\n' + + violations.map((v) => ` ${root}/${v.stem}.md [${v.tools.join(', ')}]`).join('\n'), + ); + } + if (staleExemptions.length > 0) { + parts.push( + 'lint-allowed-tools-parity: these EXEMPT entries are stale — delete them so the next\n' + + 'omission in those commands is caught rather than silently pre-forgiven:\n' + + staleExemptions.map((s) => ` ${s.stem}: ${s.reason}`).join('\n'), + ); + } + throw new ExitError(1, parts.join('\n\n')); + } + + console.log( + `ok lint-allowed-tools-parity: ${scanned} command(s) checked, ${EXEMPT.size} exemption(s) all still needed`, + ); +} + +module.exports = { parseAllowedTools, scan, EXEMPT, DEFAULT_ROOT }; + +if (require.main === module) runMain(main); diff --git a/tests/lint-allowed-tools-parity.test.cjs b/tests/lint-allowed-tools-parity.test.cjs new file mode 100644 index 000000000..30082403b --- /dev/null +++ b/tests/lint-allowed-tools-parity.test.cjs @@ -0,0 +1,157 @@ +'use strict'; + +// #4394: unit tests for the allowed-tools parity lint. The rule reads command +// frontmatter, so every arm below drives it against a synthetic commands +// directory rather than the live corpus — a test that asserted "the real tree +// is clean" would say nothing about whether the rule can DETECT anything, and +// that is exactly the failure mode this lint exists to close (the pre-existing +// generated-sync check compares the two trees only to each other, so a shared +// omission was invisible to it). + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); + +const { parseAllowedTools, scan, EXEMPT } = require('../scripts/lint-allowed-tools-parity.cjs'); +const { cleanup } = require('./helpers.cjs'); + +/** Write a synthetic commands dir; returns its absolute path. */ +function fixture(commands) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-4394-')); + for (const [stem, tools] of Object.entries(commands)) { + const frontmatter = tools === null + ? ['---', `name: gsd:${stem}`, '---', ''] + : ['---', `name: gsd:${stem}`, 'allowed-tools:', ...tools.map((t) => ` - ${t}`), 'requires: []', '---', '']; + fs.writeFileSync(path.join(dir, `${stem}.md`), `${frontmatter.join('\n')}\nbody\n`); + } + return dir; +} + +describe('#4394 parseAllowedTools', () => { + test('reads a YAML block sequence', () => { + const text = ['---', 'name: gsd:x', 'allowed-tools:', ' - Read', ' - Bash', 'requires: []', '---'].join('\n'); + assert.deepEqual(parseAllowedTools(text), ['Read', 'Bash']); + }); + + test('stops at the next frontmatter key, not at the end of the file', () => { + // A body bullet list must not be swallowed into the tool set. + const text = [ + '---', 'allowed-tools:', ' - Read', 'requires: [review]', '---', '', 'Steps:', ' - Bash', '', + ].join('\n'); + assert.deepEqual(parseAllowedTools(text), ['Read']); + }); + + test('reads an inline flow sequence and a bare inline list', () => { + assert.deepEqual(parseAllowedTools('allowed-tools: [Read, Bash]'), ['Read', 'Bash']); + assert.deepEqual(parseAllowedTools('allowed-tools: Read, Bash'), ['Read', 'Bash']); + }); + + test('strips quotes around tool names', () => { + assert.deepEqual(parseAllowedTools("allowed-tools: ['Read', \"Bash\"]"), ['Read', 'Bash']); + }); + + test('returns null when the key is absent', () => { + // Absent is NOT a violation: a command with no allowed-tools declares no + // Bash either, so the rule has nothing to say about it. Returning [] here + // would be indistinguishable from "declared, but empty". + assert.equal(parseAllowedTools('---\nname: gsd:x\n---\n'), null); + }); +}); + +describe('#4394 scan — the rule detects, and only what it should', () => { + test('flags Bash without Grep', () => { + const dir = fixture({ offender: ['Read', 'Bash'] }); + try { + const { violations } = scan(dir); + assert.deepEqual(violations.map((v) => v.stem), ['offender']); + // The message has to carry the declared set, or the fix is a guess. + assert.deepEqual(violations[0].tools, ['Read', 'Bash']); + } finally { cleanup(dir); } + }); + + test('accepts Bash WITH Grep', () => { + const dir = fixture({ fine: ['Read', 'Bash', 'Grep'] }); + try { + assert.deepEqual(scan(dir).violations, []); + } finally { cleanup(dir); } + }); + + test('ignores a command that declares no Bash', () => { + // The rule is about Bash standing in for a search the command cannot + // perform. No Bash, no substitution, nothing to say. + const dir = fixture({ reader: ['Read', 'Skill'] }); + try { + assert.deepEqual(scan(dir).violations, []); + } finally { cleanup(dir); } + }); + + test('ignores a command with no allowed-tools key at all', () => { + const dir = fixture({ bare: null }); + try { + assert.deepEqual(scan(dir).violations, []); + } finally { cleanup(dir); } + }); + + test('reports every offender, not just the first', () => { + const dir = fixture({ a: ['Bash'], b: ['Bash'], c: ['Bash', 'Grep'] }); + try { + assert.deepEqual(scan(dir).violations.map((v) => v.stem), ['a', 'b']); + } finally { cleanup(dir); } + }); +}); + +describe('#4394 scan — the exemption list cannot rot', () => { + // An exemption list that can only grow becomes a list of things nobody + // re-examined. These arms are what keep an exemption a real suppression + // rather than a comment. + const exemptStem = [...EXEMPT.keys()][0]; + + test('an exempt command that still needs its entry is silent', () => { + const dir = fixture({ [exemptStem]: ['Read', 'Write', 'Bash'] }); + try { + const { violations, staleExemptions } = scan(dir); + assert.deepEqual(violations, []); + assert.deepEqual(staleExemptions, []); + } finally { cleanup(dir); } + }); + + test('an exempt command that gained Grep is reported as stale', () => { + const dir = fixture({ [exemptStem]: ['Read', 'Bash', 'Grep'] }); + try { + const { staleExemptions } = scan(dir); + assert.deepEqual(staleExemptions.map((s) => s.stem), [exemptStem]); + assert.match(staleExemptions[0].reason, /no longer declares Bash without Grep/); + } finally { cleanup(dir); } + }); + + test('an exemption for a command that no longer exists is reported as stale', () => { + const dir = fixture({ unrelated: ['Read'] }); + try { + const { staleExemptions } = scan(dir); + assert.deepEqual(staleExemptions.map((s) => s.stem), [exemptStem]); + assert.match(staleExemptions[0].reason, /no such command/); + } finally { cleanup(dir); } + }); + + test('every exemption carries a non-empty reason', () => { + // The reason is what makes changing the set reviewable. An entry without + // one is a suppression nobody can evaluate. + for (const [stem, reason] of EXEMPT) { + assert.equal(typeof reason, 'string', `${stem} must carry a reason`); + assert.ok(reason.trim().length > 10, `${stem}'s reason must say something: ${JSON.stringify(reason)}`); + } + }); +}); + +describe('#4394 scan — against the live corpus', () => { + test('the exemption list has no stale entries on the real tree', () => { + // Kept separate from the violations count, which is deliberately NOT + // asserted here: the 21 commands #3085 identified are fixed by its own PR, + // and pinning a number would make this test a baseline that every such fix + // has to update. Staleness is different — it is a property of THIS + // script's list, and it is always this script's job to keep true. + assert.deepEqual(scan().staleExemptions, []); + }); +});