fix(harness): fail closed at review and privacy boundaries

docs/specs/2026-07-29-11-18-review-boundary-hardening.md

@@ -0,0 +1,135 @@
+# Fail-Closed Review and Privacy Boundaries
+
+## Traceability
+
+- Spec ID: review-boundary-hardening-11-18
+- Story: #11, #12, #13, #14, #15, #16, #17, #18
+- Status: Implemented
+
+## Intent
+
+Make Better Harness honor its declared privacy controls and fail closed when
+review, filesystem, or report-integrity boundaries cannot be verified. The
+change fixes eight independently reproduced High findings without expanding
+the product surface or changing successful-path report semantics.
+
+## Acceptance Scenarios
+
+- AC-01 (#11): Secret Guard normalizes supported snake_case and camelCase tool
+  payloads, scans secret-bearing Write/Edit/apply_patch content, blocks
+  synthetic secrets without echoing them, and preserves existing Bash/path
+  decisions.
+- AC-02 (#12): Workspace-scoped Qoder analysis excludes home-only sessions that
+  have no verified relationship to the requested workspace across sessions,
+  events/show, facets, insights, file-reads, and usage-summary. A matching
+  record cannot authorize foreign-cwd records from the same home session, and
+  cwd-less records require a workspace-linked session source instead of being
+  assigned to the requested workspace. An explicit global-capability pass can
+  retain home-only sessions only as `user-global` evidence.
+- AC-03 (#13): `report --no-sessions` executes no session probe and always uses
+  the static `software-fluency` route even when local sessions exist.
+- AC-04 (#14): Blast-radius collection distinguishes an invalid/unavailable
+  base ref from a real empty diff and returns an explicit fail-closed error.
+- AC-05 (#15): review-trigger argument, Git worktree probes, and runtime
+  failures exit non-zero while successful findings retain their documented
+  non-blocking result.
+- AC-06 (#16): `report --cwd` rejects empty, missing, and non-directory targets
+  before starting evidence collectors; valid-directory fallback behavior
+  remains available.
+- AC-07 (#17): findings repair never lowers `Critical` or arbitrary unknown
+  severity to `Medium`; unsupported values fail closed or use an explicit
+  conservative mapping.
+- AC-08 (#18): Git-backed cloc never follows a tracked path to a target outside
+  the repository, reads a non-regular file, or reads a regular file beyond the
+  configured size boundary; skipped results remain bounded and do not reveal
+  the external target.
+- AC-09: Focused tests, the full Node test suite, package verification, syntax
+  checks, and review-readiness checks pass on Linux-compatible local tooling;
+  cross-platform path behavior remains covered by portable fixtures.
+
+## Non-goals
+
+- Do not redesign report scoring, add a new severity level to the public
+  findings schema, or alter valid High/Medium/Low findings.
+- Do not add authentication or network sharing to Canvas preview.
+- Do not change the Medium large-untracked-file finding or architecture
+  watchlist items from the originating review.
+- Do not add dependencies.
+- Do not refactor unrelated session, report, or Git-analysis behavior.
+
+## Plan and Tasks
+
+1. Add failing regression tests for each issue before changing implementation.
+2. Normalize Secret Guard tool events once, then scan only the content fields
+   owned by matched write tools.
+3. Apply workspace ownership at both Qoder home-session discovery and per-event
+   hydration, keeping explicit `user-global` behavior separate.
+4. Make quickstart privacy and empty/missing cwd validation explicit at its CLI
+   boundary.
+5. Replace Git diff ambiguity with a structured failure from blast-radius
+   collection.
+6. Separate review-trigger argument, Git-probe, and execution failures from
+   successful non-blocking findings.
+7. Make severity repair conservative and make cloc file reads type-, size-,
+   and containment-aware.
+8. Run focused tests per module, then the complete repository checks.
+9. Perform independent code-review and architecture lanes, address all
+   Critical/High findings and any architectural Block, then run Review
+   Readiness before commit and PR.
+
+Decision rationale:
+
+- Fail closed only when the tool cannot establish the requested boundary;
+  preserve supported fallback behavior for valid inputs with partial evidence.
+- Prefer small checks at existing owner boundaries over new shared abstraction
+  layers.
+- Use synthetic credentials and temporary repositories only; tests must not
+  retain private prompts, paths, or secrets.
+
+## Test and Review Evidence
+
+- AC-01:
+  `node --test test/agent-guardrails-secret-scan.test.mjs`
+- AC-02:
+  `node --test test/session-analysis.test.mjs test/session-usage-summary.test.mjs`
+  including mixed-cwd, cwd-less, and explicit user-global home-session cases
+- AC-03 and AC-06:
+  `node --test test/harness-quickstart.test.mjs test/better-harness-cli.test.mjs`
+- AC-04:
+  `node --test test/blast-radius.test.mjs`
+- AC-05:
+  `node --test test/review-trigger.test.mjs`, including a non-Git directory
+- AC-07:
+  `node --test test/harness-findings-repair.test.mjs test/harness-report-render-cli.test.mjs`
+- AC-08:
+  `node --test test/cloc.test.mjs`, including an oversized regular file
+- AC-09:
+  `npm test`
+  `npm run pack:verify`
+  `node --test test/doc-link-graph.test.mjs`
+
+Review evidence:
+
+- Review follow-up (2026-07-29): mixed-cwd and cwd-less home-session hydration,
+  explicit user-global analysis, failed Git worktree probes, empty `--cwd=`,
+  and bounded cloc reads are covered by regressions and resolved before merge.
+- `npm run check` passed with the full Node suite and package verification.
+- The review follow-up focused run passed 80 tests across Qoder session
+  analysis, session usage summary, review-trigger, quickstart, and cloc;
+  syntax and diff checks also passed.
+- Code-reviewer recommendation: `APPROVE` after both reported High findings
+  were fixed and re-reviewed.
+- Architect status: non-blocking `WATCH`; the previous fail-open `BLOCK` was
+  resolved. The remaining watch is limited to the concurrent swap-to-symlink
+  race on platforms where `O_NOFOLLOW` is unavailable.
+- The final diff must contain no generated runtime state, credentials, or
+  unrelated source changes.
+
+Risk notes:
+
+- Qoder home-session filtering can reduce previously over-broad results; tests
+  must prove verified workspace sessions remain visible.
+- Hook exit-code changes can expose previously hidden configuration errors;
+  success-path non-blocking tests guard the intended contract.
+- Symlink policy differs by platform; fixtures must avoid requiring privileged
+  symlink creation where the platform does not support it.

hooks/git-scripts/blast-radius/analysis.mjs

@@ -1,10 +1,55 @@
 import path from "node:path";
 
 import { CONFIG_PATH, isIgnored, loadConfig } from "./config.mjs";
-import { collectGitChanges } from "./git.mjs";
+import { collectGitChanges, isGitFailureError } from "./git.mjs";
 import { buildCodeGraph, mapChangedSymbols } from "./graph.mjs";
 import { isTestFile } from "./parser.mjs";
 import { matchesAnyPattern, normalizeRelativePath, unique } from "./utils.mjs";
+
+function emptyMetrics() {
+  return {
+    changedFiles: 0,
+    changedLines: 0,
+    changedSymbols: 0,
+    impactedSymbols: 0,
+    impactedFiles: 0,
+    testGaps: 0,
+    securityRemovals: 0,
+    parsedFiles: 0,
+  };
+}
+
+function gitFailureReport(error) {
+  const message = error.code === "GIT_BASE_REF_UNAVAILABLE"
+    ? `Blast radius could not verify git base ref "${error.ref}"; failing closed because changed files cannot be distinguished from a real empty diff.`
+    : `Blast radius git collection failed (${error.code}); failing closed because changed files cannot be distinguished from a real empty diff.`;
+
+  return {
+    status: "error",
+    shouldReview: true,
+    score: 100,
+    severity: "critical",
+    reasons: [message],
+    metrics: emptyMetrics(),
+    changedFiles: [],
+    changedSymbols: [],
+    affectedSymbols: [],
+    affectedFiles: [],
+    coreHits: [],
+    testGaps: [],
+    securityRemovals: [],
+    error: {
+      type: "git",
+      code: error.code,
+      message: error.message,
+      ref: error.ref,
+      command: error.command,
+      status: error.status,
+      stderr: error.stderr,
+    },
+    configPath: CONFIG_PATH,
+  };
+}
 function enrichCallers(changedSymbols, graph, config) {
   return changedSymbols.map((symbol) => {
     const callerIds = graph.reverseEdges.get(symbol.id) ?? [];
@@ -264,7 +309,15 @@ function computeScore(metrics, changedSymbols, impact, coreHits, testGaps, secur
 export async function analyzeRepository(repoRoot, options = {}) {
   const root = path.resolve(repoRoot || process.cwd());
   const config = options.config ?? (await loadConfig(root, options.configPath));
-  const changes = options.changes ?? (await collectGitChanges(root, config));
+  let changes;
+  try {
+    changes = options.changes ?? (await collectGitChanges(root, config));
+  } catch (error) {
+    if (isGitFailureError(error)) {
+      return gitFailureReport(error);
+    }
+    throw error;
+  }
   const changedFiles = changes.files
     .map((file) => ({
       ...file,
@@ -279,16 +332,7 @@ export async function analyzeRepository(repoRoot, options = {}) {
   );
 
   if (changedFiles.length === 0) {
-    const metrics = {
-      changedFiles: 0,
-      changedLines: 0,
-      changedSymbols: 0,
-      impactedSymbols: 0,
-      impactedFiles: 0,
-      testGaps: 0,
-      securityRemovals: 0,
-      parsedFiles: 0,
-    };
+    const metrics = emptyMetrics();
     return {
       status: "ok",
       shouldReview: false,

hooks/git-scripts/blast-radius/git.mjs

@@ -6,6 +6,28 @@ import path from "node:path";
 import { SECURITY_REMOVAL_RE } from "./config.mjs";
 import { countLines } from "./utils.mjs";
 
+export class GitFailureError extends Error {
+  constructor(message, details) {
+    super(message);
+    this.name = "GitFailureError";
+    this.code = details.code ?? "GIT_COMMAND_FAILED";
+    this.command = details.command;
+    this.status = details.status;
+    this.signal = details.signal;
+    this.stdout = details.stdout;
+    this.stderr = details.stderr;
+    this.ref = details.ref;
+  }
+}
+
+export function isGitFailureError(error) {
+  return error instanceof GitFailureError;
+}
+
+function trimOutput(output) {
+  return String(output ?? "").trim().slice(0, 600);
+}
+
 function runGit(repoRoot, args, options = {}) {
   const result = spawnSync("git", args, {
     cwd: repoRoot,
@@ -18,12 +40,44 @@ function runGit(repoRoot, args, options = {}) {
     if (options.allowFailure) {
       return "";
     }
-    throw new Error(`git ${args.join(" ")} failed: ${result.stderr.trim()}`);
+    throw new GitFailureError(
+      options.message ?? `git ${args.join(" ")} failed`,
+      {
+        code: options.code,
+        command: ["git", ...args],
+        status: result.status,
+        signal: result.signal,
+        stdout: trimOutput(result.stdout),
+        stderr: trimOutput(result.stderr),
+        ref: options.ref,
+      },
+    );
   }
 
   return result.stdout;
 }
 
+function verifyBaseRef(repoRoot, baseRef) {
+  const ref = String(baseRef ?? "");
+  if (!ref.trim()) {
+    throw new GitFailureError("Blast radius git base ref is empty or unavailable", {
+      code: "GIT_BASE_REF_UNAVAILABLE",
+      command: ["git", "rev-parse", "--verify", "<empty>^{commit}"],
+      status: 1,
+      signal: null,
+      stdout: "",
+      stderr: "",
+      ref,
+    });
+  }
+
+  return runGit(repoRoot, ["rev-parse", "--verify", `${ref}^{commit}`], {
+    code: "GIT_BASE_REF_UNAVAILABLE",
+    message: `Blast radius git base ref is unavailable: ${ref}`,
+    ref,
+  }).trim();
+}
+
 export function parseUnifiedDiff(diffText) {
   return parseUnifiedDiffDetails(diffText).ranges;
 }
@@ -118,22 +172,24 @@ function parseNumstat(numstatText) {
 
 export async function collectGitChanges(repoRoot, config) {
   const baseRef = process.env.BETTER_HARNESS_BLAST_RADIUS_BASE ?? config.baseRef ?? "HEAD";
+  const baseCommit = verifyBaseRef(repoRoot, baseRef);
+
   const diffText = runGit(
     repoRoot,
-    ["diff", "--unified=0", "--no-ext-diff", "--find-renames", baseRef, "--"],
-    { allowFailure: true },
+    ["diff", "--unified=0", "--no-ext-diff", "--find-renames", baseCommit, "--"],
+    { code: "GIT_DIFF_FAILED", ref: baseRef },
   );
   const numstat = runGit(
     repoRoot,
-    ["diff", "--numstat", "--no-ext-diff", "--find-renames", baseRef, "--"],
-    { allowFailure: true },
+    ["diff", "--numstat", "--no-ext-diff", "--find-renames", baseCommit, "--"],
+    { code: "GIT_DIFF_FAILED", ref: baseRef },
   );
 
   const files = parseNumstat(numstat);
   const diffDetails = parseUnifiedDiffDetails(diffText);
   const ranges = diffDetails.ranges;
   const untracked = runGit(repoRoot, ["ls-files", "--others", "--exclude-standard", "-z"], {
-    allowFailure: true,
+    code: "GIT_UNTRACKED_FAILED",
   })
     .split("\0")
     .filter(Boolean);