diff --git a/CHANGELOG.md b/CHANGELOG.md index ec24e0c..21bbd5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,15 @@ 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 + +- `rule-create --move_to_folder` (and `searchfolder-create` source folders) now resolve a + folder name at **any nesting depth** via the recursive folder name map, matching + `message-move`/`mail-list`. Previously only immediate children of the mailbox root were + searched, so a folder nested under Inbox (e.g. `Inbox/Newsletters`) failed with "No mail + folder named '' was found". A genuinely non-existent name still raises the steering + error. No scope change. + ## [0.5.0] - 2026-06-22 ### Fixed diff --git a/docs/HANDOVER-nested-folder-rule-target.md b/docs/HANDOVER-nested-folder-rule-target.md deleted file mode 100644 index 127ed38..0000000 --- a/docs/HANDOVER-nested-folder-rule-target.md +++ /dev/null @@ -1,62 +0,0 @@ -# Handover: `rule-create --move_to_folder` cannot target a nested folder - -**Status:** OPEN — raised 2026-06-22. -**Repo context:** kypr `/kypr-triage` session surfaced this during live rule authoring. - -## The problem - -`rule-create` resolves its `--move_to_folder ` target via `_resolve_folder_id` -(`src/msgraph/graph.py:12`), which queries only the **top-level** folder collection: - -```python -data = runtime._graph_get(token, "/me/mailFolders", params={"$top": 100, "$select": "id,displayName"}) -for f in data.get("value", []): - if f.get("displayName", "").casefold() == name.casefold(): - return f["id"] -raise runtime.SteerError(f"No mail folder named '{name}' was found. ...") -``` - -`GET /me/mailFolders` returns only the immediate children of the mailbox root. It does -**not** recurse into `childFolders`. So any folder nested under another (e.g. -`Inbox/Newsletters`, `Inbox/School`) is invisible to the lookup and `rule-create` -fails with "No mail folder named '' was found" — even though the folder plainly -exists. - -This bites the two-tier folder model directly: the intended layout puts the filing -subfolders **under Inbox** (transient tier), so by design the move targets are nested — -exactly the case the resolver can't see. - -Note: existing rules that already target a nested folder keep working, because a rule -stores the resolved folder **id** and Graph fires on the id regardless of where the -folder now sits. The failure is only at *creation* time, when resolving a name. - -## The asymmetry (and the fix it points to) - -The codebase already has a depth-aware resolver: `_resolve_folder` (`graph.py:46`) uses -`_folder_name_map` (`graph.py:100`) to match a display name "at any nesting depth". It -backs `message-move --destination_folder` and `mail-list --folder`, both of which -resolve nested folders fine. - -`rule-create` simply uses the older top-level-only `_resolve_folder_id` instead. - -**Fix direction:** have `rule-create`'s move-target resolution go through the recursive -folder name map (reuse `_folder_name_map` / `_resolve_folder`) so it matches a folder at -any depth — bringing `rule-create` into line with `message-move` and `mail-list`. -Preserve the existing well-known-name fast path and the steering error when the name -genuinely doesn't exist anywhere. - -## Reproduce - -1. Mailbox with a folder nested under Inbox, e.g. `Inbox/Newsletters`. -2. `rule-create --name "kypr:newsletters:example" --header_contains "example.com" --move_to_folder "Newsletters"` -3. Observe: `error: No mail folder named 'Newsletters' was found.` -4. `folder-list` confirms `Newsletters` exists (nested under `Inbox`); `message-move - --destination_folder "Newsletters" --dry_run true` resolves it fine — proving the - depth-aware resolver already handles it. - -## Acceptance - -- `rule-create --move_to_folder ` resolves a folder at any nesting depth. -- A genuinely non-existent name still raises the existing steering error. -- Regression test constructs the nested-folder case (mock `childFolders`) and asserts the - rule's `moveToFolder` action gets the nested folder's id. diff --git a/plugin/src/msgraph/graph.py b/plugin/src/msgraph/graph.py index 55cfbde..0fe826f 100644 --- a/plugin/src/msgraph/graph.py +++ b/plugin/src/msgraph/graph.py @@ -10,11 +10,18 @@ def _resolve_folder_id(token: str, name: str) -> str: - """Look up a mail folder id by display name for the move-to-folder action (data-model).""" - data = runtime._graph_get(token, "/me/mailFolders", params={"$top": 100, "$select": "id,displayName"}) - for f in data.get("value", []): - if f.get("displayName", "").casefold() == name.casefold(): - return f["id"] + """Look up a mail folder id by display name for the move-to-folder action (data-model). + + Accepts a well-known folder name verbatim, otherwise matches a display name at ANY nesting + depth via the recursive folder name map — parity with message-move / mail-list, so a folder + nested under Inbox (e.g. Inbox/Newsletters) resolves. Raises the steering error only when the + name exists nowhere at any depth (feature 007). + """ + if name.casefold() in _WELL_KNOWN_FOLDERS: + return name.casefold() + for fid, fname in _folder_name_map(token).items(): + if fname.casefold() == name.casefold(): + return fid raise runtime.SteerError( f"No mail folder named '{name}' was found. Create it in Outlook first, or pass an " f"existing folder name (rule actions file mail to a folder; they never delete)." diff --git a/tests/test_client.py b/tests/test_client.py index ac8718e..66d68c6 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -490,8 +490,9 @@ def test_success_builds_move_to_folder_and_never_delete(self): client.record_verification(["List-Unsubscribe"], 3) def responder(method, url, **kw): - if url.endswith("/me/mailFolders?$top=100&$select=id,displayName"): - return {"value": [{"id": "folder-123", "displayName": "Newsletters"}]} + # Top-level folder match (no regression for non-nested targets). + if url.split("?", 1)[0].endswith("/me/mailFolders"): + return {"value": [{"id": "folder-123", "displayName": "Newsletters", "childFolderCount": 0}]} if method == "POST": return {"id": "rule-new"} return {} @@ -520,6 +521,70 @@ def responder(method, url, **kw): # no DELETE on any message endpoint self.assertNotIn("DELETE", rec.methods()) + def test_success_resolves_move_to_folder_nested_under_inbox(self): + # feature 007: a folder nested under Inbox resolves to the nested folder's id. + self._sign_in("Mail.Read MailboxSettings.ReadWrite offline_access") + client.record_verification(["List-Unsubscribe"], 2) + + # Folder tree: Inbox (top) → Newsletters (nested). Served path-based, ignoring query. + tree = { + "/me/mailFolders": [{"id": "inbox-1", "displayName": "Inbox", "childFolderCount": 1}], + "/me/mailFolders/inbox-1/childFolders": [ + {"id": "nested-9", "displayName": "Newsletters", "childFolderCount": 0} + ], + } + + def responder(method, url, **kw): + if method == "POST": + return {"id": "rule-new"} + base = url.split("?", 1)[0] + for path, value in tree.items(): + if base.endswith(path): + return {"value": value} + return {"value": []} + + import contextlib + import io + + rec = _HttpRecorder(responder) + runtime._http = rec + with contextlib.redirect_stdout(io.StringIO()): + self.assertEqual( + client.cmd_rule_create( + _Args( + name="Newsletters", + header_contains=["List-Unsubscribe"], + move_to_folder="Newsletters", + ) + ), + 0, + ) + post = next(c for c in rec.calls if c[0] == "POST") + self.assertEqual(post[3]["actions"]["moveToFolder"], "nested-9") + + def test_refuses_when_move_to_folder_name_exists_nowhere(self): + # feature 007: a name matching no folder at any depth still raises the steering error, + # and no rule is POSTed. + self._sign_in("Mail.Read MailboxSettings.ReadWrite offline_access") + client.record_verification(["List-Unsubscribe"], 1) + + def responder(method, url, **kw): + if url.split("?", 1)[0].endswith("/me/mailFolders"): + return {"value": [{"id": "f1", "displayName": "Inbox", "childFolderCount": 0}]} + return {"value": []} + + rec = _HttpRecorder(responder) + runtime._http = rec + with self.assertRaises(client.SteerError): + client.cmd_rule_create( + _Args( + name="X", + header_contains=["List-Unsubscribe"], + move_to_folder="DoesNotExist", + ) + ) + self.assertNotIn("POST", rec.methods()) + # ================================================================================================ # T032 — rule-remove (reversibility primitive)