From c49f78d269cc57f4d7f28b86ce70c14d1fcd4c4d Mon Sep 17 00:00:00 2001 From: L4XB Date: Sat, 12 Sep 2026 16:38:59 +0200 Subject: [PATCH] fix(review): name the check that refused an intended-untracked selection select-intended-untracked decided eight conditions in one boolean and answered all of them with the same opaque `intended-untracked-selection-binding-rejected`. A caller holding provider-issued bytes could not tell a status that moved under it from a path outside the eligible list, and the only route left was to send the same call again -- which is what #941 reports doing five times. The conditions keep their original order and their original outcome, and each now names itself: `selection-input-absent`, `binding-mismatch`, `status-field-mismatch` with the field that moved, `eligible-paths-unreadable`, `untracked-selection-invalid`, and `path-not-eligible` with the refused path. START rejections have carried `reason`/`field` since they were introduced; this is the same shape. Tests assert the reason for each existing rejection case, add a status that moves between issuing and revalidating the binding (target_identity and candidate_tree), and pin the reporter's own shape -- a candidate with no untracked files and an empty selection -- as accepted, excluding every eligible path. Refs #941 --- extensions/gentle-ai.ts | 47 ++++++++++- .../review-controller-native-routing.test.ts | 79 ++++++++++++++++++- 2 files changed, 120 insertions(+), 6 deletions(-) diff --git a/extensions/gentle-ai.ts b/extensions/gentle-ai.ts index 18835de60..a8830f09b 100644 --- a/extensions/gentle-ai.ts +++ b/extensions/gentle-ai.ts @@ -5015,6 +5015,49 @@ function validateNativeStartUntrackedSelection(value: Record): }; } +// gentle-pi#941: the pre-lineage selection had eight ways to be refused and one +// opaque outcome for all of them, so a caller holding provider-issued bytes +// could not tell a moved status from an ineligible path -- and the only route +// left was to send the same call again. START rejections have carried +// `reason`/`field` since they were introduced (nativeStartRejection above); +// name the failing check here the same way. The checks keep their original +// order, so which one is reported is the first that fails, exactly as the +// short-circuiting condition decided before. +const INTENDED_UNTRACKED_BINDING_FIELDS = ["target_identity", "projection", "base_tree", "candidate_tree"] as const; + +function intendedUntrackedSelectionRejection( + input: ReviewCollectInputV3 | undefined, + canonicalBinding: string, + status: ReviewStatusV3, + eligible: unknown, + selected: NativeStartUntrackedSelection, +): { readonly reason: string; readonly field?: string } | undefined { + if (input === undefined) return { reason: "selection-input-absent" }; + if (canonicalReviewCaptureBinding(input) !== canonicalBinding) return { reason: "binding-mismatch" }; + const bound: Record<(typeof INTENDED_UNTRACKED_BINDING_FIELDS)[number], string> = { + target_identity: status.targetIdentity, + projection: status.projection.projection, + base_tree: status.projection.baseTree, + candidate_tree: status.projection.currentCandidateTree, + }; + // A field that no longer matches the live status means the binding was + // issued against a different target, projection or tree -- the selection is + // stale rather than wrong, and a fresh inspect is the forward route. + for (const field of INTENDED_UNTRACKED_BINDING_FIELDS) { + if (exactCollectArgument(input, field) !== bound[field]) return { reason: "status-field-mismatch", field }; + } + // The eligible list is the provider's own `eligible_paths_json`; unreadable + // here means it was absent, duplicated, or not valid JSON in the status. + if (!Array.isArray(eligible)) return { reason: "eligible-paths-unreadable" }; + if (selected.reason !== undefined) return { reason: selected.reason }; + // Echoes back a path the caller itself submitted -- no information the + // caller did not already have, and with a large inventory it is the only + // way to tell which entry of a long selection was refused. + const ineligible = selected.intendedUntracked!.find((path) => !eligible.includes(path)); + if (ineligible !== undefined) return { reason: "path-not-eligible", field: ineligible }; + return undefined; +} + function nativeStartRejection(reason: string, field?: string): Record { return { operation: REVIEW_CONTROLLER_OPERATION.START, @@ -7319,8 +7362,8 @@ async function executeReviewControllerOperation( try { eligible = JSON.parse(eligibleJson ?? ""); } catch { eligible = undefined; } const scope = parameters.intendedUntracked!.length === 0 ? NATIVE_START_UNTRACKED_SCOPE.EXCLUDE : NATIVE_START_UNTRACKED_SCOPE.SELECT; const selected = validateNativeStartUntrackedSelection({ untrackedScope: scope, expectedUntrackedInventory: inventory, intendedUntracked: parameters.intendedUntracked }); - const rejected = input === undefined || canonicalReviewCaptureBinding(input) !== canonicalBinding || exactCollectArgument(input, "target_identity") !== status.targetIdentity || exactCollectArgument(input, "projection") !== status.projection.projection || exactCollectArgument(input, "base_tree") !== status.projection.baseTree || exactCollectArgument(input, "candidate_tree") !== status.projection.currentCandidateTree || !Array.isArray(eligible) || selected.reason !== undefined || selected.intendedUntracked!.some((path) => !eligible.includes(path)); - if (rejected) return { operation: parameters.operation, status: "blocked", outcome: "intended-untracked-selection-binding-rejected", mutation_performed: false, mutation_outcome: "none" }; + const rejected = intendedUntrackedSelectionRejection(input, canonicalBinding, status, eligible, selected); + if (rejected !== undefined) return { operation: parameters.operation, status: "blocked", outcome: "intended-untracked-selection-binding-rejected", reason: rejected.reason, ...(rejected.field === undefined ? {} : { field: rejected.field }), mutation_performed: false, mutation_outcome: "none" }; const submission = { argumentTokens: input.submission!.argumentTokens, value: JSON.stringify({ schema: "gentle-ai.review-intended-untracked-selection/v1", untracked_scope: scope, expected_untracked_inventory: inventory, intended_untracked: selected.intendedUntracked }) }; const result = await executeReviewControllerOperation({ operation: REVIEW_CONTROLLER_OPERATION.START, ...(parameters.workspaceRoot === undefined ? {} : { workspaceRoot: parameters.workspaceRoot }), input: JSON.stringify({ mode: REVIEW_MODE.ORDINARY, untrackedScope: scope, expectedUntrackedInventory: inventory, intendedUntracked: selected.intendedUntracked }) }, sessionCwd, nativeReviewCli, signal, candidateViews, context, retainedUntrackedSelections, pendingReviewConsentRegistry, pendingReviewConsentFallbackKey, reviewConsentNow, reviewConsentScheduleTimer, submission); return { ...result, operation: parameters.operation }; diff --git a/tests/review-controller-native-routing.test.ts b/tests/review-controller-native-routing.test.ts index 84a67f5bf..1168a9109 100644 --- a/tests/review-controller-native-routing.test.ts +++ b/tests/review-controller-native-routing.test.ts @@ -938,13 +938,19 @@ for (const statusSchema of ["gentle-ai.review-integration.status/v6", "gentle-ai assert.deepEqual(requests.map((request) => request.cwd), [cwd, cwd, cwd]); assert.equal(starts[0]!.cwd, cwd); assert.deepEqual(starts[0]!.intendedUntrackedSelection, { argumentTokens: selection.submission!.argumentTokens, value: JSON.stringify({ schema: "gentle-ai.review-intended-untracked-selection/v1", untracked_scope: "select", expected_untracked_inventory: SHA, intended_untracked: [eligible] }) }); - for (const invalid of [ - { selectionBinding: selectionBinding.replace(eligible, "docs/stale.md"), intendedUntracked: [eligible] }, - { selectionBinding, intendedUntracked: [eligible, eligible] }, { selectionBinding, intendedUntracked: ["docs/unknown.md"] }, - ]) { + // gentle-pi#941: every refusal named its own check, so a caller holding + // provider-issued bytes can tell a stale binding from an ineligible path + // instead of retrying the same call. + for (const { reason, field, ...invalid } of [ + { selectionBinding: selectionBinding.replace(eligible, "docs/stale.md"), intendedUntracked: [eligible], reason: "binding-mismatch" }, + { selectionBinding, intendedUntracked: [eligible, eligible], reason: "untracked-selection-invalid" }, + { selectionBinding, intendedUntracked: ["docs/unknown.md"], reason: "path-not-eligible", field: "docs/unknown.md" }, + ] as ReadonlyArray<{ reason: string; field?: string; selectionBinding: string; intendedUntracked: readonly string[] }>) { const rejection = await __testing.executeReviewControllerOperation({ operation: "select-intended-untracked", ...invalid } as never, cwd, native, undefined, undefined, undefined, retained); assert.equal(rejection.status, "blocked", JSON.stringify(invalid)); assert.equal(rejection.outcome, "intended-untracked-selection-binding-rejected", JSON.stringify(invalid)); + assert.equal(rejection.reason, reason, JSON.stringify(rejection)); + assert.equal(rejection.field, field, JSON.stringify(rejection)); assert.equal(rejection.mutation_performed, false, JSON.stringify(invalid)); assert.equal(rejection.mutation_outcome, "none", JSON.stringify(invalid)); } @@ -953,6 +959,71 @@ for (const statusSchema of ["gentle-ai.review-integration.status/v6", "gentle-ai assert.equal(requests.every((request) => !("lineageId" in request)), true); }); +// gentle-pi#941: a selection refused because the live status moved names the field +// that moved, and an empty selection -- the only honest answer when the candidate +// touches no untracked file -- is accepted rather than refused. +for (const scenario of [ + { name: "target_identity", moved: (status: ReviewStatusV3) => ({ ...status, targetIdentity: `sha256:${"d".repeat(64)}` }) }, + { name: "candidate_tree", moved: (status: ReviewStatusV3) => ({ ...status, projection: { ...status.projection, currentCandidateTree: "e".repeat(40) } }) }, +]) test(`a moved ${scenario.name} refuses the selection by name rather than opaquely`, async (t) => { + const cwd = realpathSync(repository(t)), eligible = "selected.md"; + writeFileSync(join(cwd, eligible), "selected\n"); + const initialTarget = startStatus(cwd, undefined, [eligible]); + const selection = intendedUntrackedSelection(initialTarget, [eligible]); + const initial = { ...initialTarget, nextTransition: { kind: "collect", reasonCode: "intended_untracked_selection_required", collect: { inputs: [selection] } }, raw: { schema: "gentle-ai.review-integration.status/v7" } } as ReviewStatusV3; + let statusCalls = 0; + const starts: Array> = [], retained = new Map(); + const native = { + reviewMode: async () => ({ operation: "status", scope: "clone", status: { global: "on", cloneLocal: "on", effective: "on", source: "clone_local" } }), + // The binding is issued against the first status and revalidated against + // the second, which is where a real repository drifts under the caller. + targetStatus: async () => (++statusCalls === 1 ? initial : scenario.moved(initial)), + start: async (request: Record) => { starts.push(request); return { lineageId: "unreachable", state: "reviewing", riskLevel: "low", selectedLenses: [], changedFiles: 1, changedLines: 1, correctionBudget: 1, action: "created", lensesRequired: false, riskReasons: [], raw: {} }; }, + } as unknown as NativeReviewCli; + const listed = await __testing.executeReviewControllerOperation({ operation: "status", workspaceRoot: cwd }, cwd, native, undefined, undefined, undefined, retained); + const rejection = await __testing.executeReviewControllerOperation({ operation: "select-intended-untracked", selectionBinding: listed.selectionBinding as string, intendedUntracked: [eligible], workspaceRoot: cwd } as never, cwd, native, undefined, undefined, undefined, retained); + assert.equal(rejection.outcome, "intended-untracked-selection-binding-rejected", JSON.stringify(rejection)); + assert.equal(rejection.reason, "status-field-mismatch", JSON.stringify(rejection)); + assert.equal(rejection.field, scenario.name, JSON.stringify(rejection)); + assert.equal(starts.length, 0); +}); + +// The reporter's own case in gentle-pi#941: the candidate touches only tracked +// files while the repository carries an unrelated untracked inventory, so the +// only honest selection is the empty one. It must start, not be refused. +test("an empty intended-untracked selection excludes every eligible path and starts", async (t) => { + const cwd = realpathSync(repository(t)), sessionCwd = repository(t); + writeFileSync(join(cwd, "qa screenshot 1.png"), "noise\n"); + writeFileSync(join(cwd, "mcp.log"), "noise\n"); + const initialTarget = startStatus(cwd), target = startStatus(cwd); + const selection = intendedUntrackedSelection(initialTarget, ["qa screenshot 1.png", "mcp.log"]); + const initial = { ...initialTarget, nextTransition: { kind: "collect", reasonCode: "intended_untracked_selection_required", collect: { inputs: [selection] } }, raw: { schema: "gentle-ai.review-integration.status/v7" } } as ReviewStatusV3; + const starts: Array> = [], retained = new Map(); + const native = { + reviewMode: async () => ({ operation: "status", scope: "clone", status: { global: "on", cloneLocal: "on", effective: "on", source: "clone_local" } }), + targetStatus: async (request: Record) => ("intendedUntrackedSelection" in request ? target : initial), + start: async (request: Record) => { starts.push(request); return { lineageId: "excluded", state: "reviewing", riskLevel: "low", selectedLenses: [], changedFiles: 1, changedLines: 1, correctionBudget: 1, action: "created", lensesRequired: false, riskReasons: [], raw: {} }; }, + } as unknown as NativeReviewCli; + const listed = await __testing.executeReviewControllerOperation({ operation: "status", workspaceRoot: cwd }, sessionCwd, native, undefined, undefined, undefined, retained); + const result = await __testing.executeReviewControllerOperation({ operation: "select-intended-untracked", selectionBinding: listed.selectionBinding as string, intendedUntracked: [], workspaceRoot: cwd } as never, sessionCwd, native, undefined, undefined, undefined, retained); + assert.equal(starts.length, 1, JSON.stringify(result)); + // EXCLUDE, with an empty selection: nothing untracked joins the candidate. + assert.deepEqual(starts[0]!.intendedUntrackedSelection, { argumentTokens: selection.submission!.argumentTokens, value: JSON.stringify({ schema: "gentle-ai.review-intended-untracked-selection/v1", untracked_scope: "exclude", expected_untracked_inventory: SHA, intended_untracked: [] }) }); + assert.equal(result.reason, undefined, JSON.stringify(result)); +}); + +function intendedUntrackedSelection(status: ReviewStatusV3, eligible: readonly string[]): ReviewCollectInputV3 { + return { + name: "intended_untracked_selection", schema: "gentle-ai.review-intended-untracked-selection/v1", captureOperation: "external.select_intended_untracked", + arguments: [ + { name: "target_identity", value: SHA }, { name: "projection", value: "workspace" }, + { name: "base_tree", value: status.projection.baseTree }, { name: "candidate_tree", value: status.projection.currentCandidateTree }, + { name: "eligible_paths_json", value: JSON.stringify([...eligible]) }, { name: "expected_untracked_inventory", value: SHA }, + ], + submission: { operationToken: "status", argumentTokens: ["--contract=gentle-ai.review-integration/v2", "--next-transition=true", "--agent=pi", "--projection=workspace", "--intended-untracked-selection={{value}}"], values: [{ slot: "intended_untracked_selection", domain: "schema_bound_json", schema: "gentle-ai.review-intended-untracked-selection/v1", substitutionLocation: 4 }] }, + } as unknown as ReviewCollectInputV3; +} + // gentle-pi#706: inspect names the exact continuation for the intended-untracked // stop and can resolve it in one call through top-level untrackedScope. function untrackedStopFixture(