ongoing Changes - #1592
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe PR adds a highest-priority Cedar deny rule for unknown MCP tool calls, tags that rule with ChangesUnknown MCP tool authorization
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 introduces a new forbid rule mcp-unknown-tool-forbid to deny unknown MCP tools by default across several Cedar policy files. It also updates the policy engine in lib/policy/src/cedar.rs to prefer using the policy's @id annotation as a stable identifier for matched policies instead of positional IDs. Feedback suggests optimizing the policy lookup in cedar.rs to avoid querying the policy set twice for the same ID.
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.
| let matched_name = policy_set | ||
| .policy(policy_id) | ||
| .and_then(|p| p.annotation("id")) | ||
| .map(|a| a.trim_matches('"').to_string()) | ||
| .unwrap_or_else(|| policy_id.to_string()); | ||
| matched_policies.push(matched_name); | ||
| if let Some(policy) = policy_set.policy(policy_id) { |
There was a problem hiding this comment.
The policy is looked up twice in policy_set for each matched policy ID: once to retrieve the @id annotation (lines 273-274) and once to perform decision escalation (line 279). We can optimize this by performing the lookup once and reusing the retrieved policy reference.
| let matched_name = policy_set | |
| .policy(policy_id) | |
| .and_then(|p| p.annotation("id")) | |
| .map(|a| a.trim_matches('"').to_string()) | |
| .unwrap_or_else(|| policy_id.to_string()); | |
| matched_policies.push(matched_name); | |
| if let Some(policy) = policy_set.policy(policy_id) { | |
| let policy = policy_set.policy(policy_id); | |
| let matched_name = policy | |
| .and_then(|p| p.annotation("id")) | |
| .map(|a| a.trim_matches('"').to_string()) | |
| .unwrap_or_else(|| policy_id.to_string()); | |
| matched_policies.push(matched_name); | |
| if let Some(policy) = policy { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/policy/src/cedar.rs (1)
273-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fold the duplicate
policy_set.policy(policy_id)lookup.
policy_set.policy(policy_id)is resolved at Line 274 formatched_nameand again at Line 279 for the annotation-escalation block. Resolve it once and reuse the reference.♻️ Resolve the policy once per matched id
for policy_id in response.diagnostics().reason() { + let matched_policy = policy_set.policy(policy_id); // Prefer the policy's `@id` annotation as the matched-policy name so // callers and SOC detection rules see a stable identifier (e.g. // `mcp_unknown_tool`) rather than Cedar's positional `policyN`, // which shifts as rules are added/reordered. Falls back to the // Cedar policy id when no `@id` annotation is present. - let matched_name = policy_set - .policy(policy_id) + let matched_name = matched_policy .and_then(|p| p.annotation("id")) .map(|a| a.trim_matches('"').to_string()) .unwrap_or_else(|| policy_id.to_string()); matched_policies.push(matched_name); - if let Some(policy) = policy_set.policy(policy_id) { + if let Some(policy) = matched_policy {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/policy/src/cedar.rs` around lines 273 - 279, Resolve the duplicate policy lookup in the matching flow by fetching policy_set.policy(policy_id) once and reusing the same reference for both matched_name construction and the annotation-escalation logic. Update the code around the matched_policies push and the subsequent if let Some(policy) block in cedar.rs so the policy is bound once, then use that binding for annotation("id") and the later annotation checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/policy/src/cedar.rs`:
- Around line 273-279: Resolve the duplicate policy lookup in the matching flow
by fetching policy_set.policy(policy_id) once and reusing the same reference for
both matched_name construction and the annotation-escalation logic. Update the
code around the matched_policies push and the subsequent if let Some(policy)
block in cedar.rs so the policy is bound once, then use that binding for
annotation("id") and the later annotation checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84ee4413-cbac-4bd7-b6d7-2dda1d0aca91
📒 Files selected for processing (6)
aegis.db-shmaegis.db-walhelm/aegis-gateway/files/policies.cedarlib/policy/src/cedar.rspolicies.cedarsrc/policies.cedar
…y rule (#1789) * ongoing Changes * fix(policy): dedupe mcp_unknown_tool in matched_policies The unknown-MCP-tool forbid rule already existed on main. Cherry-picking #1592's one real commit onto a fresh branch surfaced that its @id-annotation naming addition duplicated an existing lookup block, so matched_policies contained "mcp_unknown_tool" twice whenever that rule fired. Remove the redundant block and add a regression test asserting exactly one occurrence.
1 participant
Summary
Closes #
Type of change
Checklist
cargo test --workspaceandpython3 -m unittest discover -s sdk-python/testspass.cargo fmt -- --checkandcargo clippy --workspace --all-targets -- -D warningspass.python3 -m black --check sdk-python/ examples/passes.tenant_id; parameterized SQL only.feat:,fix:,docs:, etc.).Integrity invariants (do not weaken)
Notes for reviewers
Summary by CodeRabbit
New Features
Bug Fixes
Chores