fix(review): harden blocking failure paths - #257
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds bounded native-process and Git execution, structured diagnostic propagation, optional consent lineage validation, and fail-closed handling for native status and package-binary failures across library, runtime, controller, and test paths. ChangesNative review reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ReviewController
participant CandidateView
participant NativeReviewCli
participant NativeStatus
ReviewController->>CandidateView: resolve candidate base
CandidateView-->>ReviewController: candidate data or sanitized diagnostics
ReviewController->>NativeReviewCli: verify version and review status
NativeReviewCli->>NativeStatus: run bounded native process
NativeStatus-->>NativeReviewCli: status, output-limit, or binary failure
NativeReviewCli-->>ReviewController: structured native diagnostics
ReviewController-->>ReviewController: return blocked response or invoke START
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/native-review-cli.ts (3)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the configuration-hint text from the buffer cap constant instead of hardcoding the number. Both files hardcode the literal
67108864insideNATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT, duplicatingNATIVE_REVIEW_MAX_BUFFER_BYTES(64 * 1024 * 1024). A future change to the cap could silently drift from the message text shown to operators.
lib/native-review-cli.ts#L28-44: change the hint to a template literal referencingNATIVE_REVIEW_MAX_BUFFER_BYTESinstead of the literal67108864.runtime/native-review-cli.mjs#L29-45: apply the same template-literal change here to keep the runtime mirror in sync with the lib source.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/native-review-cli.ts` at line 1, Update NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT in both native-review-cli implementations to use a template literal that derives the displayed byte value from NATIVE_REVIEW_MAX_BUFFER_BYTES instead of hardcoding 67108864. Keep the lib source and runtime mirror synchronized.
1-1: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
ERR_CHILD_PROCESS_STDIO_MAXBUFFERforexecFileoverflow. The asyncexecFileadapter uses the promisified API, so Node reportsmaxBufferoverflow witherror.code = "ERR_CHILD_PROCESS_STDIO_MAXBUFFER"rather thanENOBUFFS; update the lib and runtime mirrors or remove the check.
745-758: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the ENOBUFS branch from the async execFile adapter.
execFileAsyncreports buffer overflow asERR_CHILD_PROCESS_STDIO_MAXBUFFERfor the async/callback APIs.ENOBUFSis used for synchronous child-process buffer failures. Thedetail.code === "ENOBUFS"check can misclassify a genericENOBUFSsystem failure asOUTPUT_LIMIT; replace it with the synchronous-specificERR_CHILD_PROCESS_STDIO_MAXBUFFERcheck if this branch also needs to cover execFileSync-style callers. This change affectslib/native-review-cli.tsand the replicated runtime copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/native-review-cli.ts` around lines 745 - 758, Update createNodeExecFileAdapter’s outputLimitExceeded detection to remove the ENOBUFS check and rely on ERR_CHILD_PROCESS_STDIO_MAXBUFFER for async execFile buffer overflow; apply the same change in the replicated runtime copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/gentle-ai.ts`:
- Around line 5202-5204: Extract the duplicated CandidateViewError handling from
both catch blocks into a shared helper such as
candidateViewStartFailure(operation, error). Have it return the
diagnostics-based nativeOperationFailure or the three recognized
nativeStartRejection results, and return undefined for other errors; update both
sites to use the helper while preserving the existing fail-closed fallback to
base-ref-unresolvable.
In `@lib/native-review-cli.ts`:
- Around line 28-44: Update NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT to
derive the maximum value from NATIVE_REVIEW_MAX_BUFFER_BYTES via a template
literal instead of hardcoding 67108864, and apply the same change to the
corresponding configuration hint in runtime/native-review-cli.mjs.
In `@lib/review-candidate-view.ts`:
- Around line 246-264: Add coverage in the candidateGit tests for the generic
failure branch by making the injected executor throw an ordinary non-zero-exit
error without ENOBUFS, ERR_CHILD_PROCESS_STDIO_MAXBUFFER, ETIMEDOUT, or killed.
Assert the resulting CandidateViewError has reason candidate-view-git-failure
and diagnostics.category set to "git-failure", while preserving the existing
timeout and output-limit tests.
In `@runtime/native-review-cli.mjs`:
- Around line 29-45: Update NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT to
derive its maximum-value text from NATIVE_REVIEW_MAX_BUFFER_BYTES instead of
duplicating the hardcoded 67108864 literal, keeping the hint synchronized with
the enforced limit.
In `@tests/review-candidate-view.test.ts`:
- Around line 31-171: Add a test in the candidate-view Git command test suite
using an executor that throws a plain non-timeout, non-output-limit error, then
assert creation raises CandidateViewError with reason candidate-view-git-failure
and diagnostics.category set to git-failure. Anchor the test around
CandidateViewRegistry.create and verify the generic failure classification
without asserting timeout or output-limit fields.
---
Outside diff comments:
In `@lib/native-review-cli.ts`:
- Line 1: Update NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT in both
native-review-cli implementations to use a template literal that derives the
displayed byte value from NATIVE_REVIEW_MAX_BUFFER_BYTES instead of hardcoding
67108864. Keep the lib source and runtime mirror synchronized.
- Around line 745-758: Update createNodeExecFileAdapter’s outputLimitExceeded
detection to remove the ENOBUFS check and rely on
ERR_CHILD_PROCESS_STDIO_MAXBUFFER for async execFile buffer overflow; apply the
same change in the replicated runtime copy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7559ac58-ba7a-48a3-b996-a0e8a40c03c4
📒 Files selected for processing (8)
extensions/gentle-ai.tslib/native-review-cli.tslib/review-candidate-view.tsruntime/native-review-cli.mjstests/native-review-cli.test.tstests/native-review-consent.test.tstests/review-candidate-view.test.tstests/review-controller-native-routing.test.ts
| if (error instanceof CandidateViewError && error.diagnostics !== undefined) return nativeOperationFailure(parameters.operation, Object.assign(error, { candidateViewPreNative: true })); | ||
| if (error instanceof CandidateViewError && (error.reason === "base-ref-ambiguous" || error.reason === "base-ref-unresolvable" || error.reason === "base-ref-moved")) return nativeStartRejection(error.reason); | ||
| return nativeStartRejection("base-ref-unresolvable"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the duplicated diagnostics-bearing CandidateViewError check.
Line 5202 and line 5265 contain the identical two-line check: tag a diagnostics-bearing CandidateViewError with candidateViewPreNative: true and route it through nativeOperationFailure, then fall back to the three known base-ref rejection reasons. This logic sits on a fail-closed security path (preventing native START after a candidate-view failure).
Extract a small helper, for example candidateViewStartFailure(operation, error), and call it from both catch blocks. This reduces the risk that a future change updates one site without the other.
♻️ Proposed refactor
function candidateViewStartFailure(operation: ReviewControllerOperation, error: unknown): Record<string, unknown> | undefined {
if (!(error instanceof CandidateViewError)) return undefined;
if (error.diagnostics !== undefined) return nativeOperationFailure(operation, Object.assign(error, { candidateViewPreNative: true }));
if (error.reason === "base-ref-ambiguous" || error.reason === "base-ref-unresolvable" || error.reason === "base-ref-moved") return nativeStartRejection(error.reason);
return undefined;
}Then at each call site:
- if (error instanceof CandidateViewError && error.diagnostics !== undefined) return nativeOperationFailure(parameters.operation, Object.assign(error, { candidateViewPreNative: true }));
- if (error instanceof CandidateViewError && (error.reason === "base-ref-ambiguous" || error.reason === "base-ref-unresolvable" || error.reason === "base-ref-moved")) return nativeStartRejection(error.reason);
+ const candidateViewFailure = candidateViewStartFailure(parameters.operation, error);
+ if (candidateViewFailure !== undefined) return candidateViewFailure;🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile, execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@extensions/gentle-ai.ts` around lines 5202 - 5204, Extract the duplicated
CandidateViewError handling from both catch blocks into a shared helper such as
candidateViewStartFailure(operation, error). Have it return the
diagnostics-based nativeOperationFailure or the three recognized
nativeStartRejection results, and return undefined for other errors; update both
sites to use the helper while preserving the existing fail-closed fallback to
base-ref-unresolvable.
| // Negotiated review/status responses can carry a complete authority inventory. | ||
| // Keep the production default large enough for that payload while retaining a | ||
| // hard 64 MiB ceiling even when GENTLE_PI_REVIEW_MAX_BUFFER_BYTES is set. | ||
| export const NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES = 16 * 1024 * 1024; | ||
| const NATIVE_REVIEW_MAX_BUFFER_BYTES = 64 * 1024 * 1024; | ||
| const NATIVE_REVIEW_MAX_BUFFER_BYTES_ENV = "GENTLE_PI_REVIEW_MAX_BUFFER_BYTES"; | ||
| const NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT = "Inspect native review state before any new START; GENTLE_PI_REVIEW_MAX_BUFFER_BYTES accepts a positive decimal up to 67108864."; | ||
|
|
||
| function resolveNativeReviewMaxBufferBytes(environment: NodeJS.ProcessEnv = process.env): number { | ||
| const value = environment[NATIVE_REVIEW_MAX_BUFFER_BYTES_ENV]; | ||
| if (value === undefined || !/^[1-9]\d*$/.test(value)) return NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES; | ||
| const parsed = Number(value); | ||
| return Number.isSafeInteger(parsed) && parsed <= NATIVE_REVIEW_MAX_BUFFER_BYTES | ||
| ? parsed | ||
| : NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES; | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Derive the configuration hint from the cap constant instead of hardcoding the number.
NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT hardcodes the literal 67108864 (line 34), duplicating NATIVE_REVIEW_MAX_BUFFER_BYTES (line 32, 64 * 1024 * 1024). If the cap ever changes, the hint text can silently drift from the actual enforced limit, since nothing ties the two together.
Use a template literal referencing the constant instead:
♻️ Proposed fix
-const NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT = "Inspect native review state before any new START; GENTLE_PI_REVIEW_MAX_BUFFER_BYTES accepts a positive decimal up to 67108864.";
+const NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT = `Inspect native review state before any new START; GENTLE_PI_REVIEW_MAX_BUFFER_BYTES accepts a positive decimal up to ${NATIVE_REVIEW_MAX_BUFFER_BYTES}.`;The rest of resolveNativeReviewMaxBufferBytes is otherwise correct: the regex rejects non-positive, non-integer, and leading-zero inputs, and the safe-integer plus upper-bound check falls back to the 16 MiB default for anything above the 64 MiB cap. Same duplication exists in runtime/native-review-cli.mjs line 35.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Negotiated review/status responses can carry a complete authority inventory. | |
| // Keep the production default large enough for that payload while retaining a | |
| // hard 64 MiB ceiling even when GENTLE_PI_REVIEW_MAX_BUFFER_BYTES is set. | |
| export const NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES = 16 * 1024 * 1024; | |
| const NATIVE_REVIEW_MAX_BUFFER_BYTES = 64 * 1024 * 1024; | |
| const NATIVE_REVIEW_MAX_BUFFER_BYTES_ENV = "GENTLE_PI_REVIEW_MAX_BUFFER_BYTES"; | |
| const NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT = "Inspect native review state before any new START; GENTLE_PI_REVIEW_MAX_BUFFER_BYTES accepts a positive decimal up to 67108864."; | |
| function resolveNativeReviewMaxBufferBytes(environment: NodeJS.ProcessEnv = process.env): number { | |
| const value = environment[NATIVE_REVIEW_MAX_BUFFER_BYTES_ENV]; | |
| if (value === undefined || !/^[1-9]\d*$/.test(value)) return NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES; | |
| const parsed = Number(value); | |
| return Number.isSafeInteger(parsed) && parsed <= NATIVE_REVIEW_MAX_BUFFER_BYTES | |
| ? parsed | |
| : NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES; | |
| } | |
| // Negotiated review/status responses can carry a complete authority inventory. | |
| // Keep the production default large enough for that payload while retaining a | |
| // hard 64 MiB ceiling even when GENTLE_PI_REVIEW_MAX_BUFFER_BYTES is set. | |
| export const NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES = 16 * 1024 * 1024; | |
| const NATIVE_REVIEW_MAX_BUFFER_BYTES = 64 * 1024 * 1024; | |
| const NATIVE_REVIEW_MAX_BUFFER_BYTES_ENV = "GENTLE_PI_REVIEW_MAX_BUFFER_BYTES"; | |
| const NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT = `Inspect native review state before any new START; GENTLE_PI_REVIEW_MAX_BUFFER_BYTES accepts a positive decimal up to ${NATIVE_REVIEW_MAX_BUFFER_BYTES}.`; | |
| function resolveNativeReviewMaxBufferBytes(environment: NodeJS.ProcessEnv = process.env): number { | |
| const value = environment[NATIVE_REVIEW_MAX_BUFFER_BYTES_ENV]; | |
| if (value === undefined || !/^[1-9]\d*$/.test(value)) return NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES; | |
| const parsed = Number(value); | |
| return Number.isSafeInteger(parsed) && parsed <= NATIVE_REVIEW_MAX_BUFFER_BYTES | |
| ? parsed | |
| : NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES; | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/native-review-cli.ts` around lines 28 - 44, Update
NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT to derive the maximum value from
NATIVE_REVIEW_MAX_BUFFER_BYTES via a template literal instead of hardcoding
67108864, and apply the same change to the corresponding configuration hint in
runtime/native-review-cli.mjs.
| function candidateGit(cwd: string, arguments_: readonly string[], env: NodeJS.ProcessEnv, encoding: "utf8" | "buffer", executor: CandidateGitExecutor): string | Buffer { | ||
| const timeoutMs = resolveCandidateGitTimeoutMs(env); | ||
| try { | ||
| return executor("git", arguments_, { cwd, encoding, env, stdio: ["ignore", "pipe", "pipe"], timeout: CANDIDATE_GIT_TIMEOUT_MS, windowsHide: true }); | ||
| return executor("git", arguments_, { | ||
| cwd, | ||
| encoding, | ||
| env, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| timeout: timeoutMs, | ||
| maxBuffer: CANDIDATE_GIT_MAX_BUFFER_BYTES, | ||
| windowsHide: true, | ||
| }); | ||
| } catch (error) { | ||
| const detail = error as NodeJS.ErrnoException & { stderr?: Buffer; killed?: boolean }; | ||
| if (detail.code === "ETIMEDOUT" || detail.killed === true) throw new CandidateViewError(`candidate view Git operation timed out after ${CANDIDATE_GIT_TIMEOUT_MS}ms`); | ||
| throw new CandidateViewError(`candidate view Git operation failed: ${detail.stderr?.toString("utf8").trim() || detail.message || "unknown Git error"}`); | ||
| const detail = error as NodeJS.ErrnoException & { killed?: boolean }; | ||
| if (detail.code === "ENOBUFS" || detail.code === "ERR_CHILD_PROCESS_STDIO_MAXBUFFER") throw candidateGitFailure(CANDIDATE_VIEW_GIT_FAILURE_CATEGORY.OUTPUT_LIMIT, arguments_, timeoutMs); | ||
| if (detail.code === "ETIMEDOUT" || detail.killed === true) throw candidateGitFailure(CANDIDATE_VIEW_GIT_FAILURE_CATEGORY.TIMEOUT, arguments_, timeoutMs); | ||
| throw candidateGitFailure(CANDIDATE_VIEW_GIT_FAILURE_CATEGORY.GIT_FAILURE, arguments_, timeoutMs); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Verify the untested git-failure category has coverage.
The candidateGit catch block classifies three failure categories: OUTPUT_LIMIT, TIMEOUT, and GIT_FAILURE. The provided tests in tests/review-candidate-view.test.ts cover the timeout and output-limit paths directly, but no test exercises the generic GIT_FAILURE branch (a Git command that fails with neither ENOBUFS/ERR_CHILD_PROCESS_STDIO_MAXBUFFER nor ETIMEDOUT/killed).
Add a test that makes the injected executor throw a plain non-zero-exit error, then assert the resulting CandidateViewError.reason is candidate-view-git-failure and its diagnostics.category is "git-failure".
As per path instructions, lib/**/*.ts: "Behavior changes here must ship with their tests in the same PR. Flag changed logic without updated tests."
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync, type ExecFileSyncOptions } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/review-candidate-view.ts` around lines 246 - 264, Add coverage in the
candidateGit tests for the generic failure branch by making the injected
executor throw an ordinary non-zero-exit error without ENOBUFS,
ERR_CHILD_PROCESS_STDIO_MAXBUFFER, ETIMEDOUT, or killed. Assert the resulting
CandidateViewError has reason candidate-view-git-failure and
diagnostics.category set to "git-failure", while preserving the existing timeout
and output-limit tests.
Source: Path instructions
| // Negotiated review/status responses can carry a complete authority inventory. | ||
| // Keep the production default large enough for that payload while retaining a | ||
| // hard 64 MiB ceiling even when GENTLE_PI_REVIEW_MAX_BUFFER_BYTES is set. | ||
| export const NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES = 16 * 1024 * 1024; | ||
| const NATIVE_REVIEW_MAX_BUFFER_BYTES = 64 * 1024 * 1024; | ||
| const NATIVE_REVIEW_MAX_BUFFER_BYTES_ENV = "GENTLE_PI_REVIEW_MAX_BUFFER_BYTES"; | ||
| const NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT = "Inspect native review state before any new START; GENTLE_PI_REVIEW_MAX_BUFFER_BYTES accepts a positive decimal up to 67108864."; | ||
|
|
||
| function resolveNativeReviewMaxBufferBytes(environment = process.env) { | ||
| const value = environment[NATIVE_REVIEW_MAX_BUFFER_BYTES_ENV]; | ||
| if (value === undefined || !/^[1-9]\d*$/.test(value)) return NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES; | ||
| const parsed = Number(value); | ||
| return Number.isSafeInteger(parsed) && parsed <= NATIVE_REVIEW_MAX_BUFFER_BYTES | ||
| ? parsed | ||
| : NATIVE_REVIEW_DEFAULT_MAX_BUFFER_BYTES; | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Same configuration-hint duplication as lib/native-review-cli.ts.
This mirrors the hardcoded 67108864 literal in NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT. See the consolidated comment for the shared fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@runtime/native-review-cli.mjs` around lines 29 - 45, Update
NATIVE_REVIEW_MAX_BUFFER_CONFIGURATION_HINT to derive its maximum-value text
from NATIVE_REVIEW_MAX_BUFFER_BYTES instead of duplicating the hardcoded
67108864 literal, keeping the hint synchronized with the enforced limit.
| test("candidate view Git commands classify bounded timeouts and block materialization before worktree execution", (t) => { | ||
| const calls: Array<{ arguments: readonly string[]; timeout: number | undefined; maxBuffer: number | undefined }> = []; | ||
| const executor: CandidateGitExecutor = (_file, arguments_, options) => { calls.push({ arguments: arguments_, timeout: options.timeout, maxBuffer: options.maxBuffer }); throw Object.assign(new Error("timed out"), { code: "ETIMEDOUT", killed: true }); }; | ||
| let failure: unknown; | ||
| try { | ||
| new CandidateViewRegistry(executor).create({ contributorRoot: repository(t) }); | ||
| } catch (error) { | ||
| failure = error; | ||
| } | ||
| assert.ok(failure instanceof CandidateViewError); | ||
| assert.equal(failure.reason, "candidate-view-timeout"); | ||
| assert.deepEqual((failure as CandidateViewError & { diagnostics?: unknown }).diagnostics, { | ||
| phase: "candidate-view", | ||
| category: "timeout", | ||
| git_subcommand: "rev-parse", | ||
| timeout_ms: 10_000, | ||
| max_buffer_bytes: 64 * 1024 * 1024, | ||
| message: "candidate-view Git command rev-parse timed out after 10000ms; inspect the candidate state before any new START", | ||
| }); | ||
| assert.deepEqual(calls, [{ arguments: ["rev-parse", "--git-common-dir"], timeout: 10_000, maxBuffer: 64 * 1024 * 1024 }]); | ||
| assert.equal(calls.some((call) => call.arguments[0] === "worktree"), false); | ||
| }); | ||
|
|
||
| test("candidate-view Git timeouts use a bounded strict-decimal override with safe fallback", (t) => { | ||
| const withTimeout = <T>(value: string | undefined, callback: () => T): T => { | ||
| const previous = process.env.GENTLE_PI_CANDIDATE_GIT_TIMEOUT_MS; | ||
| try { | ||
| if (value === undefined) delete process.env.GENTLE_PI_CANDIDATE_GIT_TIMEOUT_MS; | ||
| else process.env.GENTLE_PI_CANDIDATE_GIT_TIMEOUT_MS = value; | ||
| return callback(); | ||
| } finally { | ||
| if (previous === undefined) delete process.env.GENTLE_PI_CANDIDATE_GIT_TIMEOUT_MS; | ||
| else process.env.GENTLE_PI_CANDIDATE_GIT_TIMEOUT_MS = previous; | ||
| } | ||
| }; | ||
| for (const [name, value, expected] of [ | ||
| ["default", undefined, 10_000], | ||
| ["valid override", "45000", 45_000], | ||
| ...(["", "0", "0010", "-1", "1.5", "not-a-number", "120001", "Infinity"] as const).map((value) => [`invalid override ${JSON.stringify(value)}`, value, 10_000] as const), | ||
| ] as const) { | ||
| const timeouts: Array<number | undefined> = []; | ||
| let failure: unknown; | ||
| withTimeout(value, () => { | ||
| try { | ||
| new CandidateViewRegistry((_file, _arguments, options) => { | ||
| timeouts.push(options.timeout); | ||
| throw Object.assign(new Error("timed out"), { code: "ETIMEDOUT" }); | ||
| }).create({ contributorRoot: repository(t) }); | ||
| } catch (error) { | ||
| failure = error; | ||
| } | ||
| }); | ||
| assert.ok(failure instanceof CandidateViewError, name); | ||
| assert.equal(timeouts[0], expected, name); | ||
| assert.equal((failure as CandidateViewError & { diagnostics?: { timeout_ms?: unknown } }).diagnostics?.timeout_ms, expected, name); | ||
| } | ||
| }); | ||
|
|
||
| test("explicit base resolution preserves structured for-each-ref output-limit diagnostics", (t) => { | ||
| const contributorRoot = repository(t); | ||
| const executor: CandidateGitExecutor = (file, arguments_, options) => { | ||
| if (arguments_[0] === "for-each-ref") throw Object.assign(new Error("sensitive base-reference output"), { code: "ENOBUFS", killed: true, stderr: Buffer.from("sensitive base-reference output") }); | ||
| return execFileSync(file, arguments_, options); | ||
| }; | ||
| let failure: unknown; | ||
| try { | ||
| new CandidateViewRegistry(executor).create({ contributorRoot, baseRef: "refs/heads/main", committedOnly: true }); | ||
| } catch (error) { | ||
| failure = error; | ||
| } | ||
| assert.ok(failure instanceof CandidateViewError); | ||
| assert.equal(failure.reason, "candidate-view-output-limit"); | ||
| assert.deepEqual(failure.diagnostics, { | ||
| phase: "candidate-view", | ||
| category: "output-limit", | ||
| git_subcommand: "for-each-ref", | ||
| timeout_ms: 10_000, | ||
| max_buffer_bytes: 64 * 1024 * 1024, | ||
| message: "candidate-view Git command for-each-ref exceeded the 67108864-byte output limit; inspect the candidate state before any new START", | ||
| }); | ||
| assert.doesNotMatch(failure.message, /sensitive base-reference output/); | ||
| }); | ||
|
|
||
| test("every synchronous candidate-view Git command receives the explicit 64 MiB output limit", (t) => { | ||
| const calls: Array<{ arguments: readonly string[]; maxBuffer: number | undefined }> = []; | ||
| const executor: CandidateGitExecutor = (file, arguments_, options) => { | ||
| calls.push({ arguments: arguments_, maxBuffer: options.maxBuffer }); | ||
| return execFileSync(file, arguments_, options); | ||
| }; | ||
| const view = new CandidateViewRegistry(executor).create({ contributorRoot: repository(t) }); | ||
| try { | ||
| assert.ok(calls.length > 1); | ||
| assert.ok(calls.every((call) => call.maxBuffer === 64 * 1024 * 1024), JSON.stringify(calls)); | ||
| } finally { | ||
| view.cleanup(); | ||
| } | ||
| }); | ||
|
|
||
| test("candidate view classifies both Node synchronous-process output-limit errors ahead of killed state without exposing process output", (t) => { | ||
| const attemptedOutputBytes = 64 * 1024 * 1024 + 1; | ||
| for (const code of ["ENOBUFS", "ERR_CHILD_PROCESS_STDIO_MAXBUFFER"]) { | ||
| let failure: unknown; | ||
| try { | ||
| new CandidateViewRegistry((_file, _arguments, options) => { | ||
| assert.ok((options.maxBuffer ?? 0) < attemptedOutputBytes, code); | ||
| throw Object.assign(new Error("sensitive stderr and candidate bytes"), { code, killed: true, stderr: Buffer.from("sensitive stderr and candidate bytes") }); | ||
| }).create({ contributorRoot: repository(t) }); | ||
| } catch (error) { | ||
| failure = error; | ||
| } | ||
| assert.ok(failure instanceof CandidateViewError, code); | ||
| assert.equal(failure.reason, "candidate-view-output-limit", code); | ||
| assert.deepEqual((failure as CandidateViewError & { diagnostics?: unknown }).diagnostics, { | ||
| phase: "candidate-view", | ||
| category: "output-limit", | ||
| git_subcommand: "rev-parse", | ||
| timeout_ms: 10_000, | ||
| max_buffer_bytes: 64 * 1024 * 1024, | ||
| message: "candidate-view Git command rev-parse exceeded the 67108864-byte output limit; inspect the candidate state before any new START", | ||
| }, code); | ||
| assert.doesNotMatch(failure.message, /sensitive stderr|candidate bytes/, code); | ||
| } | ||
| }); | ||
|
|
||
| test("candidate Git accepts deterministic output above 1 MiB up to its explicit bound", () => { | ||
| const row = `:100644 100644 ${"a".repeat(40)} ${"b".repeat(40)} M\0tracked.txt\0`; | ||
| const copies = Math.ceil((1024 * 1024 + 1) / Buffer.byteLength(row)); | ||
| const output = Buffer.from(row.repeat(copies)); | ||
| assert.ok(output.length > 1024 * 1024); | ||
| assert.ok(output.length < 64 * 1024 * 1024); | ||
| const calls: Array<{ maxBuffer: number | undefined }> = []; | ||
| const manifest = deriveChangedPathManifest("/candidate", "a".repeat(40), "b".repeat(40), (_file, arguments_, options) => { | ||
| assert.deepEqual(arguments_.slice(0, 2), ["diff", "--raw"]); | ||
| calls.push({ maxBuffer: options.maxBuffer }); | ||
| return output; | ||
| }); | ||
| assert.equal(manifest.length, copies); | ||
| assert.ok(manifest.every((entry) => entry.path === "tracked.txt" && entry.status === "M")); | ||
| assert.deepEqual(calls, [{ maxBuffer: 64 * 1024 * 1024 }]); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add coverage for the generic git-failure diagnostic category.
This test suite thoroughly covers candidate-view-timeout and candidate-view-output-limit classification, including sanitization and the exact configured timeout/buffer values. It does not add a test for the third category, candidate-view-git-failure, produced when a Git command fails for a reason other than a timeout or an output-limit overflow (see lib/review-candidate-view.ts line 262).
Add a test with an executor that throws a plain error (for example, a non-zero exit without ETIMEDOUT/ENOBUFS), and assert the resulting CandidateViewError.reason is candidate-view-git-failure with diagnostics.category === "git-failure".
As per path instructions, lib/**/*.ts: "Behavior changes here must ship with their tests in the same PR. Flag changed logic without updated tests." This test file is the natural location to close that gap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/review-candidate-view.test.ts` around lines 31 - 171, Add a test in the
candidate-view Git command test suite using an executor that throws a plain
non-timeout, non-output-limit error, then assert creation raises
CandidateViewError with reason candidate-view-git-failure and
diagnostics.category set to git-failure. Anchor the test around
CandidateViewRegistry.create and verify the generic failure classification
without asserting timeout or output-limit fields.
Source: Path instructions
1 participant
Closes #254
Summary
This PR hardens the review paths that were blocking real delivery workflows. It keeps all failure modes fail-closed while preserving safe diagnostics and provider identity bindings.
GENTLE_PI_REVIEW_MAX_BUFFER_BYTESoverride.--lineagewhile preserving strict target and prebound-lineage checks.versionandreview/statusdiagnostics and adds actionable package-local binary recovery guidance.Related issues
mainby merged PR #1965 (39d62b61), which fixed the fresh large-workspace status timeout.Changes
Test plan
node --experimental-strip-types --test tests/review-candidate-view.test.ts tests/native-review-cli.test.ts tests/native-review-consent.test.ts tests/review-controller-native-routing.test.ts(287 passed)git diff --checkpnpm test: three parity-runtime tests require the external global RDD mode to be enabled; it was not changed automatically.Review workload
This PR is intentionally submitted as a
size:exception: it contains 936 additions and 58 deletions across the implementation and its regression coverage. The changes were implemented and verified as sequential work units in one candidate, but remain one PR at the user's request.Contributor checklist
type:*labelCo-Authored-BytrailersmainperformedSummary by CodeRabbit
New Features
Bug Fixes
Tests