From de2bfaf6e0bec07beaaf482e48fbb7634d4382ca Mon Sep 17 00:00:00 2001 From: Neil Foster Date: Thu, 23 Jul 2026 06:49:06 +0000 Subject: [PATCH 1/2] fix: resolve well-known source folder names to real ids in searchfolder-create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Graph's mailSearchFolder creation API does not accept a well-known folder name (e.g. "inbox") verbatim in sourceFolderIds the way destinationId fields elsewhere in this codebase do — it silently produces a search folder with zero source folders, whose filter can never match any mail regardless of how much matching mail exists. Well-known names (including the documented `inbox` default) are now resolved to real Graph folder ids via GET /me/mailFolders/{name} before creation, and the verb verifies the created folder's sourceFolderIds actually applied, refusing loudly instead of reporting a false success if it comes back short. Fixes #21. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_011NWPnfcJ9JXDvRThCCUcCH --- .gitattributes | 1 + .gitignore | 5 ++ CHANGELOG.md | 11 +++ plugin/skills/searchfolder-create/SKILL.md | 8 +- plugin/src/msgraph/graph.py | 23 +++++ plugin/src/msgraph/verbs.py | 18 ++-- tests/test_client.py | 97 +++++++++++++++++++++- tests/test_url_construction.py | 6 ++ 8 files changed, 158 insertions(+), 11 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..0c5990d --- /dev/null +++ b/.gitattributes @@ -0,0 +1 @@ +.specify/feature.json merge=ours diff --git a/.gitignore b/.gitignore index 83ee508..10712ab 100644 --- a/.gitignore +++ b/.gitignore @@ -43,3 +43,8 @@ specs/ .github/agents/speckit* .github/prompts/speckit* # --- end tredl --- +# --- kord: GitHub Spec Kit substrate --- +.specify/ +specs/ +.claude/skills/speckit-*/ +# --- end kord: GitHub Spec Kit substrate --- diff --git a/CHANGELOG.md b/CHANGELOG.md index cef94ab..4764e8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,17 @@ Add notes here under Added / Changed / Fixed / Removed. On release, move them un ## [X.Y.Z] - YYYY-MM-DD heading and bump plugin/.claude-plugin/plugin.json to match. --> +### Fixed + +- **`searchfolder-create`'s documented `--source_folders` default was inert** — well-known folder + names (including the `inbox` default) were passed to Graph verbatim as strings in + `sourceFolderIds`, mirroring the `destinationId` shortcut used elsewhere in this codebase. Graph + does not document (or honor) that shortcut for `sourceFolderIds`: it accepted the creation + request but silently produced a search folder with zero source folders — a filter that could + structurally never match any mail. Well-known names are now resolved to real Graph folder ids + before creation, and the verb now verifies the created folder's `sourceFolderIds` actually + applied, refusing loudly (rather than reporting a false success) if it comes back short. + ## [0.6.1] - 2026-06-27 ### Fixed diff --git a/plugin/skills/searchfolder-create/SKILL.md b/plugin/skills/searchfolder-create/SKILL.md index b458bff..b5eab6d 100644 --- a/plugin/skills/searchfolder-create/SKILL.md +++ b/plugin/skills/searchfolder-create/SKILL.md @@ -27,7 +27,11 @@ structurally. That separation is the scope ratchet, the heart of the safety mode **Filter.** `--category "Needs attention"` builds `categories/any(c:c eq 'Needs attention')` for you; or pass `--filter_query` to supply any OData filter directly (it overrides `--category`). `--source_folders` accepts well-known names (`inbox`, `archive`, …) or folder display names; default -is `inbox`. `--include_nested true` (default) deep-searches subfolders. +is `inbox`. Every name (well-known or not) is resolved to a real Graph folder id before creation — +Graph does not honor a well-known name placed verbatim in `sourceFolderIds`, so this verb never +creates a folder whose filter could structurally never match anything. If Graph's response ever +comes back with fewer applied source folders than requested, the verb refuses and reports an error +instead of a false success. `--include_nested true` (default) deep-searches subfolders. ## Typical flow @@ -67,4 +71,6 @@ moved or deleted. Remove anytime with searchfolder-remove. error: This action needs … run /msgraph-auth-login --mode folders. error: Refusing to create this search folder: no filter given. Pass --category NAME or --filter_query. error: No mail folder named 'X' was found. Create it in Outlook first, or pass an existing folder name. +error: Created search folder "X" (id: ...), but Graph reports only 0 of 1 requested source folder(s) +applied — its filter may never match any mail. Remove it with searchfolder-remove --folder_id ... ``` diff --git a/plugin/src/msgraph/graph.py b/plugin/src/msgraph/graph.py index 0fe826f..58aceae 100644 --- a/plugin/src/msgraph/graph.py +++ b/plugin/src/msgraph/graph.py @@ -28,6 +28,29 @@ def _resolve_folder_id(token: str, name: str) -> str: ) +def _resolve_source_folder_id(token: str, name: str) -> str: + """Resolve a --source_folders entry to a REAL Graph folder id (searchfolder-create, issue #21). + + Unlike _resolve_folder_id (used for destinationId-style fields, where Graph documents accepting + a well-known name verbatim), mailSearchFolder's sourceFolderIds is documented as a plain folder- + id string collection with no such carve-out: passing a well-known name there silently produces + an empty sourceFolderIds on the created folder, leaving its filter unable to ever match anything. + Well-known names are resolved via GET /me/mailFolders/{name} (Graph accepts a well-known name in + that position) to obtain the real id; anything else uses the existing display-name resolution. + """ + if name.casefold() in _WELL_KNOWN_FOLDERS: + folder = runtime._graph_get(token, f"/me/mailFolders/{name.casefold()}") + fid = folder.get("id") + if not fid: + raise runtime.SteerError( + f"Could not resolve the well-known folder '{name}' to a real folder id — Graph's " + f"response had none. Refusing to create a search folder that could never match " + f"any mail." + ) + return fid + return _resolve_folder_id(token, name) + + # Well-known folder names Graph accepts verbatim as a destinationId (no lookup needed). _WELL_KNOWN_FOLDERS = { "inbox", diff --git a/plugin/src/msgraph/verbs.py b/plugin/src/msgraph/verbs.py index 27fe61f..65d7e9b 100644 --- a/plugin/src/msgraph/verbs.py +++ b/plugin/src/msgraph/verbs.py @@ -422,12 +422,9 @@ def cmd_searchfolder_create(args) -> int: "(builds a category filter) or an explicit --filter_query (OData)." ) source_names = args.source_folders or ["inbox"] - # Well-known names (inbox, archive, …) are accepted verbatim by Graph; resolve any others to ids. - _WELL_KNOWN = {"inbox", "archive", "drafts", "sentitems", "deleteditems", "junkemail", "msgfolderroot"} - source_ids = [ - n if n.casefold() in _WELL_KNOWN else graph._resolve_folder_id(tok["access_token"], n) - for n in source_names - ] + # sourceFolderIds needs REAL folder ids — Graph does not accept a well-known name verbatim + # there the way it does for destinationId-style fields (issue #21); resolve every entry. + source_ids = [graph._resolve_source_folder_id(tok["access_token"], n) for n in source_names] body = { "@odata.type": "microsoft.graph.mailSearchFolder", "displayName": args.name, @@ -441,6 +438,15 @@ def cmd_searchfolder_create(args) -> int: token=tok["access_token"], body=body, ) + applied_ids = created.get("sourceFolderIds") or [] + if len(applied_ids) < len(source_ids): + raise runtime.SteerError( + f'Created search folder "{args.name}" (id: {created.get("id", "?")}), but Graph reports ' + f"only {len(applied_ids)} of {len(source_ids)} requested source folder(s) applied — its " + f"filter may never match any mail. Remove it with searchfolder-remove " + f"--folder_id {created.get('id', '?')} and retry, or inspect with searchfolder-list " + f"--format detailed." + ) print( f'Created search folder "{args.name}" (id: {created.get("id", "?")}). It presents a filtered ' f"view (filter: {filter_query}) over {source_names}; it is virtual — no mail is moved or " diff --git a/tests/test_client.py b/tests/test_client.py index 2ee7058..8549e80 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -793,7 +793,13 @@ def test_create_refuses_without_filter(self): def test_create_shapes_body_with_odata_type(self): self._sign_in("Mail.ReadWrite MailboxSettings.Read offline_access") - rec = _HttpRecorder(lambda method, url, **kw: {"id": "sf-new"}) + + def respond(method, url, **kw): + if method == "GET": + return {"id": "AAMk-inbox-real-id"} + return {"id": "sf-new", "sourceFolderIds": ["AAMk-inbox-real-id"]} + + rec = _HttpRecorder(respond) runtime._http = rec self._capture( client.cmd_searchfolder_create, @@ -810,12 +816,23 @@ def test_create_shapes_body_with_odata_type(self): body = post[3] self.assertEqual(body["@odata.type"], "microsoft.graph.mailSearchFolder") self.assertEqual(body["includeNestedFolders"], True) - self.assertEqual(body["sourceFolderIds"], ["inbox"]) # well-known name passed through + # A well-known name is resolved to a REAL folder id — never passed through verbatim + # (issue #21: Graph silently drops a verbatim well-known name from sourceFolderIds). + self.assertEqual(body["sourceFolderIds"], ["AAMk-inbox-real-id"]) self.assertEqual(body["filterQuery"], "categories/any(c:c eq 'Needs attention')") + # The resolution GET hit the well-known-name path, not a display-name search. + get_call = next(c for c in rec.calls if c[0] == "GET") + self.assertTrue(get_call[1].endswith("/me/mailFolders/inbox")) def test_create_explicit_filter_overrides_category(self): self._sign_in("Mail.ReadWrite MailboxSettings.Read offline_access") - rec = _HttpRecorder(lambda method, url, **kw: {"id": "sf"}) + + def respond(method, url, **kw): + if method == "GET": + return {"id": "AAMk-inbox-real-id"} + return {"id": "sf", "sourceFolderIds": ["AAMk-inbox-real-id"]} + + rec = _HttpRecorder(respond) runtime._http = rec self._capture( client.cmd_searchfolder_create, @@ -830,7 +847,79 @@ def test_create_explicit_filter_overrides_category(self): body = next(c for c in rec.calls if c[0] == "POST")[3] self.assertEqual(body["filterQuery"], "hasAttachments eq true") self.assertEqual(body["includeNestedFolders"], False) - self.assertEqual(body["sourceFolderIds"], ["inbox"]) # default + # Default (omitted --source_folders) resolves the same way as an explicit "inbox". + self.assertEqual(body["sourceFolderIds"], ["AAMk-inbox-real-id"]) + + def test_create_explicit_wellknown_matches_default(self): + # Acceptance Scenario 2: --source_folders inbox behaves identically to the default. + self._sign_in("Mail.ReadWrite MailboxSettings.Read offline_access") + + def respond(method, url, **kw): + if method == "GET": + return {"id": "AAMk-inbox-real-id"} + return {"id": "sf", "sourceFolderIds": ["AAMk-inbox-real-id"]} + + rec = _HttpRecorder(respond) + runtime._http = rec + self._capture( + client.cmd_searchfolder_create, + _Args( + name="Needs attention (explicit)", + category="Needs attention", + filter_query=None, + source_folders=["inbox"], + include_nested=True, + ), + ) + body = next(c for c in rec.calls if c[0] == "POST")[3] + self.assertEqual(body["sourceFolderIds"], ["AAMk-inbox-real-id"]) + + def test_create_resolves_non_wellknown_source_folder_name(self): + # FR-004: non-well-known names keep resolving via the existing display-name path. + self._sign_in("Mail.ReadWrite MailboxSettings.Read offline_access") + + def respond(method, url, **kw): + if method == "GET": + return {"value": [{"id": "nested-id", "displayName": "Newsletters", "childFolderCount": 0}]} + return {"id": "sf", "sourceFolderIds": ["nested-id"]} + + rec = _HttpRecorder(respond) + runtime._http = rec + self._capture( + client.cmd_searchfolder_create, + _Args( + name="Newsletters view", + category="Needs attention", + filter_query=None, + source_folders=["Newsletters"], + include_nested=True, + ), + ) + body = next(c for c in rec.calls if c[0] == "POST")[3] + self.assertEqual(body["sourceFolderIds"], ["nested-id"]) + + def test_create_refuses_when_source_folder_ids_come_back_short(self): + # User Story 2 / FR-003: never present a search folder that can never match anything as + # if it succeeded — refuse loudly instead. + self._sign_in("Mail.ReadWrite MailboxSettings.Read offline_access") + + def respond(method, url, **kw): + if method == "GET": + return {"id": "AAMk-inbox-real-id"} + return {"id": "sf-broken", "sourceFolderIds": []} + + rec = _HttpRecorder(respond) + runtime._http = rec + with self.assertRaises(client.SteerError): + client.cmd_searchfolder_create( + _Args( + name="Needs attention", + category="Needs attention", + filter_query=None, + source_folders=["inbox"], + include_nested=True, + ) + ) def test_list_read_scope_only(self): self._sign_in("Mail.Read MailboxSettings.Read offline_access") diff --git a/tests/test_url_construction.py b/tests/test_url_construction.py index d6c43b4..04a0004 100644 --- a/tests/test_url_construction.py +++ b/tests/test_url_construction.py @@ -98,6 +98,12 @@ def setUp(self): def capture(method, url, token=None, body=None, form=False): self.urls.append(url) + if method == "GET" and url.rstrip("/").endswith("/me/mailFolders/inbox"): + # Well-known-folder resolution (searchfolder-create, issue #21) needs a real id back. + return {"id": "AAMk-inbox-real-id"} + if method == "POST" and url.endswith("/me/mailFolders/searchfolders/childFolders"): + ids = (body or {}).get("sourceFolderIds") or [] + return {"id": "sf-new", "sourceFolderIds": ids} return {"value": []} runtime._http = capture From d8598c90ab378fa92fed770c8bd2bcf5d59775bf Mon Sep 17 00:00:00 2001 From: Neil Foster Date: Thu, 23 Jul 2026 09:12:30 +0000 Subject: [PATCH 2/2] fix: compare distinct resolved source-folder ids, not raw input count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cmd_searchfolder_create's post-creation short-list check compared len(returned_ids) against len(source_ids) — the raw requested list. A duplicate/aliased --source_folders input (two names resolving to the same folder id) would trip a false-positive SteerError even though the search folder was created correctly, since Graph legitimately dedupes sourceFolderIds. Compare against the count of distinct resolved ids instead. Add a regression test covering duplicate source_folders input resolving to the same id twice. --- plugin/src/msgraph/verbs.py | 5 +++-- tests/test_client.py | 26 ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/plugin/src/msgraph/verbs.py b/plugin/src/msgraph/verbs.py index 65d7e9b..f22ef1d 100644 --- a/plugin/src/msgraph/verbs.py +++ b/plugin/src/msgraph/verbs.py @@ -439,10 +439,11 @@ def cmd_searchfolder_create(args) -> int: body=body, ) applied_ids = created.get("sourceFolderIds") or [] - if len(applied_ids) < len(source_ids): + distinct_requested = len(set(source_ids)) + if len(applied_ids) < distinct_requested: raise runtime.SteerError( f'Created search folder "{args.name}" (id: {created.get("id", "?")}), but Graph reports ' - f"only {len(applied_ids)} of {len(source_ids)} requested source folder(s) applied — its " + f"only {len(applied_ids)} of {distinct_requested} requested source folder(s) applied — its " f"filter may never match any mail. Remove it with searchfolder-remove " f"--folder_id {created.get('id', '?')} and retry, or inspect with searchfolder-list " f"--format detailed." diff --git a/tests/test_client.py b/tests/test_client.py index 8549e80..69bbae9 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -921,6 +921,32 @@ def respond(method, url, **kw): ) ) + def test_create_does_not_false_positive_on_deduped_source_folders(self): + # Regression: a duplicate/aliased --source_folders input (two entries resolving to the + # same real folder id) must not trip the short-list refusal just because Graph legitimately + # dedupes sourceFolderIds — compare against distinct resolved ids, not the raw input count. + self._sign_in("Mail.ReadWrite MailboxSettings.Read offline_access") + + def respond(method, url, **kw): + if method == "GET": + return {"id": "AAMk-inbox-real-id"} + return {"id": "sf-dedup", "sourceFolderIds": ["AAMk-inbox-real-id"]} + + rec = _HttpRecorder(respond) + runtime._http = rec + self._capture( + client.cmd_searchfolder_create, + _Args( + name="Needs attention (dup source)", + category="Needs attention", + filter_query=None, + source_folders=["inbox", "inbox"], + include_nested=True, + ), + ) + body = next(c for c in rec.calls if c[0] == "POST")[3] + self.assertEqual(body["sourceFolderIds"], ["AAMk-inbox-real-id", "AAMk-inbox-real-id"]) + def test_list_read_scope_only(self): self._sign_in("Mail.Read MailboxSettings.Read offline_access") payload = {