* chore(#3560): delete two unreachable workflows, gate reachability in lint discovery-phase.md and plan-milestone-gaps.md shipped to all 19 runtime install trees with no command, agent, or skill referencing them. plan-milestone-gaps' command was deleted by #2790 and the workflow was left behind; discovery-phase's own header claimed a caller in plan-phase.md's mandatory_discovery step, and that step does not exist — plan-phase.md contains zero occurrences of "discovery". docs/INVENTORY.md asserted discovery-phase.md was an alternate entry for /gsd-new-project. new-project.md never referenced it. The row and the matching note sentence are removed across all five locales rather than corrected. Adds rule 6 to lint-command-contract: every shipped workflow must be reachable from a loader, walking the transitive closure over the three reference shapes this repo uses. The closure seeds ONLY from commands/agents/skills, so a workflow that references only itself and a pair that reference only each other are both correctly reported rather than satisfying themselves; a visited set makes reference cycles terminate. The measure is a mention in a LOADER — docs/ and install-tree fixtures deliberately do not count, because scan.md proved a file can be documented and shipped while entirely unreached. Ships blocking, not report-only: #3561 is in this branch's base, so the tree reports 0 unreachable from the start. Closes #3560 * test(#3560): drive rule 6 end-to-end, sweep a stale allowlist, update ADR-0002 Review findings. Rule 6 had no end-to-end coverage: the tests exercised the pure closure with in-memory data, so the wiring — file collection, exit code, diagnostic — was unproven, and #3560's acceptance list explicitly wants a fixture showing the rule FAILS on a planted orphan. Adds an optional --root to lint-command-contract (default behavior unchanged) and four tests driving the real CLI through the process seam against a temp fixture: clean=0, planted orphan=1, orphan referenced only from docs/=1, orphan reachable transitively=0. The docs/ case is what pins the Goodhart defense — a mention outside a loader must not confer reachability. Deletes two tests that were byte-identical to a third and could not assert anything loader-specific, since the closure is source-agnostic by design; that distinction lives in the lint script's file collection and is now covered above. Removes a stale ALLOWLIST entry for discovery-phase.md in planner-language-regression — the exact sweep-miss class rule 6 exists to catch, found in the PR that adds the rule. ADR-0002 described five per-file frontmatter checks; rule 6 is a repo-level reachability graph, so the Decision section now says so. Refs #3560 * test(#3560): cut the bug-3298 test pin on the deleted plan-milestone-gaps workflow The remote runner went red with four failures: tests/phase.test.cjs asserted the plan-milestone-gaps workflow exists and checked its mkdir patterns, so deleting the file broke the test that pinned it. This is the fence the epic describes — the content-sync test IS what keeps an unreachable file alive — and cutting the coupling is what makes the deletion safe. Removes only that arm. The bug-3298 block guards three workflows against phase-dir prefix drift; the import and add-backlog arms and both shared mkdir-pattern helpers are untouched. Worth recording where the sweep failed: my reachability walk covered commands, agents, skills, gsd-core and docs, and lint-removed-but-needed covers .github/workflows, gsd-core, docs and package.json. Neither looks at tests/, so a test-pinned deletion is invisible to both and surfaces only on the remote runner. The how-to added by this PR names that gap explicitly so the next deletion searches tests/ by hand. Refs #3560 * docs(#3560): add a how-to for resolving unreachable-workflow findings * chore(#3560): backfill changeset pr number to 3564 --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
@@ -33,8 +33,15 @@ const {
|
||||
parseFrontmatter,
|
||||
executionContextRefs,
|
||||
workflowPathRefs,
|
||||
unreachableWorkflows,
|
||||
} = require('../scripts/command-contract-helpers.cjs');
|
||||
|
||||
const { runNode, OUTCOME } = require('./helpers/process-seam.cjs');
|
||||
const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
|
||||
const { createTempDir, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const LINT_SCRIPT = path.join(ROOT, 'scripts', 'lint-command-contract.cjs');
|
||||
|
||||
const commandFiles = fs
|
||||
.readdirSync(COMMANDS_DIR)
|
||||
.filter(f => f.endsWith('.md'))
|
||||
@@ -284,6 +291,317 @@ describe('#3561 — every workflow path referenced by a command exists on disk',
|
||||
});
|
||||
|
||||
|
||||
describe('#3560 — unreachableWorkflows closure', () => {
|
||||
test('a planted orphan is reported', () => {
|
||||
const loaderContents = ['load workflows/live.md'];
|
||||
const gsdFiles = new Map();
|
||||
const workflowPaths = ['workflows/live.md', 'workflows/orphan.md'];
|
||||
assert.deepEqual(
|
||||
unreachableWorkflows(loaderContents, gsdFiles, workflowPaths),
|
||||
['workflows/orphan.md'],
|
||||
);
|
||||
});
|
||||
|
||||
test('eager include reaches', () => {
|
||||
const loaderContents = ['@~/.claude/gsd-core/workflows/a.md'];
|
||||
const gsdFiles = new Map();
|
||||
const workflowPaths = ['workflows/a.md'];
|
||||
assert.deepEqual(unreachableWorkflows(loaderContents, gsdFiles, workflowPaths), []);
|
||||
});
|
||||
|
||||
test('lazy path reaches', () => {
|
||||
const loaderContents = ['read `gsd-core/workflows/a.md`'];
|
||||
const gsdFiles = new Map();
|
||||
const workflowPaths = ['workflows/a.md'];
|
||||
assert.deepEqual(unreachableWorkflows(loaderContents, gsdFiles, workflowPaths), []);
|
||||
});
|
||||
|
||||
test('parent-relative reaches', () => {
|
||||
const loaderContents = ['run execute-phase/steps/s.md'];
|
||||
const gsdFiles = new Map();
|
||||
const workflowPaths = ['workflows/execute-phase/steps/s.md'];
|
||||
assert.deepEqual(unreachableWorkflows(loaderContents, gsdFiles, workflowPaths), []);
|
||||
});
|
||||
|
||||
test('reaches transitively', () => {
|
||||
const loaderContents = ['see workflows/a.md'];
|
||||
const gsdFiles = new Map([['workflows/a.md', 'see workflows/b.md']]);
|
||||
const workflowPaths = ['workflows/a.md', 'workflows/b.md'];
|
||||
assert.deepEqual(unreachableWorkflows(loaderContents, gsdFiles, workflowPaths), []);
|
||||
});
|
||||
|
||||
test('reaches at depth three', () => {
|
||||
const loaderContents = ['see workflows/a.md'];
|
||||
const gsdFiles = new Map([
|
||||
['workflows/a.md', 'see workflows/b.md'],
|
||||
['workflows/b.md', 'see workflows/c.md'],
|
||||
]);
|
||||
const workflowPaths = ['workflows/a.md', 'workflows/b.md', 'workflows/c.md'];
|
||||
assert.deepEqual(unreachableWorkflows(loaderContents, gsdFiles, workflowPaths), []);
|
||||
});
|
||||
|
||||
test('a self-reference does not reach', () => {
|
||||
// Nothing in loaderContents mentions self.md; the fact that self.md's
|
||||
// own body references itself must not count as reachability.
|
||||
const loaderContents = [];
|
||||
const gsdFiles = new Map([['workflows/self.md', 'see workflows/self.md']]);
|
||||
const workflowPaths = ['workflows/self.md'];
|
||||
assert.deepEqual(
|
||||
unreachableWorkflows(loaderContents, gsdFiles, workflowPaths),
|
||||
['workflows/self.md'],
|
||||
);
|
||||
});
|
||||
|
||||
test('a mutual-reference island does not reach', () => {
|
||||
// x and y point only at each other; no loader points at either. The
|
||||
// island looks connected from the inside but no runtime ever opens it.
|
||||
const loaderContents = [];
|
||||
const gsdFiles = new Map([
|
||||
['workflows/x.md', 'see workflows/y.md'],
|
||||
['workflows/y.md', 'see workflows/x.md'],
|
||||
]);
|
||||
const workflowPaths = ['workflows/x.md', 'workflows/y.md'];
|
||||
assert.deepEqual(
|
||||
unreachableWorkflows(loaderContents, gsdFiles, workflowPaths),
|
||||
['workflows/x.md', 'workflows/y.md'],
|
||||
);
|
||||
});
|
||||
|
||||
test('terminates on a cycle', () => {
|
||||
// a -> b -> c -> a. All three are reachable from the loader; the walk
|
||||
// must still terminate rather than looping forever on the cycle.
|
||||
const loaderContents = ['see workflows/a.md'];
|
||||
const gsdFiles = new Map([
|
||||
['workflows/a.md', 'see workflows/b.md'],
|
||||
['workflows/b.md', 'see workflows/c.md'],
|
||||
['workflows/c.md', 'see workflows/a.md'],
|
||||
]);
|
||||
const workflowPaths = ['workflows/a.md', 'workflows/b.md', 'workflows/c.md'];
|
||||
assert.deepEqual(unreachableWorkflows(loaderContents, gsdFiles, workflowPaths), []);
|
||||
});
|
||||
|
||||
test('a docs-only mention does not reach (Goodhart guard: mentioning a path is not the same as loading it)', () => {
|
||||
// Simulates a hypothetical prose/docs file that names orphan.md — that
|
||||
// string is simply never included in loaderContents (or gsdFiles), which
|
||||
// is exactly what "a docs mention" looks like to this function: absent.
|
||||
const loaderContents = [];
|
||||
const gsdFiles = new Map();
|
||||
const workflowPaths = ['workflows/orphan.md'];
|
||||
assert.deepEqual(
|
||||
unreachableWorkflows(loaderContents, gsdFiles, workflowPaths),
|
||||
['workflows/orphan.md'],
|
||||
);
|
||||
});
|
||||
|
||||
test('a dangling reference reaches nothing', () => {
|
||||
const loaderContents = ['see workflows/missing.md'];
|
||||
const gsdFiles = new Map();
|
||||
const workflowPaths = ['workflows/real-orphan.md'];
|
||||
assert.doesNotThrow(() => unreachableWorkflows(loaderContents, gsdFiles, workflowPaths));
|
||||
assert.deepEqual(
|
||||
unreachableWorkflows(loaderContents, gsdFiles, workflowPaths),
|
||||
['workflows/real-orphan.md'],
|
||||
);
|
||||
});
|
||||
|
||||
test('empty workflow set', () => {
|
||||
const loaderContents = ['see workflows/a.md'];
|
||||
const gsdFiles = new Map();
|
||||
const workflowPaths = [];
|
||||
assert.deepEqual(unreachableWorkflows(loaderContents, gsdFiles, workflowPaths), []);
|
||||
});
|
||||
|
||||
test('no loaders means nothing is reached', () => {
|
||||
const loaderContents = [];
|
||||
const gsdFiles = new Map();
|
||||
const workflowPaths = ['workflows/a.md'];
|
||||
assert.deepEqual(
|
||||
unreachableWorkflows(loaderContents, gsdFiles, workflowPaths),
|
||||
['workflows/a.md'],
|
||||
);
|
||||
});
|
||||
|
||||
test('CRLF-tolerant', () => {
|
||||
const loaderContents = ['workflows/a.md\r\nworkflows/b.md'];
|
||||
const gsdFiles = new Map();
|
||||
const workflowPaths = ['workflows/a.md', 'workflows/b.md'];
|
||||
assert.deepEqual(unreachableWorkflows(loaderContents, gsdFiles, workflowPaths), []);
|
||||
});
|
||||
|
||||
test('reports exactly the orphan among many', () => {
|
||||
const reachable = Array.from({ length: 152 }, (_, i) => `workflows/gen-${i}.md`);
|
||||
const loaderContents = reachable.slice();
|
||||
const gsdFiles = new Map();
|
||||
const orphan = 'workflows/orphan-152.md';
|
||||
const workflowPaths = [...reachable, orphan];
|
||||
const result = unreachableWorkflows(loaderContents, gsdFiles, workflowPaths);
|
||||
assert.equal(result.length, 1);
|
||||
assert.deepEqual(result, [orphan]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3560 — rule 6 fails the build on a real orphan', () => {
|
||||
// Builds the minimal fixture tree the lint CLI needs to satisfy rules 1-5
|
||||
// for a single command file, plus an eagerly-loaded live.md workflow.
|
||||
// `extra` lets each test layer on exactly the additional state it needs
|
||||
// without coupling test execution order to shared mutable fixture state.
|
||||
function buildBaseFixture(dir) {
|
||||
fs.mkdirSync(path.join(dir, 'commands', 'gsd'), { recursive: true });
|
||||
fs.mkdirSync(path.join(dir, 'gsd-core', 'workflows'), { recursive: true });
|
||||
fs.mkdirSync(path.join(dir, 'agents'), { recursive: true });
|
||||
fs.mkdirSync(path.join(dir, 'skills'), { recursive: true });
|
||||
|
||||
fs.writeFileSync(
|
||||
path.join(dir, 'commands', 'gsd', 'fixture-command.md'),
|
||||
[
|
||||
'---',
|
||||
'name: gsd:fixture-command',
|
||||
'description: Fixture command for the #3560 rule-6 regression tests.',
|
||||
'allowed-tools:',
|
||||
' - Read',
|
||||
' - Bash',
|
||||
'---',
|
||||
'',
|
||||
'<execution_context>',
|
||||
'@~/.claude/gsd-core/workflows/live.md',
|
||||
'</execution_context>',
|
||||
'',
|
||||
].join('\n'),
|
||||
);
|
||||
|
||||
fs.writeFileSync(
|
||||
path.join(dir, 'gsd-core', 'workflows', 'live.md'),
|
||||
'# live workflow\n\nNothing special here.\n',
|
||||
);
|
||||
}
|
||||
|
||||
function runLint(dir) {
|
||||
return runNode([LINT_SCRIPT, '--root', dir], { timeoutMs: PROBE_TIMEOUT_MS });
|
||||
}
|
||||
|
||||
test('clean fixture passes', (t) => {
|
||||
const dir = createTempDir('gsd-3560-clean-');
|
||||
t.after(() => cleanup(dir));
|
||||
buildBaseFixture(dir);
|
||||
|
||||
const result = runLint(dir);
|
||||
assert.equal(result.outcome, OUTCOME.EXITED);
|
||||
assert.equal(result.exitCode, 0);
|
||||
});
|
||||
|
||||
test('a planted orphan fails the build', (t) => {
|
||||
const dir = createTempDir('gsd-3560-orphan-');
|
||||
t.after(() => cleanup(dir));
|
||||
buildBaseFixture(dir);
|
||||
fs.writeFileSync(
|
||||
path.join(dir, 'gsd-core', 'workflows', 'orphan.md'),
|
||||
'# orphan workflow\n\nNo loader references this file.\n',
|
||||
);
|
||||
|
||||
const result = runLint(dir);
|
||||
assert.equal(result.outcome, OUTCOME.EXITED);
|
||||
assert.equal(result.exitCode, 1);
|
||||
assert.ok(
|
||||
(result.stdout + result.stderr).includes('gsd-core/workflows/orphan.md'),
|
||||
'diagnostic output must name the orphaned file path',
|
||||
);
|
||||
});
|
||||
|
||||
test('an orphan referenced only from docs/ still fails', (t) => {
|
||||
const dir = createTempDir('gsd-3560-docs-only-');
|
||||
t.after(() => cleanup(dir));
|
||||
buildBaseFixture(dir);
|
||||
fs.writeFileSync(
|
||||
path.join(dir, 'gsd-core', 'workflows', 'orphan.md'),
|
||||
'# orphan workflow\n\nNo loader references this file.\n',
|
||||
);
|
||||
fs.mkdirSync(path.join(dir, 'docs'), { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(dir, 'docs', 'SOMETHING.md'),
|
||||
'See gsd-core/workflows/orphan.md for details.\n',
|
||||
);
|
||||
|
||||
const result = runLint(dir);
|
||||
assert.equal(result.outcome, OUTCOME.EXITED);
|
||||
assert.equal(result.exitCode, 1);
|
||||
});
|
||||
|
||||
test('an orphan reachable only transitively passes', (t) => {
|
||||
const dir = createTempDir('gsd-3560-transitive-');
|
||||
t.after(() => cleanup(dir));
|
||||
buildBaseFixture(dir);
|
||||
fs.writeFileSync(
|
||||
path.join(dir, 'gsd-core', 'workflows', 'orphan.md'),
|
||||
'# orphan workflow\n\nReachable transitively via live.md.\n',
|
||||
);
|
||||
// live.md itself now names orphan.md; live.md is eagerly loaded by the
|
||||
// fixture command, so orphan.md becomes reachable one hop out.
|
||||
fs.writeFileSync(
|
||||
path.join(dir, 'gsd-core', 'workflows', 'live.md'),
|
||||
'# live workflow\n\nSee workflows/orphan.md for the follow-up.\n',
|
||||
);
|
||||
|
||||
const result = runLint(dir);
|
||||
assert.equal(result.outcome, OUTCOME.EXITED);
|
||||
assert.equal(result.exitCode, 0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3560 — deleted orphan workflows do not ship', () => {
|
||||
const deletedBasenames = ['discovery-phase.md', 'plan-milestone-gaps.md'];
|
||||
|
||||
for (const basename of deletedBasenames) {
|
||||
test(`${basename}: does not exist under gsd-core/workflows/`, () => {
|
||||
assert.ok(
|
||||
!fs.existsSync(path.join(GSD_ROOT, 'workflows', basename)),
|
||||
`${basename}: expected file to be deleted from gsd-core/workflows/, but it still exists`,
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
const fixturesDir = path.join(ROOT, 'tests', 'fixtures', 'install-tree');
|
||||
const fixtureFiles = fs.readdirSync(fixturesDir).filter(f => f.endsWith('.json'));
|
||||
|
||||
for (const fixtureFile of fixtureFiles) {
|
||||
test(`${fixtureFile}: install-tree manifest contains no entry for a deleted workflow`, () => {
|
||||
const manifest = JSON.parse(fs.readFileSync(path.join(fixturesDir, fixtureFile), 'utf-8'));
|
||||
for (const basename of deletedBasenames) {
|
||||
const gsdRelative = `gsd-core/workflows/${basename}`;
|
||||
assert.ok(
|
||||
!manifest.includes(gsdRelative),
|
||||
`${fixtureFile}: manifest still lists ${gsdRelative}`,
|
||||
);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
test('scan.md is untouched by this PR', () => {
|
||||
assert.ok(
|
||||
fs.existsSync(path.join(GSD_ROOT, 'workflows', 'scan.md')),
|
||||
'gsd-core/workflows/scan.md is expected to still exist — this PR only deletes ' +
|
||||
'discovery-phase.md and plan-milestone-gaps.md',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3560 — INVENTORY carries no row for a deleted workflow', () => {
|
||||
const inventoryFiles = [
|
||||
'docs/INVENTORY.md',
|
||||
'docs/ja-JP/INVENTORY.md',
|
||||
'docs/ko-KR/INVENTORY.md',
|
||||
'docs/zh-CN/INVENTORY.md',
|
||||
'docs/pt-BR/INVENTORY.md',
|
||||
];
|
||||
|
||||
for (const rel of inventoryFiles) {
|
||||
test(`${rel}: mentions neither discovery-phase.md nor plan-milestone-gaps.md`, () => {
|
||||
const content = fs.readFileSync(path.join(ROOT, rel), 'utf-8');
|
||||
assert.ok(!content.includes('discovery-phase.md'), `${rel}: still mentions discovery-phase.md`);
|
||||
assert.ok(!content.includes('plan-milestone-gaps.md'), `${rel}: still mentions plan-milestone-gaps.md`);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
// ────────────────────────────────────────────────────────────────────────
|
||||
// Folded from tests/bug-3168-task-to-agent-rename.test.cjs — consolidation epic #1969 (B3 #1972)
|
||||
// ────────────────────────────────────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user