* fix(#1012): invoke fallow with its real CLI and wire the report normalizer The /gsd-code-review structural pre-pass invoked fallow with flags no published fallow version accepts (--json, --profile, --stdin-files), so it failed on every run and degraded silently per REQ-FALLOW-02 — the feature never delivered on any fallow version. Three compounding defects: 1. Invalid flags. Real fallow audit uses --format json (not --json), -q/--quiet, --changed-since/--base for changed-files scoping (no file-list input), and --max-crap for thresholds. There is no --profile or --stdin-files. 2. Exit-code handling. fallow audit exits 1 when it FINDS issues (verdict=fail), 0 when clean. The pre-pass treated any non-zero exit as a crash and discarded the output — i.e. it threw away exactly the findings it exists to surface. Success is now decided by whether a valid fallow JSON report was produced, not by the exit code. 3. Schema mismatch. normalizeFallowReport parsed a fictional top-level schema (unusedExports/duplicates/circularDependencies) fallow never shipped, and was dead code (the workflow embedded raw JSON; its tests asserted the fictional schema, one even calling a non-existent runFallowAudit and passing vacuously). Fixes: align the invocation to fallow's documented agent-facing pattern; map the profile preset (minimal/standard/strict) to --max-crap (50/30/15); scope phase runs via --changed-since with a repo-scope fallback; rewrite the normalizer to fallow's real schema (dead_code.unused_exports/unused_files/circular_dependencies + duplication.clone_groups) and wire it into the workflow so the reviewer receives normalized findings; replace the fictional-schema fixtures and tests with real-schema ones and delete the vacuous runFallowAudit test. Closes #1012 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#1012): backfill changeset PR number to 1044 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
408 lines
17 KiB
JavaScript
408 lines
17 KiB
JavaScript
// allow-test-rule: source-text-is-the-product
|
|
// This test validates workflow/agent/config contracts stored in shipped .md/.ts/.cjs
|
|
// artifacts. Source text is the runtime product for those surfaces.
|
|
|
|
const { describe, test } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
const fs = require('node:fs');
|
|
const path = require('node:path');
|
|
const os = require('node:os');
|
|
const { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs');
|
|
|
|
const ROOT = path.resolve(__dirname, '..');
|
|
|
|
// N2: single helper — on macOS os.tmpdir() already returns /private/tmp; the
|
|
// existsSync guard is kept only as defense-in-depth fallback.
|
|
function getWritableTmp() {
|
|
const candidates = ['/private/tmp', '/tmp', os.tmpdir()];
|
|
return candidates.find((dir) => {
|
|
try {
|
|
fs.accessSync(dir, fs.constants.W_OK);
|
|
return true;
|
|
} catch {
|
|
return false;
|
|
}
|
|
});
|
|
}
|
|
|
|
describe('feat-3210: fallow integration module', () => {
|
|
test('normalizes structural findings from a fallow report', () => {
|
|
const { normalizeFallowReport } = require('../gsd-core/bin/lib/fallow-runner.cjs');
|
|
const fixture = JSON.parse(
|
|
fs.readFileSync(path.join(ROOT, 'tests', 'fixtures', 'fallow', 'sample-findings.json'), 'utf8'),
|
|
);
|
|
|
|
const normalized = normalizeFallowReport(fixture);
|
|
// Counts derived from real schema fixture fields
|
|
const expectedUnused = fixture.dead_code.unused_exports.length;
|
|
const expectedUnusedFiles = fixture.dead_code.unused_files.length;
|
|
const expectedCircular = fixture.dead_code.circular_dependencies.length;
|
|
const expectedDuplicates = fixture.duplication.clone_groups.length;
|
|
assert.deepStrictEqual(normalized.summary, {
|
|
unused_exports: expectedUnused,
|
|
unused_files: expectedUnusedFiles,
|
|
duplicates: expectedDuplicates,
|
|
circular_dependencies: expectedCircular,
|
|
total: 4,
|
|
});
|
|
assert.strictEqual(normalized.findings.length, 4);
|
|
});
|
|
|
|
test('falls back to node_modules/.bin/fallow when PATH does not contain fallow', () => {
|
|
const { resolveFallowBinary } = require('../gsd-core/bin/lib/fallow-runner.cjs');
|
|
// N2: use shared helper
|
|
const baseTmp = getWritableTmp();
|
|
const tmp = fs.mkdtempSync(path.join(baseTmp, 'gsd-fallow-bin-'));
|
|
const binDir = path.join(tmp, 'node_modules', '.bin');
|
|
fs.mkdirSync(binDir, { recursive: true });
|
|
const fallowPath = path.join(binDir, 'fallow');
|
|
fs.writeFileSync(fallowPath, '#!/usr/bin/env sh\n');
|
|
if (process.platform !== 'win32') fs.chmodSync(fallowPath, 0o755);
|
|
|
|
const resolved = resolveFallowBinary({ cwd: tmp, envPath: '' });
|
|
assert.strictEqual(resolved, fallowPath);
|
|
|
|
cleanup(tmp);
|
|
});
|
|
|
|
// H6: replaced wholesale win32 skip with platform-adapted assertion
|
|
test('ignores non-executable PATH candidate on non-Windows; prefers .cmd over bare extensionless on Windows', () => {
|
|
const { resolveFallowBinary } = require('../gsd-core/bin/lib/fallow-runner.cjs');
|
|
// N2: use shared helper
|
|
const baseTmp = getWritableTmp();
|
|
|
|
if (process.platform === 'win32') {
|
|
// H6: Windows — .cmd extension candidate must be preferred over bare extensionless file
|
|
const tmp = fs.mkdtempSync(path.join(baseTmp, 'gsd-fallow-win-'));
|
|
try {
|
|
const pathDir = path.join(tmp, 'bin');
|
|
fs.mkdirSync(pathDir, { recursive: true });
|
|
const bareFile = path.join(pathDir, 'fallow');
|
|
const cmdFile = path.join(pathDir, 'fallow.cmd');
|
|
fs.writeFileSync(bareFile, '@echo off\r\n');
|
|
fs.writeFileSync(cmdFile, '@echo off\r\n');
|
|
const resolved = resolveFallowBinary({ cwd: tmp, envPath: pathDir });
|
|
assert.strictEqual(
|
|
resolved,
|
|
cmdFile,
|
|
'Windows: .cmd candidate must be preferred over bare extensionless file',
|
|
);
|
|
} finally {
|
|
cleanup(tmp);
|
|
}
|
|
} else {
|
|
// H6: non-Windows — non-executable file in PATH must be ignored
|
|
const tmp = fs.mkdtempSync(path.join(baseTmp, 'gsd-fallow-nonexec-'));
|
|
try {
|
|
const pathDir = path.join(tmp, 'bin');
|
|
fs.mkdirSync(pathDir, { recursive: true });
|
|
const nonExec = path.join(pathDir, 'fallow');
|
|
fs.writeFileSync(nonExec, '#!/usr/bin/env sh\n');
|
|
fs.chmodSync(nonExec, 0o644);
|
|
const resolved = resolveFallowBinary({ cwd: tmp, envPath: pathDir });
|
|
assert.strictEqual(resolved, null);
|
|
} finally {
|
|
cleanup(tmp);
|
|
}
|
|
}
|
|
});
|
|
|
|
test('normalizes empty fallow report to zero findings', () => {
|
|
const { normalizeFallowReport } = require('../gsd-core/bin/lib/fallow-runner.cjs');
|
|
const fixture = JSON.parse(
|
|
fs.readFileSync(path.join(ROOT, 'tests', 'fixtures', 'fallow', 'sample-empty.json'), 'utf8'),
|
|
);
|
|
const normalized = normalizeFallowReport(fixture);
|
|
assert.deepStrictEqual(normalized.summary, {
|
|
unused_exports: 0,
|
|
unused_files: 0,
|
|
duplicates: 0,
|
|
circular_dependencies: 0,
|
|
total: 0,
|
|
});
|
|
assert.deepStrictEqual(normalized.findings, []);
|
|
});
|
|
|
|
test('throws actionable error when fallow is enabled but binary is unavailable', () => {
|
|
const { requireFallowBinary } = require('../gsd-core/bin/lib/fallow-runner.cjs');
|
|
// N2: use shared helper
|
|
const baseTmp = getWritableTmp();
|
|
const tmp = fs.mkdtempSync(path.join(baseTmp, 'gsd-fallow-missing-'));
|
|
assert.throws(
|
|
() => requireFallowBinary({ cwd: tmp, envPath: '' }),
|
|
/install fallow via `npm install -D fallow` or `cargo install fallow`/,
|
|
);
|
|
cleanup(tmp);
|
|
});
|
|
|
|
// M5: edge-case fixture — line:0 preservation, unicode path, single-instance clone_group, 3-file cycle
|
|
test('normalizes edge-case fixture: line:0 preservation, unicode path, single-instance clone_group, 3-file cycle', () => {
|
|
const { normalizeFallowReport } = require('../gsd-core/bin/lib/fallow-runner.cjs');
|
|
const fixture = JSON.parse(
|
|
fs.readFileSync(
|
|
path.join(ROOT, 'tests', 'fixtures', 'fallow', 'sample-edge-cases.json'),
|
|
'utf8',
|
|
),
|
|
);
|
|
|
|
// Real schema: unused_export with line:0 — must survive without coercion
|
|
assert.strictEqual(fixture.dead_code.unused_exports.length, 1);
|
|
assert.strictEqual(fixture.dead_code.unused_exports[0].line, 0, 'edge-case fixture: line must be 0');
|
|
// unicode file path is preserved in fixture
|
|
assert.ok(
|
|
fixture.dead_code.unused_exports[0].path.includes('café'),
|
|
'edge-case fixture: unicode file path must be present',
|
|
);
|
|
|
|
// single-instance clone_group (related_file normalizes to '')
|
|
assert.strictEqual(fixture.duplication.clone_groups.length, 1);
|
|
assert.strictEqual(fixture.duplication.clone_groups[0].instances.length, 1);
|
|
|
|
// 3-file circular dependency cycle
|
|
assert.strictEqual(fixture.dead_code.circular_dependencies.length, 1);
|
|
assert.strictEqual(
|
|
fixture.dead_code.circular_dependencies[0].files.length,
|
|
3,
|
|
'edge-case: files array must have exactly 3 entries',
|
|
);
|
|
|
|
// normalization round-trips without throwing
|
|
const normalized = normalizeFallowReport(fixture);
|
|
// 1 unused_export + 0 unused_files + 1 circular_dep + 1 clone_group = 3
|
|
const expectedTotal =
|
|
fixture.dead_code.unused_exports.length +
|
|
fixture.dead_code.unused_files.length +
|
|
fixture.dead_code.circular_dependencies.length +
|
|
fixture.duplication.clone_groups.length;
|
|
assert.strictEqual(normalized.findings.length, expectedTotal);
|
|
assert.strictEqual(normalized.summary.total, expectedTotal);
|
|
|
|
// line:0 survives normalization
|
|
const unicodeFinding = normalized.findings.find(
|
|
(f) => typeof f.file === 'string' && f.file.includes('café'),
|
|
);
|
|
assert.ok(unicodeFinding, 'unicode file path must survive normalization round-trip');
|
|
assert.strictEqual(unicodeFinding.line, 0, 'line:0 must not be coerced to null');
|
|
|
|
// single-instance clone_group: related_file must be ''
|
|
const dupFinding = normalized.findings.find((f) => f.type === 'duplicate_block');
|
|
assert.ok(dupFinding, 'duplicate_block finding must exist');
|
|
assert.strictEqual(dupFinding.related_file, '', 'single-instance clone_group: related_file must be empty string');
|
|
});
|
|
});
|
|
|
|
describe('feat-3210: H1 - line:0 preservation', () => {
|
|
test('normalizeFallowReport preserves line:0 for unused_export (not coerced to null)', () => {
|
|
const { normalizeFallowReport } = require('../gsd-core/bin/lib/fallow-runner.cjs');
|
|
const report = {
|
|
dead_code: {
|
|
unused_exports: [{ path: 'src/a.ts', export_name: 'foo', line: 0 }],
|
|
},
|
|
};
|
|
const normalized = normalizeFallowReport(report);
|
|
assert.strictEqual(normalized.findings[0].line, 0, 'line:0 must not be coerced to null via ||');
|
|
});
|
|
|
|
test('normalizeFallowReport preserves line:0 for duplicate_block instances[0].start_line (not coerced to null)', () => {
|
|
const { normalizeFallowReport } = require('../gsd-core/bin/lib/fallow-runner.cjs');
|
|
const report = {
|
|
duplication: {
|
|
clone_groups: [
|
|
{ instances: [{ file: 'src/a.ts', start_line: 0 }, { file: 'src/b.ts', start_line: 5 }] },
|
|
],
|
|
},
|
|
};
|
|
const normalized = normalizeFallowReport(report);
|
|
assert.strictEqual(normalized.findings[0].line, 0, 'start_line:0 must not be coerced to null via ||');
|
|
});
|
|
});
|
|
|
|
describe('feat-3210: M2 - node_modules/.bin resolution order', () => {
|
|
test('resolveFallowBinary prefers node_modules/.bin over PATH when both exist', () => {
|
|
const { resolveFallowBinary } = require('../gsd-core/bin/lib/fallow-runner.cjs');
|
|
// N2: use shared helper
|
|
const baseTmp = getWritableTmp();
|
|
const tmp = fs.mkdtempSync(path.join(baseTmp, 'gsd-fallow-order-'));
|
|
try {
|
|
// local node_modules/.bin/fallow
|
|
const binDir = path.join(tmp, 'node_modules', '.bin');
|
|
fs.mkdirSync(binDir, { recursive: true });
|
|
const localFallow = path.join(binDir, 'fallow');
|
|
fs.writeFileSync(localFallow, '#!/usr/bin/env sh\necho local\n');
|
|
if (process.platform !== 'win32') fs.chmodSync(localFallow, 0o755);
|
|
|
|
// PATH fallow (a different file)
|
|
const pathDir = path.join(tmp, 'pathbin');
|
|
fs.mkdirSync(pathDir, { recursive: true });
|
|
const pathFallow = path.join(pathDir, 'fallow');
|
|
fs.writeFileSync(pathFallow, '#!/usr/bin/env sh\necho path\n');
|
|
if (process.platform !== 'win32') fs.chmodSync(pathFallow, 0o755);
|
|
|
|
const resolved = resolveFallowBinary({ cwd: tmp, envPath: pathDir });
|
|
assert.strictEqual(resolved, localFallow, 'node_modules/.bin/fallow must win over PATH fallow');
|
|
} finally {
|
|
cleanup(tmp);
|
|
}
|
|
});
|
|
});
|
|
|
|
describe('feat-3210 / #1012: code-review workflow invokes fallow with the real CLI', () => {
|
|
// allow-test-rule: source-text-is-the-product — code-review.md IS the workflow the orchestrator
|
|
// executes; its fallow invocation is the product surface.
|
|
const workflowSrc = fs.readFileSync(
|
|
path.join(ROOT, 'gsd-core', 'workflows', 'code-review.md'),
|
|
'utf8',
|
|
);
|
|
|
|
test('uses audit --format json and --quiet (real fallow 2.x flags)', () => {
|
|
assert.ok(
|
|
workflowSrc.includes('audit --format json'),
|
|
'workflow must invoke: audit --format json',
|
|
);
|
|
assert.ok(
|
|
workflowSrc.includes('--quiet'),
|
|
'workflow must pass --quiet to suppress progress output',
|
|
);
|
|
});
|
|
|
|
test('does NOT use removed flags: --json , --profile, --stdin-files', () => {
|
|
assert.ok(
|
|
!workflowSrc.includes('--json '),
|
|
'workflow must not use old --json flag (note trailing space to avoid matching --format json)',
|
|
);
|
|
assert.ok(
|
|
!workflowSrc.includes('--profile'),
|
|
'workflow must not use --profile (fallow has no native profile concept)',
|
|
);
|
|
assert.ok(
|
|
!workflowSrc.includes('--stdin-files'),
|
|
'workflow must not use --stdin-files (removed in fallow 2.x)',
|
|
);
|
|
});
|
|
|
|
test('uses --max-crap for threshold control (profile maps to max-crap)', () => {
|
|
assert.ok(
|
|
workflowSrc.includes('--max-crap'),
|
|
'workflow must use --max-crap to control threshold (profile mapped to this flag)',
|
|
);
|
|
});
|
|
|
|
test('scopes phase via --changed-since (native fallow git-ref scoping)', () => {
|
|
assert.ok(
|
|
workflowSrc.includes('--changed-since'),
|
|
'workflow must use --changed-since for phase scoping',
|
|
);
|
|
});
|
|
|
|
test('normalizes fallow output via normalizeFallowReportFile before embedding', () => {
|
|
assert.ok(
|
|
workflowSrc.includes('normalizeFallowReportFile'),
|
|
'workflow must call normalizeFallowReportFile to normalize before embedding into reviewer prompt',
|
|
);
|
|
});
|
|
|
|
test('exit-handling gates on valid JSON (verdict in o), not on exit code', () => {
|
|
assert.ok(
|
|
workflowSrc.includes("'verdict' in o"),
|
|
"workflow exit-handling must use 'verdict' in o to decide success (not exit code)",
|
|
);
|
|
});
|
|
});
|
|
|
|
describe('feat-3210: workflow and config contracts', () => {
|
|
test('config schema allows code_quality.fallow.* keys in CJS and runtime manifest', () => {
|
|
// CJS config-schema and runtime consume the same manifest source-of-truth.
|
|
// Use the CJS runtime Set and the manifest directly (no inline text parsing).
|
|
const { VALID_CONFIG_KEYS } = require('../gsd-core/bin/lib/config-schema.cjs');
|
|
const manifestPath = path.join(ROOT, 'gsd-core', 'bin', 'shared', 'config-schema.manifest.json');
|
|
const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8'));
|
|
const manifestKeys = new Set(manifest.validKeys);
|
|
for (const key of [
|
|
'code_quality.fallow.enabled',
|
|
'code_quality.fallow.scope',
|
|
'code_quality.fallow.profile',
|
|
'code_quality.fallow.mcp',
|
|
]) {
|
|
assert.ok(VALID_CONFIG_KEYS.has(key), `missing CJS config key: ${key}`);
|
|
assert.ok(manifestKeys.has(key), `missing manifest key: ${key} (runtime sources from manifest)`);
|
|
}
|
|
});
|
|
|
|
test('config-set accepts code_quality.fallow keys', () => {
|
|
const originalTmpDir = process.env.TMPDIR;
|
|
// L2: fail loudly if no writable tmp dir is found (was silent skip)
|
|
const writableTmp = getWritableTmp(); // N2: use shared helper
|
|
assert.ok(writableTmp, 'no writable tmp directory found'); // L2: explicit fail-loud assertion
|
|
process.env.TMPDIR = writableTmp;
|
|
const tmpDir = createTempProject('gsd-fallow-config-');
|
|
try {
|
|
const cases = [
|
|
['code_quality.fallow.enabled', 'true'],
|
|
['code_quality.fallow.scope', 'repo'],
|
|
['code_quality.fallow.profile', 'strict'],
|
|
['code_quality.fallow.mcp', 'false'],
|
|
];
|
|
for (const [key, value] of cases) {
|
|
const result = runGsdTools(['config-set', key, value], tmpDir);
|
|
assert.ok(result.success, `config-set failed for ${key}: ${result.error || result.output}`);
|
|
}
|
|
} finally {
|
|
cleanup(tmpDir);
|
|
if (originalTmpDir === undefined) delete process.env.TMPDIR;
|
|
else process.env.TMPDIR = originalTmpDir;
|
|
}
|
|
});
|
|
|
|
// B4: replaced 5x source-grep tautologies with parse-based structural checks.
|
|
// The workflow .md uses XML-like <step> tags as its runtime DSL; we parse the step block
|
|
// structurally and assert on structural properties, not on prose strings.
|
|
test('code-review workflow structural_pre_pass step is parseable and references FALLOW.json output', () => {
|
|
const workflow = fs.readFileSync(
|
|
path.join(ROOT, 'gsd-core', 'workflows', 'code-review.md'),
|
|
'utf8',
|
|
);
|
|
|
|
// Parse: the <step name="structural_pre_pass"> block must exist and be closed
|
|
const stepMatch = workflow.match(/<step\s+name="structural_pre_pass">([\s\S]*?)<\/step>/);
|
|
assert.ok(
|
|
stepMatch,
|
|
'workflow must contain a parseable <step name="structural_pre_pass">...</step> block',
|
|
);
|
|
|
|
const stepBody = stepMatch[1];
|
|
|
|
// Structural property: the step body must reference the FALLOW.json output artifact
|
|
assert.ok(
|
|
stepBody.includes('FALLOW.json'),
|
|
'structural_pre_pass step body must reference the FALLOW.json output artifact',
|
|
);
|
|
|
|
// Structural property: the step body must gate on the fallow enabled config key
|
|
assert.ok(
|
|
stepBody.includes('code_quality.fallow.enabled'),
|
|
'structural_pre_pass step body must gate on code_quality.fallow.enabled',
|
|
);
|
|
});
|
|
|
|
// B4: agent output contract — doc-parity check (approved fallback per config-schema-docs-parity
|
|
// pattern). We confirm the heading exists in the shipped artifact, not in a live agent response.
|
|
// Live agent output is covered by /gsd-code-review e2e runs downstream.
|
|
test('reviewer prompt defines ## Structural Findings (fallow) heading and review context echoes it', () => {
|
|
const reviewer = fs.readFileSync(path.join(ROOT, 'agents', 'gsd-code-reviewer.md'), 'utf8');
|
|
const reviewContext = fs.readFileSync(path.join(ROOT, 'gsd-core', 'contexts', 'review.md'), 'utf8');
|
|
|
|
// Doc-parity: section heading must exist in the shipped agent file (the heading is a contract,
|
|
// not prose — renaming it would break every consumer that parses agent output by section)
|
|
assert.ok(
|
|
reviewer.includes('## Structural Findings (fallow)'),
|
|
'gsd-code-reviewer.md must define ## Structural Findings (fallow) section heading',
|
|
);
|
|
|
|
// Doc-parity: review context that agents receive must reference the same section
|
|
assert.ok(
|
|
reviewContext.includes('Structural Findings (fallow)'),
|
|
'review.md context must reference Structural Findings (fallow) so agents recognize the section',
|
|
);
|
|
});
|
|
});
|