Browse Source

fix(skills): summon --yes selection bypass + cp1252 title crash

Two bugs in summon.py:

- --yes silently selected ALL candidates, ignoring picks piped via
  stdin. It now only suppresses the final confirmation; the picker
  prompt is always honoured (interactive or piped). New --select
  '1,2,4'/'all' flag for fully non-interactive callers, and a proper
  y/N confirmation gate that --yes (or --dry-run) skips.
- UnicodeEncodeError on Windows cp1252 stdout when a session title
  contained non-cp1252 chars (e.g. U+2192). echo() now falls back to
  encoding with errors='replace'.

Adds skills/summon/tests/ (hermetic sandbox via redirected
HOME/USERPROFILE/APPDATA); all 7 regression tests fail on the
pre-fix script and pass now.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0xDarkMatter 3 weeks ago
parent
commit
450a1edf50
4 changed files with 310 additions and 24 deletions
  1. 2 1
      skills/summon/SKILL.md
  2. 64 23
      skills/summon/scripts/summon.py
  3. 23 0
      skills/summon/tests/run.sh
  4. 221 0
      skills/summon/tests/test_summon.py

+ 2 - 1
skills/summon/SKILL.md

@@ -68,7 +68,8 @@ Mechanically identical — the file moves are the same regardless of which frami
 | `--list-accounts` | | Show all accounts and exit |
 | `--peek <id>` | | Preview a session's last messages and exit (id prefix or full) |
 | `--flat` | | Flat list instead of grouped hierarchy |
-| `--yes` | | Skip confirmation prompt |
+| `--select <picks>` | | Non-interactive selection: `--select "1,2,4"` or `--select all`. Replaces the picker prompt for scripted callers |
+| `--yes` | | Skip the final confirmation prompt only — selection is still required (picker prompt, piped stdin, or `--select`) |
 
 ## Auto-detect rules
 

+ 64 - 23
skills/summon/scripts/summon.py

@@ -329,12 +329,26 @@ def hint(text: str, width: int = 70) -> str:
     return "\n".join(out)
 
 
+def _print_safe(line: str = "") -> None:
+    """print() that survives narrow stdout encodings (e.g. Windows cp1252).
+
+    Session titles carry arbitrary Unicode (e.g. '→') that the console
+    encoding may not represent; degrade those chars to '?' instead of
+    crashing with UnicodeEncodeError.
+    """
+    try:
+        print(line)
+    except UnicodeEncodeError:
+        enc = getattr(sys.stdout, "encoding", None) or "ascii"
+        print(str(line).encode(enc, errors="replace").decode(enc, errors="replace"))
+
+
 def echo(*lines):
     if not lines:
-        print()
+        _print_safe()
         return
     for line in lines:
-        print(line)
+        _print_safe(line)
 
 
 # ============================================================
@@ -927,8 +941,11 @@ def main():
     p.add_argument("--list-accounts", action="store_true", help="List all accounts and exit")
     p.add_argument("--peek", metavar="ID", help="Preview a session's last messages and exit (id prefix or full)")
     p.add_argument("--flat", action="store_true", help="Flat list instead of grouped hierarchy")
+    p.add_argument("--select", metavar="PICKS", default="",
+                   help="Non-interactive selection: numbers like '1,2,4', or 'a'/'all' for all")
     p.add_argument("--yes", action="store_true",
-                   help="Non-interactive: select ALL candidates and proceed without prompting")
+                   help="Skip the final confirmation prompt only — selection is still "
+                        "required (interactively, via piped stdin, or via --select)")
     args = p.parse_args()
 
     claude_dir = appdata_claude()
@@ -1051,36 +1068,60 @@ def main():
 
     echo(panel_close(hotkeys=[("#", "select"), ("a", "all"), ("blank", "cancel")]))
 
-    # Selection (default) — prompt for picks unless --yes (auto-all).
-    if args.yes:
-        chosen = list(candidates)
+    # Selection — --select answers the picker non-interactively; otherwise
+    # prompt for picks (piped stdin honoured too). --yes does NOT bypass
+    # selection; it only suppresses the final confirmation below.
+    def _parse_picks(raw: str) -> list[Session]:
+        if raw.lower() in ("a", "all"):
+            return list(candidates)
+        picks: list[Session] = []
+        for tok in raw.split(","):
+            tok = tok.strip()
+            if not tok:
+                continue
+            try:
+                i = int(tok)
+            except ValueError:
+                continue
+            if i in index_map:
+                picks.append(index_map[i])
+        return picks
+
+    if args.select:
+        chosen = _parse_picks(args.select)
+        if not chosen:
+            sys.exit(f"--select {args.select!r} matched none of the listed sessions")
     else:
         print()
         prompt = Term.color("accent", "select> ") + \
                  "(numbers like '3,5,7', 'a' for all, blank to cancel): "
-        raw = input(prompt).strip()
+        try:
+            raw = input(prompt).strip()
+        except EOFError:
+            raw = ""
         if not raw:
-            print(Term.color("meta", "cancelled."))
+            echo(Term.color("meta", "cancelled."))
             return
-        chosen: list[Session] = []
-        if raw.lower() == "a":
-            chosen = list(candidates)
-        else:
-            for tok in raw.split(","):
-                tok = tok.strip()
-                if not tok:
-                    continue
-                try:
-                    i = int(tok)
-                    if i in index_map:
-                        chosen.append(index_map[i])
-                except ValueError:
-                    continue
+        chosen = _parse_picks(raw)
         if not chosen:
-            print(Term.color("meta", "nothing selected — cancelled."))
+            echo(Term.color("meta", "nothing selected — cancelled."))
             return
     candidates = chosen
 
+    # Final are-you-sure — the only prompt --yes suppresses. Dry-run touches
+    # nothing, so it skips the confirmation too.
+    if not args.yes and not args.dry_run:
+        verb = "move" if args.move else "copy"
+        confirm = Term.color("accent", "confirm> ") + \
+                  f"{verb} {len(candidates)} session(s) to {dest_short}? (y/N): "
+        try:
+            resp = input(confirm).strip().lower()
+        except EOFError:
+            resp = ""
+        if resp not in ("y", "yes"):
+            echo(Term.color("meta", "cancelled."))
+            return
+
     dest_ws = pick_destination_workspace(dest)
 
     # Operate + render results in a fresh stacked panel (DESIGN: 2 blank lines)

+ 23 - 0
skills/summon/tests/run.sh

@@ -0,0 +1,23 @@
+#!/usr/bin/env bash
+# Self-test for the summon skill (scripts/summon.py).
+#
+# Offline-deterministic: builds a throwaway Claude Desktop dir tree in a temp
+# sandbox (HOME/USERPROFILE/APPDATA redirected), so no real account data is
+# read or written. Covers the selection/confirmation flow (--yes, --select,
+# piped stdin) and the cp1252 UnicodeEncodeError regression.
+#
+# Usage:   bash tests/run.sh
+# Exit:    0 all pass, 1 one or more failures
+
+set -uo pipefail
+
+HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+
+# Pick a python that actually executes — skips the Windows Store python3 stub.
+PYTHON=""
+for c in python python3 py; do
+  if command -v "$c" >/dev/null 2>&1 && "$c" -c "" >/dev/null 2>&1; then PYTHON="$c"; break; fi
+done
+[[ -z "$PYTHON" ]] && { echo "no working python found — skipping" >&2; exit 0; }
+
+"$PYTHON" "$HERE/test_summon.py"

+ 221 - 0
skills/summon/tests/test_summon.py

@@ -0,0 +1,221 @@
+#!/usr/bin/env python3
+"""Regression tests for summon.py — selection/confirmation flow + encoding safety.
+
+Hermetic: builds a throwaway Claude Desktop directory tree in a temp sandbox
+and points the script at it via HOME/USERPROFILE/APPDATA. No network, no real
+account data touched.
+
+Covers:
+  1. --yes does NOT bypass the selection picker (piped picks are honoured)
+  2. --select answers the picker non-interactively
+  3. --select all selects everything
+  4. Without --yes, declining the confirmation cancels (nothing copied)
+  5. Without --yes, confirming 'y' proceeds
+  6. --yes with empty stdin cancels instead of auto-selecting all
+  7. Non-cp1252 chars in session titles don't crash on cp1252 stdout
+"""
+
+from __future__ import annotations
+
+import json
+import os
+import subprocess
+import sys
+import tempfile
+import time
+import shutil
+from pathlib import Path
+
+HERE = Path(__file__).resolve().parent
+SCRIPT = HERE.parent / "scripts" / "summon.py"
+
+SRC_UUID = "aaaaaaaa-1111-4111-8111-111111111111"
+DEST_UUID = "bbbbbbbb-2222-4222-8222-222222222222"
+
+PASS = 0
+FAIL = 0
+
+
+def ok(name: str) -> None:
+    global PASS
+    PASS += 1
+    print(f"  PASS  {name}")
+
+
+def no(name: str, detail: str = "") -> None:
+    global FAIL
+    FAIL += 1
+    print(f"  FAIL  {name}" + (f" — {detail}" if detail else ""))
+
+
+def claude_dir(env: dict) -> Path:
+    if sys.platform == "win32":
+        return Path(env["APPDATA"]) / "Claude"
+    if sys.platform == "darwin":
+        return Path(env["HOME"]) / "Library/Application Support/Claude"
+    return Path(env["HOME"]) / ".config/Claude"
+
+
+def build_sandbox(tmp: Path, titles: list[str]) -> tuple[dict, Path, Path]:
+    """Create src account (len(titles) sessions, newest first = #1) + dest account."""
+    home = tmp / "home"
+    appdata = home / "AppData" / "Roaming"
+    env = os.environ.copy()
+    env["HOME"] = str(home)
+    env["USERPROFILE"] = str(home)
+    env["APPDATA"] = str(appdata)
+    env.pop("PYTHONIOENCODING", None)
+    env.pop("TERM_ASCII", None)
+
+    cdir = claude_dir(env)
+    now_ms = int(time.time() * 1000)
+
+    src_ws = cdir / "claude-code-sessions" / SRC_UUID / "11111111-aaaa-4aaa-8aaa-aaaaaaaaaaaa"
+    src_ws.mkdir(parents=True)
+    for i, title in enumerate(titles):
+        sid = f"src-sess-{i}"
+        (src_ws / f"local_{sid}.json").write_text(json.dumps({
+            "sessionId": f"local_{sid}",
+            "cliSessionId": f"cli-{sid}",
+            "title": title,
+            "cwd": "",  # no cwd -> no transcript lookup -> copy proceeds
+            "lastActivityAt": now_ms - (i + 1) * 60_000,  # newest first
+            "completedTurns": 5 + i,
+        }), encoding="utf-8")
+
+    dest_ws = cdir / "claude-code-sessions" / DEST_UUID / "22222222-bbbb-4bbb-8bbb-bbbbbbbbbbbb"
+    dest_ws.mkdir(parents=True)
+    (dest_ws / "local_dest-own.json").write_text(json.dumps({
+        "sessionId": "local_dest-own",
+        "cliSessionId": "cli-dest-own",
+        "title": "dest resident",
+        "cwd": "",
+        "lastActivityAt": now_ms,
+        "completedTurns": 1,
+    }), encoding="utf-8")
+
+    return env, src_ws, dest_ws
+
+
+def run_summon(env: dict, extra_args: list[str], stdin_text: str = "") -> tuple[int, str]:
+    cmd = [sys.executable, str(SCRIPT),
+           "--to", DEST_UUID[:8], "--from", SRC_UUID[:8], "--days", "3"] + extra_args
+    r = subprocess.run(cmd, input=stdin_text.encode("utf-8"),
+                       env=env, capture_output=True, timeout=60)
+    out = (r.stdout.decode("utf-8", "replace")
+           + r.stderr.decode("utf-8", "replace"))
+    return r.returncode, out
+
+
+def copied_titles(dest_ws: Path) -> set[str]:
+    titles = set()
+    for f in dest_ws.glob("local_src-sess-*.json"):
+        titles.add(json.loads(f.read_text(encoding="utf-8"))["title"])
+    return titles
+
+
+def with_sandbox(titles=("alpha", "beta", "gamma")):
+    tmp = Path(tempfile.mkdtemp(prefix="summon-test-"))
+    env, src_ws, dest_ws = build_sandbox(tmp, list(titles))
+    return tmp, env, src_ws, dest_ws
+
+
+def main() -> int:
+    # 1. Piped selection honoured even with --yes (the original bug: --yes
+    #    used to select ALL candidates, ignoring the piped picks).
+    tmp, env, _, dest_ws = with_sandbox()
+    try:
+        rc, out = run_summon(env, ["--yes"], stdin_text="1,3\n")
+        got = copied_titles(dest_ws)
+        if rc == 0 and got == {"alpha", "gamma"}:
+            ok("--yes honours piped selection (copies 2 of 3)")
+        else:
+            no("--yes honours piped selection", f"rc={rc} copied={sorted(got)}")
+    finally:
+        shutil.rmtree(tmp, ignore_errors=True)
+
+    # 2. --select answers the picker without stdin.
+    tmp, env, _, dest_ws = with_sandbox()
+    try:
+        rc, out = run_summon(env, ["--select", "2", "--yes"])
+        got = copied_titles(dest_ws)
+        if rc == 0 and got == {"beta"}:
+            ok("--select 2 copies exactly session #2")
+        else:
+            no("--select 2 copies exactly session #2", f"rc={rc} copied={sorted(got)}")
+    finally:
+        shutil.rmtree(tmp, ignore_errors=True)
+
+    # 3. --select all selects everything.
+    tmp, env, _, dest_ws = with_sandbox()
+    try:
+        rc, out = run_summon(env, ["--select", "all", "--yes"])
+        got = copied_titles(dest_ws)
+        if rc == 0 and got == {"alpha", "beta", "gamma"}:
+            ok("--select all copies all 3")
+        else:
+            no("--select all copies all 3", f"rc={rc} copied={sorted(got)}")
+    finally:
+        shutil.rmtree(tmp, ignore_errors=True)
+
+    # 4. Without --yes, declining the confirmation cancels.
+    tmp, env, _, dest_ws = with_sandbox()
+    try:
+        rc, out = run_summon(env, [], stdin_text="1,2\nn\n")
+        got = copied_titles(dest_ws)
+        if rc == 0 and not got and "cancelled" in out:
+            ok("confirmation 'n' cancels, nothing copied")
+        else:
+            no("confirmation 'n' cancels, nothing copied",
+               f"rc={rc} copied={sorted(got)}")
+    finally:
+        shutil.rmtree(tmp, ignore_errors=True)
+
+    # 5. Without --yes, confirming 'y' proceeds.
+    tmp, env, _, dest_ws = with_sandbox()
+    try:
+        rc, out = run_summon(env, [], stdin_text="1\ny\n")
+        got = copied_titles(dest_ws)
+        if rc == 0 and got == {"alpha"}:
+            ok("confirmation 'y' proceeds with the pick")
+        else:
+            no("confirmation 'y' proceeds with the pick",
+               f"rc={rc} copied={sorted(got)}")
+    finally:
+        shutil.rmtree(tmp, ignore_errors=True)
+
+    # 6. --yes with empty stdin cancels — must NOT fall back to select-all.
+    tmp, env, _, dest_ws = with_sandbox()
+    try:
+        rc, out = run_summon(env, ["--yes"], stdin_text="")
+        got = copied_titles(dest_ws)
+        if rc == 0 and not got and "cancelled" in out:
+            ok("--yes with no selection cancels (no auto-all)")
+        else:
+            no("--yes with no selection cancels (no auto-all)",
+               f"rc={rc} copied={sorted(got)}")
+    finally:
+        shutil.rmtree(tmp, ignore_errors=True)
+
+    # 7. Unicode title on cp1252 stdout must not crash (UnicodeEncodeError
+    #    regression: U+2192 in a title with Windows cp1252 console encoding).
+    tmp, env, _, dest_ws = with_sandbox(
+        titles=("Update docs: Dagu → process-compose gallery", "beta", "gamma"))
+    try:
+        env_cp = dict(env)
+        env_cp["PYTHONIOENCODING"] = "cp1252"
+        rc, out = run_summon(env_cp, ["--select", "all", "--yes", "--dry-run"])
+        if rc == 0 and "would copy" in out:
+            ok("cp1252 stdout survives U+2192 in session title")
+        else:
+            no("cp1252 stdout survives U+2192 in session title",
+               f"rc={rc} out-tail={out[-300:]!r}")
+    finally:
+        shutil.rmtree(tmp, ignore_errors=True)
+
+    print(f"\nsummon tests: {PASS} passed, {FAIL} failed")
+    return 1 if FAIL else 0
+
+
+if __name__ == "__main__":
+    sys.exit(main())