fix(permission): gate opencli group on its own keys, not browser#1332
Conversation
`disabled()` mapped every `opencli_*` tool to the `browser` permission key, so a configured `permission.browser: deny` hid the opencli discovery + run tools. But the opencli adapters never ask `browser` at execution — they ask `opencli_read` / `opencli_write` (command-name-scoped), while `browser` rules are URL-scoped. Conflating them let a `browser` rule appear to govern opencli in the tool listing while execution ignored it entirely. Decouple the two: opencli tools are now hidden only when BOTH `opencli_read` and `opencli_write` are a configured `*: deny` (either half allowed means an adapter is still runnable, so the group stays). `browser` deny still hides `browser_*` tools as before. Tests: updated the browser-deny disabled test to assert browser_* only, added opencli decoupling + both-denied + global-wildcard-deny cases, and switched the registry tool_info group-hiding test to deny via the opencli_* keys.
|
Warning Review limit reached
More reviews will be available in 17 minutes and 45 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
harness
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request decouples the permission gating of the opencli tool group from browser permissions, ensuring that opencli tools are governed by their own opencli_read and opencli_write keys instead of URL-scoped browser rules. The opencli group is now only hidden when both read and write permissions are wildcard denied. Feedback on the changes points out a potential issue in isWildcardDeny where a redundant specific deny rule configured after a wildcard deny rule could cause the function to incorrectly return false, and suggests a robust backward-iteration approach to fix it.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The isWildcardDeny check has now drawn the same wrong "simplification" twice
(Gemini bot, then a reviewer): replace it with `evaluate(key, "*").action ===
"deny"`. That shortcut regresses a real case — `evaluate` queries the literal
value "*", which only matches the baseline rule, so `{ "*": deny, "x": allow }`
reports denied and would hide a group that can still run `x` (a false hide,
which a cosmetic visibility gate must never do). Document the asymmetry (false
hide = real harm, false show = harmless) so the next reader doesn't reintroduce
the regression. No behavior change.
1 participant
Summary
Permission.disabled()mapped everyopencli_*tool to thebrowserpermission key, so a configuredpermission.browser: denyhid the opencli discovery + run tools from the model. But the opencli adapters never askbrowserat execution — they askopencli_read/opencli_write. Decouple the two so the opencli group is gated by its own keys, andbrowserdeny governs onlybrowser_*tools.Why
The visibility gate (
disabled(), which hides deferred tool cards / repair hints) and the execution gate (whatask()actually checks) used different permission keys for the opencli group:opencli_*asbrowser-keyed (permission/index.ts, olddisabled()).opencli_read/opencli_writefor the command, plus a per-navigationbrowserask only when an adapter drives the embedded browser (tool/opencli-run.ts).The keys are scoped differently —
browserrules are URL-scoped,opencli_read/opencli_writeare command-name-scoped — so conflating them let abrowserrule appear to govern opencli in the tool listing while execution ignored it. This was the pre-existing inconsistency surfaced by Gemini's review of #1330.Fix: opencli tools are hidden only when both
opencli_readandopencli_writeare a configured*: deny(either half allowed means an adapter is still runnable, so the discovery + run tools stay).browserdeny still hidesbrowser_*as before. A global*: denystill hides everything.Related Issue
None — found while reviewing PR #1330 (embedded-browser permission prompts). Gemini's bot flagged the opencli/browser key mismatch on that PR; this is the focused fix.
Human Review Status
Pending
Review Focus
disabled()rewrite inpermission/index.ts: opencli now gates onOPENCLI_KEYS(opencli_read+opencli_write) viaisWildcardDeny, requiring both denied to hide the group. Confirm the both-vs-either choice matches intent (either half allowed ⇒ adapter runnable ⇒ keep visible).browserdeny still fully hidesbrowser_*(unchanged path), and a global*: denystill hides opencli.Risk Notes
Behavior change limited to tool visibility under a
permission.browser: denyrule: opencli discovery/run tools are no longer hidden by a browser-only deny (they were already runnable at execution under that config, so this aligns the listing with execution — not a new capability). No schema/migration/UI/platform surface touched.Deliberate scope deferral: I did not promote
opencli_read/opencli_writeto first-class keys in the permission config schema. They already work through the config catchall, and adding schema keys would ripple into generated SDK artifacts (packages/sdk/openapi.json,packages/sdk/js/src/v2/gen/types.gen.ts) requiring regen + commit — out of scope for this focused fix.Skipped conditional checklist items: visible-UI item (no UI or copy changed); platform item (no Electron/packaging/updater/signing/paths surface — server-side permission-key logic).
How To Verify
Screenshots or Recordings
None — no visible UI or copy changed.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.