From a7ff1b3a2fb8144c831d8c3e873ec731d6487c2e Mon Sep 17 00:00:00 2001 From: mika kuns Date: Thu, 6 Aug 2026 21:00:41 +0200 Subject: [PATCH] refactor(prompts): split the list-handler prompt per session phase Both merge-helper ConPTY sessions loaded PromptKind.MergeHelper, so the post-handoff session received the phase 0-2 dedupe/enhance instructions and was told to ignore them by its brief alone. Split into MergeHelperTriage (phases 0-2 + handoff) and MergeHelperExecute (phases 3-5), so each session carries only its own phases. Consolidated the generic ask-the-user rule to one place per prompt, scoped Phase 5's summary to what the execute session actually knows, and moved the dedupe/enhance bilanz to the triage handoff. Regression guards assert neither prompt carries the other's phase headings and that the shared-checkout git rule stays in execute. --- docs/explore-notes/conpty-sessions.md | 3 +- src/ClaudeDo.Data/PromptFiles.cs | 40 +++++++--- .../Runner/InteractiveLaunchSpecService.cs | 9 ++- tests/ClaudeDo.Data.Tests/PromptFilesTests.cs | 74 ++++++++++++++----- .../InteractiveLaunchSpecServiceTests.cs | 38 +++++++++- 5 files changed, 129 insertions(+), 35 deletions(-) diff --git a/docs/explore-notes/conpty-sessions.md b/docs/explore-notes/conpty-sessions.md index 2aecb7c3..9fc9a1dc 100644 --- a/docs/explore-notes/conpty-sessions.md +++ b/docs/explore-notes/conpty-sessions.md @@ -233,7 +233,8 @@ Autonomous and interactive sessions do **not** share a system prompt. Per start | Ad-hoc directory session | `InteractiveLaunchSpecService.BuildForDirectoryAsync` | none | | Planning session start | `InteractiveLaunchSpecService.BuildPlanningStart` → `WindowsTerminalLauncher.BuildPlanningStartArgs` | `--append-system-prompt-file ` (`PromptKind.Planning`) | | Planning session resume | `InteractiveLaunchSpecService.BuildPlanningResume` → `WindowsTerminalLauncher.BuildPlanningResumeArgs` | none — only `--permission-mode default --allowedTools --resume ` | -| List handler ("Let Claude handle it") | `InteractiveLaunchSpecService.BuildForMergeHelperAsync` | `--append-system-prompt-file ` (`PromptKind.MergeHelper`), always fresh — this path never resumes | +| List handler ("Let Claude handle it"), triage | `InteractiveLaunchSpecService.BuildForMergeHelperAsync` | `--append-system-prompt-file ` (`PromptKind.MergeHelperTriage` — phases 0–2 only), always fresh — this path never resumes | +| List handler, post-handoff | `InteractiveLaunchSpecService.BuildForMergeHelperHandoffAsync` | `--append-system-prompt-file ` (`PromptKind.MergeHelperExecute` — phases 3–5 only), fresh session dir, same handler task id | So every interactive resume (task session and planning) drops the system prompt entirely — it's not that they inherit the autonomous one, it's that **no** `claude` process on any resume path diff --git a/src/ClaudeDo.Data/PromptFiles.cs b/src/ClaudeDo.Data/PromptFiles.cs index e48a5405..b5a613fe 100644 --- a/src/ClaudeDo.Data/PromptFiles.cs +++ b/src/ClaudeDo.Data/PromptFiles.cs @@ -4,7 +4,7 @@ using System.Text.Json; namespace ClaudeDo.Data; -public enum PromptKind { System, Planning, PlanningInitial, Retry, DailyPrep, WeeklyReport, ImprovementChild, Refine, MergeHelper, MergeHelperInitial, MergeHelperHandoff } +public enum PromptKind { System, Planning, PlanningInitial, Retry, DailyPrep, WeeklyReport, ImprovementChild, Refine, MergeHelperTriage, MergeHelperExecute, MergeHelperInitial, MergeHelperHandoff } /// /// How a prompt kind's on-disk override (if any) relates to the bundled default. @@ -38,7 +38,8 @@ public static class PromptFiles PromptKind.WeeklyReport => "weekly-report.md", PromptKind.ImprovementChild => "improvement-child.md", PromptKind.Refine => "refine.md", - PromptKind.MergeHelper => "merge-helper-system.md", + PromptKind.MergeHelperTriage => "merge-helper-triage.md", + PromptKind.MergeHelperExecute => "merge-helper-execute.md", PromptKind.MergeHelperInitial => "merge-helper-initial.md", PromptKind.MergeHelperHandoff => "merge-helper-handoff.md", _ => throw new ArgumentOutOfRangeException(nameof(kind)) @@ -211,7 +212,8 @@ public static class PromptFiles PromptKind.WeeklyReport => WeeklyReportDefault, PromptKind.ImprovementChild => ImprovementChildDefault, PromptKind.Refine => RefineDefault, - PromptKind.MergeHelper => MergeHelperDefault, + PromptKind.MergeHelperTriage => MergeHelperTriageDefault, + PromptKind.MergeHelperExecute => MergeHelperExecuteDefault, PromptKind.MergeHelperInitial => MergeHelperInitialDefault, PromptKind.MergeHelperHandoff => MergeHelperHandoffDefault, _ => "" @@ -404,12 +406,14 @@ public static class PromptFiles task, stop. """; - private const string MergeHelperDefault = """ - You are the ClaudeDo list handler, running as an interactive session with the user watching. Work autonomously and decide things yourself by default. Ask the user only for decisions that are genuinely theirs to make: merging a duplicate task, a task whose intent is too unclear to act on safely, a diff that looks wrong or risky, or a conflict resolution you cannot resolve with confidence. Everything else, decide and keep moving. + private const string MergeHelperTriageDefault = """ + You are the ClaudeDo list handler, running as an interactive session with the user watching. Work autonomously and decide things yourself by default. Ask the user only for decisions that are genuinely theirs to make: merging a duplicate task, or a task whose intent is too unclear to act on safely. Everything else, decide and keep moving. - Your job: take the tasks listed in the brief and drive the whole set to merged, Done work — reading them first, removing duplicates, sharpening what stays, running it, then reviewing and merging each result. You act through the mcp__claudedo__* tools. Read the brief file first (the kickoff message gives its path); it names the list, its repo, and every task's id, title and status. All tasks belong to that one list and one repo. + Your job: take the tasks listed in the brief and get the set ready to run — reading them all first, removing duplicates, then sharpening what stays. You act through the mcp__claudedo__* tools. Read the brief file first (the kickoff message gives its path); it names the list, its repo, and every task's id, title and status. All tasks belong to that one list and one repo. - Work the five phases in order. Do not start a phase before the previous one is finished. + A second session takes over after you to run, review and merge these tasks. Your deliverable is a clean, sharpened set of task descriptions — you never run or merge anything yourself, and you touch no git state. + + Work the three phases in order. Do not start a phase before the previous one is finished. ## Phase 0 — Read everything The brief is the primary source: it already lists every task's title, id, status and full description. Read it in full before acting. Only call batch_get_tasks if you need something the brief does not carry for a specific task, e.g. parent/child links. Do not act on any single task before you have read them all — Phase 1 needs the whole set in view. @@ -438,7 +442,20 @@ public static class PromptFiles If a task visibly bundles several independent features, or has a blocker that is not resolved by anything in its own description, do not force it into one description. Propose splitting it to the user; if they agree, create the pieces with add_task/add_subtask and only move the pieces the user confirmed into "surviving tasks" for the phases below. Split only what the task already asks for — the "do not invent requirements" rule still applies. ## Handoff - Once every surviving task is enhanced, call handoff_list_handler with this session's task id and the surviving task ids, in the order you intend to run them. That opens a fresh session to carry out phases 3–5 with just that list, without dragging along this session's dedupe/rewrite context. Say a short goodbye line, then stop — do not continue into phase 3 yourself. + Once every surviving task is enhanced, print your triage summary — one line per task from the brief: + title — dedupe action (kept / merged into X / cancelled as duplicate of X) — enhanced (yes/no). + + Then call handoff_list_handler with this session's task id and the surviving task ids, in the order you intend them to run. That opens a fresh session to carry out the run/review/merge phases with just that list, without dragging along this session's dedupe/rewrite context. Say a short goodbye line, then stop — do not continue into phase 3 yourself. + """; + + private const string MergeHelperExecuteDefault = """ + You are the ClaudeDo list handler, running as an interactive session with the user watching. Work autonomously and decide things yourself by default. Ask the user only for decisions that are genuinely theirs to make: a diff that looks wrong or risky, or a conflict resolution you cannot resolve with confidence. Everything else, decide and keep moving. + + Your job: take the tasks listed in the brief and drive them to merged, Done work — running them, then reviewing and merging each result. You act through the mcp__claudedo__* tools. Read the brief file first (the kickoff message gives its path); it names the list, its repo, and every task's id, title and status. All tasks belong to that one list and one repo. + + A prior session already read, deduplicated and sharpened these tasks; their descriptions are ready to run as written. Start at Phase 3. + + Work the three phases in order. Do not start a phase before the previous one is finished. ## Phase 3 — Run Do NOT use run_task_now for a batch — there is a single override slot and the second call fails with "override slot busy". @@ -478,11 +495,10 @@ public static class PromptFiles Rules for the whole session: - Never use raw `git merge`, `git reset`, or `git checkout` to force a merge. Drive merges through the MCP tools; hand-resolution is only for markers the tools left and cannot finish. - - Ask the user for anything ambiguous, risky, or destructive. ## Phase 5 — Summary - Print one line per task from the original brief: - title — dedupe action (kept / merged into X / cancelled as duplicate of X) — enhanced (yes/no) — final status — merge commit (if any) — conflicts resolved (if any). + Print one line per task from the brief: + title — final status — merge commit (if any) — conflicts resolved (if any). Then list anything you skipped or left for the user and why, and any follow-ups worth turning into new tasks. """; @@ -507,7 +523,7 @@ public static class PromptFiles Repo: {repo} A prior session already read, deduped and enhanced this list's tasks. Pick up at phase 3 - for the tasks below — their descriptions are already sharpened, so don't redo phases 0–2. + for the tasks below — their descriptions are already sharpened. {tasks} diff --git a/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs b/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs index 776b5361..d79eb512 100644 --- a/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs +++ b/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs @@ -237,7 +237,7 @@ public sealed class InteractiveLaunchSpecService : IInteractiveLaunchSpecService Directory.CreateDirectory(sessionDir); var systemPromptPath = Path.Combine(sessionDir, "system-prompt.md"); - await File.WriteAllTextAsync(systemPromptPath, PromptFiles.ReadOrDefault(PromptKind.MergeHelper), ct); + await File.WriteAllTextAsync(systemPromptPath, PromptFiles.ReadOrDefault(PromptKind.MergeHelperTriage), ct); var briefPath = Path.Combine(sessionDir, "brief.md"); await File.WriteAllTextAsync(briefPath, PromptFiles.Render(PromptKind.MergeHelperInitial, @@ -280,8 +280,9 @@ public sealed class InteractiveLaunchSpecService : IInteractiveLaunchSpecService // Builds the LaunchSpec for the fresh session a merge-helper run hands off to once Phase 2 // (enhance) is done -- SAME handler task id as the run that called handoff_list_handler, so // HandlerBaseCommit/HandlerHeadCommit and the review range stay untouched; this never creates - // a task. Reuses the merge-helper system prompt unchanged (the phase 3-5 instructions already - // live there) and only writes a fresh handoff kickoff file, in a NEW session dir -- the old + // a task. Uses the MergeHelperExecute system prompt (phases 3-5 only) rather than the Triage + // one this run started with, so the handoff session carries no dedupe/enhance instructions it + // would have to ignore. Writes a fresh handoff kickoff file in a NEW session dir -- the old // ConPTY tile keeps running against its own session-dir files untouched. public async Task BuildForMergeHelperHandoffAsync( string taskId, IReadOnlyList survivingTaskIds, CancellationToken ct) @@ -315,7 +316,7 @@ public sealed class InteractiveLaunchSpecService : IInteractiveLaunchSpecService Directory.CreateDirectory(sessionDir); var systemPromptPath = Path.Combine(sessionDir, "system-prompt.md"); - await File.WriteAllTextAsync(systemPromptPath, PromptFiles.ReadOrDefault(PromptKind.MergeHelper), ct); + await File.WriteAllTextAsync(systemPromptPath, PromptFiles.ReadOrDefault(PromptKind.MergeHelperExecute), ct); var briefPath = Path.Combine(sessionDir, "handoff.md"); await File.WriteAllTextAsync(briefPath, PromptFiles.Render(PromptKind.MergeHelperHandoff, diff --git a/tests/ClaudeDo.Data.Tests/PromptFilesTests.cs b/tests/ClaudeDo.Data.Tests/PromptFilesTests.cs index 6d3592a5..a35d43e4 100644 --- a/tests/ClaudeDo.Data.Tests/PromptFilesTests.cs +++ b/tests/ClaudeDo.Data.Tests/PromptFilesTests.cs @@ -47,41 +47,81 @@ public class PromptFilesTests [Fact] public void PathFor_merge_helper_kinds_map_to_their_files() { - Assert.EndsWith("merge-helper-system.md", PromptFiles.PathFor(PromptKind.MergeHelper)); + Assert.EndsWith("merge-helper-triage.md", PromptFiles.PathFor(PromptKind.MergeHelperTriage)); + Assert.EndsWith("merge-helper-execute.md", PromptFiles.PathFor(PromptKind.MergeHelperExecute)); Assert.EndsWith("merge-helper-initial.md", PromptFiles.PathFor(PromptKind.MergeHelperInitial)); Assert.EndsWith("merge-helper-handoff.md", PromptFiles.PathFor(PromptKind.MergeHelperHandoff)); } [Fact] - public void DefaultFor_merge_helper_covers_all_five_phases() + public void DefaultFor_merge_helper_triage_covers_phases_0_to_2_only() { - var d = PromptFiles.DefaultFor(PromptKind.MergeHelper); + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperTriage); Assert.False(string.IsNullOrWhiteSpace(d)); - Assert.Contains("Phase 0", d); - Assert.Contains("Phase 1", d); - Assert.Contains("Phase 2", d); - Assert.Contains("Phase 3", d); - Assert.Contains("Phase 4", d); - Assert.Contains("Phase 5", d); + Assert.Contains("## Phase 0", d); + Assert.Contains("## Phase 1", d); + Assert.Contains("## Phase 2", d); + // The run/review/merge phases belong to the handoff session's prompt, not this one — + // carrying them here is what made the handoff session redo dedupe work. + Assert.DoesNotContain("## Phase 3", d); + Assert.DoesNotContain("## Phase 4", d); + Assert.DoesNotContain("## Phase 5", d); } [Fact] - public void DefaultFor_merge_helper_names_the_tools_each_phase_needs() + public void DefaultFor_merge_helper_execute_covers_phases_3_to_5_only() { - var d = PromptFiles.DefaultFor(PromptKind.MergeHelper); + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperExecute); + Assert.False(string.IsNullOrWhiteSpace(d)); + Assert.Contains("## Phase 3", d); + Assert.Contains("## Phase 4", d); + Assert.Contains("## Phase 5", d); + Assert.DoesNotContain("## Phase 0", d); + Assert.DoesNotContain("## Phase 1", d); + Assert.DoesNotContain("## Phase 2", d); + } + + [Fact] + public void DefaultFor_merge_helper_triage_names_the_tools_its_phases_need() + { + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperTriage); Assert.Contains("batch_get_tasks", d); // phase 0 Assert.Contains("update_task", d); // phase 1 + 2 + Assert.Contains("handoff_list_handler", d); // handoff + // Triage never runs or merges anything. + Assert.DoesNotContain("review_task", d); + Assert.DoesNotContain("continue_merge", d); + Assert.DoesNotContain("preview_merge_set", d); + } + + [Fact] + public void DefaultFor_merge_helper_execute_names_the_tools_its_phases_need() + { + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperExecute); Assert.Contains("get_app_settings", d); // phase 3 Assert.Contains("update_task_status", d); // phase 3 + Assert.Contains("wait_for_task_change", d); // phase 3 + Assert.Contains("preview_merge_set", d); // phase 4 Assert.Contains("review_task", d); // phase 4 Assert.Contains("continue_merge", d); // phase 4 Assert.DoesNotContain("run_task_now(", d); // single override slot — must not batch-start } + [Fact] + public void DefaultFor_merge_helper_execute_keeps_the_shared_checkout_git_guard() + { + // The handler session never gets PromptKind.System, so this is the ONLY place the + // never-`git add -A`-in-a-shared-checkout rule reaches a list-handler run. + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperExecute); + Assert.Contains("git add -- ", d); + Assert.Contains("NEVER `git add -A`", d); + Assert.Contains("Never use raw `git merge`", d); + } + [Fact] public void DefaultFor_merge_helper_only_asks_dedupe_questions_when_a_candidate_exists() { - var d = PromptFiles.DefaultFor(PromptKind.MergeHelper); + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperTriage); Assert.Contains("move straight to Phase 2", d); Assert.Contains("do not ask the user to confirm the absence of duplicates", d); Assert.Contains("Cancel nothing without an explicit answer", d); @@ -91,7 +131,7 @@ public class PromptFilesTests [Fact] public void DefaultFor_merge_helper_phase0_treats_brief_as_primary_source() { - var d = PromptFiles.DefaultFor(PromptKind.MergeHelper); + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperTriage); Assert.Contains("primary source", d, StringComparison.OrdinalIgnoreCase); Assert.Contains("batch_get_tasks", d); Assert.Contains("Do not act on any single task before you have read", d); @@ -100,7 +140,7 @@ public class PromptFilesTests [Fact] public void DefaultFor_merge_helper_allows_splitting_bundled_tasks_in_phase_2() { - var d = PromptFiles.DefaultFor(PromptKind.MergeHelper); + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperTriage); Assert.Contains("Propose splitting it to the user", d); Assert.Contains("add_task/add_subtask", d); Assert.Contains("do not invent requirements", d); @@ -109,7 +149,7 @@ public class PromptFilesTests [Fact] public void DefaultFor_merge_helper_checks_effective_max_turns_before_queuing() { - var d = PromptFiles.DefaultFor(PromptKind.MergeHelper); + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperExecute); Assert.Contains("effective max-turns", d); Assert.Contains("get_list_config", d); Assert.Contains("set_task_config", d); @@ -119,7 +159,7 @@ public class PromptFilesTests [Fact] public void DefaultFor_merge_helper_checks_file_collisions_before_merge_order() { - var d = PromptFiles.DefaultFor(PromptKind.MergeHelper); + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperExecute); Assert.Contains("Default to the order the brief lists them", d); Assert.Contains("tell the user which tasks collide", d); Assert.Contains("NORMAL case, not a failure", d); @@ -192,7 +232,7 @@ public class PromptFilesTests [Fact] public void DefaultFor_merge_helper_tells_the_session_to_hand_off_after_phase_2() { - var d = PromptFiles.DefaultFor(PromptKind.MergeHelper); + var d = PromptFiles.DefaultFor(PromptKind.MergeHelperTriage); Assert.Contains("handoff_list_handler", d); Assert.Contains("do not continue into phase 3 yourself", d, StringComparison.OrdinalIgnoreCase); } diff --git a/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs index bb1514df..f033c9e8 100644 --- a/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs @@ -905,7 +905,9 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable var appendIdx = args.IndexOf("--append-system-prompt-file"); var systemPromptPath = args[appendIdx + 1]; Assert.Equal(Path.Combine(sessionDir, "system-prompt.md"), systemPromptPath); - Assert.Equal(PromptFiles.ReadOrDefault(PromptKind.MergeHelper), File.ReadAllText(systemPromptPath)); + // The handoff session gets the Execute prompt (phases 3-5), NOT the Triage prompt the + // first session ran with — otherwise it carries dedupe/enhance instructions to ignore. + Assert.Equal(PromptFiles.ReadOrDefault(PromptKind.MergeHelperExecute), File.ReadAllText(systemPromptPath)); var kickoff = args[^1]; var handoffPath = Path.Combine(sessionDir, "handoff.md"); @@ -970,6 +972,40 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable Assert.Equal(_worktreeDir, spec.Cwd); } + // Regression guard for the bug where both merge-helper builders wrote the SAME system prompt, + // so the handoff session received the phase 0-2 (dedupe/enhance) instructions and was told to + // ignore them by its brief alone. Each session must carry only its own phases. + [Fact] + public async Task MergeHelperBuilders_WriteDifferentSystemPrompts_EachScopedToItsOwnPhases() + { + var repo = Path.Combine(_tempDir, "repoPromptSplit"); + Directory.CreateDirectory(repo); + + var listId = await SeedListAsync(workingDir: repo, name: "Alpha"); + var handlerTaskId = Guid.NewGuid().ToString(); + await SeedTaskAsync(handlerTaskId, listId, TaskStatus.Idle, title: "Handler"); + var survivor = Guid.NewGuid().ToString(); + await SeedTaskAsync(survivor, listId, TaskStatus.WaitingForReview, title: "Survivor"); + + var svc = BuildService(); + + var triageSpec = await svc.BuildForMergeHelperAsync(new[] { survivor }, listId, CancellationToken.None); + var triageDir = TrackSessionDir(triageSpec); + var executeSpec = await svc.BuildForMergeHelperHandoffAsync(handlerTaskId, new[] { survivor }, CancellationToken.None); + var executeDir = TrackSessionDir(executeSpec); + + var triagePrompt = File.ReadAllText(Path.Combine(triageDir, "system-prompt.md")); + var executePrompt = File.ReadAllText(Path.Combine(executeDir, "system-prompt.md")); + + Assert.NotEqual(triagePrompt, executePrompt); + + Assert.Contains("## Phase 1", triagePrompt); + Assert.DoesNotContain("## Phase 4", triagePrompt); + + Assert.Contains("## Phase 4", executePrompt); + Assert.DoesNotContain("## Phase 1", executePrompt); + } + // Regression guard for the bug where BuildForMergeHelperHandoffAsync set MCP_TOOL_TIMEOUT to // an older, shorter value (200000) than every other ConPTY spec (930000) after a parallel // merge landed the two changes independently. Every spec this service builds must carry the