Merge pull request #4 from neilgfoster/fix/graph-query-encoding

fix: percent-encode Graph query params (mail-list InvalidURL)

Author: neilgfoster

CLAUDE.md

@@ -101,7 +101,9 @@ or advertise it.
 reference bundled files via `${CLAUDE_PLUGIN_ROOT}/...` — it resolves to `plugin/`.
 
 <!-- SPECKIT START -->
-Active feature plan: `specs/001-msgraph-mail-rules/plan.md` (spec, research, data-model,
-contracts/tools.md, quickstart alongside it). Read it for the technical context, the stdlib-only
-device-code design, the `TOOLS` catalog contract, and the safety-model decisions before implementing.
+Active feature plan: `specs/002-fix-graph-query-encoding/plan.md` (defect fix: percent-encode all
+Graph query params + real-URL regression coverage + Azure setup docs). Foundation feature plan:
+`specs/001-msgraph-mail-rules/plan.md` (spec, research, data-model, contracts/tools.md, quickstart
+alongside it) — read it for the technical context, the stdlib-only device-code design, the `TOOLS`
+catalog contract, and the safety-model decisions before implementing.
 <!-- SPECKIT END -->

DEFINITION_OF_DONE.md

@@ -39,7 +39,10 @@ plugin is "done" for its first release when **every** box below is true. Read `C
 ## Quality / hygiene
 
 - [ ] **Offline-testable.** Classification/verification/output-shaping logic is unit-tested without
-      network (Graph HTTP boundary mockable). Tests pass with stdlib only.
+      network (Graph HTTP boundary mockable). Tests pass with stdlib only. **Includes real-URL
+      construction coverage** (`tests/test_url_construction.py`): each verb's URL is built through the
+      real path and asserted free of raw spaces/control characters — the `_http`-mock tests alone
+      missed a malformed `$orderby` that crashed the first live run (feature 002).
 - [ ] **Two-tier layout.** Shippable payload lives under `plugin/` (`plugin/.claude-plugin/plugin.json`,
       `plugin/skills/`, `plugin/src/msgraph/`); the root carries `.claude-plugin/marketplace.json`
       whose plugin `source` resolves to `./plugin`.
@@ -53,7 +56,15 @@ plugin is "done" for its first release when **every** box below is true. Read `C
 
 ## Acceptance demo (the working slice)
 
-A reviewer can, from a clean checkout + a one-time Azure app reg:
+**One-time Azure app reg prerequisites** (verified on the first live run — see auth-login SKILL.md
+for the full walkthrough): a personal Microsoft account has **no tenant** by default, so you must
+create one via the Azure free trial (card-verified; tenant + app reg are free forever after the trial
+lapses); register the app at **entra.microsoft.com** (not portal.azure.com) as **Personal Microsoft
+accounts only**, blank redirect URI, **Allow public client flows = Yes** (Authentication → Settings
+sub-tab), delegated `Mail.Read` + `MailboxSettings.Read` (+ `MailboxSettings.ReadWrite`); export
+`MSGRAPH_CLIENT_ID` and `MSGRAPH_TENANT_ID="consumers"`.
+
+A reviewer can, from a clean checkout + the one-time Azure app reg above:
 
 1. `auth-login` (read-only) → succeeds.
 2. `mail-list` → sees real inbox messages.

plugin/skills/auth-login/SKILL.md

@@ -32,13 +32,36 @@ Stay in `read` until you actually need to install or remove a rule.
 
 ## One-time prerequisite (free, human)
 
-Register a free **Azure AD app** (public client, device-code/public-client flow **enabled**) with
-delegated permissions `Mail.Read` + `MailboxSettings.Read` (+ `MailboxSettings.ReadWrite` for rule
-authoring). Personal Microsoft accounts need no admin consent. Then export, before first sign-in:
+You register a free **Azure AD (Entra) app** once. The app registration and tenant are Entra-level
+and **free forever** — only the steps to *get* a tenant trip people up, so follow these exactly. (All
+verified during the first live run.)
+
+**0. A personal Microsoft account has NO tenant by default.** It sits in the shared "Microsoft
+Services" directory, where app registration is impossible. You must create your own tenant first.
+
+**1. Create a tenant.** Per Microsoft docs the prerequisite is an Azure subscription — start the free
+trial at <https://azure.microsoft.com/free>. It requires **card verification** (~£1 reversible hold,
+no auto-charge; the trial subscription is *disabled* at 30 days, not upgraded). Durability: once the
+tenant exists, the app registration and token issuance keep working at **zero cost** after the trial
+subscription is disabled — the subscription is only needed to create the tenant, not to run the app.
+
+**2. Use the Entra admin center — <https://entra.microsoft.com>, NOT portal.azure.com.** A tenant with
+no active subscription makes the Azure portal default to the wrong directory (error `AADSTS160021`).
+
+**3. Register the app** (Entra → App registrations → New registration):
+
+| Setting | Value |
+|---|---|
+| Supported account types | **Personal Microsoft accounts only** (this makes `MSGRAPH_TENANT_ID="consumers"` correct) |
+| Redirect URI | **leave blank** (device-code flow needs none) |
+| Authentication → **Allow public client flows** | **Yes** — REQUIRED, or device-code fails. In the new "Authentication (Preview)" tab this toggle lives under the **Settings** sub-tab (no longer under "Advanced settings" — docs that say otherwise are outdated). |
+| API permissions (delegated) | `Mail.Read`, `MailboxSettings.Read` (+ `MailboxSettings.ReadWrite` for rule authoring). Personal accounts need **no admin consent** — consent happens in-browser at sign-in. The default `User.Read` can stay; the kernel never requests it. |
+
+**4. Export the values** before first sign-in:
 
 ```bash
-export MSGRAPH_CLIENT_ID="<application (client) id>"
-export MSGRAPH_TENANT_ID="consumers"   # or "common" for work/school + personal
+export MSGRAPH_CLIENT_ID="<application (client) id>"   # a public client's id is NOT a secret — plaintext is fine
+export MSGRAPH_TENANT_ID="consumers"                   # "Personal Microsoft accounts only" ⇒ consumers
 ```
 
 These are read from the environment and never hardcoded. If `MSGRAPH_CLIENT_ID` is unset, the verb

plugin/src/msgraph/client.py

@@ -516,13 +516,27 @@ def _render_messages(items: list, fmt: str) -> str:
 # ================================================================================================
 # Graph helpers
 # ================================================================================================
-def _graph_get(token: str, path: str) -> dict:
-    return _http("GET", f"{GRAPH}{path}", token=token)
+def _graph_url(path: str, params: dict | None = None) -> str:
+    """Build a Graph URL, percent-encoding query values so the URL is valid for urllib/http.client.
+
+    OData values routinely contain spaces (``$orderby=receivedDateTime desc``) and other reserved
+    characters; an unencoded space raises ``http.client.InvalidURL`` on first live use. ``$`` and
+    ``,`` are kept literal because Graph expects them verbatim in option names (``$top``, ``$orderby``)
+    and ``$select`` lists; everything else (notably spaces → ``%20``) is percent-encoded.
+    """
+    url = f"{GRAPH}{path}"
+    if params:
+        url += "?" + urllib.parse.urlencode(params, quote_via=urllib.parse.quote, safe="$,")
+    return url
+
+
+def _graph_get(token: str, path: str, params: dict | None = None) -> dict:
+    return _http("GET", _graph_url(path, params), token=token)
 
 
 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 = _graph_get(token, "/me/mailFolders?$top=100&$select=id,displayName")
+    data = _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"]
@@ -532,6 +546,37 @@ def _resolve_folder_id(token: str, name: str) -> str:
     )
 
 
+# Action keys whose value is an opaque folder id we can resolve to a display name (read-only).
+_FOLDER_ACTION_KEYS = ("moveToFolder", "copyToFolder")
+
+
+def _folder_name_map(token: str) -> dict:
+    """id→displayName for all mail folders at any nesting depth.
+
+    Lets rule-list show move/copy-to-folder targets by name. Walks the folder tree breadth-first,
+    descending only into folders that report children (``childFolderCount``) so the number of GETs is
+    proportional to the folders-with-children, not the whole tree. Read-only; failures degrade
+    silently, leaving unresolved ids to fall back to the raw id so output never lies about the target.
+    """
+    names: dict = {}
+    params = {"$top": 200, "$select": "id,displayName,childFolderCount"}
+    # Queue of folder-listing paths to fetch; seed with the top level.
+    pending = ["/me/mailFolders"]
+    try:
+        while pending:
+            data = _graph_get(token, pending.pop(), params=params)
+            for f in data.get("value", []):
+                fid = f.get("id")
+                if not fid:
+                    continue
+                names[fid] = f.get("displayName", "")
+                if f.get("childFolderCount", 0):
+                    pending.append(f"/me/mailFolders/{urllib.parse.quote(fid, safe='')}/childFolders")
+    except SteerError:
+        pass  # partial map is fine — unresolved ids render as raw ids
+    return names
+
+
 # ================================================================================================
 # Verb implementations
 # ================================================================================================
@@ -595,7 +640,9 @@ def cmd_mail_list(args) -> int:
     tok = _authed_token("Mail.Read")
     sel = "id,subject,from,receivedDateTime"
     data = _graph_get(
-        tok["access_token"], f"/me/messages?$top={args.limit}&$select={sel}&$orderby=receivedDateTime desc"
+        tok["access_token"],
+        "/me/messages",
+        params={"$top": args.limit, "$select": sel, "$orderby": "receivedDateTime desc"},
     )
     print(_render_messages(data.get("value", []), args.format))
     return 0
@@ -605,7 +652,8 @@ def cmd_mail_get(args) -> int:
     """Fetch one message including internet headers (FR-006)."""
     tok = _authed_token("Mail.Read")
     sel = "id,subject,from,receivedDateTime,body,internetMessageHeaders"
-    msg = _graph_get(tok["access_token"], f"/me/messages/{args.message_id}?$select={sel}")
+    mid = urllib.parse.quote(args.message_id, safe="")
+    msg = _graph_get(tok["access_token"], f"/me/messages/{mid}", params={"$select": sel})
     if args.format == "detailed":
         print(json.dumps(msg, indent=2))
     else:
@@ -622,7 +670,11 @@ def cmd_mail_get(args) -> int:
 def _fetch_messages_with_headers(token: str, limit: int = 100) -> list:
     """Read messages + their internet headers for catch-set evaluation (read-only, GETs only)."""
     sel = "id,subject,from,receivedDateTime,internetMessageHeaders"
-    data = _graph_get(token, f"/me/messages?$top={limit}&$select={sel}&$orderby=receivedDateTime desc")
+    data = _graph_get(
+        token,
+        "/me/messages",
+        params={"$top": limit, "$select": sel, "$orderby": "receivedDateTime desc"},
+    )
     return data.get("value", [])
 
 
@@ -644,23 +696,64 @@ def cmd_rule_verify(args) -> int:
     return 0
 
 
-def _render_rules(rules: list, fmt: str) -> str:
+def _humanize_key(key: str) -> str:
+    """Turn a camelCase predicate/action name into spaced lower-case words (agent-legible)."""
+    return "".join(f" {c.lower()}" if c.isupper() else c for c in key).strip()
+
+
+def _humanize_value(value) -> str:
+    """Render a Graph predicate/action value (list, recipient list, bool, enum, range) legibly."""
+    if isinstance(value, bool):
+        return "yes" if value else "no"
+    if isinstance(value, list):
+        parts = []
+        for item in value:
+            if isinstance(item, dict):  # recipient: {"emailAddress": {"name", "address"}}
+                addr = (item.get("emailAddress") or {})
+                parts.append(addr.get("address") or addr.get("name") or json.dumps(item))
+            else:
+                parts.append(str(item))
+        return ", ".join(parts)
+    if isinstance(value, dict):  # e.g. withinSizeRange {minimumSize, maximumSize}
+        return ", ".join(f"{_humanize_key(k)} {v}" for k, v in value.items())
+    return str(value)
+
+
+def _summarize_clauses(clauses: dict, folders: dict | None = None) -> list:
+    """Summarize whichever conditions/actions are present, skipping empty/false ones.
+
+    ``folders`` (id→displayName) resolves move/copy-to-folder action ids to legible names; an
+    unresolved id falls back to the raw id so the output never misrepresents the target.
+    """
+    folders = folders or {}
+    lines = []
+    for key, value in (clauses or {}).items():
+        if value in (None, [], {}, "", False):
+            continue  # absent predicate/action — don't pretend it's set
+        if key in _FOLDER_ACTION_KEYS and isinstance(value, str) and value in folders:
+            rendered = f'"{folders[value]}"'
+        else:
+            rendered = _humanize_value(value)
+        lines.append(f"{_humanize_key(key)}: {rendered}")
+    return lines
+
+
+def _render_rules(rules: list, fmt: str, folders: dict | None = None) -> str:
     if fmt == "detailed":
         return json.dumps(rules, indent=2)
     if not rules:
         return "No inbox message rules."
     out = []
     for r in rules:
-        conds = r.get("conditions") or {}
-        hc = conds.get("headerContains") or []
-        action = r.get("actions") or {}
-        folder = action.get("moveToFolder") or ""
-        out.append(
+        conds = _summarize_clauses(r.get("conditions") or {}, folders)
+        actions = _summarize_clauses(r.get("actions") or {}, folders)
+        line = (
             f'- "{r.get("displayName", "(unnamed)")}"'
             f"{'  enabled' if r.get('isEnabled', True) else '  disabled'}"
-            f"\n    if header contains: {hc or '(other criteria)'}"
-            f"\n    → move to folder id: {folder or '(other action)'}"
+            f"\n    if   {'; '.join(conds) or '(no conditions)'}"
+            f"\n    then {'; '.join(actions) or '(no actions)'}"
         )
+        out.append(line)
     out.append(f"{len(rules)} rule(s). Pass --format detailed for ids needed by rule-remove.")
     return "\n".join(out)
 
@@ -669,7 +762,10 @@ def cmd_rule_list(args) -> int:
     """Enumerate existing inbox message rules (FR-007). Rules are mailbox settings (MailboxSettings.Read)."""
     tok = _authed_token("MailboxSettings.Read")
     data = _graph_get(tok["access_token"], "/me/mailFolders/inbox/messageRules")
-    print(_render_rules(data.get("value", []), args.format))
+    rules = data.get("value", [])
+    # Resolve folder ids to names only when needed for the legible (concise) view.
+    folders = _folder_name_map(tok["access_token"]) if args.format != "detailed" and rules else {}
+    print(_render_rules(rules, args.format, folders))
     return 0
 
 

tests/test_client.py

@@ -265,11 +265,81 @@ def test_shaping_and_empty_state(self):
         empty = self._render({"value": []})
         self.assertIn("No inbox message rules", empty)
 
-    def _render(self, payload):
+    def test_renders_non_header_predicates_and_actions(self):
+        # Most real rules use predicates other than headerContains; they must be legible, not hidden.
+        self._sign_in("Mail.Read MailboxSettings.Read offline_access")
+        rules = {
+            "value": [
+                {
+                    "id": "r2",
+                    "displayName": "From boss",
+                    "isEnabled": False,
+                    "conditions": {
+                        "senderContains": ["boss@example.com"],
+                        "fromAddresses": [{"emailAddress": {"name": "Boss", "address": "boss@example.com"}}],
+                        "sentToMe": True,
+                        "importance": "high",
+                    },
+                    "actions": {"markAsRead": True, "assignCategories": ["Work"]},
+                }
+            ]
+        }
+        out = self._render(rules)
+        self.assertIn("From boss", out)
+        self.assertIn("disabled", out)
+        self.assertIn("sender contains: boss@example.com", out)
+        self.assertIn("from addresses: boss@example.com", out)
+        self.assertIn("sent to me: yes", out)
+        self.assertIn("importance: high", out)
+        self.assertIn("mark as read: yes", out)
+        self.assertIn("assign categories: Work", out)
+        # No false "(other criteria)" / "(other action)" placeholder leaks through.
+        self.assertNotIn("other criteria", out)
+
+    def test_resolves_deeply_nested_folder_id_to_name(self):
+        # rule-list shows the folder NAME, resolved at ANY nesting depth, not the opaque id.
+        self._sign_in("Mail.Read MailboxSettings.Read offline_access")
+        rules = {
+            "value": [
+                {
+                    "id": "r3",
+                    "displayName": "Filed",
+                    "isEnabled": True,
+                    "conditions": {"senderContains": ["x@y.com"]},
+                    "actions": {"moveToFolder": "grandchild-1", "copyToFolder": "unknown-id"},
+                }
+            ]
+        }
+        # Folder tree served across the child-folder endpoints (top → child → grandchild).
+        folder_tree = {
+            "/me/mailFolders": [{"id": "top-1", "displayName": "Top", "childFolderCount": 1}],
+            "/me/mailFolders/top-1/childFolders": [
+                {"id": "child-1", "displayName": "Mid", "childFolderCount": 1}
+            ],
+            "/me/mailFolders/child-1/childFolders": [
+                {"id": "grandchild-1", "displayName": "House 2026", "childFolderCount": 0}
+            ],
+        }
+        out = self._render(rules, folder_tree)
+        self.assertIn('move to folder: "House 2026"', out)  # depth-3 child resolved
+        self.assertNotIn("grandchild-1", out)
+        self.assertIn("copy to folder: unknown-id", out)  # unresolved id shown raw, not hidden
+
+    def _render(self, payload, folder_tree=None):
         import contextlib
         import io
 
-        client._http = _HttpRecorder(lambda method, url, **kw: payload)
+        def responder(method, url, **kw):
+            if "/messageRules" in url:
+                return payload
+            if folder_tree:
+                for path, value in folder_tree.items():
+                    # Match the path portion of the URL, ignoring the query string.
+                    if url.split("?", 1)[0].endswith(path):
+                        return {"value": value}
+            return {"value": []}
+
+        client._http = _HttpRecorder(responder)
         buf = io.StringIO()
         with contextlib.redirect_stdout(buf):
             self.assertEqual(client.cmd_rule_list(_Args(format="concise")), 0)