fix(review): allow empty input for ambient inspect (#1648)

.github/workflows/ci.yml

@@ -76,6 +76,127 @@ jobs:
       - name: Verify generated runtime modules
         run: pnpm run check:runtime-modules
 
+  review-routing-focused-windows:
+    if: github.event_name == 'pull_request'
+    runs-on: windows-latest
+    timeout-minutes: 15
+    env:
+      npm_config_ignore_scripts: "true"
+    steps:
+      - name: Checkout synthetic merge
+        uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
+        with:
+          persist-credentials: false
+          fetch-depth: 2
+
+      - name: Setup Node.js
+        uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4
+        with:
+          node-version: "24"
+
+      - name: Setup pnpm
+        uses: pnpm/action-setup@b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4
+        with:
+          version: 11.1.1
+          dest: ${{ runner.temp }}/setup-pnpm
+
+      - name: Install dependencies without lifecycle scripts
+        shell: pwsh
+        run: pnpm install --frozen-lockfile --ignore-scripts --store-dir "$env:RUNNER_TEMP\pnpm-store"
+
+      - name: Verify merged inventory and run focused routing cases
+        shell: pwsh
+        timeout-minutes: 12
+        run: |
+          $log = Join-Path $env:RUNNER_TEMP 'review-routing-focused-windows.tap.log'
+          function Evidence([string]$message) { $message | Tee-Object -FilePath $log -Append }
+          # Conservative: even benign inherited GIT_* variables block, never normalize them.
+          $overrides = @(Get-ChildItem Env: | Where-Object Name -Like 'GIT_*' | Select-Object -ExpandProperty Name)
+          if ($overrides.Count -gt 0) {
+            Evidence ('Blocked inherited Git overrides (names only): ' + ($overrides -join ', '))
+            throw 'Inherited Git environment equivalence is unproved'
+          }
+          $event = Get-Content -LiteralPath $env:GITHUB_EVENT_PATH -Raw | ConvertFrom-Json
+          $head = & git rev-parse HEAD
+          if ($LASTEXITCODE -ne 0) { throw 'Cannot resolve checkout HEAD' }
+          $parents = (& git rev-list --parents -n 1 HEAD) -split ' '
+          if ($LASTEXITCODE -ne 0) { throw 'Cannot resolve checkout parents' }
+          $prHead = [string]$event.pull_request.head.sha
+          $prBase = [string]$event.pull_request.base.sha
+          $nodeVersion = & node --version
+          if ($LASTEXITCODE -ne 0) { throw 'Cannot resolve Node version' }
+          $pnpmVersion = & pnpm --version
+          if ($LASTEXITCODE -ne 0) { throw 'Cannot resolve pnpm version' }
+          Evidence "checkout=$head parents=$($parents -join ' ') prHead=$prHead prBase=$prBase node=$nodeVersion pnpm=$pnpmVersion"
+          if ($parents.Count -ne 3 -or $parents[0] -ne $head -or $parents[1] -ne $prBase -or $parents[2] -ne $prHead) {
+            throw 'Checkout is not the declared PR synthetic merge of base and head'
+          }
+          $names = @(
+            'STATUS on approved target preserves workspaceRoot in next_action when distinct from process cwd'
+            'approved acknowledgement burn tears down the retained candidate view and keeps its projection'
+            'approved acknowledgement reports the burn truthfully when candidate-view cleanup fails after it'
+            'capture route recovery uses a trusted committed projection through unknown-outcome reconciliation'
+            'REPAIR retains frozen committed collect selectors and leaves workspace routes unselected'
+            'selectorless STATUS resumes a retained committed correction lineage'
+            'selectorless STATUS uses a retained committed selector only at its retained workspace'
+            'INSPECT rejects malformed committed-range selectors before negotiated STATUS'
+            'INSPECT accepts empty object and empty string input for ambient inspection'
+            'INSPECT ambient input with top-level selected-empty stays read-only when selection is not required'
+          )
+          $file = 'tests/review-controller-native-routing.test.ts'
+          $source = Get-Content -LiteralPath $file -Raw
+          foreach ($name in $names) {
+            $declaration = '(?m)^test\("' + [regex]::Escape($name) + '"\s*,'
+            $count = [regex]::Matches($source, $declaration).Count
+            Evidence "inventory count=$count name=$name"
+            if ($count -ne 1) { throw "Missing or duplicate top-level declaration: $name" }
+          }
+          $pattern = '^(?:' + (($names | ForEach-Object { [regex]::Escape($_) }) -join '|') + ')$'
+          Evidence 'Name filtering does not prevent module imports or global side effects; this is not the full two-file gate.'
+          $previousErrorActionPreference = $ErrorActionPreference
+          $PSNativeCommandUseErrorActionPreference = $false
+          $ErrorActionPreference = 'Continue'
+          & node --experimental-strip-types --test --test-reporter=tap --test-name-pattern $pattern $file 2>&1 | Tee-Object -FilePath $log -Append
+          $status = $LASTEXITCODE
+          $ErrorActionPreference = $previousErrorActionPreference
+          Evidence "nodeExit=$status"
+          if ($status -ne 0) { exit $status }
+          $tap = Get-Content -LiteralPath $log -Raw -ErrorAction Stop
+          $plans = [regex]::Matches($tap, '(?m)^1\.\.(\d+)\r?$')
+          $results = [regex]::Matches($tap, '(?m)^(ok|not ok) (\d+) - (.+)\r?$')
+          $summary = [regex]::Matches($tap, '(?m)^# tests (\d+)\r?\n# suites (\d+)\r?\n# pass (\d+)\r?\n# fail (\d+)\r?\n# cancelled (\d+)\r?\n# skipped (\d+)\r?\n# todo (\d+)\r?\n# duration_ms [\d.]+\r?\n')
+          if ($plans.Count -ne 1 -or $summary.Count -ne 1 -or $summary[0].Index -le $plans[0].Index) {
+            throw 'Missing or duplicate complete top-level TAP plan/final summary'
+          }
+          $s = $summary[0].Groups
+          Evidence "counts tests=$($s[1].Value) suites=$($s[2].Value) pass=$($s[3].Value) fail=$($s[4].Value) cancelled=$($s[5].Value) skipped=$($s[6].Value) todo=$($s[7].Value)"
+          if ([long]$s[4].Value -ne 0 -or [long]$s[5].Value -ne 0 -or [long]$s[7].Value -ne 0 -or [long]$s[3].Value -lt $names.Count) {
+            throw 'Failures, cancellations, TODOs or insufficient executed passes'
+          }
+          if ($results.Count -ne [long]$plans[0].Groups[1].Value -or [long]$s[1].Value -ne ([long]$s[3].Value + [long]$s[6].Value)) {
+            throw 'Incomplete TAP result accounting'
+          }
+          foreach ($name in $names) {
+            $matches = @($results | Where-Object { $_.Groups[3].Value.TrimEnd("`r") -eq $name })
+            if ($matches.Count -ne 1 -or $matches[0].Groups[1].Value -ne 'ok') {
+              throw "Expected exactly one executed PASS without SKIP/TODO: $name"
+            }
+          }
+          $executed = @($results | Where-Object { $_.Groups[3].Value -notmatch '\s+# SKIP\b' })
+          if ($executed.Count -ne $names.Count -or @($results | Where-Object { $_.Groups[1].Value -ne 'ok' -or $_.Groups[3].Value -match '\s+# TODO\b' }).Count -ne 0) {
+            throw 'Unexpected executed top-level cases or failing/TODO results'
+          }
+          Evidence 'Unselected skips are not coverage. Completion is not child-termination or unasserted cleanup proof.'
+
+      - name: Upload focused Windows routing log
+        if: always()
+        uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
+        with:
+          name: review-routing-focused-windows-log
+          path: ${{ runner.temp }}/review-routing-focused-windows.tap.log
+          retention-days: 7
+          if-no-files-found: warn
+
   review-repository-windows:
     runs-on: windows-latest
     steps:

extensions/gentle-ai.ts

@@ -8156,7 +8156,7 @@ async function executeReviewControllerOperation(
 		parameters.operation === REVIEW_CONTROLLER_OPERATION.INSPECT &&
 		nativeReviewCli !== null
 	) {
-		const rawInspect = parameters.input === undefined
+		const rawInspect = parameters.input === undefined || parameters.input.trim() === ""
 			? undefined
 			: parseControllerJson(parameters.input, REVIEW_CONTROLLER_OPERATION.INSPECT);
 		const unknownField = rawInspect === undefined
@@ -8166,7 +8166,7 @@ async function executeReviewControllerOperation(
 		const baseRef = rawInspect?.baseRef;
 		if (baseRef !== undefined && !isCanonicalProcessString(baseRef)) return nativeInspectInputRejection("base-ref-invalid");
 		if (baseRef !== undefined && rawInspect?.committedOnly !== true) return nativeInspectInputRejection("committed-only-required");
-		if (rawInspect !== undefined && baseRef === undefined) return nativeInspectInputRejection("committed-only-invalid");
+		if (rawInspect?.committedOnly !== undefined && baseRef === undefined) return nativeInspectInputRejection("committed-only-invalid");
 		let canonicalBaseRef: string | undefined;
 		if (typeof baseRef === "string") {
 			try {

odd/tasks/fix-1648-inspect-empty-input-dead-end.md

@@ -0,0 +1,52 @@
+# Fix #1648: allow empty input `{}` and empty string for ambient inspect
+
+## Objective
+
+Prevent `gentle_review inspect` from rejecting ambient inspection calls when `input` is provided as an empty JSON object `"{}"` or empty string `""`. Ensure `committed-only-invalid` is only returned when `committedOnly` is actually supplied without `baseRef`.
+
+## Problem
+
+When a caller (such as an LLM conforming to tool schemas) calls `gentle_review` with `{"operation": "inspect", "input": "{}"}`:
+`extensions/gentle-ai.ts:7843` checks:
+```ts
+if (rawInspect !== undefined && baseRef === undefined) return nativeInspectInputRejection("committed-only-invalid");
+```
+Because `rawInspect` is `{}` (not `undefined`) and `baseRef` is `undefined`, the controller returns `committed-only-invalid` even though `committedOnly` was never passed.
+Additionally, passing empty string `input: ""` throws a JSON parse error instead of treating it as omitted input.
+
+## Scope
+
+- In `extensions/gentle-ai.ts`, treat empty/whitespace input string as `undefined` for `inspect`.
+- In `extensions/gentle-ai.ts`, reject `committed-only-invalid` only if `rawInspect?.committedOnly !== undefined && baseRef === undefined`.
+- When `rawInspect` is empty `{}` (no `baseRef` and no `committedOnly`), allow it to proceed to ambient inspect.
+- In `tests/review-controller-native-routing.test.ts`, add tests proving:
+  - `input: "{}"` proceeds to ambient inspect.
+  - `input: ""` proceeds to ambient inspect.
+  - `input: '{"committedOnly": true}'` still rejects `committed-only-invalid`.
+  - `input: '{"committedOnly": false}'` still rejects `committed-only-invalid`.
+
+## Tasks
+
+- [x] T1 Reproduce #1648 with failing unit tests in `tests/review-controller-native-routing.test.ts` (RED).
+- [x] T2 Fix inspect input validation in `extensions/gentle-ai.ts` (GREEN).
+- [x] T3 Verify full test suite, runtime module checks, and typechecks.
+- [x] T4 Commit work unit and document verification evidence (commit `975e1709`).
+
+## Verification Evidence
+
+- **RED observed**:
+  - `INSPECT accepts empty object and empty string input for ambient inspection`: failed with `AssertionError: expected ready for input "{}" ('blocked' !== 'ready')`, rejected with `native-inspect-input-invalid` and `reason: "committed-only-invalid"`.
+- **GREEN observed**:
+  - Test passed for `"{}"`, `""`, and `"   "`, successfully executing ambient `targetStatus` (3/3 calls).
+  - Malformed committed-range selectors (`{ committedOnly: true }`, `{ committedOnly: false }`) continue to fail closed with `committed-only-invalid`.
+  - `node --experimental-strip-types --test tests/review-controller-native-routing.test.ts`: 89 passed, 0 failed.
+  - `pnpm run typecheck`: clean (187 recorded baseline diagnostics, 0 regressions).
+  - `pnpm run check:runtime-modules`: clean (8 generated modules).
+  - `pnpm test`: 4,539 passed, 0 failed, 34 skipped (all three stages PASS: `unit-tests`, `provider-contract`, `runtime-harness`).
+
+
+## Acceptance Criteria
+
+- `executeReviewControllerOperation` with `operation: "inspect"` and `input: "{}"` or `input: ""` succeeds and dispatches ambient inspection.
+- Calls providing `committedOnly` without `baseRef` continue to fail closed with `committed-only-invalid`.
+- All tests in `tests/review-controller-native-routing.test.ts` and full test suite pass.

tests/gentle-ai.test.ts

@@ -1,7 +1,8 @@
 import assert from "node:assert/strict";
 import { execFileSync } from "node:child_process";
 import { createHash } from "node:crypto";
-import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, symlinkSync, writeFileSync } from "node:fs";
+import fs, { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, symlinkSync, writeFileSync } from "node:fs";
+import { syncBuiltinESMExports } from "node:module";
 import { tmpdir } from "node:os";
 import { dirname, join } from "node:path";
 import test from "node:test";
@@ -905,6 +906,7 @@ test("ordinary native capture exposes a registered schema and STATUS binding cop
 
 	assert.ok(tools.has("gentle_review_capture"));
 	assert.deepEqual(tools.get("gentle_review_capture")?.parameters.required, ["lineageId", "collectBinding"]);
+	assert.deepEqual(tools.get("gentle_review")?.parameters.required, ["operation"]);
 
 	const sha = `sha256:${"a".repeat(64)}`;
 	const lineageId = "ordinary-capture";
@@ -3106,18 +3108,54 @@ test("p pins the selected profile for the clone without touching the global rout
 });
 
 test("the profile pin scope note sanitizes its worktree-derived path", (t) => {
-	const { fixture, writeStore } = profilesStoreFixture(t);
+	const { fixture, localPinPath: safeLocalPath, writeStore } = profilesStoreFixture(t);
 	writeStore({ team: { worker: { model: "openai/alpha" } } }, "team");
 	const commonDir = join(fixture.root, "git-\x1b]52;c;payload\x07-common");
 	const localPath = join(commonDir, "gentle-ai", "profile-pin.json");
-	writeProfilePinSync(localPath, "team");
+	writeProfilePinSync(safeLocalPath, "team");
 	setProfilePinWorktreeResolverForTesting(() => ({ root: fixture.root, commonDir }));
 
-	const note = __testing.profilePinScopeNote(fixture.root);
-	assert.ok(note);
-	assert.doesNotMatch(note, /[\x00-\x1f\x7f-\x9f]/);
-	assert.doesNotMatch(note, /payload/);
-	assert.match(note, /profile-pin\.json/);
+	// Windows cannot create this hostile filename. Redirect only its disk reads;
+	// the real resolver still passes the unsanitized path to production rendering.
+	const originalExistsSync = fs.existsSync;
+	const originalReadFileSync = fs.readFileSync;
+	let hostileExistsCalls = 0;
+	let hostileReadCalls = 0;
+	try {
+		t.mock.method(fs, "existsSync", (path: Parameters<typeof existsSync>[0]) => {
+			if (path === localPath) {
+				hostileExistsCalls++;
+				return originalExistsSync(safeLocalPath);
+			}
+			return originalExistsSync(path);
+		});
+		t.mock.method(fs, "readFileSync", (...args: Parameters<typeof readFileSync>) => {
+			if (args[0] === localPath) {
+				hostileReadCalls++;
+				args[0] = safeLocalPath;
+			}
+			return originalReadFileSync(...args);
+		});
+		syncBuiltinESMExports();
+
+		const note = __testing.profilePinScopeNote(fixture.root);
+		assert.ok(note);
+		assert.doesNotMatch(note, /[\x00-\x1f\x7f-\x9f]/);
+		assert.doesNotMatch(note, /payload/);
+		assert.match(note, /profile-pin\.json/);
+		assert.equal(hostileExistsCalls, 1);
+		assert.equal(hostileReadCalls, 1);
+	} finally {
+		try {
+			t.mock.restoreAll();
+		} finally {
+			syncBuiltinESMExports();
+		}
+	}
+	assert.equal(fs.existsSync, originalExistsSync);
+	assert.equal(fs.readFileSync, originalReadFileSync);
+	assert.equal(existsSync, originalExistsSync);
+	assert.equal(readFileSync, originalReadFileSync);
 });
 
 test("P declares the profile in the worktree so the routing can be committed", async (t) => {