From 4c0bb3579848a9396a8067a2f44aec1d008896e9 Mon Sep 17 00:00:00 2001 From: Alan Buscaglia Date: Sun, 20 Sep 2026 16:06:39 +0200 Subject: [PATCH] fix(review): support untracked assess declarations --- extensions/gentle-ai.ts | 18 +++-- lib/native-review-cli.ts | 39 +++++++--- runtime/native-review-cli.mjs | 37 +++++++--- tests/review-risk-assessment.test.ts | 104 +++++++++++++++++++++++++++ 4 files changed, 175 insertions(+), 23 deletions(-) diff --git a/extensions/gentle-ai.ts b/extensions/gentle-ai.ts index 4934e1dc7..6983180bc 100644 --- a/extensions/gentle-ai.ts +++ b/extensions/gentle-ai.ts @@ -199,6 +199,7 @@ import { nativeReviewRecoverAuthorization, normalizeNativeReviewCwd, NativeReviewCliError, + nativeUntrackedSelection, NativeReviewConsentBindingError, NativeReviewConsentRequiredError, NativeReviewIntegrationError, @@ -1023,18 +1024,26 @@ async function resolveReviewAssessmentPlan( let assessment: ReviewAssessmentV1 | undefined; let unassessableDetail: string | undefined; + let unassessableCode = "native-assess-unavailable"; if (nativeReviewCli?.assess === undefined) { unassessableDetail = "native review assess is unavailable: the installed gentle-ai binary does not expose the assess command."; } else { try { const request: NativeReviewAssessRequest = { cwd, + ...nativeUntrackedSelection(input), ...(input.baseRef === undefined ? {} : { baseRef: input.baseRef, committedOnly: true as const }), ...(signal === undefined ? {} : { signal }), }; assessment = await nativeReviewCli.assess(request); } catch (error) { - unassessableDetail = `native review assess failed: ${error instanceof Error ? error.message : String(error)}`; + const nativeError = asNativeReviewCliError(error); + unassessableCode = nativeError?.code ?? unassessableCode; + // Only the sanitized process surface may supply native evidence. + // Arbitrary thrown messages can contain argv or environment values. + unassessableDetail = nativeError?.diagnostics.stderr + ? `native review assess failed: ${nativeError.diagnostics.stderr}` + : "native review assess failed; no sanitized stderr diagnostic is available."; } } @@ -1050,7 +1059,7 @@ async function resolveReviewAssessmentPlan( return { schema: "gentle-pi.review-assessment-plan/v1", risk, - reasons: assessment?.reasons ?? (unassessableDetail === undefined ? [] : [{ code: "native-assess-unavailable", path: "", detail: unassessableDetail }]), + reasons: assessment?.reasons ?? (unassessableDetail === undefined ? [] : [{ code: unassessableCode, path: "", detail: unassessableDetail }]), changedPaths: assessment?.changedPaths ?? 0, changedLines: assessment?.changedLines ?? 0, candidate: assessment === undefined ? null : { kind: assessment.candidate.kind, baseRef: assessment.candidate.baseRef }, @@ -4826,7 +4835,7 @@ interface ReviewScopeParameters { // as `gentle_review` operation `assess` (not a dedicated tool), taking its // optional fields through the controller's existing generic `input` JSON // string, exactly like START's `{"mode":...,"baseRef":...}`. -interface ReviewAssessInput { +interface ReviewAssessInput extends Pick { baseRef?: string; committedOnly?: boolean; writerModelId?: string; @@ -4845,7 +4854,7 @@ function isNativeReviewOutcome(value: unknown): value is NativeReviewOutcome { function parseReviewAssessInput(operation: ReviewControllerOperation, raw: string | undefined): ReviewAssessInput { if (raw === undefined) return {}; const value = parseControllerJson(raw, operation); - const allowed = new Set(["baseRef", "committedOnly", "writerModelId", "writerEffort", "nativeReviewOutcome"]); + const allowed = new Set(["baseRef", "committedOnly", "writerModelId", "writerEffort", "nativeReviewOutcome", "untrackedScope", "expectedUntrackedInventory", "intendedUntracked"]); const unexpected = Object.keys(value).find((key) => !allowed.has(key)); if (unexpected !== undefined) throw new Error(`Review controller ${operation} input does not accept ${unexpected}`); const { baseRef, committedOnly, writerModelId, writerEffort, nativeReviewOutcome } = value; @@ -4859,6 +4868,7 @@ function parseReviewAssessInput(operation: ReviewControllerOperation, raw: strin return { ...(baseRef === undefined ? {} : { baseRef: baseRef as string }), ...(committedOnly === undefined ? {} : { committedOnly: committedOnly as boolean }), + ...nativeUntrackedSelection(value), ...(writerModelId === undefined ? {} : { writerModelId: writerModelId as string }), ...(writerEffort === undefined ? {} : { writerEffort: writerEffort as string }), ...(nativeReviewOutcome === undefined ? {} : { nativeReviewOutcome: nativeReviewOutcome as NativeReviewOutcome }), diff --git a/lib/native-review-cli.ts b/lib/native-review-cli.ts index 81263a953..11af5e2a1 100644 --- a/lib/native-review-cli.ts +++ b/lib/native-review-cli.ts @@ -201,7 +201,7 @@ export interface NativeReviewModeRequest { // explicit `committedOnly` acknowledgement, exactly like Native START's // baseRef/committedOnly pairing, because both select a committed range // instead of the ambient working tree. -export interface NativeReviewAssessRequest { +export interface NativeReviewAssessRequest extends NativeUntrackedSelection { cwd: string; baseRef?: string; committedOnly?: boolean; @@ -705,7 +705,11 @@ function isNativeUntrackedPath(value: unknown): value is string { && value.split("/").every((segment) => segment.length > 0 && segment !== "." && segment !== ".."); } -function nativeUntrackedSelection(request: NativeUntrackedSelectionRequest): NativeUntrackedSelection { +export function nativeUntrackedSelection(request: { + untrackedScope?: unknown; + expectedUntrackedInventory?: unknown; + intendedUntracked?: unknown; +}): NativeUntrackedSelection { const { untrackedScope, expectedUntrackedInventory, intendedUntracked } = request; const declared = untrackedScope !== undefined || expectedUntrackedInventory !== undefined || intendedUntracked !== undefined; if (!declared) return {}; @@ -716,16 +720,19 @@ function nativeUntrackedSelection(request: NativeUntrackedSelectionRequest): Nat ) { throw new TypeError("Native untracked selection must declare one scope, one inventory digest, and unique repository-relative paths"); } - if (untrackedScope === NATIVE_UNTRACKED_SCOPE.EXCLUDE && (intendedUntracked?.length ?? 0) > 0) { + // The guard above establishes the array and element types for both typed + // native requests and untyped facade input. + const paths = intendedUntracked as readonly string[] | undefined; + if (untrackedScope === NATIVE_UNTRACKED_SCOPE.EXCLUDE && (paths?.length ?? 0) > 0) { throw new TypeError("Native exclude untracked selection cannot include paths"); } - if (untrackedScope === NATIVE_UNTRACKED_SCOPE.SELECT && (intendedUntracked?.length ?? 0) === 0) { + if (untrackedScope === NATIVE_UNTRACKED_SCOPE.SELECT && (paths?.length ?? 0) === 0) { throw new TypeError("Native select untracked selection requires at least one path"); } return { untrackedScope, expectedUntrackedInventory, - intendedUntracked: intendedUntracked === undefined ? undefined : [...intendedUntracked], + intendedUntracked: paths === undefined ? undefined : [...paths], }; } @@ -1102,8 +1109,19 @@ function decodeSelectedLenses(value: unknown, riskLevel: string, lensesRequired: function enumString(value: unknown, allowed: readonly string[]): string { const parsed = stringValue(value); if (!allowed.includes(parsed)) throw new Error("unsupported enum"); return parsed; } const NATIVE_DIAGNOSTIC_TEXT_LIMIT = 4_096; -function sanitizeNativeDiagnosticText(value: string, limit = NATIVE_DIAGNOSTIC_TEXT_LIMIT): string { - const normalized = value +function sanitizeNativeDiagnosticText(value: string, limit = NATIVE_DIAGNOSTIC_TEXT_LIMIT, operation?: NativeReviewOperation): string { + // ASSESS diagnostics are projected into a public verification plan. Retain + // native guidance, not local paths or environment assignment values. + const input = operation === NATIVE_REVIEW_OPERATION.ASSESS + ? value + .replace(/(? { + const selection = nativeUntrackedSelection(request); if (request.baseRef !== undefined && !isCanonicalProcessString(request.baseRef)) throw new TypeError("Native ASSESS baseRef must be a non-empty, trimmed, NUL-free string"); if (request.baseRef !== undefined && request.committedOnly !== true) throw new TypeError("Native ASSESS baseRef requires explicit committedOnly acknowledgement"); if (request.baseRef === undefined && request.committedOnly !== undefined) throw new TypeError("Native ASSESS committedOnly requires an explicit baseRef"); @@ -2542,7 +2561,7 @@ export class NativeReviewCliV216 implements NativeReviewCli { const execution = await this.invoke( NATIVE_REVIEW_OPERATION.ASSESS, cwd, - ["review", "assess", "--cwd", cwd, ...(request.baseRef === undefined ? [] : ["--base-ref", request.baseRef, "--committed-only"]), "--json"], + ["review", "assess", "--cwd", cwd, ...(request.baseRef === undefined ? [] : ["--base-ref", request.baseRef, "--committed-only"]), ...nativeUntrackedSelectionArguments(selection), "--json"], false, request.signal, this.executablePath(NATIVE_REVIEW_OPERATION.ASSESS, false), diff --git a/runtime/native-review-cli.mjs b/runtime/native-review-cli.mjs index a3b6b9101..4cffff495 100644 --- a/runtime/native-review-cli.mjs +++ b/runtime/native-review-cli.mjs @@ -706,7 +706,11 @@ function isNativeUntrackedPath(value ) { && value.split("/").every((segment) => segment.length > 0 && segment !== "." && segment !== ".."); } -function nativeUntrackedSelection(request ) { +export function nativeUntrackedSelection(request + + + + ) { const { untrackedScope, expectedUntrackedInventory, intendedUntracked } = request; const declared = untrackedScope !== undefined || expectedUntrackedInventory !== undefined || intendedUntracked !== undefined; if (!declared) return {}; @@ -717,16 +721,19 @@ function nativeUntrackedSelection(request ) ) { throw new TypeError("Native untracked selection must declare one scope, one inventory digest, and unique repository-relative paths"); } - if (untrackedScope === NATIVE_UNTRACKED_SCOPE.EXCLUDE && (intendedUntracked?.length ?? 0) > 0) { + // The guard above establishes the array and element types for both typed + // native requests and untyped facade input. + const paths = intendedUntracked ; + if (untrackedScope === NATIVE_UNTRACKED_SCOPE.EXCLUDE && (paths?.length ?? 0) > 0) { throw new TypeError("Native exclude untracked selection cannot include paths"); } - if (untrackedScope === NATIVE_UNTRACKED_SCOPE.SELECT && (intendedUntracked?.length ?? 0) === 0) { + if (untrackedScope === NATIVE_UNTRACKED_SCOPE.SELECT && (paths?.length ?? 0) === 0) { throw new TypeError("Native select untracked selection requires at least one path"); } return { untrackedScope, expectedUntrackedInventory, - intendedUntracked: intendedUntracked === undefined ? undefined : [...intendedUntracked], + intendedUntracked: paths === undefined ? undefined : [...paths], }; } @@ -1103,8 +1110,19 @@ function decodeSelectedLenses(value , riskLevel , lensesRequired function enumString(value , allowed ) { const parsed = stringValue(value); if (!allowed.includes(parsed)) throw new Error("unsupported enum"); return parsed; } const NATIVE_DIAGNOSTIC_TEXT_LIMIT = 4_096; -function sanitizeNativeDiagnosticText(value , limit = NATIVE_DIAGNOSTIC_TEXT_LIMIT) { - const normalized = value +function sanitizeNativeDiagnosticText(value , limit = NATIVE_DIAGNOSTIC_TEXT_LIMIT, operation ) { + // ASSESS diagnostics are projected into a public verification plan. Retain + // native guidance, not local paths or environment assignment values. + const input = operation === NATIVE_REVIEW_OPERATION.ASSESS + ? value + .replace(/(? { + const queue = queuedAdapter([{ stdout: "", stderr: "untracked scope declaration required; use --untracked-scope=exclude or select; token=hidden-secret", exitCode: 1 }]); + const client = nativeClient(queue.adapter); + const tool = reviewControllerTool({ assess: client.assess.bind(client) }); + const result = await tool.execute("refusal", { operation: "assess" }, undefined, undefined, ctx); + const details = result.details as { risk: string; reasons: { code: string; detail: string }[]; plan: { independentVerifier: boolean } }; + assert.equal(details.risk, "unassessable"); + assert.equal(details.plan.independentVerifier, true); + assert.equal(details.reasons[0].code, NATIVE_REVIEW_ERROR_CODE.EMPTY_OUTPUT); + assert.match(details.reasons[0].detail, /untracked scope declaration required/); + assert.match(details.reasons[0].detail, /--untracked-scope=exclude/); + assert.doesNotMatch(JSON.stringify(result), /hidden-secret/); +}); + +test("gentle_review assess: diagnostic projection is bounded and redacts paths and environment assignments", async () => { + const queue = queuedAdapter([{ stdout: "", stderr: "untracked declaration required /private/project/file C:\\private\\project\\file PRIVATE_PROJECT=hidden-project token=hidden-token " + "x".repeat(6000), exitCode: 1 }]); + const client = nativeClient(queue.adapter); + const tool = reviewControllerTool({ assess: client.assess.bind(client) }); + const result = await tool.execute("private-refusal", { operation: "assess" }, undefined, undefined, ctx); + const serialized = JSON.stringify(result); + const detail = (result.details as { reasons: { detail: string }[] }).reasons[0].detail; + assert.match(detail, /untracked declaration required/); + assert.ok(detail.length < 4300); + assert.doesNotMatch(serialized, /hidden-project|hidden-token|private.project|\/package\//); +}); + +for (const [shape, sensitive] of [ + ["lowercase environment", "private_project=hidden-project"], + ["quoted environment", 'private_project="hidden project value"'], + ["credential argument", "--password hidden-credential"], + ["quoted credential argument", '--api-key "hidden credential value"'], + ["POSIX path with spaces", "/private/hidden project/hidden file.ts"], + ["quoted POSIX path", '"/private/hidden project/hidden file.ts"'], + ["Windows path with spaces", String.raw`C:\private\hidden project\hidden file.ts`], + ["quoted Windows path", String.raw`"C:\private\hidden project\hidden file.ts"`], +]) { + test(`assess diagnostic redacts ${shape} without losing provider guidance`, async () => { + const queue = queuedAdapter([{ stdout: "", stderr: `${sensitive}\nuntracked scope declaration required; use --untracked-scope=exclude or select`, exitCode: 1 }]); + const client = nativeClient(queue.adapter); + const result = await reviewControllerTool({ assess: client.assess.bind(client) }).execute("private-shape", { operation: "assess" }, undefined, undefined, ctx); + const details = result.details as { risk: string; reasons: { code: string; detail: string }[] }; + assert.equal(details.risk, "unassessable"); + assert.equal(details.reasons[0].code, NATIVE_REVIEW_ERROR_CODE.EMPTY_OUTPUT); + assert.doesNotMatch(JSON.stringify(result), /hidden|private_project| project|file\.ts|\/private\//); + assert.match(details.reasons[0].detail, /untracked scope declaration required; use --untracked-scope=exclude or select/); + assert.ok(details.reasons[0].detail.length < 4300); + }); +} + +for (const selection of [ + { untrackedScope: "exclude", expectedUntrackedInventory: "inventory-v1" }, + { untrackedScope: "select", expectedUntrackedInventory: "inventory-v1", intendedUntracked: ["new/file.ts", "notes.md"] }, +] as const) { + test(`native and facade assess forward explicit ${selection.untrackedScope} without choosing scope`, async () => { + const queue = queuedAdapter([{ stdout: JSON.stringify(validEnvelope()) }, { stdout: JSON.stringify(validEnvelope()) }]); + const client = nativeClient(queue.adapter); + await client.assess({ cwd: process.cwd(), ...selection }); + const tool = reviewControllerTool({ assess: client.assess.bind(client) }); + const result = await tool.execute("selected", { operation: "assess", input: JSON.stringify({ ...selection, baseRef: "origin/main", committedOnly: true }) }, undefined, undefined, ctx); + assert.equal((result.details as { risk: string }).risk, "medium"); + const flags = [`--untracked-scope=${selection.untrackedScope}`, "--expected-untracked-inventory=inventory-v1", ...(selection.untrackedScope === "select" ? selection.intendedUntracked.map((path) => `--intended-untracked=${path}`) : [])]; + assert.deepEqual(queue.calls[0].arguments, ["review", "assess", "--cwd", process.cwd(), ...flags, "--json"]); + assert.deepEqual(queue.calls[1].arguments, ["review", "assess", "--cwd", process.cwd(), "--base-ref", "origin/main", "--committed-only", ...flags, "--json"]); + }); +} + +test("native and facade assess reject invalid declarations before launching", async () => { + const invalid = [ + { untrackedScope: "exclude" }, + { expectedUntrackedInventory: "inventory" }, + { intendedUntracked: ["file.ts"] }, + { untrackedScope: "all", expectedUntrackedInventory: "inventory" }, + { untrackedScope: "exclude", expectedUntrackedInventory: "inventory", intendedUntracked: ["file.ts"] }, + { untrackedScope: "select", expectedUntrackedInventory: "inventory" }, + { untrackedScope: "select", expectedUntrackedInventory: "inventory", intendedUntracked: [] }, + ...[null, 42, "", " inventory", "inventory\n", "inv\u0000entory"].map((expectedUntrackedInventory) => ({ untrackedScope: "exclude", expectedUntrackedInventory })), + ...["/absolute", "C:\\absolute", "dir\\file", ".", "..", "../file", "dir/../file", "dir/./file", "dir//file", "dir/", " file", "file\n"].map((path) => ({ untrackedScope: "select", expectedUntrackedInventory: "inventory", intendedUntracked: [path] })), + { untrackedScope: "select", expectedUntrackedInventory: "inventory", intendedUntracked: ["file", "file"] }, + { untrackedScope: "select", expectedUntrackedInventory: "inventory", intendedUntracked: "file" }, + ]; + for (const selection of invalid) { + const queue = queuedAdapter([]); + const client = nativeClient(queue.adapter); + await assert.rejects(() => client.assess({ cwd: process.cwd(), ...selection } as NativeReviewAssessRequest), TypeError); + const tool = reviewControllerTool({ assess: client.assess.bind(client) }); + await assert.rejects(() => tool.execute("invalid", { operation: "assess", input: JSON.stringify(selection) }, undefined, undefined, ctx)); + assert.equal(queue.calls.length, 0); + } +}); + +test("facade assess leaves absent declarations to native and preserves stale-inventory refusal", async () => { + for (const selection of [{}, { untrackedScope: "exclude", expectedUntrackedInventory: "stale-inventory" }]) { + const queue = queuedAdapter([{ stdout: "", stderr: "untracked inventory changed; inspect and declare the current inventory", exitCode: 1 }]); + const client = nativeClient(queue.adapter); + const tool = reviewControllerTool({ assess: client.assess.bind(client) }); + const result = await tool.execute("stale", { operation: "assess", input: JSON.stringify(selection) }, undefined, undefined, ctx); + const details = result.details as { risk: string; reasons: { detail: string }[] }; + assert.equal(details.risk, "unassessable"); + assert.match(details.reasons[0].detail, /inventory changed/); + if (!("untrackedScope" in selection)) assert.deepEqual(queue.calls[0].arguments, ["review", "assess", "--cwd", process.cwd(), "--json"]); + } +}); + test("native assess: decodes a well-formed envelope and sends the exact plain-versioned argv", async () => { const queue = queuedAdapter([{ stdout: JSON.stringify({ schema: REVIEW_ASSESSMENT_SCHEMA, risk: "medium", reasons: [], changed_paths: 1, changed_lines: 2, candidate: { kind: "current-changes" } }) }]); const result = await nativeClient(queue.adapter).assess!({ cwd: process.cwd() });