From 624ec7a66876548cdf37e44d0d83cb809111e9e3 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Fri, 24 Jul 2026 12:38:45 +0200 Subject: [PATCH] fix(planning): use default permission mode so MCP planning tools don't prompt Interactive planning sessions launched with --permission-mode plan, which gates EVERY MCP tool call regardless of --allowedTools (verified: even a read-only mcp__claudedo__list_task_lists is denied under plan mode). So the session prompted the user on the first CreateChildTask -- the whole point of a planning session. Switch BuildPlanningStartArgs/BuildPlanningResumeArgs to --permission-mode default, which honours the allowlist. File edits stay blocked via the planning system prompt + AllowedTools omitting Write/Edit/Bash. Resume also re-passes --allowedTools, since the CLI does not restore it across --resume. The earlier 'glob does not match' hypothesis was empirically falsified: mcp__claudedo__*, the bare server name, and the explicit tool name all allow the tool with zero permission_denials in default mode. --- docs/open.md | 1 - .../Planning/WindowsTerminalLauncher.cs | 16 +++++++++++----- .../Planning/WindowsTerminalLauncherTests.cs | 14 +++++++++++--- .../Runner/InteractiveLaunchSpecServiceTests.cs | 9 +++++++-- 4 files changed, 29 insertions(+), 11 deletions(-) diff --git a/docs/open.md b/docs/open.md index 9e279963..1299a605 100644 --- a/docs/open.md +++ b/docs/open.md @@ -8,7 +8,6 @@ Stand: 2026-07-24. Diese Datei listet die **aktiv verifizierten Findings** aus d - **OUTCOME-Karte rendert rohes Structured-Output-JSON:** `TaskMonitorViewModel.ApplyOutcome` (ClaudeDo.Ui) setzt bei Tasks ohne Roadblock-Marker `SessionOutcome = result` wörtlich (Zeile ~261). Der Worker legt in `task.Result` das rohe `{"summary":…,"files_changed":[…]}` ab (die lesbare Fassung steht in `task_runs.resultMarkdown`), also zeigt die Detail-Insel OUTCOME als JSON-Blob statt als Text. Fix: (a) UI parst ein JSON-Result und zeigt `summary`, oder (b) Worker schreibt `summary`/`resultMarkdown` statt des JSON in `task.Result`. Verifiziert am Task `verif §1 diff matrix`. Deckt sich mit Memory `worker_testing_findings`. - **Approve & Merge schluckt einen „blocked"-Merge still:** `DetailsIslandViewModel.ApproveReviewAsync` reagiert nur auf `result.Status == "conflict"` (öffnet den Resolver); bei `"blocked"` (z.B. Ziel-Working-Tree hat uncommittete getrackte Änderungen) und anderen Nicht-`merged`/Nicht-`conflict`-Status passiert **nichts** — kein Footer-Fehler, kein Dialog (der `catch` greift nur bei Exceptions, „blocked" ist aber ein normaler Rückgabewert mit `ErrorMessage`). User sieht „Klick tut nichts". Doppelt verifiziert (Konflikt-Approve UND sauberer additiver Approve `verif §1b`, beide bei dirty `main`-Checkout still). Fix: bei `blocked`/unerwartetem Status `result.ErrorMessage` via `ShowErrorAsync`/`FlashFooterError` surfacen. Verstößt gegen `feedback_ui_error_surfacing`. (Der Konflikt-Resolver + 3-Pane-Editor funktionieren, sobald der Ziel-Tree sauber ist.) -- **Planning-Session fragt nach Permission für das claudedo-MCP-Tool:** Obwohl `WindowsTerminalLauncher` `--allowedTools "mcp__claudedo__*,Read,Grep,Glob,WebFetch,WebSearch,Skill"` + `--permission-mode plan` setzt (Zeile 33/130), prompted die interaktive Planning-Session beim ersten `mcp__claudedo__create_child_task`-Aufruf. Verdacht: der `mcp__claudedo__*`-Glob matcht in CLI 2.1.207 nicht (Syntax evtl. `mcp__claudedo` für den ganzen Server), oder Plan-Mode gated MCP-Writes generell. User kann in der TUI approven, sollte aber nicht müssen. Verifiziert §3. - **„Waiting for Improvements" für Planning-Parents (Terminologie):** `taskStatus.waitingForChildren` = „Waiting for Improvements" (en.json:504), `agentStatus.children` dito (503), `childOutcomesLabel` = „IMPROVEMENTS" (191). Seit dem unified-parent-Modell gilt `WaitingForChildren` für Planning **und** Improvement-Parents — „Improvements" ist für einen Planning-Parent falsch. Auf neutrales „Waiting for Subtasks"/„Subtasks" umstellen (en **und** de — Localization.Tests-Parität). Verifiziert §3. - **Kind-Rows aktualisieren nach Parent-Planning-Transitionen nicht live:** Parent-getriebene Änderungen propagieren nicht auf die Kind-`TaskRowViewModel`s ohne Listen-Reload. (a) Nach „Finalize planning" bleiben Kind-Rows auf „Draft" (`IsDraft`) statt „Planned" (`IsPlanned`) — der geänderte `ParentFinalized` kommt nicht über das Parent-`TaskUpdated`-Broadcast an. (b) Nach „Discard" bleiben die (in der DB gelöschten) Draft-Kind-Rows sichtbar, bis die Liste neu geladen wird. Gemeinsame Ursache: die Kinderliste/-zustände werden bei Parent-Transitionen nicht live neu aufgelöst. Doppelt verifiziert §3 (Finalize **und** Discard, 2026-07-24). - **AskUser-Frage erscheint NUR in Mission Control, nicht in der Task-Detail-Insel:** Der `ask_user`-Banner (Frage + Antwort-Eingabe) lebt ausschließlich in `MonitorPaneView` (Mission Control). Die Detail-Insel (`DetailsIslandView`) streamt zwar den Live-Log eines laufenden Tasks, zeigt aber **kein** Frage-Banner — ein Nutzer, der nur die Detailansicht offen hat, sieht nicht, dass der Run auf eine Antwort wartet, und läuft nach 3 min in den Timeout-Fallback. Verifiziert §7 (User schaute Detailansicht, Frage war unsichtbar; erst in Mission Control sichtbar). Fix: Frage-Banner + Inline-Antwort auch in der Detail-Insel für den gebundenen laufenden Task surfacen (VM-Zustand liegt bereits in `TaskMonitorViewModel`, müsste für die Detail-Insel repliziert/geteilt werden). **Von Mika bei der §7-Verifikation ausdrücklich gewünscht** („vermisse diese Interaktion in der Detail-Ansicht"). §7. diff --git a/src/ClaudeDo.Worker/Planning/WindowsTerminalLauncher.cs b/src/ClaudeDo.Worker/Planning/WindowsTerminalLauncher.cs index 6cad82a2..3780e2f5 100644 --- a/src/ClaudeDo.Worker/Planning/WindowsTerminalLauncher.cs +++ b/src/ClaudeDo.Worker/Planning/WindowsTerminalLauncher.cs @@ -99,8 +99,8 @@ public sealed class WindowsTerminalLauncher : ITerminalLauncher return Task.CompletedTask; } - // Resumes a session by id in default (interactive) permission mode: the user drives - // tool approvals in the terminal, unlike planning which pins --permission-mode plan. + // Resumes a session by id with no extra flags: the user drives every tool approval in the + // terminal, unlike planning which additionally allowlists its MCP planning tools. internal static string BuildResumeCommand(string claudePath, string claudeSessionId) => BuildPwshCommand(claudePath, BuildResumeArgs(claudeSessionId)); @@ -130,7 +130,12 @@ public sealed class WindowsTerminalLauncher : ITerminalLauncher return new[] { "--model", Model, - "--permission-mode", "plan", + // NOT --permission-mode plan: plan mode gates EVERY MCP tool call regardless of + // --allowedTools, so the session would prompt the user on the first CreateChildTask + // (the whole point of a planning session). Default mode honours the allowlist below, + // and file edits stay blocked anyway — the planning system prompt forbids them and + // AllowedTools omits Write/Edit/Bash (an unexpected edit would prompt, not run silently). + "--permission-mode", "default", "--allowedTools", AllowedTools, "--add-dir", ctx.Files.SessionDirectory, "--append-system-prompt-file", ctx.Files.SystemPromptPath, @@ -139,9 +144,10 @@ public sealed class WindowsTerminalLauncher : ITerminalLauncher } // The raw claude CLI args for an interactive planning RESUME, shared with the embedded-ConPTY - // planning path. Pins --permission-mode plan (unlike a plain --resume pickup). + // planning path. Re-passes the planning allowlist in default mode (not --resume alone): the CLI + // does not restore --allowedTools across a resume, and plan mode would gate the MCP tools. internal static IReadOnlyList BuildPlanningResumeArgs(string claudeSessionId) => - new[] { "--permission-mode", "plan", "--resume", claudeSessionId }; + new[] { "--permission-mode", "default", "--allowedTools", AllowedTools, "--resume", claudeSessionId }; private string ResolveWtOrThrow() => Resolve(_wtPath) ?? throw new TerminalLaunchException($"Windows Terminal not found: {_wtPath}"); diff --git a/tests/ClaudeDo.Worker.Tests/Planning/WindowsTerminalLauncherTests.cs b/tests/ClaudeDo.Worker.Tests/Planning/WindowsTerminalLauncherTests.cs index 6c0ad077..fa43178b 100644 --- a/tests/ClaudeDo.Worker.Tests/Planning/WindowsTerminalLauncherTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Planning/WindowsTerminalLauncherTests.cs @@ -80,7 +80,9 @@ public sealed class WindowsTerminalLauncherTests Assert.Equal("--model", args[0]); var permIdx = args.ToList().IndexOf("--permission-mode"); Assert.True(permIdx >= 0); - Assert.Equal("plan", args[permIdx + 1]); + // Default mode, NOT plan mode: plan mode gates every MCP tool call regardless of the + // allowlist, which would prompt on the first CreateChildTask. + Assert.Equal("default", args[permIdx + 1]); Assert.Contains("--allowedTools", args); Assert.Contains(ctx.Files.SessionDirectory, args); Assert.Contains(ctx.Files.SystemPromptPath, args); @@ -89,10 +91,16 @@ public sealed class WindowsTerminalLauncherTests } [Fact] - public void BuildPlanningResumeArgs_PinsPlanModeAndResume() + public void BuildPlanningResumeArgs_DefaultModeAllowlistsMcpAndResumes() { var args = WindowsTerminalLauncher.BuildPlanningResumeArgs("sess-9"); - Assert.Equal(new[] { "--permission-mode", "plan", "--resume", "sess-9" }, args); + Assert.Equal("--permission-mode", args[0]); + Assert.Equal("default", args[1]); + Assert.Equal("--allowedTools", args[2]); + // The MCP planning tools must be re-allowlisted on resume: the CLI does not restore + // --allowedTools across a --resume, so without it CreateChildTask would prompt again. + Assert.Contains("mcp__claudedo__", args[3]); + Assert.Equal(new[] { "--resume", "sess-9" }, args.Skip(4).ToArray()); } [Fact] diff --git a/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs index 5c65ffc2..14cd1a28 100644 --- a/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs @@ -361,7 +361,8 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable Assert.Equal(_worktreeDir, spec.Cwd); Assert.Equal(_claudeStubPath, spec.Exe); Assert.Contains("--permission-mode", spec.Args); - Assert.Contains("plan", spec.Args); + // Default mode, not plan mode -- plan mode would gate the MCP planning tools. + Assert.Contains("default", spec.Args); Assert.Equal("tok-1", spec.Env["CLAUDEDO_PLANNING_TOKEN"]); Assert.Equal("20000", spec.Env["MAX_THINKING_TOKENS"]); } @@ -375,7 +376,11 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable var spec = BuildService().BuildPlanningResume(ctx); - Assert.Equal(new[] { "--permission-mode", "plan", "--resume", "sess-42" }, spec.Args); + Assert.Equal("--permission-mode", spec.Args[0]); + Assert.Equal("default", spec.Args[1]); + Assert.Equal("--allowedTools", spec.Args[2]); + Assert.Contains("mcp__claudedo__", spec.Args[3]); + Assert.Equal(new[] { "--resume", "sess-42" }, spec.Args.Skip(4).ToArray()); Assert.Equal("tok-2", spec.Env["CLAUDEDO_PLANNING_TOKEN"]); Assert.Equal(_worktreeDir, spec.Cwd); }