From d9f42d7d4877d66b9b49805449696b2ce1767d22 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Fri, 2 Oct 2026 12:09:19 +0200 Subject: [PATCH] fix(conpty): stop pinning --effort on hand-driven interactive sessions Task, ad-hoc, planning, handoff and resume sessions now inherit the CLI default / the user's /effort setting. List-handler sessions keep their per-role effort. --- docs/explore-notes/conpty-sessions.md | 17 +++---- docs/open.md | 2 +- .../Runner/InteractiveLaunchSpecService.cs | 48 ++++--------------- .../InteractiveLaunchSpecServiceTests.cs | 41 +++++++--------- 4 files changed, 37 insertions(+), 71 deletions(-) diff --git a/docs/explore-notes/conpty-sessions.md b/docs/explore-notes/conpty-sessions.md index bbeab8fa..b64a8dd0 100644 --- a/docs/explore-notes/conpty-sessions.md +++ b/docs/explore-notes/conpty-sessions.md @@ -15,9 +15,9 @@ Autonomous queue tasks are **not** covered here — they stay on the stream-json | Kind | Hub spec method | Notes | |---|---|---| -| Task session | `GetInteractiveLaunchSpec` | Effort from the task/list model preset | -| Ad-hoc | `GetAdHocLaunchSpec` | Effort from the global default | -| Planning | (planning start/resume) | Effort from `PlanningAlias`; uses `--permission-mode default`, **not** `plan` | +| Task session | `GetInteractiveLaunchSpec` | No `--effort` — CLI default | +| Ad-hoc | `GetAdHocLaunchSpec` | No `--effort` — CLI default | +| Planning | (planning start/resume) | No `--effort`; uses `--permission-mode default`, **not** `plan` | | List handler | `GetMergeHelperLaunchSpec` / `GetMergeHelperHandoffLaunchSpec` | Model + effort fixed per role (`ModelRegistry.HandlerTriageAlias`/`HandlerWaitAlias`/`HandlerMergeAlias`), **not** from list config; `--permission-mode` via `PermissionModeResolver` (unattended) | ## ⚠️ Gotcha: never pass task free-text as a CLI argument @@ -39,9 +39,10 @@ description skips the file **and** the positional arg entirely — it still gets ## Argument ordering -Every spec passes `--effort ` from the relevant model's preset. It **leads** the args — -except for a fresh task session with a brief, where `--add-dir ` must come first so -`--effort` (a single-value flag) can sit directly before the positional kickoff. +Only the list-handler specs pass `--effort` (from the role model's preset). Hand-driven sessions +(task, ad-hoc, planning, handoff, resume) deliberately pass none, so the CLI default / the user's +`/effort` setting applies. A positional kickoff must follow a single-value flag (`--session-id`, +`--append-system-prompt-file`), never a variadic one (`--add-dir`, `--allowedTools`). `--model` is deliberately **NOT** forced on an interactive session — the user can still switch models in the TUI. @@ -87,7 +88,7 @@ own transcript: `TaskRepository.SetInteractiveSessionIdAsync` — **before** the `LaunchSpec` is returned, i.e. before the ConPTY host ever spawns `claude`. `BuildFreshTaskArgsAsync` then passes it as `--session-id `, placed as the single-value flag directly before the positional kickoff - (or, with no brief, right after `--effort`). + (or, with no brief, as the only arg). 3. **Fresh worktree wins.** `isFreshWorktree` forces `run` to `null` *and* is checked before reading `task.InteractiveSessionId`, so a recreated worktree never resumes a stale id from either source — it always takes the fresh path, which overwrites the stale @@ -355,7 +356,7 @@ Autonomous and interactive sessions do **not** share a system prompt. Per start |---|---|---| | Autonomous run/continue/retry | `TaskRunner.ResolveConfigAsync` → `ClaudeArgsBuilder.Build` | `--append-system-prompt `, recomputed and re-sent on **every** invocation including a `--resume` continue (`PromptKind.System` + improvement/list/task overrides) | | Interactive task session, fresh | `InteractiveLaunchSpecService.BuildForTaskAsync` → `BuildFreshTaskArgsAsync` | none — no `--append-system-prompt(-file)` at all | -| Interactive task session, resume | `InteractiveLaunchSpecService.BuildForTaskAsync` → `WindowsTerminalLauncher.BuildResumeArgs` | none — only `--resume ` (+ `--effort`) | +| Interactive task session, resume | `InteractiveLaunchSpecService.BuildForTaskAsync` → `WindowsTerminalLauncher.BuildResumeArgs` | none — only `--resume ` | | 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 ` | diff --git a/docs/open.md b/docs/open.md index 2d997e27..54528851 100644 --- a/docs/open.md +++ b/docs/open.md @@ -32,7 +32,7 @@ Der Sichtprüfungs-Block dieses Batches (Ctrl+K/`#`, Titel-Edit, ConPTY-/Refine- Interactive-Chip, Diff-Viewer-Abstände, Manual-Tasks, Vorgaben-pro-Modell-Tabelle) ist von Mika verifiziert und deshalb hier entfernt. Es bleiben die Entscheidungen, die daran hängen: -- Interaktive ConPTY-Sessions bekommen `--effort`, aber **kein** `--model` — die Session läuft weiter unter dem Modell aus Mikas Claude-Config, der Effort kommt aus dem Preset des Modells, das ClaudeDo für die Task auflösen würde. Falls ClaudeDo auch interaktiv das Modell erzwingen soll, ist das ein Folge-Task. +- Interaktive (handgesteuerte) ConPTY-Sessions bekommen weder `--model` noch `--effort` — beides kommt aus Mikas Claude-Config bzw. dem CLI-Default. Nur die List-Handler-Sessions pinnen Modell + Effort pro Rolle. - `AppSettings.DefaultMaxTurns` ist jetzt tatsächlich verdrahtet (`ModelPresets.For(..., global.DefaultMaxTurns)` in `TaskRunner.ResolveConfigAsync`): reiner Fallback für ein Modell, das auch nach `ModelRegistry.TryNormalizeAlias` auf keine Preset-Zeile trifft — vorher war das Feld tot (hartkodierte 30). Hat weiterhin keinen eigenen Editor mehr (nur die Preset-Tabelle pro Modell); Spalte könnte später entfallen, falls das nie zutrifft. ## Beobachtung (offen — Entscheidung Mika) diff --git a/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs b/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs index f5d9d295..e97fd035 100644 --- a/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs +++ b/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs @@ -170,11 +170,6 @@ public sealed class InteractiveLaunchSpecService var resolvedClaude = WindowsTerminalLauncher.Resolve(_claudePath) ?? throw new InvalidOperationException($"claude executable not found: {_claudePath}"); - // Start the hand-driven session at the effort configured for the model this task would run - // under, instead of inheriting whatever the user's global Claude Code config happens to be. - // The model itself is deliberately NOT forced here — the user can still switch it in the TUI. - var effort = EffortFor(globalSettings, task.Model ?? listConfig?.Model); - // Resume this task's own last interactive conversation when it has one -- it takes // precedence over the latest autonomous run's session, since an interactive session is a // distinct conversation from an autonomous run even against the same worktree. Fall back @@ -193,13 +188,13 @@ public sealed class InteractiveLaunchSpecService if (resumeSessionId is { Length: > 0 }) { sessionId = resumeSessionId; - args = WithEffort(WindowsTerminalLauncher.BuildResumeArgs(resumeSessionId), effort); + args = WindowsTerminalLauncher.BuildResumeArgs(resumeSessionId); } else { sessionId = Guid.NewGuid().ToString(); await new TaskRepository(ctx).SetInteractiveSessionIdAsync(taskId, sessionId, ct); - args = await BuildFreshTaskArgsAsync(task, effort, sessionId, ct); + args = await BuildFreshTaskArgsAsync(task, sessionId, ct); } if (paneId is not null) @@ -242,8 +237,7 @@ public sealed class InteractiveLaunchSpecService return new LaunchSpec( ctx.WorkingDir, resolvedClaude, - WithEffort(WindowsTerminalLauncher.BuildPlanningStartArgs(ctx), - EffortFor(ReadSettings(), ModelRegistry.PlanningAlias)), + WindowsTerminalLauncher.BuildPlanningStartArgs(ctx), env); } @@ -262,8 +256,7 @@ public sealed class InteractiveLaunchSpecService return new LaunchSpec( ctx.WorkingDir, resolvedClaude, - WithEffort(WindowsTerminalLauncher.BuildPlanningResumeArgs(ctx.ClaudeSessionId), - EffortFor(ReadSettings(), ModelRegistry.PlanningAlias)), + WindowsTerminalLauncher.BuildPlanningResumeArgs(ctx.ClaudeSessionId), env); } @@ -280,15 +273,13 @@ public sealed class InteractiveLaunchSpecService ?? throw new InvalidOperationException($"claude executable not found: {_claudePath}"); await using var ctx = await _dbFactory.CreateDbContextAsync(ct); - var settings = await new AppSettingsRepository(ctx).GetAsync(ct); var env = new Dictionary { ["MCP_TOOL_TIMEOUT"] = McpToolTimeoutMs, }; - // No task and no list here — the global default model's preset decides the effort. - var args = WithEffort(Array.Empty(), EffortFor(settings, settings.DefaultModel)); + IReadOnlyList args = []; if (paneId is not null) { // Pre-assigned so the pane entry can carry it -- a handoff records it as the brief's origin. @@ -322,16 +313,13 @@ public sealed class InteractiveLaunchSpecService ?? throw new InvalidOperationException($"claude executable not found: {_claudePath}"); await using var ctx = await _dbFactory.CreateDbContextAsync(ct); - var settings = await new AppSettingsRepository(ctx).GetAsync(ct); var sessionId = Guid.NewGuid().ToString(); - var model = settings.DefaultModel; TaskEntity? task = null; if (taskId is not null) { var taskRepo = new TaskRepository(ctx); task = await taskRepo.GetByIdAsync(taskId, ct) ?? throw new KeyNotFoundException($"Task not found: {taskId}"); - model = task.Model ?? (await new ListRepository(ctx).GetConfigAsync(task.ListId, ct))?.Model; await taskRepo.SetInteractiveSessionIdAsync(taskId, sessionId, ct); } @@ -352,7 +340,6 @@ public sealed class InteractiveLaunchSpecService var args = new List(await PaneSessionArgsAsync(paneId, sessionId, ct)) { "--add-dir", Path.GetDirectoryName(briefPath)!, - "--effort", EffortFor(settings, model), "--session-id", sessionId, $"Read the file {briefPath} first. It is the handoff brief from the session that worked here " + "before you. After reading it, continue the work as it describes.", @@ -379,12 +366,11 @@ public sealed class InteractiveLaunchSpecService var resolvedClaude = WindowsTerminalLauncher.Resolve(_claudePath) ?? throw new InvalidOperationException($"claude executable not found: {_claudePath}"); - var settings = await new AppSettingsRepository(ctx).GetAsync(ct); IReadOnlyList args = [ .. await PaneSessionArgsAsync(paneId, sessionId, ct), - .. WithEffort(WindowsTerminalLauncher.BuildResumeArgs(sessionId), EffortFor(settings, settings.DefaultModel)), + .. WindowsTerminalLauncher.BuildResumeArgs(sessionId), ]; var env = new Dictionary { @@ -836,21 +822,6 @@ public sealed class InteractiveLaunchSpecService private static string EffortFor(AppSettingsEntity settings, string? model) => ModelPresets.For(settings.ModelPresets, model ?? settings.DefaultModel).Effort; - // Prepends `--effort `. It has to lead: a positional kickoff prompt must stay last, and - // it may only follow a single-value flag — a variadic flag would swallow it. - private static IReadOnlyList WithEffort(IReadOnlyList args, string effort) - { - var result = new List(args.Count + 2) { "--effort", effort }; - result.AddRange(args); - return result; - } - - private AppSettingsEntity ReadSettings() - { - using var ctx = _dbFactory.CreateDbContext(); - return new AppSettingsRepository(ctx).GetAsync().GetAwaiter().GetResult(); - } - // The task's brief (title + description, unmodified/multi-line) must never travel as a CLI // argument: the ConPTY host flattens Args into one command line to spawn the process, and // claude re-splits that line on whitespace, so any token in the description that starts with @@ -858,18 +829,18 @@ public sealed class InteractiveLaunchSpecService // the brief at its first line even if it didn't. So it's written to a file instead -- // same pattern as BuildForMergeHelperAsync / WindowsTerminalLauncher.BuildPlanningStartArgs -- // and claude is pointed at it with a single-line kickoff. --add-dir exposes the session dir to - // Read; --effort (single-value) must sit directly before the positional kickoff so the + // Read; --session-id (single-value) must sit directly before the positional kickoff so the // preceding variadic --add-dir doesn't swallow the kickoff as another directory. // No brief (task has neither a title nor a description) -> no positional arg at all. // `--session-id` pre-assigns the claude session id the caller already persisted (see // BuildForTaskAsync) so this fresh conversation is resumable from its very first turn -- // it's a single-value flag, so it may sit directly before the positional kickoff. private static async Task> BuildFreshTaskArgsAsync( - TaskEntity task, string effort, string sessionId, CancellationToken ct) + TaskEntity task, string sessionId, CancellationToken ct) { var brief = BuildTaskBrief(task); if (string.IsNullOrEmpty(brief)) - return new[] { "--effort", effort, "--session-id", sessionId }; + return new[] { "--session-id", sessionId }; var sessionDir = Path.Combine(Paths.AppDataRoot(), "task-sessions", task.Id); Directory.CreateDirectory(sessionDir); @@ -879,7 +850,6 @@ public sealed class InteractiveLaunchSpecService return new[] { "--add-dir", sessionDir, - "--effort", effort, "--session-id", sessionId, $"Read the file {briefPath} first. It contains the task you must work on. " + "Then, before changing anything, reply with a one-line restatement of what you intend to do " + diff --git a/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs index 8079a82c..f44a033b 100644 --- a/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs @@ -215,7 +215,7 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable var secondSpec = await svc.BuildForTaskAsync(taskId, CancellationToken.None); Assert.Equal(_tempDir, secondSpec.Cwd); - Assert.Equal(new[] { "--resume", firstSessionId }, ArgsAfterEffort(secondSpec)); + Assert.Equal(new[] { "--resume", firstSessionId }, HandDrivenArgs(secondSpec)); using var readCtx = _db.CreateContext(); Assert.Null(await new WorktreeRepository(readCtx).GetByTaskIdAsync(taskId)); @@ -352,7 +352,7 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable Assert.Equal(_worktreeDir, spec.Cwd); Assert.Equal(_claudeStubPath, spec.Exe); - Assert.Equal(new[] { "--resume", "sess-123" }, ArgsAfterEffort(spec)); + Assert.Equal(new[] { "--resume", "sess-123" }, HandDrivenArgs(spec)); Assert.Equal(InteractiveLaunchSpecService.McpToolTimeoutMs, spec.Env["MCP_TOOL_TIMEOUT"]); } @@ -368,7 +368,7 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable var svc = BuildService(); var spec = await svc.BuildForTaskAsync(taskId, CancellationToken.None); - Assert.Equal(new[] { "--resume", "sess-kept" }, ArgsAfterEffort(spec)); + Assert.Equal(new[] { "--resume", "sess-kept" }, HandDrivenArgs(spec)); } [Fact] @@ -427,7 +427,7 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable var svc = BuildService(); var spec = await svc.BuildForTaskAsync(taskId, CancellationToken.None); - Assert.Equal(new[] { "--resume", "interactive-session-1" }, ArgsAfterEffort(spec)); + Assert.Equal(new[] { "--resume", "interactive-session-1" }, HandDrivenArgs(spec)); } [Fact] @@ -447,7 +447,7 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable // start a new conversation. var secondSpec = await svc.BuildForTaskAsync(taskId, CancellationToken.None); - Assert.Equal(new[] { "--resume", firstSessionId }, ArgsAfterEffort(secondSpec)); + Assert.Equal(new[] { "--resume", firstSessionId }, HandDrivenArgs(secondSpec)); } [Fact] @@ -544,7 +544,7 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable var spec = await svc.BuildForTaskAsync(taskId, CancellationToken.None); // no brief -> no --add-dir, no positional arg -- but a session id is still pre-assigned - var rest = ArgsAfterEffort(spec); + var rest = HandDrivenArgs(spec); Assert.Equal("--session-id", rest[0]); Assert.Equal(rest[1], await ReadInteractiveSessionIdAsync(taskId)); } @@ -582,7 +582,7 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable Assert.Equal(_tempDir, spec.Cwd); Assert.Equal(_claudeStubPath, spec.Exe); - Assert.Empty(ArgsAfterEffort(spec)); + Assert.Empty(HandDrivenArgs(spec)); Assert.Equal(InteractiveLaunchSpecService.McpToolTimeoutMs, spec.Env["MCP_TOOL_TIMEOUT"]); Assert.Empty(_seeder.Calls); } @@ -761,33 +761,28 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable // ── Merge helper ── - /// Every ConPTY spec now leads with `--effort ` from the per-model preset (the seeded - /// settings row has no overrides, so the shipped default for the default model applies). - /// Asserts that pair and returns the rest of the args for the test's own comparison. - private static string[] ArgsAfterEffort(LaunchSpec spec) + /// Hand-driven ConPTY specs never pass `--effort` -- the CLI's own default (or the user's + /// /effort setting) applies. Asserts that and returns the args for the test's own comparison. + private static string[] HandDrivenArgs(LaunchSpec spec) { - var args = spec.Args.ToList(); - Assert.Equal("--effort", args[0]); - Assert.Equal(ModelPresets.For(ModelPresets.Defaults, ModelRegistry.DefaultAlias).Effort, args[1]); - return args.Skip(2).ToArray(); + Assert.DoesNotContain("--effort", spec.Args); + return spec.Args.ToArray(); } /// A fresh (never-resumed) task with a title/description produces - /// `--add-dir --effort --session-id ` -- the brief + /// `--add-dir --session-id ` -- the brief /// travels via the file at /brief.md, never as a raw CLI argument. Asserts that /// exact shape and returns (sessionDir, briefPath, kickoff, sessionId) for the test's own /// checks. Cleanup happens via `_seededTaskIds` in Dispose (the session dir is keyed by task id). private static (string SessionDir, string BriefPath, string Kickoff, string SessionId) ParseFreshTaskArgs(LaunchSpec spec) { var args = spec.Args.ToList(); - Assert.Equal(7, args.Count); + Assert.Equal(5, args.Count); Assert.Equal("--add-dir", args[0]); var sessionDir = args[1]; - Assert.Equal("--effort", args[2]); - Assert.Equal(ModelPresets.For(ModelPresets.Defaults, ModelRegistry.DefaultAlias).Effort, args[3]); - Assert.Equal("--session-id", args[4]); - var sessionId = args[5]; - var kickoff = args[6]; + Assert.Equal("--session-id", args[2]); + var sessionId = args[3]; + var kickoff = args[4]; var briefPath = Path.Combine(sessionDir, "brief.md"); return (sessionDir, briefPath, kickoff, sessionId); @@ -1562,7 +1557,7 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable var spec = BuildService().BuildPlanningResume(ctx); - var args = ArgsAfterEffort(spec); + var args = HandDrivenArgs(spec); Assert.Equal("--permission-mode", args[0]); Assert.Equal("default", args[1]); Assert.Equal("--allowedTools", args[2]);