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..5a4b715 --- /dev/null +++ b/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. 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/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 2868f36..ce1980b 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, + readSync, + realpathSync, +} from "node:fs"; import path from "node:path"; import { @@ -28,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 { @@ -77,6 +112,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)) { @@ -253,20 +294,66 @@ 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" }; } - 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" }; + } + if (stat.size > fileSizeLimit) { + return { path: normalized, skipped: true, reason: "too-large" }; + } + 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); + const openedStat = fstatSync(descriptor); + if (!openedStat.isFile()) { + return { path: normalized, skipped: true, reason: "non-regular" }; + } + 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 { + 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..78e2f0c 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,67 @@ 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 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, + 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 +212,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", @@ -232,7 +295,17 @@ 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) { + 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..6e47306 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"); } @@ -159,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; } @@ -316,8 +328,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 +342,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 +398,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 +442,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 +485,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 +507,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 +534,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..4ceb930 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,11 @@ function createSessionRecord(sessionId, workspace) { }; } +function sessionHasWorkspaceEvidence(session) { + return (session?.sourceRefMap ? [...session.sourceRefMap.values()] : []) + .some((ref) => ref.kind !== "home-session" && ref.planningScope !== "user-global"); +} + function sourceKey(ref) { return `${ref.kind}:${ref.path}`; } @@ -1157,17 +1185,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 planningScope = probe.workspaceMatched || verifiedWorkspaceSession ? "workspace" : "user-global"; + if (planningScope === "user-global" && !scope.includeGlobalCapabilities) { + continue; + } + addSessionRef(sessions, sessionId, scope.workspace, { kind: "home-session", path: filePath, eventType: "home-session", timestamp: probe.timestamp, + planningScope, }); } } @@ -1413,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") { @@ -1420,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; @@ -1437,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/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..4945033 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,101 @@ 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", + }); + 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..7c2f9d7 100644 --- a/test/harness-quickstart.test.mjs +++ b/test/harness-quickstart.test.mjs @@ -97,6 +97,103 @@ 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 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"); + + 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..2df1489 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,85 @@ 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 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", + }); + 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..a3e91fc 100644 --- a/test/session-analysis.test.mjs +++ b/test/session-analysis.test.mjs @@ -615,6 +615,232 @@ 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 }, + }, + { + 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, [ + { + 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/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(); @@ -686,7 +912,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);