From e4dfa6b9ea7907cff83c31cc9c961d6df3920df3 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 11 Jun 2026 11:36:50 -0400 Subject: [PATCH] fix(#1012): invoke fallow with its real CLI and wire the report normalizer (#1044) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * chore(#1012): backfill changeset PR number to 1044 Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- .changeset/1012-fallow-real-cli-and-schema.md | 5 + docs/CONFIGURATION.md | 2 +- gsd-core/workflows/code-review.md | 65 ++++-- src/fallow-runner.cts | 157 +++++++++++---- tests/feat-3210-fallow-integration.test.cjs | 185 +++++++++++------- tests/fixtures/fallow/sample-edge-cases.json | 48 +---- tests/fixtures/fallow/sample-empty.json | 6 +- tests/fixtures/fallow/sample-findings.json | 34 +--- 8 files changed, 288 insertions(+), 214 deletions(-) create mode 100644 .changeset/1012-fallow-real-cli-and-schema.md diff --git a/.changeset/1012-fallow-real-cli-and-schema.md b/.changeset/1012-fallow-real-cli-and-schema.md new file mode 100644 index 000000000..267e154b9 --- /dev/null +++ b/.changeset/1012-fallow-real-cli-and-schema.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1044 +--- +**`/gsd-code-review`'s fallow structural pre-pass now actually runs and delivers findings** — it invoked fallow with flags no published fallow version accepts (`--json`, `--profile`, `--stdin-files`), so the pre-pass failed on every run and silently degraded (the structural-findings feature never delivered on any fallow version). It now uses fallow's real CLI (`audit --format json --quiet`, `--changed-since` for phase scope, and `--max-crap` mapped from the `code_quality.fallow.profile` preset: minimal→50, standard→30, strict→15), treats fallow's exit code 1 ("issues found") as a successful run instead of a crash (gating on a valid JSON report, not the exit code), and normalizes fallow's real `audit --format json` schema (`dead_code.*`, `duplication.clone_groups`) into the reviewer's `` contract. The report normalizer — previously dead code parsing a schema fallow never shipped — is wired to the real schema and exercised against real fallow output. (#1012) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 3b77b2632..2e1dc86b0 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -292,7 +292,7 @@ The `code_quality.*` namespace gates optional structural-analysis tooling that a |---------|------|---------|-------------| | `code_quality.fallow.enabled` | boolean | `false` | Enables fallow structural pre-pass for `/gsd-code-review`. When `false`, no fallow binary probe or JSON artifact is produced. | | `code_quality.fallow.scope` | string | `phase` | Scope for fallow analysis: `phase` (current review file scope) or `repo` (entire repository). | -| `code_quality.fallow.profile` | string | `standard` | Fallow profile selector passed to the pre-pass runner (`minimal`, `standard`, `strict`). | +| `code_quality.fallow.profile` | string | `standard` | Strictness preset for the fallow pre-pass (`minimal`, `standard`, `strict`). Fallow has no native profile concept, so this maps to its `--max-crap` complexity threshold: `minimal`→50, `standard`→30, `strict`→15 (lower = stricter). | | `code_quality.fallow.mcp` | boolean | `false` | **Reserved — not yet implemented.** When `true`, enables MCP-backed structural findings mode for runtimes that support MCP server routing. Setting this to `true` is currently a no-op and emits a runtime warning. | ## Ship Settings diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index 43fe0f282..08562c3e7 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -329,12 +329,19 @@ FALLOW_ENABLED=$(gsd_run query config-get code_quality.fallow.enabled 2>/dev/nul FALLOW_SCOPE=$(gsd_run query config-get code_quality.fallow.scope 2>/dev/null || echo "phase") FALLOW_PROFILE=$(gsd_run query config-get code_quality.fallow.profile 2>/dev/null || echo "standard") FALLOW_MCP=$(gsd_run query config-get code_quality.fallow.mcp 2>/dev/null || echo "false") +# profile maps to a --max-crap threshold since fallow has no native profile concept. +# minimal=50 (more lenient), standard=30 (default), strict=15 (tighter). +case "$FALLOW_PROFILE" in + minimal) FALLOW_MAX_CRAP=50 ;; + strict) FALLOW_MAX_CRAP=15 ;; + *) FALLOW_MAX_CRAP=30 ;; # standard (default) +esac ``` Defaults are fail-closed and opt-in: - `enabled=false` (skip entirely) - `scope=phase` -- `profile=standard` +- `profile=standard` (maps to `--max-crap 30`; minimal=50, standard=30, strict=15 — fallow has no native profile concept) - `mcp=false` When `FALLOW_ENABLED=true`: @@ -357,30 +364,49 @@ if [ -z \"$FALLOW_BIN\" ]; then fi ``` -3) Execute structural pass and persist JSON (bounded at 120s; on timeout, behaves as a fallow crash): +3) Execute structural pass and persist JSON (bounded at 120s). Note: `fallow audit` exits 0 when clean and 1 when issues are found — BOTH are successful runs. Only a timeout (124), usage error (2), or crash yields no usable JSON; success is decided by whether the output parses as a valid fallow report, not by exit code: ```bash FALLOW_JSON_PATH="${PHASE_DIR}/FALLOW.json" FALLOW_STDERR_TMP=$(mktemp) -if [ \"$FALLOW_SCOPE\" = \"repo\" ]; then - timeout 120 \"$FALLOW_BIN\" audit --json --profile \"$FALLOW_PROFILE\" > \"${FALLOW_JSON_PATH}.tmp\" 2>\"$FALLOW_STDERR_TMP\" - FALLOW_EXIT=$? -else - # phase scope: pass the already-computed review file set - printf '%s\n' \"${REVIEW_FILES[@]}\" | timeout 120 \"$FALLOW_BIN\" audit --json --profile \"$FALLOW_PROFILE\" --stdin-files > \"${FALLOW_JSON_PATH}.tmp\" 2>\"$FALLOW_STDERR_TMP\" - FALLOW_EXIT=$? + +# Phase scope uses fallow's native changed-files scoping (--changed-since ). +# Derive the phase base commit; if none is found, fall back to repo scope (fallow +# auto-detects the base branch). +FALLOW_SCOPE_ARGS=() +if [ \"$FALLOW_SCOPE\" = \"phase\" ]; then + FALLOW_PHASE_COMMITS=$(git log --oneline --all --grep=\"${PADDED_PHASE}\" --format=\"%H\" 2>/dev/null) + if [ -n \"$FALLOW_PHASE_COMMITS\" ]; then + FALLOW_BASE=$(echo \"$FALLOW_PHASE_COMMITS\" | tail -1)^ + FALLOW_SCOPE_ARGS=(--changed-since \"$FALLOW_BASE\") + fi fi -if [ $FALLOW_EXIT -ne 0 ]; then + +timeout 120 \"$FALLOW_BIN\" audit --format json --quiet --max-crap \"$FALLOW_MAX_CRAP\" \"${FALLOW_SCOPE_ARGS[@]+\"${FALLOW_SCOPE_ARGS[@]}\"}\" > \"${FALLOW_JSON_PATH}.tmp\" 2>\"$FALLOW_STDERR_TMP\" +FALLOW_EXIT=$? + +# fallow exits 0 (clean) or 1 (issues found) — BOTH are successful runs that produce a +# valid JSON report. Only a timeout (124), usage error (2), or crash yields no usable JSON. +# Decide success by whether the output parses as a fallow report, not by exit code. +FALLOW_OK=$(FALLOW_TMP=\"${FALLOW_JSON_PATH}.tmp\" node -e \" + try { + const fs = require('fs'); + const txt = fs.readFileSync(process.env.FALLOW_TMP, 'utf8'); + const o = JSON.parse(txt); + process.stdout.write(o && typeof o === 'object' && 'verdict' in o ? '1' : '0'); + } catch { process.stdout.write('0'); } +\") +if [ \"$FALLOW_OK\" != \"1\" ]; then FALLOW_STDERR_SUMMARY=$(head -5 \"$FALLOW_STDERR_TMP\") rm -f \"${FALLOW_JSON_PATH}.tmp\" \"$FALLOW_STDERR_TMP\" - echo \"WARNING: fallow structural pre-pass failed: ${FALLOW_STDERR_SUMMARY}\" - FALLOW_JSON_PATH="" + echo \"WARNING: fallow structural pre-pass failed (exit ${FALLOW_EXIT}): ${FALLOW_STDERR_SUMMARY}\" + FALLOW_JSON_PATH=\"\" else mv \"${FALLOW_JSON_PATH}.tmp\" \"$FALLOW_JSON_PATH\" rm -f \"$FALLOW_STDERR_TMP\" fi ``` -On any failure of the structural pre-pass (binary missing, non-zero exit, timeout, or JSON parse error), the workflow continues with no `` injection; the reviewer agent receives a normal review request. +On any failure of the structural pre-pass (binary missing, timeout, empty output, or unparseable JSON), the workflow continues with no `` injection; the reviewer agent receives a normal review request. 4) Optional MCP bridge path (runtime-dependent): - If `FALLOW_MCP=true`, set reviewer input mode to MCP-backed structural findings. @@ -429,10 +455,19 @@ Build structural findings block for agent: STRUCTURAL_FINDINGS_BLOCK="" MAX_FINDINGS_SIZE=50000 if [ -n "$FALLOW_JSON_PATH" ] && [ -f "$FALLOW_JSON_PATH" ]; then - FALLOW_JSON_SIZE=$(wc -c < "$FALLOW_JSON_PATH" | tr -d '[:space:]') + # Normalize fallow's raw report into the compact {summary, findings[]} contract + # the reviewer consumes (real fallow schema -> normalized findings). + FALLOW_NORMALIZED_PATH="${PHASE_DIR}/FALLOW-normalized.json" + FALLOW_SRC="$FALLOW_JSON_PATH" FALLOW_OUT="$FALLOW_NORMALIZED_PATH" node -e " + const fs = require('fs'); + const { normalizeFallowReportFile } = require('./gsd-core/bin/lib/fallow-runner.cjs'); + const n = normalizeFallowReportFile(process.env.FALLOW_SRC); + fs.writeFileSync(process.env.FALLOW_OUT, JSON.stringify(n, null, 2)); + " 2>/dev/null && FALLOW_EMBED_PATH="$FALLOW_NORMALIZED_PATH" || FALLOW_EMBED_PATH="$FALLOW_JSON_PATH" + FALLOW_JSON_SIZE=$(wc -c < "$FALLOW_EMBED_PATH" | tr -d '[:space:]') if [ "$FALLOW_JSON_SIZE" -le "$MAX_FINDINGS_SIZE" ]; then # Escape any literal closing tag before embedding; the closing tag literal is escaped to prevent prompt-structure breakage if a fallow finding's file path or message contains the sequence. - SAFE_FALLOW_JSON=$(sed 's##<\/structural_findings>#g' "$FALLOW_JSON_PATH") + SAFE_FALLOW_JSON=$(sed 's##<\/structural_findings>#g' "$FALLOW_EMBED_PATH") STRUCTURAL_FINDINGS_BLOCK=$(printf '\n%s\n\n' "$SAFE_FALLOW_JSON") else echo "Warning: skipping structural findings embed (${FALLOW_JSON_SIZE} bytes > ${MAX_FINDINGS_SIZE} bytes). Re-run with narrower scope/profile if needed." diff --git a/src/fallow-runner.cts b/src/fallow-runner.cts index 5c891aaa8..42833141e 100644 --- a/src/fallow-runner.cts +++ b/src/fallow-runner.cts @@ -4,6 +4,9 @@ * ADR-457 build-at-publish: the hand-written bin/lib/fallow-runner.cjs * collapsed to a TypeScript source of truth. Behaviour is preserved * byte-for-behaviour from the prior hand-written .cjs; only types are added. + * + * Parses the real fallow `audit --format json` schema (schema_version 3 + * envelope, nested dead_code/duplication sections). See fallow 2.70.0+. */ import fs from 'node:fs'; @@ -67,35 +70,79 @@ export function requireFallowBinary({ cwd, envPath = process.env['PATH'] ?? '' } ); } +// --- Real fallow audit --format json schema (schema_version 3) interfaces --- + interface FallowUnusedExport { - symbol?: string; - file?: string; + path?: string; + export_name?: string; + is_type_only?: boolean; line?: number | null; + col?: number | null; + span_start?: number | null; + is_re_export?: boolean; + actions?: unknown[]; + introduced?: boolean; } -interface FallowDuplicateItem { +interface FallowUnusedFile { + path?: string; + actions?: unknown[]; + introduced?: boolean; +} + +interface FallowCircularDependency { + files?: string[]; + length?: number; + line?: number | null; + col?: number | null; + actions?: unknown[]; + introduced?: boolean; +} + +interface FallowCloneInstance { file?: string; - start?: number | null; + start_line?: number | null; + end_line?: number | null; + start_col?: number | null; + end_col?: number | null; + fragment?: string; } -interface FallowDuplicate { - similarity?: number; - left?: FallowDuplicateItem; - right?: FallowDuplicateItem; +interface FallowCloneGroup { + instances?: FallowCloneInstance[]; } -interface FallowCircular { - cycle?: string[]; +interface FallowDeadCode { + unused_exports?: FallowUnusedExport[]; + unused_files?: FallowUnusedFile[]; + circular_dependencies?: FallowCircularDependency[]; + summary?: unknown; + schema_version?: number; +} + +interface FallowDuplication { + clone_groups?: FallowCloneGroup[]; + stats?: unknown; } interface FallowReport { - unusedExports?: unknown[]; - duplicates?: unknown[]; - circularDependencies?: unknown[]; + schema_version?: number; + version?: string; + command?: string; + verdict?: string; + changed_files_count?: number; + base_ref?: string; + head_sha?: string; + elapsed_ms?: number; + summary?: unknown; + attribution?: unknown; + dead_code?: FallowDeadCode; + duplication?: FallowDuplication; + complexity?: unknown; } export interface FallowFinding { - type: 'unused_export' | 'duplicate_block' | 'circular_dependency'; + type: 'unused_export' | 'unused_file' | 'duplicate_block' | 'circular_dependency'; message: string; file: string; line: number | null; @@ -105,6 +152,7 @@ export interface FallowFinding { export interface NormalizedFallowReport { summary: { unused_exports: number; + unused_files: number; duplicates: number; circular_dependencies: number; total: number; @@ -113,53 +161,84 @@ export interface NormalizedFallowReport { } export function normalizeFallowReport(report: FallowReport | null | undefined): NormalizedFallowReport { - const unused: FallowUnusedExport[] = Array.isArray(report?.unusedExports) - ? (report.unusedExports as FallowUnusedExport[]) - : []; - const duplicates: FallowDuplicate[] = Array.isArray(report?.duplicates) - ? (report.duplicates as FallowDuplicate[]) - : []; - const circular: FallowCircular[] = Array.isArray(report?.circularDependencies) - ? (report.circularDependencies as FallowCircular[]) - : []; + const deadCodeRaw = report?.dead_code; + const duplicationRaw = report?.duplication; + const unusedExports: FallowUnusedExport[] = (Array.isArray(deadCodeRaw?.unused_exports) + ? (deadCodeRaw?.unused_exports ?? []) + : []).filter((x): x is FallowUnusedExport => x !== null && typeof x === 'object'); + const unusedFiles: FallowUnusedFile[] = (Array.isArray(deadCodeRaw?.unused_files) + ? (deadCodeRaw?.unused_files ?? []) + : []).filter((x): x is FallowUnusedFile => x !== null && typeof x === 'object'); + const circularDeps: FallowCircularDependency[] = (Array.isArray(deadCodeRaw?.circular_dependencies) + ? (deadCodeRaw?.circular_dependencies ?? []) + : []).filter((x): x is FallowCircularDependency => x !== null && typeof x === 'object'); + const cloneGroups: FallowCloneGroup[] = (Array.isArray(duplicationRaw?.clone_groups) + ? (duplicationRaw?.clone_groups ?? []) + : []).filter((x): x is FallowCloneGroup => x !== null && typeof x === 'object'); const findings: FallowFinding[] = []; - for (const item of unused) { + for (const item of unusedExports) { + if (!item || typeof item !== 'object') continue; findings.push({ type: 'unused_export', - message: `Unused export ${item.symbol ?? ''}`, - file: item.file ?? '', + message: `Unused export ${item.export_name ?? ''}`, + file: item.path ?? '', line: item.line ?? null, }); } - for (const item of duplicates) { + for (const item of unusedFiles) { + if (!item || typeof item !== 'object') continue; findings.push({ - type: 'duplicate_block', - message: `Duplicate block (${Math.round((item.similarity ?? 0) * 100)}% similarity)`, - file: item.left?.file ?? '', - line: item.left?.start ?? null, - related_file: item.right?.file ?? '', + type: 'unused_file', + message: `Unused file ${item.path ?? ''}`, + file: item.path ?? '', + line: null, }); } - for (const item of circular) { + for (const item of circularDeps) { + if (!item || typeof item !== 'object') continue; + const files = Array.isArray(item.files) ? item.files : []; findings.push({ type: 'circular_dependency', - message: `Circular dependency: ${(item.cycle ?? []).join(' -> ')}`, - file: Array.isArray(item.cycle) && item.cycle.length > 0 ? item.cycle[0] : '', - line: null, + message: `Circular dependency: ${files.join(' -> ')}`, + file: files.length > 0 ? files[0] : '', + line: item.line ?? null, + }); + } + + for (const group of cloneGroups) { + if (!group || typeof group !== 'object') continue; + const instances = Array.isArray(group.instances) ? group.instances : []; + findings.push({ + type: 'duplicate_block', + message: `Duplicate block (${instances.length} instances)`, + file: instances[0]?.file ?? '', + line: instances[0]?.start_line ?? null, + related_file: instances[1]?.file ?? '', }); } return { summary: { - unused_exports: unused.length, - duplicates: duplicates.length, - circular_dependencies: circular.length, + unused_exports: unusedExports.length, + unused_files: unusedFiles.length, + duplicates: cloneGroups.length, + circular_dependencies: circularDeps.length, total: findings.length, }, findings, }; } + +export function normalizeFallowReportFile(filePath: string): NormalizedFallowReport { + try { + const raw = fs.readFileSync(filePath, 'utf8'); + const parsed = JSON.parse(raw) as FallowReport; + return normalizeFallowReport(parsed); + } catch { + return normalizeFallowReport(null); + } +} diff --git a/tests/feat-3210-fallow-integration.test.cjs b/tests/feat-3210-fallow-integration.test.cjs index 92ab45b40..59ea3e4dd 100644 --- a/tests/feat-3210-fallow-integration.test.cjs +++ b/tests/feat-3210-fallow-integration.test.cjs @@ -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. diff --git a/tests/fixtures/fallow/sample-edge-cases.json b/tests/fixtures/fallow/sample-edge-cases.json index 78a1cb35e..86d7b243e 100644 --- a/tests/fixtures/fallow/sample-edge-cases.json +++ b/tests/fixtures/fallow/sample-edge-cases.json @@ -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": [] } } diff --git a/tests/fixtures/fallow/sample-empty.json b/tests/fixtures/fallow/sample-empty.json index 3a890d96b..c1edd6f9d 100644 --- a/tests/fixtures/fallow/sample-empty.json +++ b/tests/fixtures/fallow/sample-empty.json @@ -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": [] } } diff --git a/tests/fixtures/fallow/sample-findings.json b/tests/fixtures/fallow/sample-findings.json index 83d6e9e98..e80f34929 100644 --- a/tests/fixtures/fallow/sample-findings.json +++ b/tests/fixtures/fallow/sample-findings.json @@ -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": [] } }