Repository navigation
fix(folder): support subfolder paths in move-emails via getFolderIdByName path resolution - #71
rafaga2469 wants to merge 3 commits into
Conversation
…h resolution Enhance getFolderIdByName() to split folder names on '/' and traverse the Graph folder hierarchy level-by-level using childFolders endpoints. Backwards compatible: flat folder names (no '/') use identical code path. - Add resolveSegmentInParent() helper for per-segment exact + case-insensitive fallback - Rewrite getFolderIdByName() with path splitting, empty segment filtering, short-circuit on null - Add 11 unit tests covering all 8 spec scenarios (142/142 total passing) - Extend mock-data.js with CHILD_FOLDERS map for test-mode server - Restore token-storage.js warning text to match existing test expectation - Add OpenSpec SDD artifacts (proposal, spec, design, tasks, verify, archive) Closes ryaker#67, closes ryaker#68
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds hierarchical Outlook folder-path resolution with exact and case-insensitive segment matching across child folders. It also adds tests, mocks, specifications, verification reports, environment loading, configuration updates, a dependency update, and skill registry metadata. ChangesFolder resolution
Project setup and metadata
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant MoveEmails
participant FolderUtils
participant GraphAPI
MoveEmails->>FolderUtils: resolve target folder path
FolderUtils->>GraphAPI: query top-level folder segment
GraphAPI-->>FolderUtils: parent folder ID
FolderUtils->>GraphAPI: query child-folder segment
GraphAPI-->>FolderUtils: target folder ID
FolderUtils-->>MoveEmails: folder ID or null
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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.
Inline comments:
In @.atl/skill-registry.md:
- Around line 4-5: Remove the machine-specific user-level path and username from
the skill registry, including the entries covered by lines 9-82. Replace them
with portable skill identifiers or project-relative paths; if the registry must
represent user-level skills, generate that data outside version control.
In `@auth/token-storage.js`:
- Line 31: Update the warning in TokenStorage to reference both supported
credential variable pairs, MS_CLIENT_ID/MS_CLIENT_SECRET and
OUTLOOK_CLIENT_ID/OUTLOOK_CLIENT_SECRET, so it accurately reflects fallback
configuration options. Update the corresponding exact-string test to match the
revised warning text.
In `@email/folder-utils.js`:
- Around line 69-105: Update resolveSegmentInParent to escape apostrophes in
segment by replacing each single quote with two single quotes before
constructing the OData displayName filter. Use the escaped value only for the
exact-match request, while preserving the existing case-insensitive fallback and
original segment comparison behavior.
In `@openspec/specs/email/spec.md`:
- Around line 9-11: Update the Path Resolution requirement for getFolderIdByName
and resolveSegmentInParent to specify pagination when folder lookups exceed the
initial 100 results, covering both top-level and childFolders queries;
alternatively, explicitly document the $top: 100 limitation if that is the
intended contract.
- Around line 71-79: Update resolveSegmentInParent() so folder-name segments are
escaped as valid OData string literals before being interpolated into the
displayName filter, including apostrophes such as in “Bob’s”. Preserve
slash-based path interpretation and add or update the specification scenario to
document and test the chosen behavior.
In `@package.json`:
- Line 34: Update the package.json engines.node constraint to require Node.js
>=22.7.5, matching the `@modelcontextprotocol/inspector` dependency version, while
preserving the existing inspector dependency declaration.
In `@utils/mock-data.js`:
- Around line 123-144: Update the mock mail-folder response logic in the path
resolver to honor queryParams.$filter when selecting top-level folders,
returning only the matching folder instead of the entire collection. Extend
CHILD_FOLDERS with the documented tramite hierarchy, including the REQ-104951
child, so Tramite/REQ-104951 resolution works in test mode while preserving
existing inbox behavior.
🪄 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 Plus
Run ID: 60396948-a954-4bef-8af7-826e376652d2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
.atl/.skill-registry.cache.json.atl/skill-registry.mdauth/token-storage.jsemail/folder-utils.jsindex.jsopenspec/changes/archive/2026-07-28-fix-subfolder-move-emails/archive-report.mdopenspec/changes/archive/2026-07-28-fix-subfolder-move-emails/design.mdopenspec/changes/archive/2026-07-28-fix-subfolder-move-emails/exploration.mdopenspec/changes/archive/2026-07-28-fix-subfolder-move-emails/proposal.mdopenspec/changes/archive/2026-07-28-fix-subfolder-move-emails/specs/email/spec.mdopenspec/changes/archive/2026-07-28-fix-subfolder-move-emails/tasks.mdopenspec/changes/archive/2026-07-28-fix-subfolder-move-emails/verify-report.mdopenspec/config.yamlopenspec/specs/email/spec.mdpackage.jsontest/email/folder-utils.test.jsutils/mock-data.js
| Scope: user-level skills (D:\Usuarios\ricardo.pinto\.config\opencode\skills) | ||
| Project-level skills: none detected |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not commit machine-specific user-level paths.
The registry exposes the local username ricardo.pinto and hard-codes Windows paths that will be invalid for other developers and CI. Store portable skill identifiers/project-relative paths, or generate user-level registry data outside version control.
Also applies to: 9-82
🤖 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 @.atl/skill-registry.md around lines 4 - 5, Remove the machine-specific
user-level path and username from the skill registry, including the entries
covered by lines 9-82. Replace them with portable skill identifiers or
project-relative paths; if the registry must represent user-level skills,
generate that data outside version control.
|
|
||
| if (!this.config.clientId || !this.config.clientSecret) { | ||
| console.warn("TokenStorage: Client ID or Secret is not configured (checked MS_CLIENT_ID/OUTLOOK_CLIENT_ID). Token refresh will fail."); | ||
| console.warn("TokenStorage: MS_CLIENT_ID or MS_CLIENT_SECRET is not configured. Token operations might fail."); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the warning consistent with supported credential fallbacks.
TokenStorage also accepts OUTLOOK_CLIENT_ID and OUTLOOK_CLIENT_SECRET at Lines 11-13, but this warning names only MS_*. When fallback credentials are incomplete, the message points users toward the wrong variable names. Update the warning and its exact-string test accordingly.
Proposed wording
- console.warn("TokenStorage: MS_CLIENT_ID or MS_CLIENT_SECRET is not configured. Token operations might fail.");
+ console.warn("TokenStorage: no supported client ID/secret pair is configured (MS_CLIENT_ID/MS_CLIENT_SECRET or OUTLOOK_CLIENT_ID/OUTLOOK_CLIENT_SECRET). Token operations might fail.");📝 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.
| console.warn("TokenStorage: MS_CLIENT_ID or MS_CLIENT_SECRET is not configured. Token operations might fail."); | |
| console.warn("TokenStorage: no supported client ID/secret pair is configured (MS_CLIENT_ID/MS_CLIENT_SECRET or OUTLOOK_CLIENT_ID/OUTLOOK_CLIENT_SECRET). Token operations might fail."); |
🤖 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 `@auth/token-storage.js` at line 31, Update the warning in TokenStorage to
reference both supported credential variable pairs,
MS_CLIENT_ID/MS_CLIENT_SECRET and OUTLOOK_CLIENT_ID/OUTLOOK_CLIENT_SECRET, so it
accurately reflects fallback configuration options. Update the corresponding
exact-string test to match the revised warning text.
| ### Requirement: Path Resolution | ||
|
|
||
| `getFolderIdByName()` MUST resolve folder paths separated by `/` by traversing the folder hierarchy level by level. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^openspec/specs/email/spec\.md$|email/folder-utils\.js$|email/.*test.*\.js$|test.*email.*\.js$)' || true
echo
echo "spec.md section:"
if [ -f openspec/specs/email/spec.md ]; then
cat -n openspec/specs/email/spec.md
fi
echo
echo "folder-utils outline:"
if [ -f email/folder-utils.js ]; then
ast-grep outline email/folder-utils.js || true
echo "--- folder-utils.js ---"
cat -n email/folder-utils.js
fiRepository: ryaker/outlook-mcp
Length of output: 11190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for getFolderIdByName usages/tests/definitions:"
rg -n "getFolderIdByName|nextLink|odata\.nextLink|\\$top|/pages/|folder-utils" . --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo
echo "Diff stat/name-only if available:"
git diff --stat || true
git diff -- openspec/specs/email/spec.md email/folder-utils.js || trueRepository: ryaker/outlook-mcp
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for getFolderIdByName usages/tests/definitions:"
rg -n "getFolderIdByName|nextLink|odata\.nextLink|\$top|childFolders|folder-utils" . --glob '!node_modules' --glob '!dist' --glob '!build' || true
echo
echo "Tests around folder resolution:"
if [ -f test/email/folder-utils.test.js ]; then
cat -n test/email/folder-utils.test.js
fi
echo
if [ -f test/email/list.test.js ]; then
cat -n test/email/list.test.js
fiRepository: ryaker/outlook-mcp
Length of output: 50374
Specify pagination for folder lookups.
resolveSegmentInParent() limits the case-insensitive fallback to $top: 100, so a valid segment beyond the first page is not resolvable for both top-level and childFolders lookups. Contract this as paginated or add a limitation that $top: 100 is sufficient.
🤖 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 `@openspec/specs/email/spec.md` around lines 9 - 11, Update the Path Resolution
requirement for getFolderIdByName and resolveSegmentInParent to specify
pagination when folder lookups exceed the initial 100 results, covering both
top-level and childFolders queries; alternatively, explicitly document the $top:
100 limitation if that is the intended contract.
| }, | ||
| "devDependencies": { | ||
| "@modelcontextprotocol/inspector": "^0.10.2", | ||
| "@modelcontextprotocol/inspector": "^0.22.0", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '"`@modelcontextprotocol/inspector`"|inspector' \
package.json package-lock.json npm-shrinkwrap.json 2>/dev/null || true
npm view `@modelcontextprotocol/inspector`@0.22.0 version peerDependencies enginesRepository: ryaker/outlook-mcp
Length of output: 2353
🌐 Web query:
@modelcontextprotocol/inspector 0.22.0 breaking changes package entry points CLI
💡 Result:
There are no intended breaking changes regarding package entry points or CLI functionality in version 0.22.0 of @modelcontextprotocol/inspector; the release was primarily a version synchronization of the monorepo packages [1][2]. However, users of the inspector CLI encountered a persistent functional defect (ERR_MODULE_NOT_FOUND) introduced in version 0.17.0 and remaining in 0.22.0 [3]. This issue was caused by an attempt to import the CLI's package.json file, which was excluded from the published npm tarball because it was missing from the root package's files array [4][3]. Key details regarding this state include: 1. Scope of Impact: The CLI functionality via npx @modelcontextprotocol/inspector --cli remained broken in version 0.22.0 due to this packaging configuration error [4][3]. 2. Resolution: A fix was identified (PR #1506) to include cli/package.json in the files array of the root package.json, ensuring the file is included in future published npm tarballs [4][3]. 3. CLI Evolution: While not a breaking change in 0.22.0, the project has been actively planning for a CLI v2, which intends to introduce mcp as a primary command entry point that functions similarly to the existing launcher dispatch logic [5]. Official documentation and releases confirm that 0.22.0 was intended as a version bump without functional feature changes to the CLI [2].
Citations:
- 1: https://github.com/modelcontextprotocol/inspector/releases/tag/0.22.0
- 2: chore: bump version to 0.22.0 modelcontextprotocol/inspector#1428
- 3: CLI broken since v0.17.0: ERR_MODULE_NOT_FOUND for cli/package.json modelcontextprotocol/inspector#1503
- 4: fix: include cli/package.json in published npm files modelcontextprotocol/inspector#1506
- 5: Inspector mcpdo client modelcontextprotocol/inspector#1432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "node constraint files:"
for f in package.json engines.json .node-version .tool-versions nvmrc Dockerfile docker-compose.yml; do
[ -e "$f" ] && echo "--- $f ---" && sed -n '1,160p' "$f"
done
echo
echo "inspector CLI entrypoint declarations:"
node - <<'JS'
const fs = require('fs');
for (const name of ['node_modules/@modelcontextprotocol/inspector/package.json','node_modules/@modelcontextprotocol/inspector-cli/package.json']) {
if (fs.existsSync(name)) {
const p = JSON.parse(fs.readFileSync(name, 'utf8'));
console.log(name);
console.log({ bin: p.bin, main: p.main, type: p.type, files: p.files });
}
}
JSRepository: ryaker/outlook-mcp
Length of output: 1223
Set the Node constraint for the upgraded inspector.
package.json still declares engines.node: ">=14.0.0", while @modelcontextprotocol/inspector@0.22.0 requires node: ">=22.7.5". This can be caught by future CI or local usage before running npm run inspect. Update engines.node or add an explicit Node 22.7.5 requirement.
🤖 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 `@package.json` at line 34, Update the package.json engines.node constraint to
require Node.js >=22.7.5, matching the `@modelcontextprotocol/inspector`
dependency version, while preserving the existing inspector dependency
declaration.
| // Child folder lookup support for path-style folder references | ||
| const CHILD_FOLDERS = { | ||
| "inbox": [ | ||
| { id: "inbox-child", displayName: "InboxChild" } | ||
| ] | ||
| }; | ||
|
|
||
| if (path.includes('childFolders')) { | ||
| const parentId = path.split('/').find((segment, index, parts) => { | ||
| return parts[index - 1] === 'mailFolders' && parts[index + 1] === 'childFolders'; | ||
| }); | ||
| return { value: CHILD_FOLDERS[parentId] || [] }; | ||
| } | ||
|
|
||
| // Simulate a mail folders response | ||
| return { | ||
| value: [ | ||
| { id: "inbox", displayName: "Inbox" }, | ||
| { id: "drafts", displayName: "Drafts" }, | ||
| { id: "sentItems", displayName: "Sent Items" }, | ||
| { id: "deleteditems", displayName: "Deleted Items" } | ||
| { id: "deleteditems", displayName: "Deleted Items" }, | ||
| { id: "tramite", displayName: "Tramite" } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make test-mode responses honor $filter and model the documented hierarchy.
resolveSegmentInParent() treats any non-empty response as an exact match, but this mock returns every top-level folder regardless of queryParams.$filter; Tramite therefore resolves to Inbox. It also has no tramite children, so the documented Tramite/REQ-104951 test-mode flow cannot work.
Proposed fix
+ const TOP_LEVEL_FOLDERS = [
+ { id: "inbox", displayName: "Inbox" },
+ { id: "drafts", displayName: "Drafts" },
+ { id: "sentItems", displayName: "Sent Items" },
+ { id: "deleteditems", displayName: "Deleted Items" },
+ { id: "tramite", displayName: "Tramite" }
+ ];
const CHILD_FOLDERS = {
"inbox": [
{ id: "inbox-child", displayName: "InboxChild" }
+ ],
+ "tramite": [
+ { id: "req-104951-id", displayName: "REQ-104951" }
]
};
- if (path.includes('childFolders')) {
+ const folders = path.includes('childFolders')
+ ? (() => {
const parentId = path.split('/').find((segment, index, parts) => {
return parts[index - 1] === 'mailFolders' && parts[index + 1] === 'childFolders';
});
- return { value: CHILD_FOLDERS[parentId] || [] };
- }
-
- // Simulate a mail folders response
- return {
- value: [
- { id: "inbox", displayName: "Inbox" },
- { id: "drafts", displayName: "Drafts" },
- { id: "sentItems", displayName: "Sent Items" },
- { id: "deleteditems", displayName: "Deleted Items" },
- { id: "tramite", displayName: "Tramite" }
- ]
- };
+ return CHILD_FOLDERS[parentId] || [];
+ })()
+ : TOP_LEVEL_FOLDERS;
+ const requestedName = queryParams.$filter
+ ?.match(/^displayName eq '((?:''|[^'])*)'$/)?.[1]
+ ?.replace(/''/g, "'");
+ return {
+ value: requestedName
+ ? folders.filter(folder => folder.displayName === requestedName)
+ : folders
+ };📝 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.
| // Child folder lookup support for path-style folder references | |
| const CHILD_FOLDERS = { | |
| "inbox": [ | |
| { id: "inbox-child", displayName: "InboxChild" } | |
| ] | |
| }; | |
| if (path.includes('childFolders')) { | |
| const parentId = path.split('/').find((segment, index, parts) => { | |
| return parts[index - 1] === 'mailFolders' && parts[index + 1] === 'childFolders'; | |
| }); | |
| return { value: CHILD_FOLDERS[parentId] || [] }; | |
| } | |
| // Simulate a mail folders response | |
| return { | |
| value: [ | |
| { id: "inbox", displayName: "Inbox" }, | |
| { id: "drafts", displayName: "Drafts" }, | |
| { id: "sentItems", displayName: "Sent Items" }, | |
| { id: "deleteditems", displayName: "Deleted Items" } | |
| { id: "deleteditems", displayName: "Deleted Items" }, | |
| { id: "tramite", displayName: "Tramite" } | |
| // Child folder lookup support for path-style folder references | |
| const TOP_LEVEL_FOLDERS = [ | |
| { id: "inbox", displayName: "Inbox" }, | |
| { id: "drafts", displayName: "Drafts" }, | |
| { id: "sentItems", displayName: "Sent Items" }, | |
| { id: "deleteditems", displayName: "Deleted Items" }, | |
| { id: "tramite", displayName: "Tramite" } | |
| ]; | |
| const CHILD_FOLDERS = { | |
| "inbox": [ | |
| { id: "inbox-child", displayName: "InboxChild" } | |
| ], | |
| "tramite": [ | |
| { id: "req-104951-id", displayName: "REQ-104951" } | |
| ] | |
| }; | |
| const folders = path.includes('childFolders') | |
| ? (() => { | |
| const parentId = path.split('/').find((segment, index, parts) => { | |
| return parts[index - 1] === 'mailFolders' && parts[index + 1] === 'childFolders'; | |
| }); | |
| return CHILD_FOLDERS[parentId] || []; | |
| })() | |
| : TOP_LEVEL_FOLDERS; | |
| const requestedName = queryParams.$filter | |
| ?.match(/^displayName eq '((?:''|[^'])*)'$/)?.[1] | |
| ?.replace(/''/g, "'"); | |
| return { | |
| value: requestedName | |
| ? folders.filter(folder => folder.displayName === requestedName) | |
| : folders | |
| }; |
🤖 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 `@utils/mock-data.js` around lines 123 - 144, Update the mock mail-folder
response logic in the path resolver to honor queryParams.$filter when selecting
top-level folders, returning only the matching folder instead of the entire
collection. Extend CHILD_FOLDERS with the documented tramite hierarchy,
including the REQ-104951 child, so Tramite/REQ-104951 resolution works in test
mode while preserving existing inbox behavior.
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
1 participant
Linked Issues
Closes #67
Closes #68
Summary
move-emailsfails when targeting subfolders (e.g.Tramite/REQ-104951) becausegetFolderIdByName()only searches top-level folders viame/mailFolders?$filter=displayName eq '{name}'getFolderIdByName()to split folder paths on/and traverse the Graph folder hierarchy level-by-level usingme/mailFolders/{parentId}/childFoldersendpoints/) use the identical code path as before — zero blast radius for existing callersChanges
email/folder-utils.jsresolveSegmentInParent()helper; rewrotegetFolderIdByName()with path splitting, empty segment filtering, and short-circuit on nulltest/email/folder-utils.test.jsutils/mock-data.jsCHILD_FOLDERSmock map andchildFoldersbranch for test-mode serverauth/token-storage.jsHow It Works
Each segment has a case-insensitive fallback (fetch all, match by
toLowerCase()) matching the existing top-level pattern.Test Plan
npm test) — 24 in folder-utils, 118 across other modules//), all-empty path, API error mid-traversal, helper exact match, helper case-insensitive fallbackKnown Limitations
/in their display name are not supported —/is treated exclusively as a path separator. This is documented in the spec and is extremely rare in practice.Contributor Checklist
fix(folder): ...)Co-Authored-BytrailersSummary by CodeRabbit
.envfile at startup.