From 878287af1d28fc8afc87f29d28da6f5594f237d0 Mon Sep 17 00:00:00 2001 From: CoderMageFox Date: Wed, 29 Jul 2026 12:23:07 +0800 Subject: [PATCH 1/2] fix(harness): fail closed at review and privacy boundaries Honor workspace, filesystem, and review boundaries before collecting evidence, and propagate invalid paths, Git refs, and collector failures as explicit errors. Normalize write-tool payloads and preserve severe findings without silently lowering risk. Implements #11, #12, #13, #14, #15, #16, #17, and #18 using docs/specs/2026-07-29-11-18-review-boundary-hardening.md. The change splits cleanly across Secret Guard, Qoder session ownership, quickstart input validation, Git/review failure propagation, severity repair, and cloc containment. Validated with npm run check (852 tests plus package verification), the 6-test documentation link graph, focused boundary tests, syntax checks, and staged diff checks. Constraint: Preserve successful-path and non-blocking finding behavior across supported hosts Rejected: Treat boundary failures as empty or warning-only evidence | this hides review and privacy failures Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep workspace, Git, and filesystem boundary failures explicit when adding collectors Tested: npm run check; node --test test/doc-link-graph.test.mjs; focused 68-test boundary run; node --check; git diff --cached --check Not-tested: Hostile concurrent symlink replacement on platforms without O_NOFOLLOW Co-authored-by: OmX --- ...6-07-29-11-18-review-boundary-hardening.md | 123 ++++++++++++++ hooks/git-scripts/blast-radius/analysis.mjs | 68 ++++++-- hooks/git-scripts/blast-radius/git.mjs | 68 +++++++- scripts/agent-guardrails/secret-scan.mjs | 63 ++++++- scripts/cloc/count-file.mjs | 57 ++++++- .../harness-analysis/repair-findings-json.mjs | 7 +- scripts/harness-quickstart/cli.mjs | 56 ++++++- scripts/review-trigger/cli.mjs | 88 +++++++++- scripts/session-analysis/platforms/qoder.mjs | 52 +++++- test/agent-guardrails-secret-scan.test.mjs | 158 ++++++++++++++++++ test/blast-radius.test.mjs | 84 ++++++++++ test/cloc.test.mjs | 76 ++++++++- test/harness-findings-repair.test.mjs | 47 ++++++ test/harness-quickstart.test.mjs | 82 +++++++++ test/review-trigger.test.mjs | 74 ++++++++ test/session-analysis.test.mjs | 108 +++++++++++- test/session-usage-summary.test.mjs | 60 ++++++- 17 files changed, 1228 insertions(+), 43 deletions(-) create mode 100644 docs/specs/2026-07-29-11-18-review-boundary-hardening.md diff --git a/docs/specs/2026-07-29-11-18-review-boundary-hardening.md b/docs/specs/2026-07-29-11-18-review-boundary-hardening.md new file mode 100644 index 0000000..6fa0b10 --- /dev/null +++ b/docs/specs/2026-07-29-11-18-review-boundary-hardening.md @@ -0,0 +1,123 @@ +# 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. +- 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 and runtime failures exit non-zero while + successful findings retain their documented non-blocking result. +- AC-06 (#16): `report --cwd` rejects 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 or reads a non-regular file; 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 one workspace-ownership predicate to Qoder home-session discovery and + hydration, keeping explicit global behavior separate. +4. Make quickstart privacy and cwd validation explicit at its CLI boundary. +5. Replace Git diff ambiguity with a structured failure from blast-radius + collection. +6. Separate review-trigger execution failure from successful non-blocking + findings. +7. Make severity repair conservative and make cloc file reads type- 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` +- 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` +- 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` +- AC-09: + `npm test` + `npm run pack:verify` + `node --test test/doc-link-graph.test.mjs` + +Review evidence: + +- `npm run check` passed with 852 tests and package verification. +- The final blocker-focused run passed 68 tests across Secret Guard, + review-trigger, blast-radius, 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. diff --git a/hooks/git-scripts/blast-radius/analysis.mjs b/hooks/git-scripts/blast-radius/analysis.mjs index 3b25f16..f23d3f5 100644 --- a/hooks/git-scripts/blast-radius/analysis.mjs +++ b/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, diff --git a/hooks/git-scripts/blast-radius/git.mjs b/hooks/git-scripts/blast-radius/git.mjs index f430a06..0b14850 100644 --- a/hooks/git-scripts/blast-radius/git.mjs +++ b/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", "^{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); diff --git a/scripts/agent-guardrails/secret-scan.mjs b/scripts/agent-guardrails/secret-scan.mjs index e6c7b16..fa3cd24 100644 --- a/scripts/agent-guardrails/secret-scan.mjs +++ b/scripts/agent-guardrails/secret-scan.mjs @@ -464,8 +464,7 @@ export async function handleHookEvent({ mode, platform, host, event, failOn = "m } function preToolBlockReason(event, failOn) { - const toolName = String(event.tool_name ?? event.toolName ?? ""); - const toolInput = event.tool_input && typeof event.tool_input === "object" ? event.tool_input : {}; + const { toolName, toolInput } = normalizePreToolEvent(event); if (/^Bash$/i.test(toolName)) { const command = String(toolInput.command ?? ""); @@ -486,7 +485,18 @@ function preToolBlockReason(event, failOn) { } } - const filePath = String(toolInput.file_path ?? toolInput.path ?? toolInput.absolute_path ?? ""); + const contentTargets = writeContentTargets(toolName, toolInput); + for (const target of contentTargets) { + const findings = scanTextRaw(target.content, target.file, defaultOptions({ failOn, redact: true })); + if (findings.some((finding) => shouldFailFinding(finding, failOn))) { + return { + reasonCode: "secret-in-tool-content", + message: "Secret guard blocked write content containing a likely credential. Move the value to a secret manager or environment variable and use a redacted fixture instead.", + }; + } + } + + const filePath = String(toolInput.file_path ?? toolInput.filePath ?? toolInput.path ?? toolInput.absolute_path ?? toolInput.absolutePath ?? ""); if (filePath && isProtectedCredentialPath(filePath)) { return { reasonCode: "protected-credential-path", @@ -497,6 +507,53 @@ function preToolBlockReason(event, failOn) { return null; } +function normalizePreToolEvent(event) { + const source = event && typeof event === "object" ? event : {}; + const data = source.data && typeof source.data === "object" && !Array.isArray(source.data) + ? source.data + : {}; + const toolName = String( + source.tool_name + ?? source.toolName + ?? data.tool_name + ?? data.toolName + ?? source.name + ?? data.name + ?? "", + ); + const input = source.tool_input + ?? source.toolInput + ?? data.tool_input + ?? data.toolInput + ?? source.input + ?? source.args + ?? data.input + ?? data.args; + return { + toolName, + toolInput: input && typeof input === "object" && !Array.isArray(input) + ? input + : (/^apply_patch$/i.test(toolName) && typeof input === "string" ? { patch: input } : {}), + }; +} + +function writeContentTargets(toolName, toolInput) { + if (/^Write$/i.test(toolName)) { + return stringContentTargets(toolInput.content, ""); + } + if (/^Edit$/i.test(toolName)) { + return stringContentTargets(toolInput.new_string ?? toolInput.newString, ""); + } + if (/^apply_patch$/i.test(toolName)) { + return stringContentTargets(toolInput.patch, ""); + } + return []; +} + +function stringContentTargets(value, file) { + return typeof value === "string" ? [{ content: value, file }] : []; +} + function blockHook({ platform, eventName, reasonCode, message }) { return renderSecretGuardBlock({ platform, eventName, reasonCode, message }); } diff --git a/scripts/cloc/count-file.mjs b/scripts/cloc/count-file.mjs index 2868f36..b09b026 100644 --- a/scripts/cloc/count-file.mjs +++ b/scripts/cloc/count-file.mjs @@ -1,4 +1,12 @@ -import { existsSync, readFileSync } from "node:fs"; +import { + closeSync, + constants as fsConstants, + fstatSync, + lstatSync, + openSync, + readFileSync, + realpathSync, +} from "node:fs"; import path from "node:path"; import { @@ -77,6 +85,12 @@ function pushRecord(records, filePath, languageId, text, segment = "") { } } +function isPathInside(root, candidate) { + const relative = path.relative(root, candidate); + return relative === "" + || (relative !== ".." && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative)); +} + export function isCountablePath(filePath) { const base = path.posix.basename(toPosix(filePath)).toLowerCase(); if (LOCK_FILE_NAMES.has(base)) { @@ -257,16 +271,51 @@ export function countFile(repoRoot, filePath, options = {}) { return { path: normalized, skipped: true, reason: "unsupported" }; } - const absolutePath = path.join(repoRoot, normalized); - if (!existsSync(absolutePath)) { + const root = path.resolve(repoRoot); + const absolutePath = path.resolve(root, normalized); + if (!isPathInside(root, absolutePath)) { + return { path: normalized, skipped: true, reason: "outside-repo" }; + } + + let stat; + try { + stat = lstatSync(absolutePath); + } catch { return { path: normalized, skipped: true, reason: "missing" }; } + if (!stat.isFile()) { + return { path: normalized, skipped: true, reason: "non-regular" }; + } + try { + const realRoot = realpathSync(root); + const realPath = realpathSync(absolutePath); + if (!isPathInside(realRoot, realPath)) { + return { path: normalized, skipped: true, reason: "outside-repo" }; + } + } catch { + return { path: normalized, skipped: true, reason: "unreadable" }; + } + + let descriptor; let buffer; try { - buffer = readFileSync(absolutePath); + const noFollow = fsConstants.O_NOFOLLOW ?? 0; + descriptor = openSync(absolutePath, fsConstants.O_RDONLY | noFollow); + if (!fstatSync(descriptor).isFile()) { + return { path: normalized, skipped: true, reason: "non-regular" }; + } + buffer = readFileSync(descriptor); } catch { return { path: normalized, skipped: true, reason: "unreadable" }; + } finally { + if (descriptor !== undefined) { + try { + closeSync(descriptor); + } catch { + // The read result already captures the useful failure boundary. + } + } } const records = countKnownFileBuffer(buffer, normalized, options); diff --git a/scripts/harness-analysis/repair-findings-json.mjs b/scripts/harness-analysis/repair-findings-json.mjs index b75af2f..c64687b 100644 --- a/scripts/harness-analysis/repair-findings-json.mjs +++ b/scripts/harness-analysis/repair-findings-json.mjs @@ -26,6 +26,9 @@ const RISK_LABEL_MAP = new Map([ ["high", "High"], ["medium", "Medium"], ["low", "Low"], + ["critical", "High"], + ["blocker", "High"], + ["severe", "High"], ["高", "High"], ["中", "Medium"], ["低", "Low"], @@ -130,10 +133,10 @@ function addChange(changes, pathName, before, after) { changes.push({ path: pathName, before, after }); } -function normalizeRiskLabel(value, fallback = "Medium") { +function normalizeRiskLabel(value) { const text = String(value ?? "").trim(); if (VALID_RISK_LABELS.has(text)) return text; - return RISK_LABEL_MAP.get(text.toLowerCase()) ?? fallback; + return RISK_LABEL_MAP.get(text.toLowerCase()) ?? text; } function normalizeSurface(value) { diff --git a/scripts/harness-quickstart/cli.mjs b/scripts/harness-quickstart/cli.mjs index e0b30ab..090343e 100644 --- a/scripts/harness-quickstart/cli.mjs +++ b/scripts/harness-quickstart/cli.mjs @@ -6,6 +6,7 @@ // the skill, keeping product judgment in the skill and not in code. import { spawnSync } from "node:child_process"; +import { statSync } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -27,6 +28,7 @@ hands off to the Better Harness skill, which synthesizes the report. Options: --cwd Repository to inspect (default: current directory) --qoder-home Qoder data root for the session-source probe (default: ~/.qoder) + --no-sessions Skip session probing and use the static Software Fluency route --json Emit machine-readable evidence and handoff as JSON -h, --help Print this help `; @@ -107,6 +109,53 @@ function runSessionProbe(cwd, qoderHome) { } } +function staticSessionRoute() { + return { available: false, usableSessionCount: 0, modelId: "software-fluency", route: "static-fallback" }; +} + +function cwdValidationError(cwd) { + try { + const stat = statSync(cwd); + if (!stat.isDirectory()) { + return { + code: "INVALID_CWD", + message: `Repository cwd is not a directory: ${cwd}`, + hint: "Pass --cwd with an existing repository directory.", + }; + } + } catch (error) { + if (error?.code === "ENOENT") { + return { + code: "INVALID_CWD", + message: `Repository cwd does not exist: ${cwd}`, + hint: "Pass --cwd with an existing repository directory.", + }; + } + return { + code: "INVALID_CWD", + message: `Repository cwd is unavailable: ${cwd}`, + hint: "Pass --cwd with an existing repository directory.", + }; + } + return null; +} + +function errorPayload(error) { + return { + ok: false, + format_version: "1.0", + error, + }; +} + +function renderError(error, machine) { + if (machine) { + process.stdout.write(`${JSON.stringify(errorPayload(error), null, 2)}\n`); + } else { + process.stderr.write(`${error.message}\n\n${error.hint}\n`); + } +} + function summarizeProject(evidence) { if (!evidence.available) { return { available: false, hint: evidence.hint }; @@ -149,7 +198,7 @@ function summarizeWorktree(evidence) { } function buildResult(cwd, options = {}) { - const sessions = runSessionProbe(cwd, options["qoder-home"]); + const sessions = options["no-sessions"] ? staticSessionRoute() : runSessionProbe(cwd, options["qoder-home"]); return { schemaVersion: 1, kind: "quickstart", @@ -233,6 +282,11 @@ export function main(argv = process.argv.slice(2)) { } const options = parseArgs(argv); const cwd = options.cwd ? path.resolve(String(options.cwd)) : process.cwd(); + const validationError = cwdValidationError(cwd); + if (validationError) { + renderError(validationError, Boolean(options.json)); + return 1; + } const result = buildResult(cwd, options); if (options.json) { process.stdout.write(`${JSON.stringify(result, null, 2)}\n`); diff --git a/scripts/review-trigger/cli.mjs b/scripts/review-trigger/cli.mjs index 2ab9b9a..177247a 100644 --- a/scripts/review-trigger/cli.mjs +++ b/scripts/review-trigger/cli.mjs @@ -2,6 +2,7 @@ import { createHash } from "node:crypto"; import { spawnSync } from "node:child_process"; +import { stat } from "node:fs/promises"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -15,6 +16,14 @@ const SEVERITY_ORDER = { error: 3, }; +class ReviewTriggerCliError extends Error { + constructor(code, message) { + super(message); + this.name = "ReviewTriggerCliError"; + this.code = code; + } +} + function sha256(value) { return createHash("sha256").update(value).digest("hex"); } @@ -316,8 +325,11 @@ export async function collectReviewTriggerFindings(options = {}) { config, })); } - } catch (error) { - warnings.push(`change-test-evidence scan failed: ${error.message}`); + } catch { + throw new ReviewTriggerCliError( + "runtime-failure", + "review-trigger could not verify change-test evidence", + ); } return { @@ -327,8 +339,23 @@ export async function collectReviewTriggerFindings(options = {}) { }; } +async function assertDirectory(cwd) { + try { + const stats = await stat(cwd); + if (!stats.isDirectory()) { + throw new ReviewTriggerCliError("runtime-failure", "review-trigger cwd is not a directory"); + } + } catch (error) { + if (error instanceof ReviewTriggerCliError) { + throw error; + } + throw new ReviewTriggerCliError("runtime-failure", "review-trigger cwd is unavailable"); + } +} + export async function runReviewTrigger(options = {}) { const cwd = path.resolve(options.cwd ?? process.env.QODER_CWD ?? process.cwd()); + await assertDirectory(cwd); if (options.input?.stop_hook_active) { return { ok: true, @@ -368,11 +395,19 @@ function parseArgs(argv) { const arg = argv[index]; const value = (name) => { const next = argv[index + 1]; - if (!next) throw new Error(`${name} requires a value`); + if (!next || next.startsWith("--")) { + throw new ReviewTriggerCliError("invalid-arguments", `${name} requires a value`); + } index += 1; return next; }; - const assignMaybeEquals = (name) => arg.includes("=") ? arg.slice(arg.indexOf("=") + 1) : value(name); + const assignMaybeEquals = (name) => { + const next = arg.includes("=") ? arg.slice(arg.indexOf("=") + 1) : value(name); + if (!next) { + throw new ReviewTriggerCliError("invalid-arguments", `${name} requires a value`); + } + return next; + }; if (arg === "--help" || arg === "-h") options.help = true; else if (arg === "--json") options.json = true; @@ -404,6 +439,33 @@ async function readStdin() { return Buffer.concat(chunks).toString("utf8"); } +function failureEnvelope(error) { + const code = error instanceof ReviewTriggerCliError ? error.code : "runtime-failure"; + const message = code === "invalid-arguments" + ? "review-trigger received invalid arguments" + : "review-trigger runtime failed"; + return { + ok: false, + kind: "better-harness.review-trigger", + status: "error", + exitCode: 1, + error: { + code, + message, + }, + }; +} + +function emitFailure(error, options = {}) { + const envelope = failureEnvelope(error); + if (options.json) { + process.stdout.write(`${JSON.stringify(envelope, null, 2)}\n`); + } else { + process.stderr.write(`${envelope.error.message}\n`); + } + return envelope.exitCode; +} + function formatHuman(result) { if (result.findings.length === 0) { return ""; @@ -420,7 +482,13 @@ function formatHuman(result) { } export async function main(argv = process.argv.slice(2)) { - const args = parseArgs(argv); + let args; + try { + args = parseArgs(argv); + } catch (error) { + return emitFailure(error, { json: argv.includes("--json") }); + } + if (args.help) { process.stdout.write(usage()); return 0; @@ -436,7 +504,12 @@ export async function main(argv = process.argv.slice(2)) { } } - const result = await runReviewTrigger({ ...args, input }); + let result; + try { + result = await runReviewTrigger({ ...args, input }); + } catch (error) { + return emitFailure(error, { json: args.json }); + } if (args.json) { process.stdout.write(`${JSON.stringify({ @@ -458,7 +531,6 @@ if (isCli) { main().then((code) => { process.exitCode = code; }).catch((error) => { - process.stderr.write(`review-trigger failed: ${error.stack ?? error.message}\n`); - process.exitCode = 0; + process.exitCode = emitFailure(error, { json: process.argv.slice(2).includes("--json") }); }); } diff --git a/scripts/session-analysis/platforms/qoder.mjs b/scripts/session-analysis/platforms/qoder.mjs index 3df164e..d34700a 100644 --- a/scripts/session-analysis/platforms/qoder.mjs +++ b/scripts/session-analysis/platforms/qoder.mjs @@ -623,6 +623,29 @@ async function readSessionIdFromJsonl(filePath, fallbackRef) { return { sessionId: found, timestamp: firstTimestamp }; } +async function readHomeSessionProbe(filePath, fallbackRef, scope) { + const probe = { sessionId: fallbackRef.sessionId ?? null, timestamp: null, workspaceMatched: false }; + const observe = (raw) => { + probe.sessionId = probe.sessionId ?? inferSessionId(raw, fallbackRef); + probe.timestamp = probe.timestamp ?? inferTimestamp(raw, fallbackRef); + if (isWorkspaceMatch(inferCwd(raw), scope.workspace)) { + probe.workspaceMatched = true; + } + return probe.sessionId && probe.timestamp && probe.workspaceMatched ? false : undefined; + }; + + if (filePath.endsWith(JSONL_EXT)) { + await forEachJsonLine(filePath, observe, { maxLines: 200 }); + return probe; + } + + const raw = await readJson(filePath).catch(() => null); + if (raw) { + observe(raw); + } + return probe; +} + function createSessionRecord(sessionId, workspace) { return { sessionId, @@ -635,6 +658,20 @@ function createSessionRecord(sessionId, workspace) { }; } +function sessionHasWorkspaceEvidence(session) { + return (session?.sourceRefMap ? [...session.sourceRefMap.values()] : []) + .some((ref) => ref.kind !== "home-session" && ref.planningScope !== "user-global"); +} + +function sessionHasGlobalEvidence(session) { + return (session?.sourceRefMap ? [...session.sourceRefMap.values()] : []) + .some((ref) => ref.kind !== "home-session" && ref.planningScope === "user-global"); +} + +function homeSessionPlanningScope(session) { + return sessionHasWorkspaceEvidence(session) ? "workspace" : "user-global"; +} + function sourceKey(ref) { return `${ref.kind}:${ref.path}`; } @@ -1157,17 +1194,24 @@ export class QoderSessionAnalyzer extends SessionAnalyzer { }); for (const filePath of files) { const fallbackId = path.basename(filePath).replace(/\.(jsonl|json)$/u, ""); - const probe = filePath.endsWith(JSONL_EXT) - ? await readSessionIdFromJsonl(filePath, { sessionId: fallbackId, kind: "home-session", path: filePath }) - : { sessionId: fallbackId, timestamp: null }; + const fallbackRef = { sessionId: fallbackId, kind: "home-session", path: filePath }; + const probe = await readHomeSessionProbe(filePath, fallbackRef, scope); if (!withinTimeRange(probe.timestamp, scope)) { continue; } - addSessionRef(sessions, probe.sessionId ?? fallbackId, scope.workspace, { + const sessionId = probe.sessionId ?? fallbackId; + const existingSession = sessions.get(sessionId); + const verifiedWorkspaceSession = sessionHasWorkspaceEvidence(existingSession); + const verifiedGlobalSession = scope.includeGlobalCapabilities && sessionHasGlobalEvidence(existingSession); + if (!probe.workspaceMatched && !verifiedWorkspaceSession && !verifiedGlobalSession) { + continue; + } + addSessionRef(sessions, sessionId, scope.workspace, { kind: "home-session", path: filePath, eventType: "home-session", timestamp: probe.timestamp, + planningScope: probe.workspaceMatched ? "workspace" : homeSessionPlanningScope(existingSession), }); } } diff --git a/test/agent-guardrails-secret-scan.test.mjs b/test/agent-guardrails-secret-scan.test.mjs index 8a84d7e..8fb2874 100644 --- a/test/agent-guardrails-secret-scan.test.mjs +++ b/test/agent-guardrails-secret-scan.test.mjs @@ -293,6 +293,164 @@ test("PreToolUse hook blocks credential file reads and avoids full command echo" assert.doesNotMatch(result.stdout, /OPENAI_API_KEY/); }); +test("PreToolUse hook scans write tool content without echoing secrets", async () => { + const cases = [ + { + name: "Write.content", + tool_name: "Write", + tool_input: { + file_path: "notes.txt", + content: `OPENAI_API_KEY=${syntheticOpenAiKey()}`, + }, + }, + { + name: "Edit.new_string", + tool_name: "Edit", + tool_input: { + file_path: "notes.txt", + old_string: "OPENAI_API_KEY=", + new_string: `OPENAI_API_KEY=${syntheticOpenAiKey()}`, + }, + }, + { + name: "apply_patch patch", + tool_name: "apply_patch", + tool_input: { + patch: [ + "*** Begin Patch", + "*** Add File: notes.txt", + `+OPENAI_API_KEY=${syntheticOpenAiKey()}`, + "*** End Patch", + ].join("\n"), + }, + }, + ]; + + for (const row of cases) { + const secret = row.tool_input.content + ?? row.tool_input.new_string + ?? row.tool_input.patch.match(/sk-proj-[A-Za-z0-9_-]+/u)[0]; + const result = await handleHookEvent({ + mode: "pre-tool", + platform: "codex", + event: row, + }); + + assert.equal(result.blocked, true, row.name); + assert.equal(result.exitCode, 0, row.name); + const payload = JSON.parse(result.stdout); + assert.equal(payload.decision, "block", row.name); + assert.match(payload.reason, /credential|secret|凭据/i, row.name); + assert.doesNotMatch(result.stdout, new RegExp(secret.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")), row.name); + } +}); + +test("PreToolUse hook scans freeform apply_patch payloads", async () => { + const secret = syntheticOpenAiKey(); + const patch = [ + "*** Begin Patch", + "*** Add File: notes.txt", + `+OPENAI_API_KEY=${secret}`, + "*** End Patch", + ].join("\n"); + const events = [ + { + tool_name: "apply_patch", + tool_input: patch, + }, + { + toolName: "apply_patch", + toolInput: patch, + }, + ]; + + for (const event of events) { + const result = await handleHookEvent({ + mode: "pre-tool", + platform: "codex", + event, + }); + + assert.equal(result.blocked, true); + assert.equal(JSON.parse(result.stdout).reasonCode, "secret-in-tool-content"); + assert.doesNotMatch(result.stdout, new RegExp(secret.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"))); + } +}); + +test("PreToolUse hook normalizes camelCase Bash and Read payloads", async () => { + const bashResult = await handleHookEvent({ + mode: "pre-tool", + platform: "codex", + event: { + hookEventName: "PreToolUse", + toolName: "Bash", + toolInput: { + command: "cat .env && printenv OPENAI_API_KEY", + }, + }, + }); + + assert.equal(bashResult.blocked, true); + assert.equal(JSON.parse(bashResult.stdout).reasonCode, "read-env-file"); + assert.doesNotMatch(bashResult.stdout, /cat \.env/); + assert.doesNotMatch(bashResult.stdout, /OPENAI_API_KEY/); + + const readResult = await handleHookEvent({ + mode: "pre-tool", + platform: "codex", + event: { + hookEventName: "PreToolUse", + toolName: "Read", + toolInput: { + filePath: ".env.local", + }, + }, + }); + + assert.equal(readResult.blocked, true); + assert.equal(JSON.parse(readResult.stdout).reasonCode, "protected-credential-path"); + assert.doesNotMatch(readResult.stdout, /\.env\.local/); +}); + +test("PreToolUse hook normalizes args and nested data payloads", async () => { + const argsResult = await handleHookEvent({ + mode: "pre-tool", + platform: "codex", + event: { + name: "Write", + args: { + filePath: "notes.txt", + content: `OPENAI_API_KEY=${syntheticOpenAiKey()}`, + }, + }, + }); + + assert.equal(argsResult.blocked, true); + assert.equal(JSON.parse(argsResult.stdout).reasonCode, "secret-in-tool-content"); + assert.doesNotMatch(argsResult.stdout, /OPENAI_API_KEY/); + + const nestedResult = await handleHookEvent({ + mode: "pre-tool", + platform: "codex", + event: { + name: "PreToolUse", + input: { + eventId: "outer-envelope", + }, + data: { + toolName: "Read", + toolInput: { + filePath: ".env.local", + }, + }, + }, + }); + + assert.equal(nestedResult.blocked, true); + assert.equal(JSON.parse(nestedResult.stdout).reasonCode, "protected-credential-path"); + assert.doesNotMatch(nestedResult.stdout, /\.env\.local/); +}); + test("installSecretGuard merges Qoder settings and is idempotent", async () => { const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-secret-guard-qoder-")); const target = path.join(root, "project"); diff --git a/test/blast-radius.test.mjs b/test/blast-radius.test.mjs index dafdff1..0d133f3 100644 --- a/test/blast-radius.test.mjs +++ b/test/blast-radius.test.mjs @@ -107,6 +107,90 @@ test("analyzeRepository short-circuits clean worktrees without parsing source fi } }); +test("analyzeRepository stays clean with a valid clean base ref", async () => { + const repo = await makeRepo({ + "src/app.ts": "export function app() {\n return 1;\n}\n", + }); + + try { + const report = await analyzeRepository(repo, { + config: { + baseRef: "HEAD", + ignore: [], + core: [], + thresholds: { + reviewScore: 10, + changedFiles: { warn: 5, high: 10, critical: 20 }, + changedLines: { warn: 50, high: 100, critical: 200 }, + changedSymbols: { warn: 5, high: 10, critical: 20 }, + impactedSymbols: { warn: 5, high: 10, critical: 20 }, + impactedFiles: { warn: 5, high: 10, critical: 20 }, + callerCount: { warn: 5, high: 10, critical: 20 }, + }, + limits: { maxSourceFiles: 100, blastRadiusDepth: 2, maxImpactSymbols: 100 }, + }, + }); + + assert.equal(report.status, "ok"); + assert.equal(report.shouldReview, false); + assert.equal(report.metrics.changedFiles, 0); + assert.deepEqual(report.changedFiles, []); + } finally { + await rm(repo, { recursive: true, force: true }); + } +}); + +test("analyzeRepository fails closed when base ref is unavailable", async () => { + const repo = await makeRepo({ + "src/core/payment.ts": [ + "export function charge(amount: number) {", + " if (amount <= 0) throw new Error('invalid');", + " return amount;", + "}", + "", + ].join("\n"), + }); + + try { + await writeFile( + path.join(repo, "src/core/payment.ts"), + [ + "export function charge(amount: number) {", + " return Math.abs(amount);", + "}", + "", + ].join("\n"), + ); + + const report = await analyzeRepository(repo, { + config: { + baseRef: "origin/main", + ignore: [], + core: [{ name: "payment core", paths: ["src/core/**"], symbols: [], risk: "critical" }], + thresholds: { + reviewScore: 10, + changedFiles: { warn: 5, high: 10, critical: 20 }, + changedLines: { warn: 50, high: 100, critical: 200 }, + changedSymbols: { warn: 5, high: 10, critical: 20 }, + impactedSymbols: { warn: 5, high: 10, critical: 20 }, + impactedFiles: { warn: 5, high: 10, critical: 20 }, + callerCount: { warn: 5, high: 10, critical: 20 }, + }, + limits: { maxSourceFiles: 100, blastRadiusDepth: 2, maxImpactSymbols: 100 }, + }, + }); + + assert.equal(report.status, "error"); + assert.equal(report.shouldReview, true); + assert.equal(report.severity, "critical"); + assert.match(report.error.message, /base ref/i); + assert.match(report.error.ref, /origin\/main/); + assert.match(report.reasons.join("\n"), /Blast radius could not verify git base ref/); + } finally { + await rm(repo, { recursive: true, force: true }); + } +}); + test("core path rules and change-size thresholds trigger review guidance", async () => { const repo = await makeRepo({ "src/core/payment.ts": [ diff --git a/test/cloc.test.mjs b/test/cloc.test.mjs index c002500..dc729f7 100644 --- a/test/cloc.test.mjs +++ b/test/cloc.test.mjs @@ -1,7 +1,7 @@ import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; import { existsSync } from "node:fs"; -import { mkdtemp, mkdir, rm, writeFile } from "node:fs/promises"; +import { mkdtemp, mkdir, rm, symlink, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import test from "node:test"; @@ -174,6 +174,80 @@ test("unsupported and lock files are skipped before reading content", async () = } }); +test("git-backed cloc skips tracked symlinks without leaking external targets", async (t) => { + const repo = await makeRepo({ + "src/app.js": "const a = 1;\n", + }); + const external = await mkdtemp(path.join(os.tmpdir(), "better-harness-cloc-external-")); + + try { + const externalTarget = path.join(external, "secret.js"); + await writeFile(externalTarget, "const secret = true;\n"); + try { + await symlink(externalTarget, path.join(repo, "src", "external-link.js")); + } catch (error) { + t.skip(`symlink fixture unavailable on this platform: ${error.code ?? error.message}`); + return; + } + git(repo, ["add", "src/external-link.js"]); + git(repo, ["commit", "-q", "-m", "track symlink"]); + + const direct = countFile(repo, "src/external-link.js"); + assert.deepEqual(direct, { + path: "src/external-link.js", + skipped: true, + reason: "non-regular", + }); + + const result = await analyzeCloc({ cwd: repo, workers: 1, trackedOnly: true }); + assert.equal(result.status, "ok"); + assert.equal(result.totals.files, 1); + assert.deepEqual(result.skippedFiles, [{ + path: "src/external-link.js", + reason: "non-regular", + }]); + assert.equal(JSON.stringify(result).includes(externalTarget), false); + } finally { + await removeFixtureTree(repo); + await removeFixtureTree(external); + } +}); + +test("cloc skips path candidates outside the repository boundary before reading", async () => { + const repo = await mkdtemp(path.join(os.tmpdir(), "better-harness-cloc-boundary-")); + const external = await mkdtemp(path.join(os.tmpdir(), "better-harness-cloc-boundary-external-")); + + try { + const externalTarget = path.join(external, "secret.js"); + await writeFile(externalTarget, "const secret = true;\n"); + const escapedPath = path.relative(repo, externalTarget); + + assert.deepEqual(countFile(repo, escapedPath), { + path: escapedPath.split(path.sep).join("/"), + skipped: true, + reason: "outside-repo", + }); + } finally { + await removeFixtureTree(repo); + await removeFixtureTree(external); + } +}); + +test("cloc accepts repository paths whose segment begins with two dots", async () => { + const repo = await mkdtemp(path.join(os.tmpdir(), "better-harness-cloc-dotdot-name-")); + + try { + await writeFixtureFile(repo, "..generated/app.js", "const generated = true;\n"); + + const result = countFile(repo, "..generated/app.js"); + assert.equal(result.skipped, false); + assert.equal(result.path, "..generated/app.js"); + assert.equal(result.records[0].code, 1); + } finally { + await removeFixtureTree(repo); + } +}); + test("worker and single-thread results match for mixed language fixtures", async () => { const repo = await makeRepo({ "src/app.js": "const a = 1;\n// comment\n", diff --git a/test/harness-findings-repair.test.mjs b/test/harness-findings-repair.test.mjs index 85341c7..ae95be9 100644 --- a/test/harness-findings-repair.test.mjs +++ b/test/harness-findings-repair.test.mjs @@ -185,6 +185,53 @@ test("repairFindingsJsonData preserves valid human-authored repair prompts", () assert.equal(validation.status, "pass", validation.errors.join("\n")); }); +test("repairFindingsJsonData maps known severe labels and rejects arbitrary severities", () => { + const input = validFindingsJson(); + input.findings = [ + { + ...input.findings[0], + id: "critical-finding", + title: "Critical finding stays high risk", + severity: "Critical", + }, + { + ...input.findings[0], + id: "blocker-finding", + title: "Blocker finding stays high risk", + severity: "Blocker", + }, + { + ...input.findings[0], + id: "medium-finding", + title: "Medium finding stays medium risk", + severity: "Medium", + }, + ]; + + const repaired = repairFindingsJsonData(input, { targetPath: "/tmp/fixture-project" }); + + assert.equal(repaired.data.findings[0].severity, "High"); + assert.equal(repaired.data.findings[1].severity, "High"); + assert.equal(repaired.data.findings[2].severity, "Medium"); + assert.ok(repaired.changes.some((change) => change.path === "findings[0].severity")); + assert.ok(repaired.changes.some((change) => change.path === "findings[1].severity")); + + const validation = evaluateFindingsJson(JSON.stringify(repaired.data), null); + assert.equal(validation.status, "pass", validation.errors.join("\n")); + + input.findings[1].severity = "Cosmic"; + const rejected = repairFindingsJsonData(input, { targetPath: "/tmp/fixture-project" }); + assert.equal(rejected.data.findings[1].severity, "Cosmic"); + assert.equal( + rejected.changes.some((change) => change.path === "findings[1].severity"), + false, + ); + + const rejectedValidation = evaluateFindingsJson(JSON.stringify(rejected.data), null); + assert.equal(rejectedValidation.status, "fail"); + assert.ok(rejectedValidation.errors.some((error) => error.includes("severity"))); +}); + test("repairFindingsJsonData strips non-openable Memory directory paths but keeps count and scope", () => { const input = validFindingsJson(); input.summary.aiAgentPractice.coverageRows.push({ diff --git a/test/harness-quickstart.test.mjs b/test/harness-quickstart.test.mjs index 4ef8f84..cbb43b1 100644 --- a/test/harness-quickstart.test.mjs +++ b/test/harness-quickstart.test.mjs @@ -97,6 +97,88 @@ test("Better Harness report uses Software Fluency only when no sessions are avai } }); +test("Better Harness report --no-sessions ignores real session fixtures and uses static route", async () => { + const fixture = await makeRepoFixture({ withSession: true }); + + try { + const result = runBetterHarness([ + "report", + "--cwd", + fixture.root, + "--qoder-home", + fixture.qoderHome, + "--no-sessions", + "--json", + ]); + + assert.equal(result.status, 0, result.stderr); + const payload = JSON.parse(result.stdout); + assert.equal(payload.evidence.sessions.usableSessionCount, 0); + assert.equal(payload.evidence.sessions.route, "static-fallback"); + assert.equal(payload.nextStep.modelId, "software-fluency"); + } finally { + await rm(fixture.root, { recursive: true, force: true }); + await rm(fixture.qoderHome, { recursive: true, force: true }); + } +}); + +test("Better Harness report rejects a missing cwd before collecting evidence", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-quickstart-missing-cwd-")); + const missing = path.join(root, "missing"); + + try { + const result = runBetterHarness(["report", "--cwd", missing, "--json"]); + + assert.equal(result.status, 1); + assert.equal(result.stderr, ""); + const payload = JSON.parse(result.stdout); + assert.equal(payload.ok, false); + assert.equal(payload.error.code, "INVALID_CWD"); + assert.match(payload.error.message, /Repository cwd does not exist/u); + assert.match(payload.error.message, /missing/u); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("Better Harness report rejects a regular-file cwd before collecting evidence", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-quickstart-file-cwd-")); + const filePath = path.join(root, "not-a-directory"); + + try { + await writeFile(filePath, "not a directory\n"); + const result = runBetterHarness(["report", "--cwd", filePath, "--json"]); + + assert.equal(result.status, 1); + assert.equal(result.stderr, ""); + const payload = JSON.parse(result.stdout); + assert.equal(payload.ok, false); + assert.equal(payload.error.code, "INVALID_CWD"); + assert.match(payload.error.message, /Repository cwd is not a directory/u); + assert.match(payload.error.message, /not-a-directory/u); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("Better Harness report preserves partial fallback for valid directories", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-quickstart-partial-")); + + try { + const result = runBetterHarness(["report", "--cwd", root, "--no-sessions", "--json"]); + + assert.equal(result.status, 0, result.stderr); + assert.equal(result.stderr, ""); + const payload = JSON.parse(result.stdout); + assert.equal(payload.kind, "quickstart"); + assert.equal(payload.status, "ok"); + assert.equal(payload.evidence.sessions.route, "static-fallback"); + assert.equal(payload.nextStep.modelId, "software-fluency"); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + test("Better Harness report prints a human two-step handoff by default", async () => { const fixture = await makeRepoFixture({ withSession: true }); diff --git a/test/review-trigger.test.mjs b/test/review-trigger.test.mjs index 0156af4..78e0f59 100644 --- a/test/review-trigger.test.mjs +++ b/test/review-trigger.test.mjs @@ -44,6 +44,18 @@ async function makeRepo(files) { return repo; } +function runCli(args, options = {}) { + return spawnSync(process.execPath, [ + path.join(process.cwd(), "scripts/review-trigger/cli.mjs"), + ...args, + ], { + encoding: "utf8", + env: options.env ? { ...process.env, ...options.env } : process.env, + input: options.input, + stdio: ["pipe", "pipe", "pipe"], + }); +} + test("normalizes AGENTS.md review findings for proactive payloads", () => { const findings = normalizeAgentInstructionFindings({ findings: [{ @@ -216,6 +228,68 @@ test("CLI JSON output does not expose host-open fields", async () => { } }); +test("CLI argument failures exit non-zero without exposing input", () => { + for (const args of [["--cwd", "--json"], ["--mode", "--json"], ["--cwd=", "--json"], ["--mode=", "--json"]]) { + const result = runCli(args, { + input: JSON.stringify({ prompt: "private user prompt" }), + }); + assert.notEqual(result.status, 0); + const payload = JSON.parse(result.stdout); + assert.equal(payload.ok, false); + assert.equal(payload.kind, "better-harness.review-trigger"); + assert.equal(payload.status, "error"); + assert.equal(payload.error.code, "invalid-arguments"); + assert.doesNotMatch(result.stdout, /private user prompt/u); + assert.doesNotMatch(result.stderr, /private user prompt/u); + } +}); + +test("CLI runtime failures exit non-zero without exposing cwd input", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-review-trigger-missing-")); + const missingCwd = path.join(root, "does-not-exist"); + + try { + const result = runCli(["--cwd", missingCwd, "--json"]); + assert.notEqual(result.status, 0); + const payload = JSON.parse(result.stdout); + assert.equal(payload.ok, false); + assert.equal(payload.kind, "better-harness.review-trigger"); + assert.equal(payload.status, "error"); + assert.equal(payload.error.code, "runtime-failure"); + assert.doesNotMatch(result.stdout, new RegExp(root.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"), "u")); + assert.doesNotMatch(result.stderr, new RegExp(root.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"), "u")); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("CLI fails closed when the blast-radius base ref is unavailable", async () => { + const repo = await makeRepo({ + "src/app.ts": "export const value = 1;\n", + }); + const unavailableRef = "refs/heads/private-missing-review-base"; + + try { + await writeFile(path.join(repo, "src/app.ts"), "export const value = 2;\n"); + const result = runCli(["--cwd", repo, "--json"], { + env: { + BETTER_HARNESS_BLAST_RADIUS_BASE: unavailableRef, + }, + }); + + assert.notEqual(result.status, 0); + const payload = JSON.parse(result.stdout); + assert.equal(payload.ok, false); + assert.equal(payload.status, "error"); + assert.equal(payload.error.code, "runtime-failure"); + assert.doesNotMatch(result.stdout, new RegExp(repo.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"), "u")); + assert.doesNotMatch(result.stdout, new RegExp(unavailableRef.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"), "u")); + assert.equal(result.stderr, ""); + } finally { + await rm(repo, { recursive: true, force: true }); + } +}); + test("runtime ignores harness-owned artifacts before change-test evidence", async () => { const repo = await makeRepo({ "AGENTS.md": "# Project Rules\n\nRun tests before finishing.\nNever commit secrets.\nNode workspace.\n", diff --git a/test/session-analysis.test.mjs b/test/session-analysis.test.mjs index a41c5f0..f391c33 100644 --- a/test/session-analysis.test.mjs +++ b/test/session-analysis.test.mjs @@ -615,6 +615,112 @@ test("Qoder analyzer discovers source roots and merges source coverage by sessio } }); +test("Qoder workspace analysis excludes unrelated home-only sessions", async () => { + const fixture = await makeQoderFixture(); + const analyzer = new QoderSessionAnalyzer(); + const unrelatedSessionId = "unrelated-home-only"; + const retainedHomePath = path.join(fixture.home, "sessions", `${fixture.sessionId}.jsonl`); + const unrelatedHomePath = path.join(fixture.home, "sessions", `${unrelatedSessionId}.jsonl`); + + await writeJsonl(retainedHomePath, [ + { + type: "tool.requested", + sessionId: fixture.sessionId, + timestamp: "2026-06-18T10:00:11.000Z", + cwd: fixture.workspace, + data: { tool_name: "Read", args: { file_path: "src/retained.ts" } }, + }, + { + type: "model.response.completed", + sessionId: fixture.sessionId, + timestamp: "2026-06-18T10:00:12.000Z", + cwd: fixture.workspace, + model: "workspace-model", + usage: { input_tokens: 10, output_tokens: 5 }, + }, + ]); + await writeJsonl(unrelatedHomePath, [ + { + type: "user", + sessionId: unrelatedSessionId, + timestamp: "2026-06-18T10:00:12.000Z", + cwd: path.join(fixture.root, "other-workspace"), + message: "/plan unrelated private project", + }, + { + type: "tool.requested", + sessionId: unrelatedSessionId, + timestamp: "2026-06-18T10:00:13.000Z", + cwd: path.join(fixture.root, "other-workspace"), + data: { tool_name: "Read", args: { file_path: "src/unrelated-secret.ts" } }, + }, + { + type: "model.response.completed", + sessionId: unrelatedSessionId, + timestamp: "2026-06-18T10:00:14.000Z", + cwd: path.join(fixture.root, "other-workspace"), + model: "unrelated-model", + usage: { input_tokens: 10, output_tokens: 5 }, + }, + ]); + + try { + const sessions = await analyzer.analyze({ + home: fixture.home, + workspace: fixture.workspace, + command: "sessions", + }); + assert.deepEqual(sessions.sessions.map((item) => item.sessionId), [fixture.sessionId]); + assert.equal( + sessions.sessions[0].sourceRefs.some((ref) => ref.kind === "home-session" && ref.path === retainedHomePath), + true, + ); + assert.equal(JSON.stringify(sessions).includes(unrelatedSessionId), false); + + const show = await analyzer.analyze({ + home: fixture.home, + workspace: fixture.workspace, + command: "show", + "session-id": unrelatedSessionId, + "include-events": true, + }); + assert.equal(show.sessions.length, 0); + + const facets = await analyzer.analyze({ + home: fixture.home, + workspace: fixture.workspace, + command: "facets", + limit: 10, + }); + assert.equal(facets.sessions.some((session) => session.sessionId === unrelatedSessionId), false); + assert.equal(facets.facets.sourceCoverage["home-session"], 1); + assert.equal(facets.facets.topTools.some((item) => item.name === "Read"), true); + assert.equal(facets.facets.planningSignals.some((item) => item.name === "/plan"), false); + + const insights = await analyzer.analyze({ + home: fixture.home, + workspace: fixture.workspace, + command: "insights", + selection: "all-eligible", + limit: 10, + }); + assert.equal(JSON.stringify(insights).includes(unrelatedSessionId), false); + assert.equal(insights.facets.topModels.some((item) => item.name === "unrelated-model"), false); + assert.equal(insights.insights.keySignals.usageEfficiency.coverage.responseCount, 1); + + const fileReads = await analyzer.analyze({ + home: fixture.home, + workspace: fixture.workspace, + command: "file-reads", + limit: 10, + }); + assert.equal(JSON.stringify(fileReads.fileReads).includes("src/retained.ts"), true); + assert.equal(JSON.stringify(fileReads.fileReads).includes("src/unrelated-secret.ts"), false); + } finally { + await rm(fixture.root, { recursive: true, force: true }); + } +}); + test("Qoder facts route returns only a compact privacy-safe envelope", async () => { const fixture = await makeQoderFixture(); const analyzer = new QoderSessionAnalyzer(); @@ -686,7 +792,7 @@ test("Qoder facts route returns only a compact privacy-safe envelope", async () assert.equal(result.candidateSelection.strategy, "agent-work-loop-portfolio-v1"); assert.equal(result.candidateSelection.emittedClasses["validation-repair"], 1); assert.equal(result.candidates.length <= 3, true); - assert.equal(result.omitted.homeSessionOnly >= 1, true); + assert.equal(result.omitted.homeSessionOnly, 0); assert.equal(result.cost.serializedBytes <= 8_192, true); assert.equal(result.cost.estimatedTokens <= 2_000, true); assert.deepEqual(result.candidates[0].changes, { edits: 1, files: 1 }); diff --git a/test/session-usage-summary.test.mjs b/test/session-usage-summary.test.mjs index 159a4e0..31c86b0 100644 --- a/test/session-usage-summary.test.mjs +++ b/test/session-usage-summary.test.mjs @@ -1,11 +1,12 @@ import assert from "node:assert/strict"; import { existsSync } from "node:fs"; -import { mkdir, mkdtemp, readdir, rm } from "node:fs/promises"; +import { mkdir, mkdtemp, readdir, rm, writeFile } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; import { spawnSync } from "node:child_process"; import test from "node:test"; +import { workspaceToQoderSlug } from "../scripts/session-analysis/platforms/qoder.mjs"; import { buildUsageSummary } from "../scripts/session-analysis/usage-summary.mjs"; const ROOT = path.resolve(import.meta.dirname, ".."); @@ -18,6 +19,11 @@ function runCli(args) { }); } +async function writeJsonl(filePath, rows) { + await mkdir(path.dirname(filePath), { recursive: true }); + await writeFile(filePath, `${rows.map((row) => JSON.stringify(row)).join("\n")}\n`); +} + test("usage summary keeps the decision boundary and removes private detail", () => { const summary = buildUsageSummary({ scope: { workspace: "/Users/private/repo" }, @@ -106,6 +112,58 @@ test("public usage summary is read-only and emits compact JSON", async () => { } }); +test("public usage summary excludes unrelated Qoder home-only sessions", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-usage-boundary-")); + const workspace = path.join(root, "workspace"); + const otherWorkspace = path.join(root, "other-workspace"); + const home = path.join(root, "qoder-home"); + const workspaceSessionId = "workspace-session"; + const unrelatedSessionId = "unrelated-home-only"; + const slug = workspaceToQoderSlug(workspace); + await mkdir(workspace, { recursive: true }); + + await writeJsonl(path.join(home, "projects", slug, `${workspaceSessionId}.jsonl`), [ + { + type: "model.response.completed", + sessionId: workspaceSessionId, + timestamp: "2026-06-18T10:00:00.000Z", + cwd: workspace, + model: "workspace-model", + usage: { input_tokens: 20, output_tokens: 4 }, + }, + ]); + await writeJsonl(path.join(home, "sessions", `${unrelatedSessionId}.jsonl`), [ + { + type: "model.response.completed", + sessionId: unrelatedSessionId, + timestamp: "2026-06-18T10:01:00.000Z", + cwd: otherWorkspace, + model: "unrelated-model", + usage: { input_tokens: 200, output_tokens: 40 }, + }, + ]); + + try { + const result = runCli([ + "session-analysis", + "usage-summary", + "--platform", "qoder", + "--workspace", workspace, + "--home", home, + "--format", "json", + ]); + assert.equal(result.status, 0, result.stderr); + const payload = JSON.parse(result.stdout); + assert.equal(payload.selection.eligibleCount, 1); + assert.equal(payload.usageEfficiency.coverage.responseCount, 1); + assert.equal(payload.usageEfficiency.coverage.nonZeroUsageCount, 1); + assert.equal(payload.usageEfficiency.modelUsage.some((item) => item.model === "workspace-model"), true); + assert.equal(payload.usageEfficiency.modelUsage.some((item) => item.model === "unrelated-model"), false); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + test("public usage summary help exposes its no-write contract", () => { const result = runCli(["session-analysis", "usage-summary", "--help"]); assert.equal(result.status, 0, result.stderr); From db3963f6e0576aad202c92290c1c47cead76c356 Mon Sep 17 00:00:00 2001 From: Phodal Huang Date: Wed, 29 Jul 2026 13:11:59 +0800 Subject: [PATCH 2/2] fix(harness): close review boundary gaps Filter Qoder home-session evidence per record while preserving explicit user-global analysis. Fail review-trigger on Git probe errors, reject empty report cwd values, and bound cloc reads before loading file content. Completes the PR review follow-ups for #12, #15, #16, and #18 under docs/specs/2026-07-29-11-18-review-boundary-hardening.md. Validated with the 80-test focused boundary run, npm run check, the documentation link graph, syntax checks, and staged diff checks. --- ...6-07-29-11-18-review-boundary-hardening.md | 52 +++++--- scripts/cloc/analyze.mjs | 1 + scripts/cloc/count-file.mjs | 44 ++++++- scripts/harness-quickstart/cli.mjs | 19 +++ scripts/review-trigger/cli.mjs | 7 +- scripts/session-analysis/platforms/qoder.mjs | 45 ++++--- test/cloc.test.mjs | 21 +++ test/harness-quickstart.test.mjs | 15 +++ test/review-trigger.test.mjs | 17 +++ test/session-analysis.test.mjs | 120 ++++++++++++++++++ 10 files changed, 301 insertions(+), 40 deletions(-) diff --git a/docs/specs/2026-07-29-11-18-review-boundary-hardening.md b/docs/specs/2026-07-29-11-18-review-boundary-hardening.md index 6fa0b10..5a4b715 100644 --- a/docs/specs/2026-07-29-11-18-review-boundary-hardening.md +++ b/docs/specs/2026-07-29-11-18-review-boundary-hardening.md @@ -21,22 +21,28 @@ the product surface or changing successful-path report semantics. 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. + 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 and runtime failures exit non-zero while - successful findings retain their documented non-blocking result. -- AC-06 (#16): `report --cwd` rejects missing and non-directory targets before - starting evidence collectors; valid-directory fallback behavior remains - available. +- 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 or reads a non-regular file; skipped results remain bounded - and do not reveal the external target. + 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. @@ -56,15 +62,16 @@ the product surface or changing successful-path report semantics. 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 one workspace-ownership predicate to Qoder home-session discovery and - hydration, keeping explicit global behavior separate. -4. Make quickstart privacy and cwd validation explicit at its CLI boundary. +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 execution failure from successful non-blocking - findings. -7. Make severity repair conservative and make cloc file reads type- and - containment-aware. +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 @@ -85,16 +92,17 @@ Decision rationale: `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` + `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` + `node --test test/cloc.test.mjs`, including an oversized regular file - AC-09: `npm test` `npm run pack:verify` @@ -102,9 +110,13 @@ Decision rationale: Review evidence: -- `npm run check` passed with 852 tests and package verification. -- The final blocker-focused run passed 68 tests across Secret Guard, - review-trigger, blast-radius, and cloc; syntax and diff checks also passed. +- 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 diff --git a/scripts/cloc/analyze.mjs b/scripts/cloc/analyze.mjs index ed4f636..1aaaaf2 100644 --- a/scripts/cloc/analyze.mjs +++ b/scripts/cloc/analyze.mjs @@ -178,6 +178,7 @@ export async function analyzeCloc(options = {}) { const workerCount = chooseWorkerCount(options.workers, fileList.files.length); const countOptions = { markdownCode: Boolean(options.markdownCode), + maxFileBytes: options.maxFileBytes, }; const includeFiles = options.includeFiles !== false; if (!includeFiles) { diff --git a/scripts/cloc/count-file.mjs b/scripts/cloc/count-file.mjs index b09b026..ce1980b 100644 --- a/scripts/cloc/count-file.mjs +++ b/scripts/cloc/count-file.mjs @@ -4,7 +4,7 @@ import { fstatSync, lstatSync, openSync, - readFileSync, + readSync, realpathSync, } from "node:fs"; import path from "node:path"; @@ -36,6 +36,33 @@ const LOCK_FILE_NAMES = new Set([ "poetry.lock", "yarn.lock", ]); +const DEFAULT_MAX_FILE_BYTES = 16 * 1024 * 1024; +const READ_CHUNK_BYTES = 64 * 1024; + +function maxFileBytes(options = {}) { + const configured = Number(options.maxFileBytes); + return Number.isSafeInteger(configured) && configured > 0 + ? configured + : DEFAULT_MAX_FILE_BYTES; +} + +function readBoundedFile(descriptor, limit) { + const chunks = []; + let total = 0; + while (total <= limit) { + const chunk = Buffer.allocUnsafe(Math.min(READ_CHUNK_BYTES, limit - total + 1)); + const bytesRead = readSync(descriptor, chunk, 0, chunk.length, null); + if (bytesRead === 0) { + return Buffer.concat(chunks, total); + } + total += bytesRead; + if (total > limit) { + return null; + } + chunks.push(chunk.subarray(0, bytesRead)); + } + return null; +} function emptyTotals() { return { @@ -267,6 +294,7 @@ export function countKnownFileBuffer(buffer, filePath, options = {}) { export function countFile(repoRoot, filePath, options = {}) { const normalized = toPosix(filePath); + const fileSizeLimit = maxFileBytes(options); if (!isCountablePath(normalized)) { return { path: normalized, skipped: true, reason: "unsupported" }; } @@ -286,6 +314,9 @@ export function countFile(repoRoot, filePath, options = {}) { if (!stat.isFile()) { return { path: normalized, skipped: true, reason: "non-regular" }; } + if (stat.size > fileSizeLimit) { + return { path: normalized, skipped: true, reason: "too-large" }; + } try { const realRoot = realpathSync(root); @@ -302,10 +333,17 @@ export function countFile(repoRoot, filePath, options = {}) { try { const noFollow = fsConstants.O_NOFOLLOW ?? 0; descriptor = openSync(absolutePath, fsConstants.O_RDONLY | noFollow); - if (!fstatSync(descriptor).isFile()) { + const openedStat = fstatSync(descriptor); + if (!openedStat.isFile()) { return { path: normalized, skipped: true, reason: "non-regular" }; } - buffer = readFileSync(descriptor); + if (openedStat.size > fileSizeLimit) { + return { path: normalized, skipped: true, reason: "too-large" }; + } + buffer = readBoundedFile(descriptor, fileSizeLimit); + if (buffer === null) { + return { path: normalized, skipped: true, reason: "too-large" }; + } } catch { return { path: normalized, skipped: true, reason: "unreadable" }; } finally { diff --git a/scripts/harness-quickstart/cli.mjs b/scripts/harness-quickstart/cli.mjs index 090343e..78e2f0c 100644 --- a/scripts/harness-quickstart/cli.mjs +++ b/scripts/harness-quickstart/cli.mjs @@ -140,6 +140,20 @@ function cwdValidationError(cwd) { return null; } +function explicitCwdValidationError(options) { + if (!Object.prototype.hasOwnProperty.call(options, "cwd")) { + return null; + } + if (typeof options.cwd === "string" && options.cwd.trim()) { + return null; + } + return { + code: "INVALID_CWD", + message: "Repository cwd requires a non-empty directory value.", + hint: "Pass --cwd with an existing repository directory.", + }; +} + function errorPayload(error) { return { ok: false, @@ -281,6 +295,11 @@ export function main(argv = process.argv.slice(2)) { return 0; } const options = parseArgs(argv); + const optionError = explicitCwdValidationError(options); + if (optionError) { + renderError(optionError, Boolean(options.json)); + return 1; + } const cwd = options.cwd ? path.resolve(String(options.cwd)) : process.cwd(); const validationError = cwdValidationError(cwd); if (validationError) { diff --git a/scripts/review-trigger/cli.mjs b/scripts/review-trigger/cli.mjs index 177247a..6e47306 100644 --- a/scripts/review-trigger/cli.mjs +++ b/scripts/review-trigger/cli.mjs @@ -168,8 +168,11 @@ function hasLocalChanges(cwd) { stdio: ["ignore", "pipe", "ignore"], timeout: 10_000, }); - if (result.status !== 0) { - return false; + if (result.error || result.status !== 0) { + throw new ReviewTriggerCliError( + "runtime-failure", + "review-trigger could not inspect the Git worktree", + ); } return result.stdout.trim().length > 0; } diff --git a/scripts/session-analysis/platforms/qoder.mjs b/scripts/session-analysis/platforms/qoder.mjs index d34700a..4ceb930 100644 --- a/scripts/session-analysis/platforms/qoder.mjs +++ b/scripts/session-analysis/platforms/qoder.mjs @@ -663,15 +663,6 @@ function sessionHasWorkspaceEvidence(session) { .some((ref) => ref.kind !== "home-session" && ref.planningScope !== "user-global"); } -function sessionHasGlobalEvidence(session) { - return (session?.sourceRefMap ? [...session.sourceRefMap.values()] : []) - .some((ref) => ref.kind !== "home-session" && ref.planningScope === "user-global"); -} - -function homeSessionPlanningScope(session) { - return sessionHasWorkspaceEvidence(session) ? "workspace" : "user-global"; -} - function sourceKey(ref) { return `${ref.kind}:${ref.path}`; } @@ -1202,8 +1193,8 @@ export class QoderSessionAnalyzer extends SessionAnalyzer { const sessionId = probe.sessionId ?? fallbackId; const existingSession = sessions.get(sessionId); const verifiedWorkspaceSession = sessionHasWorkspaceEvidence(existingSession); - const verifiedGlobalSession = scope.includeGlobalCapabilities && sessionHasGlobalEvidence(existingSession); - if (!probe.workspaceMatched && !verifiedWorkspaceSession && !verifiedGlobalSession) { + const planningScope = probe.workspaceMatched || verifiedWorkspaceSession ? "workspace" : "user-global"; + if (planningScope === "user-global" && !scope.includeGlobalCapabilities) { continue; } addSessionRef(sessions, sessionId, scope.workspace, { @@ -1211,7 +1202,7 @@ export class QoderSessionAnalyzer extends SessionAnalyzer { path: filePath, eventType: "home-session", timestamp: probe.timestamp, - planningScope: probe.workspaceMatched ? "workspace" : homeSessionPlanningScope(existingSession), + planningScope, }); } } @@ -1457,6 +1448,9 @@ export class QoderSessionAnalyzer extends SessionAnalyzer { const includeCommandText = parseBooleanFlag(options["include-command-text"] ?? options.includeCommandText ?? false); const includeUserText = parseBooleanFlag(options["include-user-text"] ?? options.includeUserText ?? false); const refs = session.sourceRefs ?? []; + const workspaceLinked = refs.some( + (ref) => ref.kind !== "home-session" && ref.planningScope !== "user-global", + ); for (const ref of refs) { if (ref.kind === "audit-jsonl") { @@ -1464,12 +1458,21 @@ export class QoderSessionAnalyzer extends SessionAnalyzer { } else if (ref.kind === "project-state") { await this.readStateEvent(session.sessionId, ref, events, { includeContent, includeCommandText, includeUserText }); } else if (ref.path.endsWith(JSONL_EXT)) { - await this.readJsonlEvents(session.sessionId, ref, events, { includeContent, includeCommandText, includeUserText }); + await this.readJsonlEvents( + session.sessionId, + scope, + ref, + events, + { includeContent, includeCommandText, includeUserText }, + { workspaceLinked }, + ); } } return events - .map((event) => event.cwd ? event : { ...event, cwd: scope.workspace }) + .map((event) => event.cwd || event.planningScope === "user-global" + ? event + : { ...event, cwd: scope.workspace }) .filter((event) => withinTimeRange(event.timestamp, scope)) .sort((a, b) => { const left = timestampMillis(a.timestamp) ?? 0; @@ -1481,13 +1484,25 @@ export class QoderSessionAnalyzer extends SessionAnalyzer { }); } - async readJsonlEvents(sessionId, ref, events, options) { + async readJsonlEvents(sessionId, scope, ref, events, options, { workspaceLinked = false } = {}) { await forEachJsonLine(ref.path, (raw, line) => { const sourceRef = { ...ref, line, sessionId }; const rawSessionId = inferSessionId(raw, sourceRef); if (rawSessionId && rawSessionId !== sessionId && ref.kind !== "logs-session") { return; } + if (ref.kind === "home-session") { + if (ref.planningScope === "user-global") { + if (!scope.includeGlobalCapabilities) { + return; + } + } else { + const recordCwd = inferCwd(raw); + if (recordCwd ? !isWorkspaceMatch(recordCwd, scope.workspace) : !workspaceLinked) { + return; + } + } + } events.push(this.normalizeEvent(raw, sourceRef, options)); }); } diff --git a/test/cloc.test.mjs b/test/cloc.test.mjs index dc729f7..4945033 100644 --- a/test/cloc.test.mjs +++ b/test/cloc.test.mjs @@ -174,6 +174,27 @@ test("unsupported and lock files are skipped before reading content", async () = } }); +test("cloc skips regular files beyond the configured read boundary", async () => { + const repo = await mkdtemp(path.join(os.tmpdir(), "better-harness-cloc-size-boundary-")); + + try { + await writeFixtureFile(repo, "src/large.js", "const value = 'this file exceeds the test boundary';\n"); + + assert.deepEqual(countFile(repo, "src/large.js", { maxFileBytes: 16 }), { + path: "src/large.js", + skipped: true, + reason: "too-large", + }); + assert.equal(countFile(repo, "src/large.js", { maxFileBytes: 1_024 }).skipped, false); + + const report = await analyzeCloc({ cwd: repo, useGit: false, workers: 1, maxFileBytes: 16 }); + assert.equal(report.fileList.counted, 0); + assert.deepEqual(report.skippedFiles, [{ path: "src/large.js", reason: "too-large" }]); + } finally { + await removeFixtureTree(repo); + } +}); + test("git-backed cloc skips tracked symlinks without leaking external targets", async (t) => { const repo = await makeRepo({ "src/app.js": "const a = 1;\n", diff --git a/test/harness-quickstart.test.mjs b/test/harness-quickstart.test.mjs index cbb43b1..7c2f9d7 100644 --- a/test/harness-quickstart.test.mjs +++ b/test/harness-quickstart.test.mjs @@ -141,6 +141,21 @@ test("Better Harness report rejects a missing cwd before collecting evidence", a } }); +test("Better Harness report rejects an explicitly empty cwd", () => { + const machine = runBetterHarness(["report", "--cwd=", "--json"]); + assert.equal(machine.status, 1); + assert.equal(machine.stderr, ""); + const payload = JSON.parse(machine.stdout); + assert.equal(payload.ok, false); + assert.equal(payload.error.code, "INVALID_CWD"); + assert.match(payload.error.message, /requires a non-empty directory value/u); + + const human = runBetterHarness(["report", "--cwd="]); + assert.equal(human.status, 1); + assert.equal(human.stdout, ""); + assert.match(human.stderr, /requires a non-empty directory value/u); +}); + test("Better Harness report rejects a regular-file cwd before collecting evidence", async () => { const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-quickstart-file-cwd-")); const filePath = path.join(root, "not-a-directory"); diff --git a/test/review-trigger.test.mjs b/test/review-trigger.test.mjs index 78e0f59..2df1489 100644 --- a/test/review-trigger.test.mjs +++ b/test/review-trigger.test.mjs @@ -263,6 +263,23 @@ test("CLI runtime failures exit non-zero without exposing cwd input", async () = } }); +test("CLI fails closed when Git worktree inspection fails", async () => { + const nonGitCwd = await mkdtemp(path.join(os.tmpdir(), "better-harness-review-trigger-non-git-")); + + try { + const result = runCli(["--cwd", nonGitCwd, "--json"]); + assert.notEqual(result.status, 0); + const payload = JSON.parse(result.stdout); + assert.equal(payload.ok, false); + assert.equal(payload.status, "error"); + assert.equal(payload.error.code, "runtime-failure"); + assert.doesNotMatch(result.stdout, new RegExp(nonGitCwd.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"), "u")); + assert.equal(result.stderr, ""); + } finally { + await rm(nonGitCwd, { recursive: true, force: true }); + } +}); + test("CLI fails closed when the blast-radius base ref is unavailable", async () => { const repo = await makeRepo({ "src/app.ts": "export const value = 1;\n", diff --git a/test/session-analysis.test.mjs b/test/session-analysis.test.mjs index f391c33..a3e91fc 100644 --- a/test/session-analysis.test.mjs +++ b/test/session-analysis.test.mjs @@ -638,6 +638,12 @@ test("Qoder workspace analysis excludes unrelated home-only sessions", async () model: "workspace-model", usage: { input_tokens: 10, output_tokens: 5 }, }, + { + type: "tool.requested", + sessionId: fixture.sessionId, + timestamp: "2026-06-18T10:00:13.000Z", + data: { tool_name: "Read", args: { file_path: "src/linked-without-cwd.ts" } }, + }, ]); await writeJsonl(unrelatedHomePath, [ { @@ -715,12 +721,126 @@ test("Qoder workspace analysis excludes unrelated home-only sessions", async () limit: 10, }); assert.equal(JSON.stringify(fileReads.fileReads).includes("src/retained.ts"), true); + assert.equal(JSON.stringify(fileReads.fileReads).includes("src/linked-without-cwd.ts"), true); assert.equal(JSON.stringify(fileReads.fileReads).includes("src/unrelated-secret.ts"), false); } finally { await rm(fixture.root, { recursive: true, force: true }); } }); +test("Qoder home-session hydration filters foreign and unverified cwd-less records", async () => { + const fixture = await makeQoderFixture(); + const analyzer = new QoderSessionAnalyzer(); + const sessionId = "mixed-home-session"; + const otherWorkspace = path.join(fixture.root, "other-workspace"); + + await writeJsonl(path.join(fixture.home, "sessions", `${sessionId}.jsonl`), [ + { + type: "user", + sessionId, + timestamp: "2026-06-18T10:00:00.000Z", + cwd: fixture.workspace, + message: "workspace request", + }, + { + type: "user", + sessionId, + timestamp: "2026-06-18T10:00:01.000Z", + cwd: otherWorkspace, + message: "foreign private request", + }, + { + type: "user", + sessionId, + timestamp: "2026-06-18T10:00:02.000Z", + message: "unverified private request", + }, + ]); + + try { + const scope = await analyzer.resolveScope({ + home: fixture.home, + workspace: fixture.workspace, + command: "events", + "session-id": sessionId, + }); + const roots = await analyzer.discoverSourceRoots(scope); + const sessions = await analyzer.discoverSessions(scope, roots); + const session = sessions.find((candidate) => candidate.sessionId === sessionId); + assert.ok(session); + + const events = await analyzer.readSession(session, scope, { + includeContent: true, + includeUserText: true, + }); + assert.deepEqual(events.map((event) => event.userText), ["workspace request"]); + assert.deepEqual(events.map((event) => event.cwd), [fixture.workspace]); + assert.equal(JSON.stringify(events).includes("foreign private request"), false); + assert.equal(JSON.stringify(events).includes("unverified private request"), false); + } finally { + await rm(fixture.root, { recursive: true, force: true }); + } +}); + +test("Qoder explicit global analysis retains home-only sessions as user-global", async () => { + const fixture = await makeQoderFixture(); + const analyzer = new QoderSessionAnalyzer(); + const sessionId = "global-home-only"; + const otherWorkspace = path.join(fixture.root, "other-workspace"); + + await writeJsonl(path.join(fixture.home, "sessions", `${sessionId}.jsonl`), [ + { + type: "user", + sessionId, + timestamp: "2026-06-18T10:00:00.000Z", + cwd: otherWorkspace, + message: "explicit global request", + }, + { + type: "user", + sessionId, + timestamp: "2026-06-18T10:00:01.000Z", + message: "global request without cwd", + }, + ]); + + try { + const defaultResult = await analyzer.analyze({ + home: fixture.home, + workspace: fixture.workspace, + command: "sessions", + }); + assert.equal(defaultResult.sessions.some((session) => session.sessionId === sessionId), false); + + const scope = await analyzer.resolveScope({ + home: fixture.home, + workspace: fixture.workspace, + command: "events", + "session-id": sessionId, + includeGlobalCapabilities: true, + }); + const roots = await analyzer.discoverSourceRoots(scope); + const sessions = await analyzer.discoverSessions(scope, roots); + const session = sessions.find((candidate) => candidate.sessionId === sessionId); + assert.ok(session); + assert.equal(session.sourceRefs.find((ref) => ref.kind === "home-session")?.planningScope, "user-global"); + + const events = await analyzer.readSession(session, scope, { + includeContent: true, + includeUserText: true, + }); + assert.deepEqual(events.map((event) => event.userText), [ + "explicit global request", + "global request without cwd", + ]); + assert.equal(events.every((event) => event.planningScope === "user-global"), true); + assert.equal(events[0].cwd, otherWorkspace); + assert.equal(Object.hasOwn(events[1], "cwd"), false); + } finally { + await rm(fixture.root, { recursive: true, force: true }); + } +}); + test("Qoder facts route returns only a compact privacy-safe envelope", async () => { const fixture = await makeQoderFixture(); const analyzer = new QoderSessionAnalyzer();