feat(rules): add delete-rule tool and fix edit-rule-sequence imports - #55
feat(rules): add delete-rule tool and fix edit-rule-sequence imports#55FelixAsencioEncephalon wants to merge 1 commit into
Conversation
Adds a new delete-rule MCP tool that removes an existing inbox rule
via DELETE /me/mailFolders/inbox/messageRules/{id}. Supports lookup
by name or explicit ruleId. When multiple rules share a name, the
handler returns a disambiguated list with IDs and their conditions
so the caller can re-invoke with a specific ruleId.
Also fixes a pre-existing bug in rules/index.js where the
handleEditRuleSequence function references ensureAuthenticated and
callGraphAPI without importing them at the top of the file, which
would cause edit-rule-sequence to throw ReferenceError at runtime.
Hoisting those imports next to the delete-rule imports so both work.
Closes the gap where users who accidentally create a mis-scoped rule
(e.g. a filter that's too broad) can only remove it by clicking
through the Outlook Web settings UI, which defeats the purpose of
having an MCP abstraction over mailbox rule management.
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
📝 WalkthroughWalkthroughA new delete-rule handler module is introduced that validates input parameters, authenticates users, resolves rule names to IDs via Microsoft Graph, and executes rule deletion with comprehensive error messaging for various scenarios. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Handler as Delete Rule Handler
participant Auth as Authentication
participant Graph as Microsoft Graph API
User->>Handler: Call delete-rule (ruleName or ruleId)
Handler->>Auth: Ensure authenticated
Auth-->>Handler: Authentication verified
Handler->>Graph: GET /me/mailFolders/inbox/messageRules
Graph-->>Handler: List of rules
Handler->>Handler: Resolve ruleName to ruleId
alt No matches or multiple matches
Handler-->>User: User-facing message (ID/condition list)
else Single match found
Handler->>Graph: DELETE /me/mailFolders/inbox/messageRules/{targetRuleId}
Graph-->>Handler: Deletion confirmed
Handler-->>User: Success response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 the delete-rule tool for deleting Outlook inbox rules by name or ID, including logic to resolve name conflicts. The review feedback suggests using optional chaining when mapping email addresses to improve resilience against malformed API responses and recommends implementing pagination for rule retrieval to avoid missing entries when a user has many rules.
| .map((r, i) => { | ||
| const conditions = []; | ||
| if (r.conditions?.fromAddresses?.length > 0) { | ||
| conditions.push(`from: ${r.conditions.fromAddresses.map(a => a.emailAddress.address).join(', ')}`); |
There was a problem hiding this comment.
The mapping function assumes that each object in fromAddresses has an emailAddress property with an address. While the Microsoft Graph API typically provides this structure, using optional chaining would make the code more resilient to unexpected or malformed API responses and prevent a potential runtime crash.
| conditions.push(`from: ${r.conditions.fromAddresses.map(a => a.emailAddress.address).join(', ')}`); | |
| conditions.push(`from: ${r.conditions.fromAddresses.map(a => a.emailAddress?.address || 'unknown').join(', ')}`); |
|
|
||
| // If only ruleName was provided, resolve it to an ID | ||
| if (!targetRuleId) { | ||
| const rules = await getInboxRules(accessToken); |
There was a problem hiding this comment.
The getInboxRules function (imported from ./list) currently uses a non-paginated Graph API call. If a user has a large number of inbox rules (typically more than 10-100 depending on API defaults), this tool might fail to find a rule by ruleName if it resides on a subsequent page of results. Consider updating getInboxRules in rules/list.js to use callGraphAPIPaginated to ensure all rules are searched.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rules/index.js (1)
174-187: Push the “ruleName OR ruleId required” constraint into JSON schema.The schema currently accepts an empty object and relies on runtime rejection. Adding schema-level validation gives earlier feedback and cleaner tool-contract enforcement.
Suggested fix
inputSchema: { type: "object", properties: { ruleName: { type: "string", description: "Name of the rule to delete. If multiple rules share this name, the tool will return the list and ask for a specific ID." }, ruleId: { type: "string", description: "Specific rule ID (from Microsoft Graph) to delete. Use this when multiple rules share the same name." } }, - required: [] + anyOf: [ + { required: ["ruleName"] }, + { required: ["ruleId"] } + ] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rules/index.js` around lines 174 - 187, Update the inputSchema object so the JSON Schema enforces that at least one of ruleName or ruleId is provided (instead of allowing an empty object); modify the schema (inside rules/index.js where inputSchema is defined) to keep the properties for ruleName and ruleId but add a oneOf (or anyOf) clause requiring either ["ruleName"] or ["ruleId"] so the validator fails early when both are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rules/delete.js`:
- Around line 13-15: The function handleDeleteRule destructures ruleName and
ruleId from args which will throw if args is undefined; guard by checking that
args is present (e.g., if (!args || typeof args !== 'object') return a
structured MCP error response) or use a safe default (const { ruleName, ruleId }
= args || {}) before using them; update handleDeleteRule to validate
ruleName/ruleId after guarding and return the appropriate MCP response when
missing so the handler never throws.
---
Nitpick comments:
In `@rules/index.js`:
- Around line 174-187: Update the inputSchema object so the JSON Schema enforces
that at least one of ruleName or ruleId is provided (instead of allowing an
empty object); modify the schema (inside rules/index.js where inputSchema is
defined) to keep the properties for ruleName and ruleId but add a oneOf (or
anyOf) clause requiring either ["ruleName"] or ["ruleId"] so the validator fails
early when both are missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0400c32a-0bfc-492c-bb83-2191be5622d6
📒 Files selected for processing (2)
rules/delete.jsrules/index.js
| async function handleDeleteRule(args) { | ||
| const { ruleName, ruleId } = args; | ||
|
|
There was a problem hiding this comment.
Guard args before destructuring to avoid runtime throw.
On Line 14, destructuring from args will throw when the tool is invoked without arguments, so the handler never returns a structured MCP response.
Suggested fix
-async function handleDeleteRule(args) {
- const { ruleName, ruleId } = args;
+async function handleDeleteRule(args = {}) {
+ const { ruleName, ruleId } = args;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function handleDeleteRule(args) { | |
| const { ruleName, ruleId } = args; | |
| async function handleDeleteRule(args = {}) { | |
| const { ruleName, ruleId } = args; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rules/delete.js` around lines 13 - 15, The function handleDeleteRule
destructures ruleName and ruleId from args which will throw if args is
undefined; guard by checking that args is present (e.g., if (!args || typeof
args !== 'object') return a structured MCP error response) or use a safe default
(const { ruleName, ruleId } = args || {}) before using them; update
handleDeleteRule to validate ruleName/ruleId after guarding and return the
appropriate MCP response when missing so the handler never throws.
2 participants
Summary
Adds a new
delete-ruleMCP tool that removes an existing inbox rule viaDELETE /me/mailFolders/inbox/messageRules/{id}. Also fixes a pre-existing ReferenceError bug inrules/index.jsthat would crashedit-rule-sequenceat runtime.Motivation
ryaker/outlook-mcpcurrently exposescreate-rule,list-rules, andedit-rule-sequencebut notdelete-rule. This creates a trap: if a caller accidentally creates a mis-scoped rule (e.g. a filter that's too broad and catches legitimate mail), the only way to remove it is to click through the Outlook Web settings UI — which defeats the purpose of having an MCP abstraction over mailbox rule management.I hit this in real use: I created a rule that matched all emails from `no-reply@substack.com` when I actually wanted it scoped to specific subject patterns. Without `delete-rule`, I had to either manually clean up in Outlook or reach past the MCP to call Graph API directly.
What's in this PR
1. New `delete-rule` tool (`rules/delete.js`)
2. Bug fix in `rules/index.js`
`handleEditRuleSequence` uses `ensureAuthenticated` and `callGraphAPI` but these aren't imported at the top of the file. Anyone calling `edit-rule-sequence` would get a `ReferenceError: ensureAuthenticated is not defined` at runtime. Hoisted the imports alongside the new `delete-rule` imports so both paths work.
Testing
Tool description tradeoff
The tool description explicitly warns about the multi-match → single-match transition: if you delete one of two duplicate-named rules, the remaining one can be deleted by the same `ruleName` call that previously returned a disambiguated list. This is documented so MCP clients don't use delete-rule as a verification probe after a delete.
Files changed
No dependencies added. No breaking changes.
🤖 Generated with Claude Code
Summary by CodeRabbit