[PR #2495] [MERGED] fix(gemini): compare consolidated allowlists through one normalizer #5272

New issue

Closed

opened 2026-10-31 01:05:21 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:05:21 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2495
Author: @XiaoBinGan
Created: 10/13/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/gemini-allowlist-normalization

📝 Commits (9)

f57ef97 fix(gemini): compare consolidated allowlists through one normalizer
90096eb test(gemini): cover normalized exclusions and padded allowlist entries
d9b3719 chore: sync captured upstream CI baseline
459c21a fix(process): report no remaining time after a provider deadline fires
0dfa2ed fix(setup): retain monotonic deadline and launcher coverage
e92f231 Merge remote-tracking branch 'origin/main' into fix/gemini-allowlist-normalization
d0d31a9 Merge remote-tracking branch 'origin/main' into fix/gemini-allowlist-normalization
2cb02c0 Merge remote-tracking branch 'origin/main' into fix/gemini-allowlist-normalization
b77b5c2 Merge branch 'main' into fix/gemini-allowlist-normalization

📊 Changes

2 files changed (+102 additions, -6 deletions)

View changed files

📝 src/application/ClientConfigurationDocument.ts (+17 -6)
📝 tests/boundary/cli/clientProfiles.test.ts (+85 -0)

📄 Description

The Gemini policy check consolidates allowlists by normalized intersection, but then tested the consolidated list against the raw server key. The same set of policies could therefore block or allow the server depending only on which policy happened to supply the spelling that survived the intersection.

Summary

geminiServerPolicyBlock is documented as consolidating "excludes by union and allowlists by normalized intersection", and the intersection is indeed normalized:

const normalized = new Set(current.map((name) => name.toLowerCase().trim()));
return previous.filter((name) => normalized.has(name.toLowerCase().trim()));
...
if (allowed !== undefined && allowed.length > 0 && !allowed.includes(serverKey))
  return `Gemini mcp.allowed does not include ${serverKey}. ...`;

The membership test that decides the block is raw, so the surviving spelling decides the outcome. With serverKey === "rea" and two policies allowed: ["REA"] and allowed: ["rea"]:

system ["REA"], default ["rea"]  -> blocked, "Gemini mcp.allowed does not include rea"
system ["rea"], default ["REA"]  -> not blocked, setup proceeds

Two runs, same effective policy set, opposite results. The excluded test has the mirror-image asymmetry: excluded: ["REA"] does not block rea while the normalized allowlist intersection would have matched it. Settings files are hand-authored, so a spelling of the product key cannot be assumed to match PRODUCT_IDENTITY.mcpServerKey exactly, and the function's own doc already declares normalization as the rule.

Change and scope

ClientConfigurationDocument introduces one normalizer, normalizeGeminiServerName, and applies it to all three rules: the exclusion test, the intersection (which already normalized, now through the shared function) and the membership test. The comparison key is normalized once, and the messages keep the caller-facing serverKey spelling, so lookup and display stay separate.

No new option, schema, error code or message text is introduced, and the intersection result itself is unchanged — only the decision made from it. --yes semantics, the empty-allowlist-is-unrestricted rule, the sources suffix appended by the caller and every other client profile are untouched.

Contract and boundary impact

Semantic owner and earliest changed stage: server-name comparison in the Gemini settings policy consolidation, before any file write. The settings readers, the other client profiles and the registration lifecycle are untouched.
CLI and MCP/tool-catalog contract: no tool name, command, input field, output schema or error code changes. setup --client gemini_cli accepts the same policies it accepted before, minus the order-dependent refusal; both failure messages are unchanged in wording.
Provider, target-format or platform compatibility: none — settings-file policy logic, platform-independent. Applies to the system, system-defaults and user scopes alike.
Evidence, artifact, provenance or reconstruction contract: none. Setup writes the same consolidated configuration; a run that previously exited needs_human with appliedActions: [] now proceeds and writes the merged settings, which is the behavior the existing same-spelling test already pins.
Process execution, authorization, cleanup or containment impact: none. No approval, backup or idempotency behavior changes; the change only removes a spurious human-approval stop.
Generated metadata, package or installation impact: none.

Evidence and regression coverage

A new cliTest, "Gemini matches allowlist spellings independently of their policy order", in clientProfiles.test.ts runs the real setup --client gemini_cli --yes --json workflow twice with the same two policies in both spelling arrangements and requires status: "ready" and exit code 0 in each.

Before the change exactly one arrangement failed with rea+REA: expected 1 to be +0; the 12 existing cases in that file passed throughout, and the existing test for this function only uses the lowercase spelling rea, which is why the asymmetry was uncovered. Verification on macOS 26.5.1 arm64 with Node.js 24.18.0 and npm 11.16.0, based on bb54082:

npm run test:focused over clientProfiles, clientProfileLifecycle, clientRegistrationStatus and clientLifecycle: 88 passed, 0 failed.
npm run check:fast: passed.
npm run verify:test-discovery: all 1015 test files discovered exactly once.

Unavailable coverage: no real gemini CLI installation and no Windows run, so Gemini's own manager was not exercised — the assertions are on REA's setup workflow and the configuration REA writes. Platform-conditional code is not touched.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2495 **Author:** [@XiaoBinGan](https://github.com/XiaoBinGan) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/gemini-allowlist-normalization` --- ### 📝 Commits (9) - [`f57ef97`](https://github.com/morluto/rea/commit/f57ef97b7f52e92c4d5f22fb48f9e70220cf6088) fix(gemini): compare consolidated allowlists through one normalizer - [`90096eb`](https://github.com/morluto/rea/commit/90096ebc93dc686cba5b235edfe94bc6ad14a7f7) test(gemini): cover normalized exclusions and padded allowlist entries - [`d9b3719`](https://github.com/morluto/rea/commit/d9b371928b77e4b36adbbf310df5b13af976b8c7) chore: sync captured upstream CI baseline - [`459c21a`](https://github.com/morluto/rea/commit/459c21a09349e41c53b2737d44e02885a20380ee) fix(process): report no remaining time after a provider deadline fires - [`0dfa2ed`](https://github.com/morluto/rea/commit/0dfa2ede43113ad9c716deac79c238b7ab3fb089) fix(setup): retain monotonic deadline and launcher coverage - [`e92f231`](https://github.com/morluto/rea/commit/e92f231415ea7d8a4abc3469f41a7024f946002d) Merge remote-tracking branch 'origin/main' into fix/gemini-allowlist-normalization - [`d0d31a9`](https://github.com/morluto/rea/commit/d0d31a9088a82341d47ec87d09a184d77e0a472c) Merge remote-tracking branch 'origin/main' into fix/gemini-allowlist-normalization - [`2cb02c0`](https://github.com/morluto/rea/commit/2cb02c07e49be5e7adc63887992546be60f0666f) Merge remote-tracking branch 'origin/main' into fix/gemini-allowlist-normalization - [`b77b5c2`](https://github.com/morluto/rea/commit/b77b5c2e5cb833e9daf86300975dc5b299f47520) Merge branch 'main' into fix/gemini-allowlist-normalization ### 📊 Changes **2 files changed** (+102 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `src/application/ClientConfigurationDocument.ts` (+17 -6) 📝 `tests/boundary/cli/clientProfiles.test.ts` (+85 -0) </details> ### 📄 Description The Gemini policy check consolidates allowlists by normalized intersection, but then tested the consolidated list against the raw server key. The same set of policies could therefore block or allow the server depending only on which policy happened to supply the spelling that survived the intersection. ## Summary [`geminiServerPolicyBlock`](https://github.com/morluto/rea/blob/bb54082/src/application/ClientConfigurationDocument.ts) is documented as consolidating "excludes by union and allowlists by normalized intersection", and the intersection is indeed normalized: ```ts const normalized = new Set(current.map((name) => name.toLowerCase().trim())); return previous.filter((name) => normalized.has(name.toLowerCase().trim())); ... if (allowed !== undefined && allowed.length > 0 && !allowed.includes(serverKey)) return `Gemini mcp.allowed does not include ${serverKey}. ...`; ``` The membership test that decides the block is raw, so the surviving spelling decides the outcome. With `serverKey === "rea"` and two policies `allowed: ["REA"]` and `allowed: ["rea"]`: ``` system ["REA"], default ["rea"] -> blocked, "Gemini mcp.allowed does not include rea" system ["rea"], default ["REA"] -> not blocked, setup proceeds ``` Two runs, same effective policy set, opposite results. The `excluded` test has the mirror-image asymmetry: `excluded: ["REA"]` does not block `rea` while the normalized allowlist intersection would have matched it. Settings files are hand-authored, so a spelling of the product key cannot be assumed to match `PRODUCT_IDENTITY.mcpServerKey` exactly, and the function's own doc already declares normalization as the rule. ## Change and scope [`ClientConfigurationDocument`](https://github.com/XiaoBinGan/rea/blob/HEAD/src/application/ClientConfigurationDocument.ts) introduces one normalizer, `normalizeGeminiServerName`, and applies it to all three rules: the exclusion test, the intersection (which already normalized, now through the shared function) and the membership test. The comparison key is normalized once, and the messages keep the caller-facing `serverKey` spelling, so lookup and display stay separate. No new option, schema, error code or message text is introduced, and the intersection result itself is unchanged — only the decision made from it. `--yes` semantics, the empty-allowlist-is-unrestricted rule, the sources suffix appended by the caller and every other client profile are untouched. ## Contract and boundary impact - **Semantic owner and earliest changed stage:** server-name comparison in the Gemini settings policy consolidation, before any file write. The settings readers, the other client profiles and the registration lifecycle are untouched. - **CLI and MCP/tool-catalog contract:** no tool name, command, input field, output schema or error code changes. `setup --client gemini_cli` accepts the same policies it accepted before, minus the order-dependent refusal; both failure messages are unchanged in wording. - **Provider, target-format or platform compatibility:** none — settings-file policy logic, platform-independent. Applies to the `system`, `system-defaults` and `user` scopes alike. - **Evidence, artifact, provenance or reconstruction contract:** none. Setup writes the same consolidated configuration; a run that previously exited `needs_human` with `appliedActions: []` now proceeds and writes the merged settings, which is the behavior the existing same-spelling test already pins. - **Process execution, authorization, cleanup or containment impact:** none. No approval, backup or idempotency behavior changes; the change only removes a spurious human-approval stop. - **Generated metadata, package or installation impact:** none. ## Evidence and regression coverage A new `cliTest`, "Gemini matches allowlist spellings independently of their policy order", in [`clientProfiles.test.ts`](https://github.com/XiaoBinGan/rea/blob/HEAD/tests/boundary/cli/clientProfiles.test.ts) runs the real `setup --client gemini_cli --yes --json` workflow twice with the same two policies in both spelling arrangements and requires `status: "ready"` and exit code 0 in each. Before the change exactly one arrangement failed with `rea+REA: expected 1 to be +0`; the 12 existing cases in that file passed throughout, and the existing test for this function only uses the lowercase spelling `rea`, which is why the asymmetry was uncovered. Verification on macOS 26.5.1 arm64 with Node.js 24.18.0 and npm 11.16.0, based on `bb54082`: - `npm run test:focused` over `clientProfiles`, `clientProfileLifecycle`, `clientRegistrationStatus` and `clientLifecycle`: 88 passed, 0 failed. - `npm run check:fast`: passed. - `npm run verify:test-discovery`: all 1015 test files discovered exactly once. Unavailable coverage: no real `gemini` CLI installation and no Windows run, so Gemini's own manager was not exercised — the assertions are on REA's setup workflow and the configuration REA writes. Platform-conditional code is not touched. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:05:21 +00:00