Skip to content

Commit db3963f

Browse files
committed
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.
1 parent 878287a commit db3963f

10 files changed

Lines changed: 301 additions & 40 deletions

File tree

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

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -21,22 +21,28 @@ the product surface or changing successful-path report semantics.
2121
decisions.
2222
- AC-02 (#12): Workspace-scoped Qoder analysis excludes home-only sessions that
2323
have no verified relationship to the requested workspace across sessions,
24-
events/show, facets, insights, file-reads, and usage-summary.
24+
events/show, facets, insights, file-reads, and usage-summary. A matching
25+
record cannot authorize foreign-cwd records from the same home session, and
26+
cwd-less records require a workspace-linked session source instead of being
27+
assigned to the requested workspace. An explicit global-capability pass can
28+
retain home-only sessions only as `user-global` evidence.
2529
- AC-03 (#13): `report --no-sessions` executes no session probe and always uses
2630
the static `software-fluency` route even when local sessions exist.
2731
- AC-04 (#14): Blast-radius collection distinguishes an invalid/unavailable
2832
base ref from a real empty diff and returns an explicit fail-closed error.
29-
- AC-05 (#15): review-trigger argument and runtime failures exit non-zero while
30-
successful findings retain their documented non-blocking result.
31-
- AC-06 (#16): `report --cwd` rejects missing and non-directory targets before
32-
starting evidence collectors; valid-directory fallback behavior remains
33-
available.
33+
- AC-05 (#15): review-trigger argument, Git worktree probes, and runtime
34+
failures exit non-zero while successful findings retain their documented
35+
non-blocking result.
36+
- AC-06 (#16): `report --cwd` rejects empty, missing, and non-directory targets
37+
before starting evidence collectors; valid-directory fallback behavior
38+
remains available.
3439
- AC-07 (#17): findings repair never lowers `Critical` or arbitrary unknown
3540
severity to `Medium`; unsupported values fail closed or use an explicit
3641
conservative mapping.
3742
- AC-08 (#18): Git-backed cloc never follows a tracked path to a target outside
38-
the repository or reads a non-regular file; skipped results remain bounded
39-
and do not reveal the external target.
43+
the repository, reads a non-regular file, or reads a regular file beyond the
44+
configured size boundary; skipped results remain bounded and do not reveal
45+
the external target.
4046
- AC-09: Focused tests, the full Node test suite, package verification, syntax
4147
checks, and review-readiness checks pass on Linux-compatible local tooling;
4248
cross-platform path behavior remains covered by portable fixtures.
@@ -56,15 +62,16 @@ the product surface or changing successful-path report semantics.
5662
1. Add failing regression tests for each issue before changing implementation.
5763
2. Normalize Secret Guard tool events once, then scan only the content fields
5864
owned by matched write tools.
59-
3. Apply one workspace-ownership predicate to Qoder home-session discovery and
60-
hydration, keeping explicit global behavior separate.
61-
4. Make quickstart privacy and cwd validation explicit at its CLI boundary.
65+
3. Apply workspace ownership at both Qoder home-session discovery and per-event
66+
hydration, keeping explicit `user-global` behavior separate.
67+
4. Make quickstart privacy and empty/missing cwd validation explicit at its CLI
68+
boundary.
6269
5. Replace Git diff ambiguity with a structured failure from blast-radius
6370
collection.
64-
6. Separate review-trigger execution failure from successful non-blocking
65-
findings.
66-
7. Make severity repair conservative and make cloc file reads type- and
67-
containment-aware.
71+
6. Separate review-trigger argument, Git-probe, and execution failures from
72+
successful non-blocking findings.
73+
7. Make severity repair conservative and make cloc file reads type-, size-,
74+
and containment-aware.
6875
8. Run focused tests per module, then the complete repository checks.
6976
9. Perform independent code-review and architecture lanes, address all
7077
Critical/High findings and any architectural Block, then run Review
@@ -85,26 +92,31 @@ Decision rationale:
8592
`node --test test/agent-guardrails-secret-scan.test.mjs`
8693
- AC-02:
8794
`node --test test/session-analysis.test.mjs test/session-usage-summary.test.mjs`
95+
including mixed-cwd, cwd-less, and explicit user-global home-session cases
8896
- AC-03 and AC-06:
8997
`node --test test/harness-quickstart.test.mjs test/better-harness-cli.test.mjs`
9098
- AC-04:
9199
`node --test test/blast-radius.test.mjs`
92100
- AC-05:
93-
`node --test test/review-trigger.test.mjs`
101+
`node --test test/review-trigger.test.mjs`, including a non-Git directory
94102
- AC-07:
95103
`node --test test/harness-findings-repair.test.mjs test/harness-report-render-cli.test.mjs`
96104
- AC-08:
97-
`node --test test/cloc.test.mjs`
105+
`node --test test/cloc.test.mjs`, including an oversized regular file
98106
- AC-09:
99107
`npm test`
100108
`npm run pack:verify`
101109
`node --test test/doc-link-graph.test.mjs`
102110

103111
Review evidence:
104112

105-
- `npm run check` passed with 852 tests and package verification.
106-
- The final blocker-focused run passed 68 tests across Secret Guard,
107-
review-trigger, blast-radius, and cloc; syntax and diff checks also passed.
113+
- Review follow-up (2026-07-29): mixed-cwd and cwd-less home-session hydration,
114+
explicit user-global analysis, failed Git worktree probes, empty `--cwd=`,
115+
and bounded cloc reads are covered by regressions and resolved before merge.
116+
- `npm run check` passed with the full Node suite and package verification.
117+
- The review follow-up focused run passed 80 tests across Qoder session
118+
analysis, session usage summary, review-trigger, quickstart, and cloc;
119+
syntax and diff checks also passed.
108120
- Code-reviewer recommendation: `APPROVE` after both reported High findings
109121
were fixed and re-reviewed.
110122
- Architect status: non-blocking `WATCH`; the previous fail-open `BLOCK` was

scripts/cloc/analyze.mjs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,7 @@ export async function analyzeCloc(options = {}) {
178178
const workerCount = chooseWorkerCount(options.workers, fileList.files.length);
179179
const countOptions = {
180180
markdownCode: Boolean(options.markdownCode),
181+
maxFileBytes: options.maxFileBytes,
181182
};
182183
const includeFiles = options.includeFiles !== false;
183184
if (!includeFiles) {

scripts/cloc/count-file.mjs

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import {
44
fstatSync,
55
lstatSync,
66
openSync,
7-
readFileSync,
7+
readSync,
88
realpathSync,
99
} from "node:fs";
1010
import path from "node:path";
@@ -36,6 +36,33 @@ const LOCK_FILE_NAMES = new Set([
3636
"poetry.lock",
3737
"yarn.lock",
3838
]);
39+
const DEFAULT_MAX_FILE_BYTES = 16 * 1024 * 1024;
40+
const READ_CHUNK_BYTES = 64 * 1024;
41+
42+
function maxFileBytes(options = {}) {
43+
const configured = Number(options.maxFileBytes);
44+
return Number.isSafeInteger(configured) && configured > 0
45+
? configured
46+
: DEFAULT_MAX_FILE_BYTES;
47+
}
48+
49+
function readBoundedFile(descriptor, limit) {
50+
const chunks = [];
51+
let total = 0;
52+
while (total <= limit) {
53+
const chunk = Buffer.allocUnsafe(Math.min(READ_CHUNK_BYTES, limit - total + 1));
54+
const bytesRead = readSync(descriptor, chunk, 0, chunk.length, null);
55+
if (bytesRead === 0) {
56+
return Buffer.concat(chunks, total);
57+
}
58+
total += bytesRead;
59+
if (total > limit) {
60+
return null;
61+
}
62+
chunks.push(chunk.subarray(0, bytesRead));
63+
}
64+
return null;
65+
}
3966

4067
function emptyTotals() {
4168
return {
@@ -267,6 +294,7 @@ export function countKnownFileBuffer(buffer, filePath, options = {}) {
267294

268295
export function countFile(repoRoot, filePath, options = {}) {
269296
const normalized = toPosix(filePath);
297+
const fileSizeLimit = maxFileBytes(options);
270298
if (!isCountablePath(normalized)) {
271299
return { path: normalized, skipped: true, reason: "unsupported" };
272300
}
@@ -286,6 +314,9 @@ export function countFile(repoRoot, filePath, options = {}) {
286314
if (!stat.isFile()) {
287315
return { path: normalized, skipped: true, reason: "non-regular" };
288316
}
317+
if (stat.size > fileSizeLimit) {
318+
return { path: normalized, skipped: true, reason: "too-large" };
319+
}
289320

290321
try {
291322
const realRoot = realpathSync(root);
@@ -302,10 +333,17 @@ export function countFile(repoRoot, filePath, options = {}) {
302333
try {
303334
const noFollow = fsConstants.O_NOFOLLOW ?? 0;
304335
descriptor = openSync(absolutePath, fsConstants.O_RDONLY | noFollow);
305-
if (!fstatSync(descriptor).isFile()) {
336+
const openedStat = fstatSync(descriptor);
337+
if (!openedStat.isFile()) {
306338
return { path: normalized, skipped: true, reason: "non-regular" };
307339
}
308-
buffer = readFileSync(descriptor);
340+
if (openedStat.size > fileSizeLimit) {
341+
return { path: normalized, skipped: true, reason: "too-large" };
342+
}
343+
buffer = readBoundedFile(descriptor, fileSizeLimit);
344+
if (buffer === null) {
345+
return { path: normalized, skipped: true, reason: "too-large" };
346+
}
309347
} catch {
310348
return { path: normalized, skipped: true, reason: "unreadable" };
311349
} finally {

scripts/harness-quickstart/cli.mjs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,20 @@ function cwdValidationError(cwd) {
140140
return null;
141141
}
142142

143+
function explicitCwdValidationError(options) {
144+
if (!Object.prototype.hasOwnProperty.call(options, "cwd")) {
145+
return null;
146+
}
147+
if (typeof options.cwd === "string" && options.cwd.trim()) {
148+
return null;
149+
}
150+
return {
151+
code: "INVALID_CWD",
152+
message: "Repository cwd requires a non-empty directory value.",
153+
hint: "Pass --cwd with an existing repository directory.",
154+
};
155+
}
156+
143157
function errorPayload(error) {
144158
return {
145159
ok: false,
@@ -281,6 +295,11 @@ export function main(argv = process.argv.slice(2)) {
281295
return 0;
282296
}
283297
const options = parseArgs(argv);
298+
const optionError = explicitCwdValidationError(options);
299+
if (optionError) {
300+
renderError(optionError, Boolean(options.json));
301+
return 1;
302+
}
284303
const cwd = options.cwd ? path.resolve(String(options.cwd)) : process.cwd();
285304
const validationError = cwdValidationError(cwd);
286305
if (validationError) {

scripts/review-trigger/cli.mjs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -168,8 +168,11 @@ function hasLocalChanges(cwd) {
168168
stdio: ["ignore", "pipe", "ignore"],
169169
timeout: 10_000,
170170
});
171-
if (result.status !== 0) {
172-
return false;
171+
if (result.error || result.status !== 0) {
172+
throw new ReviewTriggerCliError(
173+
"runtime-failure",
174+
"review-trigger could not inspect the Git worktree",
175+
);
173176
}
174177
return result.stdout.trim().length > 0;
175178
}

scripts/session-analysis/platforms/qoder.mjs

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -663,15 +663,6 @@ function sessionHasWorkspaceEvidence(session) {
663663
.some((ref) => ref.kind !== "home-session" && ref.planningScope !== "user-global");
664664
}
665665

666-
function sessionHasGlobalEvidence(session) {
667-
return (session?.sourceRefMap ? [...session.sourceRefMap.values()] : [])
668-
.some((ref) => ref.kind !== "home-session" && ref.planningScope === "user-global");
669-
}
670-
671-
function homeSessionPlanningScope(session) {
672-
return sessionHasWorkspaceEvidence(session) ? "workspace" : "user-global";
673-
}
674-
675666
function sourceKey(ref) {
676667
return `${ref.kind}:${ref.path}`;
677668
}
@@ -1202,16 +1193,16 @@ export class QoderSessionAnalyzer extends SessionAnalyzer {
12021193
const sessionId = probe.sessionId ?? fallbackId;
12031194
const existingSession = sessions.get(sessionId);
12041195
const verifiedWorkspaceSession = sessionHasWorkspaceEvidence(existingSession);
1205-
const verifiedGlobalSession = scope.includeGlobalCapabilities && sessionHasGlobalEvidence(existingSession);
1206-
if (!probe.workspaceMatched && !verifiedWorkspaceSession && !verifiedGlobalSession) {
1196+
const planningScope = probe.workspaceMatched || verifiedWorkspaceSession ? "workspace" : "user-global";
1197+
if (planningScope === "user-global" && !scope.includeGlobalCapabilities) {
12071198
continue;
12081199
}
12091200
addSessionRef(sessions, sessionId, scope.workspace, {
12101201
kind: "home-session",
12111202
path: filePath,
12121203
eventType: "home-session",
12131204
timestamp: probe.timestamp,
1214-
planningScope: probe.workspaceMatched ? "workspace" : homeSessionPlanningScope(existingSession),
1205+
planningScope,
12151206
});
12161207
}
12171208
}
@@ -1457,19 +1448,31 @@ export class QoderSessionAnalyzer extends SessionAnalyzer {
14571448
const includeCommandText = parseBooleanFlag(options["include-command-text"] ?? options.includeCommandText ?? false);
14581449
const includeUserText = parseBooleanFlag(options["include-user-text"] ?? options.includeUserText ?? false);
14591450
const refs = session.sourceRefs ?? [];
1451+
const workspaceLinked = refs.some(
1452+
(ref) => ref.kind !== "home-session" && ref.planningScope !== "user-global",
1453+
);
14601454

14611455
for (const ref of refs) {
14621456
if (ref.kind === "audit-jsonl") {
14631457
await this.readAuditEvents(session.sessionId, scope, ref, events, { includeContent, includeCommandText, includeUserText });
14641458
} else if (ref.kind === "project-state") {
14651459
await this.readStateEvent(session.sessionId, ref, events, { includeContent, includeCommandText, includeUserText });
14661460
} else if (ref.path.endsWith(JSONL_EXT)) {
1467-
await this.readJsonlEvents(session.sessionId, ref, events, { includeContent, includeCommandText, includeUserText });
1461+
await this.readJsonlEvents(
1462+
session.sessionId,
1463+
scope,
1464+
ref,
1465+
events,
1466+
{ includeContent, includeCommandText, includeUserText },
1467+
{ workspaceLinked },
1468+
);
14681469
}
14691470
}
14701471

14711472
return events
1472-
.map((event) => event.cwd ? event : { ...event, cwd: scope.workspace })
1473+
.map((event) => event.cwd || event.planningScope === "user-global"
1474+
? event
1475+
: { ...event, cwd: scope.workspace })
14731476
.filter((event) => withinTimeRange(event.timestamp, scope))
14741477
.sort((a, b) => {
14751478
const left = timestampMillis(a.timestamp) ?? 0;
@@ -1481,13 +1484,25 @@ export class QoderSessionAnalyzer extends SessionAnalyzer {
14811484
});
14821485
}
14831486

1484-
async readJsonlEvents(sessionId, ref, events, options) {
1487+
async readJsonlEvents(sessionId, scope, ref, events, options, { workspaceLinked = false } = {}) {
14851488
await forEachJsonLine(ref.path, (raw, line) => {
14861489
const sourceRef = { ...ref, line, sessionId };
14871490
const rawSessionId = inferSessionId(raw, sourceRef);
14881491
if (rawSessionId && rawSessionId !== sessionId && ref.kind !== "logs-session") {
14891492
return;
14901493
}
1494+
if (ref.kind === "home-session") {
1495+
if (ref.planningScope === "user-global") {
1496+
if (!scope.includeGlobalCapabilities) {
1497+
return;
1498+
}
1499+
} else {
1500+
const recordCwd = inferCwd(raw);
1501+
if (recordCwd ? !isWorkspaceMatch(recordCwd, scope.workspace) : !workspaceLinked) {
1502+
return;
1503+
}
1504+
}
1505+
}
14911506
events.push(this.normalizeEvent(raw, sourceRef, options));
14921507
});
14931508
}

test/cloc.test.mjs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,27 @@ test("unsupported and lock files are skipped before reading content", async () =
174174
}
175175
});
176176

177+
test("cloc skips regular files beyond the configured read boundary", async () => {
178+
const repo = await mkdtemp(path.join(os.tmpdir(), "better-harness-cloc-size-boundary-"));
179+
180+
try {
181+
await writeFixtureFile(repo, "src/large.js", "const value = 'this file exceeds the test boundary';\n");
182+
183+
assert.deepEqual(countFile(repo, "src/large.js", { maxFileBytes: 16 }), {
184+
path: "src/large.js",
185+
skipped: true,
186+
reason: "too-large",
187+
});
188+
assert.equal(countFile(repo, "src/large.js", { maxFileBytes: 1_024 }).skipped, false);
189+
190+
const report = await analyzeCloc({ cwd: repo, useGit: false, workers: 1, maxFileBytes: 16 });
191+
assert.equal(report.fileList.counted, 0);
192+
assert.deepEqual(report.skippedFiles, [{ path: "src/large.js", reason: "too-large" }]);
193+
} finally {
194+
await removeFixtureTree(repo);
195+
}
196+
});
197+
177198
test("git-backed cloc skips tracked symlinks without leaking external targets", async (t) => {
178199
const repo = await makeRepo({
179200
"src/app.js": "const a = 1;\n",

test/harness-quickstart.test.mjs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,21 @@ test("Better Harness report rejects a missing cwd before collecting evidence", a
141141
}
142142
});
143143

144+
test("Better Harness report rejects an explicitly empty cwd", () => {
145+
const machine = runBetterHarness(["report", "--cwd=", "--json"]);
146+
assert.equal(machine.status, 1);
147+
assert.equal(machine.stderr, "");
148+
const payload = JSON.parse(machine.stdout);
149+
assert.equal(payload.ok, false);
150+
assert.equal(payload.error.code, "INVALID_CWD");
151+
assert.match(payload.error.message, /requires a non-empty directory value/u);
152+
153+
const human = runBetterHarness(["report", "--cwd="]);
154+
assert.equal(human.status, 1);
155+
assert.equal(human.stdout, "");
156+
assert.match(human.stderr, /requires a non-empty directory value/u);
157+
});
158+
144159
test("Better Harness report rejects a regular-file cwd before collecting evidence", async () => {
145160
const root = await mkdtemp(path.join(os.tmpdir(), "better-harness-quickstart-file-cwd-"));
146161
const filePath = path.join(root, "not-a-directory");

0 commit comments

Comments
 (0)