fix(gemini): handle workspace-trust gate from gemini-cli v0.39.1+ (#26)

[Lykhoyda]

Closes #26.

Summary

• Sets GEMINI_TRUST_WORKSPACE=true by default before spawning gemini, restoring fresh-install parity against gemini-cli v0.39.1+ workspace-trust gate (upstream PR google-gemini/gemini-cli#25814)
• Detects FatalUntrustedWorkspaceError / "not running in a trusted directory" patterns and short-circuits the catch block with a friendly remediation message (no Flash retry — same dir would just fail again)
• Adds opt-out via ASK_GEMINI_REQUIRE_WORKSPACE_TRUST=1 for users who want trust enforcement (e.g., shared CI runners)
• Bumps ask-gemini-mcp 1.6.0 → 1.6.1 and ask-llm-mcp 0.3.1 → 0.3.2 (orchestrator's prepack-bundle.mjs ships gemini-mcp source transitively)

This is action #1 only from the issue's §5 punch-list — the only item flagged "🔴 Hotfix required". Actions #2–#7 (stdin piping for >16 KiB prompts, Codex stabilized-hooks smoke test, sessionId/untrusted-workspace integration test, --ignore-user-config/--ignore-rules on Codex ephemeral path, Gemini tool-event progress forwarding, upstream version watch) are deferred to follow-up PRs as the issue itself recommended.

Why env var, not --skip-trust flag

The local Gemini at the time of authoring is v0.38.2, which has no --skip-trust flag — adding that flag to argv would error on older Geminis with "unknown option." GEMINI_TRUST_WORKSPACE=true is the upstream-blessed compatibility surface (gemini-cli's own E2E CI uses it) and unknown env vars are silently ignored on older versions, making it forward- and backward-compatible. Argv shape is unchanged, so every existing toEqual([...]) argv test stays green untouched. Full rationale in ADR-069 (docs/DECISIONS.md).

Test plan

• yarn test — 422 passing across 6 packages (was 414; +6 new trust-handling tests in gemini-mcp; package count 82 → 88)
• yarn lint — clean (Biome + tsc) across all 6 packages
• yarn build — clean across the monorepo
• Husky pre-push smoke tests pass against live Gemini, Codex, Ollama
• Reviewer to verify: smoke-test against a fresh untrusted dir on gemini-cli v0.39.1+ before publishing the bumped tag (action #4 from the issue — a proper integration test for this case is a follow-up PR)

🤖 Generated with Claude Code

[claude]

Claude finished @Lykhoyda's task in 2m 51s —— View job

---

Re-review complete

• Gather context (CLAUDE.md, PR diff)
• Review geminiExecutor.ts changes
• Review test coverage
• Check constants and type definitions
• Verify previous findings are addressed
• Post final review

---

All three findings from the first review are correctly applied. The implementation is solid. One non-blocking observation below.

Previous findings — all addressed ✅

Finding	Status
Logger.error → Logger.warn for trust-detected log line	✅ Applied
Case-insensitive isWorkspaceTrustError	✅ Applied
Opt-out × trust-error coverage test	✅ Applied, plus a case-insensitivity regression test (FATALUNTRUSTEDWORKSPACEERROR)

One non-blocking observation

sanitizeErrorForLLM may silently drop the trust pattern before isWorkspaceTrustError sees it.

Tracing the error path through commandExecutor.ts:

1. gemini-cli exits non-zero → sanitizeErrorForLLM(stderr, "gemini") runs
2. FatalUntrustedWorkspaceError doesn't hit any of the early-return branches (no quota match, not ENOENT/EACCES)
3. Falls through to: first 3 non-empty lines, joined and returned if < 500 chars

If gemini-cli emits a banner, a warning, or any preamble before the error line, FatalUntrustedWorkspaceError can be pushed to line 4+ and isWorkspaceTrustError silently misses it — the user then gets the raw (first-3-lines) error instead of the friendly remediation message.

The unit tests don't exercise this because they mock executeCommand and reject with the full string directly — they bypass sanitizeErrorForLLM entirely. So the tests give green while the gap exists.

In practice this probably works: gemini-cli exception names are typically the first line of a stack trace. And even if FatalUntrustedWorkspaceError is suppressed, the second pattern "not running in a trusted directory" is a human-readable string that may appear earlier. But it's worth flagging so a future contributor knows the detection relies on gemini-cli's output ordering.

A forward-safe guard would be to add WORKSPACE_TRUST_PATTERNS to commandExecutor's QUOTA_PASSTHROUGH_PATTERNS list (which passes the full stderr through) — but that's a shared-package change, and the ADR's Option-C rationale already ruled out touching commandExecutor. If gemini-cli output changes and users report silent fall-through, the fix is cheap.

Fix this →

Everything else looks good

• ensureWorkspaceTrustEnv idempotency guards are correct (opt-out, pre-set, default)
• Trust check short-circuits before the quota check in the catch block — correct, prevents Flash retry on the same untrusted dir
• Env mutation propagates to child processes via getSpawnEnv()'s {...process.env} spread — no commandExecutor.ts changes needed
• afterEach env restore is correct and prevents cross-test leakage
• Version bumps, README, ADR, BUGS.md, ROADMAP.md all consistent

This is ready to merge as-is. The sanitizeErrorForLLM gap is a minor fragility, not a correctness regression from the pre-PR state.

[claude]

Minor semantic nit: Logger.error is used here for a detected-and-handled condition rather than an actual unrecoverable error. The stderr handler just logs for observability — consider Logger.warn instead, so the log level matches severity. The actual error surface is the friendly Error(ERROR_MESSAGES.WORKSPACE_TRUST_REQUIRED) thrown below in the catch block.

Suggested change

	if (!trustLogged && WORKSPACE_TRUST_PATTERNS.some((pattern) => buffer.includes(pattern))) {
	Logger.warn(STATUS_MESSAGES.WORKSPACE_TRUST_DETECTED);

[claude]

Case-sensitivity inconsistency: QUOTA_PATTERNS matching uses .toLowerCase() on both sides (line 519), but isWorkspaceTrustError uses exact-case includes. The patterns "FatalUntrustedWorkspaceError" and "not running in a trusted directory" are stable upstream identifiers, so this is likely fine in practice — but it's inconsistent with the defensive approach taken for quota errors. If you want to align the patterns, a simple normalisation here would do it:

Suggested change

	return WORKSPACE_TRUST_PATTERNS.some((pattern) => message.includes(pattern));
	function isWorkspaceTrustError(message: string): boolean {
	const lower = message.toLowerCase();
	return WORKSPACE_TRUST_PATTERNS.some((pattern) => lower.includes(pattern.toLowerCase()));
	}

[claude]

This test confirms quota errors still trigger Flash retry — good. One edge case not covered: ASK_GEMINI_REQUIRE_WORKSPACE_TRUST=1 set + gemini-cli fires FatalUntrustedWorkspaceError. With opt-out active, GEMINI_TRUST_WORKSPACE is never set, so the error fires naturally and should still be caught by isWorkspaceTrustError and throw the friendly message (step (a) of the remediation). A test like this would make the opt-out → trust-error flow explicit:

it("throws friendly error when ASK_GEMINI_REQUIRE_WORKSPACE_TRUST=1 and workspace is untrusted", async () => {
  process.env.ASK_GEMINI_REQUIRE_WORKSPACE_TRUST = "1";
  mockExecuteCommand.mockRejectedValueOnce(new Error("FatalUntrustedWorkspaceError: workspace not trusted"));

  await expect(executeGeminiCLI({ prompt: "hello" })).rejects.toThrow(ERROR_MESSAGES.WORKSPACE_TRUST_REQUIRED);
  expect(mockExecuteCommand).toHaveBeenCalledOnce();
});

Not blocking — the two individual tests imply this already works — but it would document the intended UX for the opt-out path.

[Lykhoyda]

Thanks for the review @claude. All three findings applied in e03cfee:

1. Logger.error → Logger.warn ✅ Applied verbatim — the stderr handler logs for observability of a detected and handled condition; the user-facing error surface remains the thrown ERROR_MESSAGES.WORKSPACE_TRUST_REQUIRED in the catch block.

2. Case-insensitive isWorkspaceTrustError ✅ Applied verbatim. Justification beyond consistency: issue Gemini fallback model #21 (multi-pattern quota detection for newer Gemini CLI versions) is direct precedent that upstream error formatting does drift; the .toLowerCase() pair on a cold path is free. Note: I deliberately kept the stderr handler's buffer.includes(pattern) exact-case — that's a logging-only path where a missed log line is cosmetic, not load-bearing. Asymmetry is intentional.

3. Opt-out × trust-error coverage test ✅ Plus a bonus case-insensitivity regression test (FATALUNTRUSTEDWORKSPACEERROR) that locks in finding #2 so a future contributor can't "simplify" the matching back to exact-case without failing CI.

Test count: gemini-mcp 88 → 90; project 422 → 424.
Lint + build: clean.
Pre-push smoke tests: pass against live Gemini, Codex, Ollama.

Ready for re-review.

[claude]

Claude finished @Lykhoyda's task in 1m 43s —— View job

---

Re-review: all three findings applied correctly ✅

• Read CLAUDE.md for repo instructions
• Diff all changes on branch vs origin/main
• Review commit e03cfee (applied feedback changes)
• Review the full geminiExecutor.ts changes
• Review the full geminiExecutor.test.ts changes
• Post final re-review

---

Finding 1 — Logger.warn (geminiExecutor.ts:404)

Applied correctly. The stderr handler now calls Logger.warn(STATUS_MESSAGES.WORKSPACE_TRUST_DETECTED) — the right severity for an observability log of a detected-and-handled condition. The user-facing error surface is still the thrown ERROR_MESSAGES.WORKSPACE_TRUST_REQUIRED in the catch block.

Finding 2 — Case-insensitive isWorkspaceTrustError (geminiExecutor.ts:415-418)

Applied correctly. The .toLowerCase() pair is on the cold catch path so there's no performance concern. The intentional asymmetry between the catch block (case-insensitive) and the stderr handler (exact-case) is documented and sound — the stderr handler is logging-only, so a missed log is cosmetic rather than load-bearing.

Finding 3 — New tests (geminiExecutor.test.ts:784-798)

Both new tests added:

• ASK_GEMINI_REQUIRE_WORKSPACE_TRUST=1 + trust error → friendly message AND GEMINI_TRUST_WORKSPACE stays undefined — exactly the UX contract for the opt-out path.
• FATALUNTRUSTEDWORKSPACEERROR (all-caps) → still matches — locks in finding #2 as a CI regression guard.

The beforeEach/afterEach save-and-restore pattern for the env vars is clean and symmetric.

Full trust-handling suite (8 tests — all code paths covered)

Test	Covers
Default sets GEMINI_TRUST_WORKSPACE=true	happy-path env injection
Opt-out skips injection	ASK_GEMINI_REQUIRE_WORKSPACE_TRUST=1 guard
User-supplied value preserved	GEMINI_TRUST_WORKSPACE=false existing-value guard
FatalUntrustedWorkspaceError → friendly error, no Flash retry	error detection + short-circuit
"not running in a trusted directory" → friendly error	second pattern
Quota errors not misidentified as trust errors	non-regression
Opt-out + trust error → friendly message + no env var set	opt-out × error path
FATALUNTRUSTEDWORKSPACEERROR all-caps → still matches	case-insensitivity regression

One minor wording note (non-blocking)

ERROR_MESSAGES.WORKSPACE_TRUST_REQUIRED (constants.ts:13-14) says "workspace-trust enforcement is enabled". This is accurate for the opt-out path (ASK_GEMINI_REQUIRE_WORKSPACE_TRUST=1 explicitly set) and for the GEMINI_TRUST_WORKSPACE=false explicit-override path, but would be mildly confusing in the hypothetical case where GEMINI_TRUST_WORKSPACE=true was set by the default and gemini-cli still fired the error (where fixes (a) and (b) wouldn't apply). That scenario shouldn't occur in practice — GEMINI_TRUST_WORKSPACE=true is the upstream-blessed bypass — so this is purely cosmetic and not worth changing.

Verdict

LGTM — ready to merge. All three review findings were applied as suggested, the test suite covers every code path, and the implementation is clean. No further changes needed.