Merge pull request #218 from Raftersecurity/fix/217-betterleaks-null-report

fix(scanners): treat betterleaks null report as empty result set (#217)

Author: Rome-1

.github/copilot-instructions.md

@@ -32,4 +32,4 @@ Rafter is a security CLI for AI coding agents. It ships as two feature-identical
 - Scanners use dual-engine: Betterleaks binary first, regex fallback. Patterns defined in `secret-patterns.ts` / `secret_patterns.py`
 - Risk classification: critical > high > medium > low
 - Audit log: JSONL format, append-only, documented schema in CLI_SPEC.md
-- MCP server: 4 tools + 2 resources over stdio transport
+- MCP server: 4 tools + 3 resources over stdio transport

CLAUDE.md

@@ -60,7 +60,7 @@ cd python && poetry install && pytest
 
 **Secret scanning**: Dual-engine — tries Betterleaks binary first (higher accuracy), falls back to built-in regex patterns (21+ patterns, zero dependencies). Deterministic for a given version. Betterleaks is the gitleaks successor maintained by the original gitleaks authors. Existing installs with a leftover `~/.rafter/bin/gitleaks` are detected by `agent verify`/`status` so users get an upgrade hint, but the legacy CLI flags (`--with-gitleaks`, `--engine gitleaks`, `update-gitleaks`) have been removed.
 
-**MCP server**: `rafter mcp serve` exposes 4 tools (`scan_secrets`, `evaluate_command`, `read_audit_log`, `get_config`) and 2 resources (`rafter://config`, `rafter://policy`) over stdio.
+**MCP server**: `rafter mcp serve` exposes 4 tools (`scan_secrets`, `evaluate_command`, `read_audit_log`, `get_config`) and 3 resources (`rafter://config`, `rafter://policy`, `rafter://docs`) over stdio.
 
 ## Development
 

demo/README.md

@@ -18,7 +18,7 @@ The `/rafter-showcase` skill walks through all 9 core features with live command
 4. Audit logging (JSONL trail)
 5. Pre-commit hooks
 6. CI/CD integration (GitHub Actions)
-7. MCP server (4 tools, 2 resources)
+7. MCP server (4 tools, 3 resources)
 8. Skill auditing
 9. Remote SAST/SCA (requires API key)
 

node/src/commands/agent/scan.ts

@@ -16,7 +16,7 @@ import {
   policyIgnoreToSuppressions,
 } from "../../core/custom-patterns.js";
 import type { ScanIgnoreRule } from "../../core/config-schema.js";
-import { execSync, execFileSync } from "child_process";
+import { execFileSync } from "child_process";
 import fs from "fs";
 import os from "os";
 import path from "path";
@@ -492,7 +492,9 @@ async function runGitAddedLineScan(
 
     if (!patch.trim()) {
       if (!opts.quiet) {
-        console.log(`\n${fmt.success(emptyMessage)}\n`);
+        // Status line, so stderr — stdout must stay parseable as JSON under
+        // --json, and outputScanResults owns the single stdout success line.
+        console.error(fmt.success(emptyMessage));
       }
       outputScanResults([], opts, contextLabel, true, suppressions);
       return;
@@ -501,7 +503,9 @@ async function runGitAddedLineScan(
     const addedLines = parseUnifiedDiffAddedLines(patch);
     if (addedLines.length === 0) {
       if (!opts.quiet) {
-        console.log(`\n${fmt.success(emptyMessage)}\n`);
+        // Status line, so stderr — stdout must stay parseable as JSON under
+        // --json, and outputScanResults owns the single stdout success line.
+        console.error(fmt.success(emptyMessage));
       }
       outputScanResults([], opts, contextLabel, true, suppressions);
       return;

node/src/scanners/betterleaks.ts

@@ -206,6 +206,13 @@ export class BetterleaksScanner {
         return [];
       }
       const parsed = JSON.parse(content);
+      // #217 — betterleaks >=1.1.2 writes the literal `null` (not `[]`) for a
+      // clean scan via the `dir`/`git` subcommands we invoke. That is a valid
+      // empty result, not a version mismatch, so it must not reach the warning
+      // branch below — otherwise every clean file scanned emits warning noise.
+      if (parsed === null) {
+        return [];
+      }
       if (!Array.isArray(parsed)) {
         // sable-o4k — a stale/incompatible binary emits a non-array shape and
         // would otherwise silently yield zero findings. The managed binary is

node/tests/agent-commands.test.ts

@@ -707,7 +707,12 @@ describe("agent status", () => {
     const binDir = path.join(home, ".rafter", "bin");
     fs.mkdirSync(binDir, { recursive: true });
     fs.writeFileSync(path.join(binDir, "gitleaks"), "#!/bin/sh\necho fake\n", { mode: 0o755 });
-    const r = runCli("agent status", home);
+    // `agent status` probes `betterleaks` on PATH first and only falls through
+    // to the legacy-gitleaks hint when that fails — so the assertion below is
+    // only meaningful with an empty PATH. Otherwise this passes on CI and fails
+    // on any dev box that has betterleaks installed.
+    const emptyDir = fs.mkdtempSync(path.join(os.tmpdir(), "rafter-empty-path-"));
+    const r = runCli("agent status", home, { PATH: emptyDir });
     expect(r.stdout).toMatch(/legacy gitleaks/i);
     expect(r.stdout).toMatch(/update-betterleaks/i);
   });

node/tests/betterleaks-parse-report.test.ts

@@ -0,0 +1,77 @@
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
+import fs from "fs";
+import os from "os";
+import path from "path";
+import { BetterleaksScanner } from "../src/scanners/betterleaks.js";
+
+// parseResults is private; exercise it directly rather than shelling out to the
+// real binary, so these stay hermetic and run without betterleaks installed.
+const scanner = new BetterleaksScanner();
+const parseResults = (scanner as any).parseResults.bind(scanner);
+
+let tmpDir: string;
+let stderrSpy: ReturnType<typeof vi.spyOn>;
+
+function writeReport(content: string): string {
+  const p = path.join(tmpDir, "report.json");
+  fs.writeFileSync(p, content, "utf-8");
+  return p;
+}
+
+beforeEach(() => {
+  tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "bl-parse-test-"));
+  stderrSpy = vi.spyOn(console, "error").mockImplementation(() => {});
+});
+
+afterEach(() => {
+  stderrSpy.mockRestore();
+  fs.rmSync(tmpDir, { recursive: true, force: true });
+});
+
+describe("BetterleaksScanner.parseResults", () => {
+  // #217 — betterleaks >=1.1.2 writes the literal `null` for a clean scan via
+  // the `dir`/`git` subcommands. Regression guard: this is an empty result, not
+  // a version mismatch, and must not emit warning noise on every clean file.
+  it("treats a literal `null` report as an empty result set", () => {
+    expect(parseResults(writeReport("null"))).toEqual([]);
+  });
+
+  it("does not warn on a `null` report", () => {
+    parseResults(writeReport("null"));
+    expect(stderrSpy).not.toHaveBeenCalled();
+  });
+
+  it("tolerates trailing whitespace around `null`", () => {
+    expect(parseResults(writeReport("null\n"))).toEqual([]);
+    expect(stderrSpy).not.toHaveBeenCalled();
+  });
+
+  it("returns an empty result set for an empty report", () => {
+    expect(parseResults(writeReport(""))).toEqual([]);
+    expect(stderrSpy).not.toHaveBeenCalled();
+  });
+
+  it("returns findings from a normal array report", () => {
+    const findings = [{ RuleID: "aws-secret-key", Description: "AWS key", StartLine: 3 }];
+    expect(parseResults(writeReport(JSON.stringify(findings)))).toEqual(findings);
+    expect(stderrSpy).not.toHaveBeenCalled();
+  });
+
+  it("returns an empty result set for an empty array report", () => {
+    expect(parseResults(writeReport("[]"))).toEqual([]);
+    expect(stderrSpy).not.toHaveBeenCalled();
+  });
+
+  // sable-o4k — the stale-binary guard must survive the #217 fix.
+  it("still warns about a non-array object report", () => {
+    expect(parseResults(writeReport('{"findings": []}'))).toEqual([]);
+    expect(stderrSpy).toHaveBeenCalledOnce();
+    expect(String(stderrSpy.mock.calls[0][0])).toContain("possible version mismatch");
+  });
+
+  it("still warns about malformed JSON", () => {
+    expect(parseResults(writeReport("{not json"))).toEqual([]);
+    expect(stderrSpy).toHaveBeenCalledOnce();
+    expect(String(stderrSpy.mock.calls[0][0])).toContain("Failed to parse");
+  });
+});

python/rafter_cli/commands/agent.py

@@ -1711,6 +1711,31 @@ def run_patterns() -> list[ScanResult]:
     return run_patterns()
 
 
+def _output_empty_diff_scan(
+    empty_message: str,
+    json_output: bool,
+    quiet: bool,
+    context_label: str,
+    format: str,
+    suppressions,
+) -> None:
+    """Emit the result of a diff scan that had no files to scan.
+
+    ``empty_message`` ("No files changed since <ref>") is a *status* line, so it
+    goes to stderr — stdout has to stay parseable as JSON under ``--json``, and
+    ``_output_scan_results`` owns the single stdout success line. Mirrors the
+    empty branches of ``runGitAddedLineScan`` in
+    node/src/commands/agent/scan.ts — keep the two in sync.
+    """
+    if not quiet:
+        # rprint, not print — fmt.success returns rich markup that plain print
+        # would emit literally as "[green]...[/green]".
+        rprint(fmt.success(empty_message), file=sys.stderr)
+    _output_scan_results(
+        [], json_output, quiet, context_label, format=format, suppressions=suppressions
+    )
+
+
 def _run_git_added_line_scan(
     git_args: list[str],
     git_cwd: str | None,
@@ -1743,14 +1768,16 @@ def _run_git_added_line_scan(
         raise typer.Exit(code=2)
 
     if not patch.strip():
-        if not quiet:
-            rprint(fmt.success(empty_message))
+        _output_empty_diff_scan(
+            empty_message, json_output, quiet, context_label, format, suppressions
+        )
         raise typer.Exit(code=0)
 
     added = parse_unified_diff_added_lines(patch)
     if not added:
-        if not quiet:
-            rprint(fmt.success(empty_message))
+        _output_empty_diff_scan(
+            empty_message, json_output, quiet, context_label, format, suppressions
+        )
         raise typer.Exit(code=0)
 
     try:

python/rafter_cli/scanners/betterleaks.py

@@ -164,29 +164,46 @@ def _run_scan(self, target: str, *, use_git: bool = False) -> list[dict]:
                     )
                 return []
 
-            try:
-                with open(report_path) as f:
-                    content = f.read().strip()
-                if not content:
-                    return []
-                parsed = json.loads(content)
-            except json.JSONDecodeError as exc:
-                print(f"[rafter] Warning: Failed to parse Betterleaks report: {exc}", file=sys.stderr)
-                return []
+            return self._parse_report(report_path)
 
-            if not isinstance(parsed, list):
-                # sable-o4k — a stale/incompatible binary emits a non-array
-                # shape and would otherwise silently yield zero findings. The
-                # managed binary is auto-updated upstream; this covers a stale
-                # binary on PATH, which we can't safely overwrite — so point the
-                # user at the fix.
-                print(
-                    "[rafter] Warning: Betterleaks output is not an array — possible version mismatch. "
-                    "Run: rafter agent update-betterleaks",
-                    file=sys.stderr,
-                )
+    @staticmethod
+    def _parse_report(report_path: str) -> list[dict]:
+        """Read a betterleaks JSON report into a findings list.
+
+        Mirrors `parseResults` in node/src/scanners/betterleaks.ts — keep the
+        two in sync.
+        """
+        try:
+            with open(report_path) as f:
+                content = f.read().strip()
+            if not content:
                 return []
-            return parsed
+            parsed = json.loads(content)
+        except json.JSONDecodeError as exc:
+            print(f"[rafter] Warning: Failed to parse Betterleaks report: {exc}", file=sys.stderr)
+            return []
+
+        # #217 — betterleaks >=1.1.2 writes the literal `null` (not `[]`)
+        # for a clean scan via the `dir`/`git` subcommands we invoke. That
+        # is a valid empty result, not a version mismatch, so it must not
+        # reach the warning branch below — otherwise every clean file
+        # scanned emits warning noise.
+        if parsed is None:
+            return []
+
+        if not isinstance(parsed, list):
+            # sable-o4k — a stale/incompatible binary emits a non-array
+            # shape and would otherwise silently yield zero findings. The
+            # managed binary is auto-updated upstream; this covers a stale
+            # binary on PATH, which we can't safely overwrite — so point the
+            # user at the fix.
+            print(
+                "[rafter] Warning: Betterleaks output is not an array — possible version mismatch. "
+                "Run: rafter agent update-betterleaks",
+                file=sys.stderr,
+            )
+            return []
+        return parsed
 
     @staticmethod
     def _convert(result: dict) -> PatternMatch:

python/tests/test_betterleaks_parse_report.py

@@ -0,0 +1,59 @@
+"""Tests for BetterleaksScanner._parse_report.
+
+Mirrors node/tests/betterleaks-parse-report.test.ts — keep the two in sync.
+Exercises the parser directly so the tests stay hermetic and run without the
+betterleaks binary installed.
+"""
+from __future__ import annotations
+
+import json
+
+import pytest
+
+from rafter_cli.scanners.betterleaks import BetterleaksScanner
+
+
+@pytest.fixture
+def write_report(tmp_path):
+    def _write(content: str) -> str:
+        p = tmp_path / "report.json"
+        p.write_text(content)
+        return str(p)
+
+    return _write
+
+
+class TestParseReport:
+    # #217 — betterleaks >=1.1.2 writes the literal `null` for a clean scan via
+    # the `dir`/`git` subcommands. Regression guard: this is an empty result,
+    # not a version mismatch, and must not emit warning noise on every clean
+    # file scanned.
+    def test_null_report_is_empty_result(self, write_report, capsys):
+        assert BetterleaksScanner._parse_report(write_report("null")) == []
+        assert capsys.readouterr().err == ""
+
+    def test_null_report_with_trailing_whitespace(self, write_report, capsys):
+        assert BetterleaksScanner._parse_report(write_report("null\n")) == []
+        assert capsys.readouterr().err == ""
+
+    def test_empty_report(self, write_report, capsys):
+        assert BetterleaksScanner._parse_report(write_report("")) == []
+        assert capsys.readouterr().err == ""
+
+    def test_array_report_returns_findings(self, write_report, capsys):
+        findings = [{"RuleID": "aws-secret-key", "Description": "AWS key", "StartLine": 3}]
+        assert BetterleaksScanner._parse_report(write_report(json.dumps(findings))) == findings
+        assert capsys.readouterr().err == ""
+
+    def test_empty_array_report(self, write_report, capsys):
+        assert BetterleaksScanner._parse_report(write_report("[]")) == []
+        assert capsys.readouterr().err == ""
+
+    # sable-o4k — the stale-binary guard must survive the #217 fix.
+    def test_non_array_object_still_warns(self, write_report, capsys):
+        assert BetterleaksScanner._parse_report(write_report('{"findings": []}')) == []
+        assert "possible version mismatch" in capsys.readouterr().err
+
+    def test_malformed_json_still_warns(self, write_report, capsys):
+        assert BetterleaksScanner._parse_report(write_report("{not json")) == []
+        assert "Failed to parse" in capsys.readouterr().err