* 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>
This commit is contained in:
Tom Boucher
@@ -33,18 +33,19 @@ describe('feat-3210: fallow integration module', () => {
|
||||
);
|
||||
|
||||
const normalized = normalizeFallowReport(fixture);
|
||||
// M6: fixture: 1 unusedExport + 1 duplicate + 1 circularDep = 3; counts derived from fixture, not hardcoded
|
||||
const expectedUnused = fixture.unusedExports.length;
|
||||
const expectedDuplicates = fixture.duplicates.length;
|
||||
const expectedCircular = fixture.circularDependencies.length;
|
||||
const expectedTotal = expectedUnused + expectedDuplicates + expectedCircular;
|
||||
// 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: expectedTotal,
|
||||
total: 4,
|
||||
});
|
||||
assert.strictEqual(normalized.findings.length, expectedTotal);
|
||||
assert.strictEqual(normalized.findings.length, 4);
|
||||
});
|
||||
|
||||
test('falls back to node_modules/.bin/fallow when PATH does not contain fallow', () => {
|
||||
@@ -114,6 +115,7 @@ describe('feat-3210: fallow integration module', () => {
|
||||
const normalized = normalizeFallowReport(fixture);
|
||||
assert.deepStrictEqual(normalized.summary, {
|
||||
unused_exports: 0,
|
||||
unused_files: 0,
|
||||
duplicates: 0,
|
||||
circular_dependencies: 0,
|
||||
total: 0,
|
||||
@@ -133,45 +135,8 @@ describe('feat-3210: fallow integration module', () => {
|
||||
cleanup(tmp);
|
||||
});
|
||||
|
||||
// L3: runFallowAudit against a non-zero-exit binary must surface error state
|
||||
test('runFallowAudit surfaces error state when binary exits non-zero', async () => {
|
||||
const { runFallowAudit } = require('../gsd-core/bin/lib/fallow-runner.cjs');
|
||||
// N2: use shared helper
|
||||
const baseTmp = getWritableTmp();
|
||||
const tmp = fs.mkdtempSync(path.join(baseTmp, 'gsd-fallow-fail-'));
|
||||
const shimName = process.platform === 'win32' ? 'fallow.cmd' : 'fallow';
|
||||
const shimPath = path.join(tmp, shimName);
|
||||
|
||||
if (process.platform === 'win32') {
|
||||
fs.writeFileSync(shimPath, '@echo fallow-error-stderr 1>&2\r\n@exit 1\r\n');
|
||||
} else {
|
||||
fs.writeFileSync(shimPath, '#!/usr/bin/env sh\necho "fallow-error-stderr" >&2\nexit 1\n');
|
||||
fs.chmodSync(shimPath, 0o755);
|
||||
}
|
||||
|
||||
try {
|
||||
let errorState;
|
||||
try {
|
||||
errorState = await runFallowAudit({ cwd: tmp, env: { ...process.env, FALLOW_BIN_PATH: shimPath } });
|
||||
} catch (err) {
|
||||
// acceptable: some implementations throw rather than returning error state
|
||||
assert.ok(
|
||||
err.message.includes('fallow-error-stderr') || err.exitCode !== 0 || err.code !== 0,
|
||||
`expected thrown error to carry stderr content or non-zero exit; got: ${err.message}`,
|
||||
);
|
||||
return;
|
||||
}
|
||||
assert.ok(
|
||||
errorState && (errorState.error || errorState.exitCode !== 0 || errorState.failed),
|
||||
'runFallowAudit must return error state (error/exitCode/failed) when binary exits non-zero',
|
||||
);
|
||||
} finally {
|
||||
cleanup(tmp);
|
||||
}
|
||||
});
|
||||
|
||||
// M5: edge-case fixture — missing severity, similarity extremes, 3-node cycle, unicode path
|
||||
test('normalizes edge-case fixture: missing severity, similarity extremes, 3-node cycle, unicode path', () => {
|
||||
// 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(
|
||||
@@ -180,42 +145,49 @@ describe('feat-3210: fallow integration module', () => {
|
||||
),
|
||||
);
|
||||
|
||||
// M5: unusedExport with no severity field — round-trips without throwing
|
||||
assert.strictEqual(fixture.unusedExports.length, 1);
|
||||
assert.strictEqual(
|
||||
Object.prototype.hasOwnProperty.call(fixture.unusedExports[0], 'severity'),
|
||||
false,
|
||||
'edge-case fixture: unusedExport severity field must be absent',
|
||||
);
|
||||
// M5: unicode file path is preserved in fixture
|
||||
// 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.unusedExports[0].file.includes('café'),
|
||||
fixture.dead_code.unused_exports[0].path.includes('café'),
|
||||
'edge-case fixture: unicode file path must be present',
|
||||
);
|
||||
|
||||
// M5: duplicate entries with similarity at extremes 0.0 and 1.0
|
||||
assert.strictEqual(fixture.duplicates.length, 2);
|
||||
assert.strictEqual(fixture.duplicates[0].similarity, 0.0);
|
||||
assert.strictEqual(fixture.duplicates[1].similarity, 1.0);
|
||||
// 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);
|
||||
|
||||
// M5: 3-node circular dependency cycle (cycle array has 4 elements: A→B→C→A)
|
||||
assert.strictEqual(fixture.circularDependencies.length, 1);
|
||||
const cycle = fixture.circularDependencies[0].cycle;
|
||||
const uniqueNodes = new Set(cycle.slice(0, -1)); // last element repeats first
|
||||
assert.strictEqual(uniqueNodes.size, 3, 'edge-case: cycle must have exactly 3 unique nodes');
|
||||
// 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.unusedExports.length + fixture.duplicates.length + fixture.circularDependencies.length;
|
||||
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);
|
||||
|
||||
// M5: unicode path survives normalization
|
||||
// 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');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -223,23 +195,25 @@ 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 = {
|
||||
unusedExports: [{ file: 'src/a.ts', symbol: 'foo', line: 0 }],
|
||||
duplicates: [],
|
||||
circularDependencies: [],
|
||||
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 left.start (not coerced to null)', () => {
|
||||
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 = {
|
||||
unusedExports: [],
|
||||
duplicates: [{ left: { file: 'src/a.ts', start: 0 }, right: { file: 'src/b.ts', start: 5 }, similarity: 0.9 }],
|
||||
circularDependencies: [],
|
||||
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, 'left.start:0 must not be coerced to null via ||');
|
||||
assert.strictEqual(normalized.findings[0].line, 0, 'start_line:0 must not be coerced to null via ||');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -272,6 +246,69 @@ describe('feat-3210: M2 - node_modules/.bin resolution order', () => {
|
||||
});
|
||||
});
|
||||
|
||||
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.
|
||||
|
||||
48
tests/fixtures/fallow/sample-edge-cases.json
vendored
48
tests/fixtures/fallow/sample-edge-cases.json
vendored
@@ -1,47 +1 @@
|
||||
{
|
||||
"unusedExports": [
|
||||
{
|
||||
"file": "src/café/utils.ts",
|
||||
"symbol": "helperFn",
|
||||
"line": 42
|
||||
}
|
||||
],
|
||||
"duplicates": [
|
||||
{
|
||||
"left": {
|
||||
"file": "src/a.ts",
|
||||
"start": 1,
|
||||
"end": 5
|
||||
},
|
||||
"right": {
|
||||
"file": "src/b.ts",
|
||||
"start": 1,
|
||||
"end": 5
|
||||
},
|
||||
"similarity": 0.0
|
||||
},
|
||||
{
|
||||
"left": {
|
||||
"file": "src/c.ts",
|
||||
"start": 10,
|
||||
"end": 20
|
||||
},
|
||||
"right": {
|
||||
"file": "src/d.ts",
|
||||
"start": 10,
|
||||
"end": 20
|
||||
},
|
||||
"similarity": 1.0
|
||||
}
|
||||
],
|
||||
"circularDependencies": [
|
||||
{
|
||||
"cycle": [
|
||||
"src/x.ts",
|
||||
"src/y.ts",
|
||||
"src/z.ts",
|
||||
"src/x.ts"
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
{ "schema_version": 3, "version": "2.70.0", "command": "audit", "verdict": "fail", "changed_files_count": 3, "base_ref": "main", "head_sha": "def5678", "elapsed_ms": 88, "summary": { "dead_code_issues": 2, "dead_code_has_errors": true, "complexity_findings": 0, "max_cyclomatic": null, "duplication_clone_groups": 1 }, "attribution": { "gate": "new-only", "dead_code_introduced": 2, "dead_code_inherited": 0, "complexity_introduced": 0, "complexity_inherited": 0, "duplication_introduced": 1, "duplication_inherited": 0 }, "dead_code": { "schema_version": 6, "summary": { "total_issues": 2, "unused_files": 0, "unused_exports": 1, "circular_dependencies": 1 }, "unused_files": [], "unused_exports": [ { "path": "src/café/résumé.ts", "export_name": "naïveHelper", "is_type_only": false, "line": 0, "col": 0, "span_start": 0, "is_re_export": false, "introduced": true, "actions": [] } ], "circular_dependencies": [ { "files": ["src/a.ts", "src/b.ts", "src/c.ts"], "length": 3, "line": 2, "col": 4, "introduced": true, "actions": [] } ] }, "duplication": { "clone_groups": [ { "instances": [ { "file": "src/only-one.ts", "start_line": 5, "end_line": 30, "start_col": 0, "end_col": 1, "fragment": "..." } ] } ], "stats": { "clone_groups": 1 } }, "complexity": { "findings": [] } }
|
||||
|
||||
6
tests/fixtures/fallow/sample-empty.json
vendored
6
tests/fixtures/fallow/sample-empty.json
vendored
@@ -1,5 +1 @@
|
||||
{
|
||||
"unusedExports": [],
|
||||
"duplicates": [],
|
||||
"circularDependencies": []
|
||||
}
|
||||
{ "schema_version": 3, "version": "2.70.0", "command": "audit", "verdict": "pass", "changed_files_count": 0, "base_ref": "main", "head_sha": "0000000", "elapsed_ms": 5, "summary": { "dead_code_issues": 0, "dead_code_has_errors": false, "complexity_findings": 0, "max_cyclomatic": null, "duplication_clone_groups": 0 }, "attribution": { "gate": "new-only", "dead_code_introduced": 0, "dead_code_inherited": 0, "complexity_introduced": 0, "complexity_inherited": 0, "duplication_introduced": 0, "duplication_inherited": 0 }, "dead_code": { "schema_version": 6, "summary": { "total_issues": 0, "unused_files": 0, "unused_exports": 0, "circular_dependencies": 0 }, "unused_files": [], "unused_exports": [], "circular_dependencies": [] }, "duplication": { "clone_groups": [], "stats": { "clone_groups": 0 } }, "complexity": { "findings": [] } }
|
||||
|
||||
34
tests/fixtures/fallow/sample-findings.json
vendored
34
tests/fixtures/fallow/sample-findings.json
vendored
@@ -1,33 +1 @@
|
||||
{
|
||||
"unusedExports": [
|
||||
{
|
||||
"file": "sdk/src/query/commit.ts",
|
||||
"symbol": "commitToSubrepo",
|
||||
"line": 289
|
||||
}
|
||||
],
|
||||
"duplicates": [
|
||||
{
|
||||
"left": {
|
||||
"file": "gsd-core/bin/lib/config-schema.cjs",
|
||||
"start": 14,
|
||||
"end": 22
|
||||
},
|
||||
"right": {
|
||||
"file": "sdk/src/query/config-schema.ts",
|
||||
"start": 9,
|
||||
"end": 17
|
||||
},
|
||||
"similarity": 0.98
|
||||
}
|
||||
],
|
||||
"circularDependencies": [
|
||||
{
|
||||
"cycle": [
|
||||
"src/a.ts",
|
||||
"src/b.ts",
|
||||
"src/a.ts"
|
||||
]
|
||||
}
|
||||
]
|
||||
}
|
||||
{ "schema_version": 3, "version": "2.70.0", "command": "audit", "verdict": "fail", "changed_files_count": 5, "base_ref": "main", "head_sha": "abc1234", "elapsed_ms": 120, "summary": { "dead_code_issues": 3, "dead_code_has_errors": true, "complexity_findings": 0, "max_cyclomatic": null, "duplication_clone_groups": 1 }, "attribution": { "gate": "new-only", "dead_code_introduced": 3, "dead_code_inherited": 0, "complexity_introduced": 0, "complexity_inherited": 0, "duplication_introduced": 1, "duplication_inherited": 0 }, "dead_code": { "schema_version": 6, "summary": { "total_issues": 3, "unused_files": 1, "unused_exports": 1, "circular_dependencies": 1 }, "unused_files": [ { "path": "src/orphan.ts", "introduced": true, "actions": [] } ], "unused_exports": [ { "path": "sdk/src/query/commit.ts", "export_name": "commitToSubrepo", "is_type_only": false, "line": 289, "col": 16, "span_start": 1234, "is_re_export": false, "introduced": true, "actions": [] } ], "circular_dependencies": [ { "files": ["src/a.ts", "src/b.ts"], "length": 2, "line": 1, "col": 9, "introduced": true, "actions": [] } ] }, "duplication": { "clone_groups": [ { "instances": [ { "file": "gsd-core/bin/lib/config-schema.cjs", "start_line": 14, "end_line": 22, "start_col": 0, "end_col": 1, "fragment": "..." }, { "file": "sdk/src/query/config-schema.ts", "start_line": 9, "end_line": 17, "start_col": 0, "end_col": 1, "fragment": "..." } ] } ], "stats": { "clone_groups": 1 } }, "complexity": { "findings": [] } }
|
||||
|
||||
Reference in New Issue
Block a user