From 3972ce50a6c85be4bb66bc8cd44dee7a8d62895d Mon Sep 17 00:00:00 2001 From: mika kuns Date: Wed, 5 Aug 2026 10:54:48 +0200 Subject: [PATCH] fix(worker): transport ConPTY task brief via file, not CLI argument BuildForTaskAsync's fresh-session path flattened the task title+description into one positional CLI argument, which the ConPTY host joins into a single command line and claude re-splits on whitespace -- any dash-leading token in the description (e.g. "->", "--abort") was misread as an unknown option, and a raw multi-line prompt truncated at its first newline regardless. Now the brief is written to ~/.todo-app/task-sessions//brief.md and exposed via --add-dir, with a single-line kickoff pointing claude at it -- the same pattern BuildForMergeHelperAsync and the planning launcher already use. --- src/ClaudeDo.Worker/CLAUDE.md | 2 +- .../Runner/InteractiveLaunchSpecService.cs | 53 ++++++++--- .../InteractiveLaunchSpecServiceTests.cs | 92 ++++++++++++++++++- 3 files changed, 129 insertions(+), 18 deletions(-) diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index 480252d8..7543c2a4 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -157,7 +157,7 @@ Each CLI invocation is recorded in the `task_runs` table via `TaskRunRepository` - Review/merge: `ApproveReview(taskId, targetBranch) -> MergeResultDto` (childless task: merges its worktree then Done, conflict stays WaitingForReview; task with children: drives `PlanningMergeOrchestrator` to merge the whole unit), `ContinuePlanningMerge` / `AbortPlanningMerge` (resolve a unit-merge conflict), `PreviewMerge(taskId, targetBranch) -> MergePreviewDto` (non-destructive mergeability check), `RejectReviewToQueue`, `RejectReviewToIdle`, `CancelReview`, `MergeTask`, `GetMergeTargets` - Single-task conflict resolver (Layer C): `StartConflictMerge`, `GetMergeConflictDocuments` (segments), `WriteConflictResolution`, `ContinueConflictMerge`, `AbortConflictMerge` (service-level `TaskMergeService.ContinueMergeAsync`/`AbortMergeAsync` keep their names) - Planning sessions: `StartPlanningSession`, `ResumePlanningSession`, `DiscardPlanningSession`, `FinalizePlanningSession`, `QueuePlanningSubtasks`, `GetPendingDraftCount`, `GetPlanningAggregate` (per-subtask diffs), `BuildPlanningIntegrationBranch` (combined diff) -- Interactive sessions (embedded ConPTY, UI process): `GetInteractiveLaunchSpec`, `GetAdHocLaunchSpec`, `GetMergeHelperLaunchSpec`, `CreateMergeHelperTask` (creates the ClaudeDo task that owns a list-handler run — `Idle`/`IsManual=true`, `HandlerBaseCommit` stamped to the list repo's current HEAD via `InteractiveLaunchSpecService.CreateMergeHelperTaskAsync` — called by the UI before it opens the task-based ConPTY tile), `SubmitTaskForReview` (branches on whether the task has a `WorktreeEntity`: with one, commits it and moves on; without one, it's a worktree-less list-handler host task and it just stamps `HandlerHeadCommit` to the list repo's current HEAD — both paths then flip the task Idle/Failed → WaitingForReview). **Every** ConPTY spec that `InteractiveLaunchSpecService` builds leads with `--effort ` from the relevant model's preset (task/list model for a task session, `PlanningAlias` for planning, list config for the list handler, global default for ad-hoc). `--model` is deliberately NOT forced on an interactive session — the user can still switch models in the TUI. The list-handler spec (`BuildForMergeHelperAsync`) uses `--permission-mode auto` so it runs unattended; the `--allowedTools` allowlist (`mcp__claudedo__*,Read,Grep,Glob,Edit,Bash,WebFetch,WebSearch,Skill`) remains the security boundary. +- Interactive sessions (embedded ConPTY, UI process): `GetInteractiveLaunchSpec`, `GetAdHocLaunchSpec`, `GetMergeHelperLaunchSpec`, `CreateMergeHelperTask` (creates the ClaudeDo task that owns a list-handler run — `Idle`/`IsManual=true`, `HandlerBaseCommit` stamped to the list repo's current HEAD via `InteractiveLaunchSpecService.CreateMergeHelperTaskAsync` — called by the UI before it opens the task-based ConPTY tile), `SubmitTaskForReview` (branches on whether the task has a `WorktreeEntity`: with one, commits it and moves on; without one, it's a worktree-less list-handler host task and it just stamps `HandlerHeadCommit` to the list repo's current HEAD — both paths then flip the task Idle/Failed → WaitingForReview). Every ConPTY spec that `InteractiveLaunchSpecService` builds passes `--effort ` from the relevant model's preset (task/list model for a task session, `PlanningAlias` for planning, list config for the list handler, global default for ad-hoc) — 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 (see below). `--model` is deliberately NOT forced on an interactive session — the user can still switch models in the TUI. The list-handler spec (`BuildForMergeHelperAsync`) uses `--permission-mode auto` so it runs unattended; the `--allowedTools` allowlist (`mcp__claudedo__*,Read,Grep,Glob,Edit,Bash,WebFetch,WebSearch,Skill`) remains the security boundary. **No ConPTY path ever passes task free-text (title/description/brief) as a CLI argument** — every one of them (task session, planning start/resume, list handler) writes it to a file first and hands claude a single-line kickoff pointing at that file, exposed via `--add-dir`. Reason: the ConPTY host flattens `Args` into one command line to spawn the process, and claude re-splits that line on whitespace, so any token starting with `-` (e.g. `->`, `--abort`) in real task text would be misread as an unknown option — and a raw multi-line positional prompt truncates at its first newline regardless. A fresh task session's brief lives at `~/.todo-app/task-sessions//brief.md` (`InteractiveLaunchSpecService.BuildFreshTaskArgsAsync`); a task with neither title nor description skips the file and the positional arg entirely. - Worktrees: `CleanupFinishedWorktrees`, `ResetAllWorktrees`, `GetWorktreesOverview`, `SetWorktreeState`, `ForceRemoveWorktree` - Agents/settings/lists: `GetAgents`, `RefreshAgents`, `RestoreDefaultAgents`, `GetAppSettings`, `UpdateAppSettings`, `UpdateList`, `UpdateListConfig`, `GetListConfig`, `UpdateTaskAgentSettings` - Reports/notes/prep: `GetWeekReport`, `GenerateWeekReport`, `GetDailyNotes`, `AddDailyNote`, `UpdateDailyNote`, `DeleteDailyNote`, `RunDailyPrepNow`, `ClearMyDay`, `GetLastPrepLog`, `ListPrimeSchedules`, `UpsertPrimeSchedule`, `DeletePrimeSchedule` diff --git a/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs b/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs index 2c5d07cb..2bfde47b 100644 --- a/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs +++ b/src/ClaudeDo.Worker/Runner/InteractiveLaunchSpecService.cs @@ -94,17 +94,17 @@ public sealed class InteractiveLaunchSpecService : IInteractiveLaunchSpecService var resolvedClaude = WindowsTerminalLauncher.Resolve(_claudePath) ?? throw new InvalidOperationException($"claude executable not found: {_claudePath}"); - // Resume an existing session as-is; for a fresh session, seed the interactive TUI with - // the task's prompt (title + description) as claude's positional prompt so it starts on - // the task -- the user then supervises/answers rather than retyping it. - var args = run?.SessionId is { Length: > 0 } sessionId - ? WindowsTerminalLauncher.BuildResumeArgs(sessionId) - : BuildFreshPromptArgs(task); - // 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. - args = WithEffort(args, EffortFor(globalSettings, task.Model ?? listConfig?.Model)); + var effort = EffortFor(globalSettings, task.Model ?? listConfig?.Model); + + // Resume an existing session as-is; for a fresh session, seed the interactive TUI with + // the task's brief (title + description) via a file, never as a positional CLI argument -- + // see BuildFreshTaskArgsAsync for why. + var args = run?.SessionId is { Length: > 0 } sessionId + ? WithEffort(WindowsTerminalLauncher.BuildResumeArgs(sessionId), effort) + : await BuildFreshTaskArgsAsync(task, effort, ct); // Same run environment variable ClaudeProcess sets for every headless run: the // AskUser MCP tool call caps at 60s unless raised, and lifting it is harmless for @@ -324,20 +324,47 @@ public sealed class InteractiveLaunchSpecService : IInteractiveLaunchSpecService return new AppSettingsRepository(ctx).GetAsync().GetAwaiter().GetResult(); } - // The positional prompt claude opens the interactive session on. Empty (no positional arg) - // if the task has neither a title nor a description. - private static IReadOnlyList BuildFreshPromptArgs(TaskEntity task) + // 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 + // '-' (e.g. "->", "--abort") gets misread as an unknown option, and a newline would truncate + // 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 + // 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. + private static async Task> BuildFreshTaskArgsAsync(TaskEntity task, string effort, CancellationToken ct) + { + var brief = BuildTaskBrief(task); + if (string.IsNullOrEmpty(brief)) + return new[] { "--effort", effort }; + + var sessionDir = Path.Combine(Paths.AppDataRoot(), "task-sessions", task.Id); + Directory.CreateDirectory(sessionDir); + var briefPath = Path.Combine(sessionDir, "brief.md"); + await File.WriteAllTextAsync(briefPath, brief, ct); + + return new[] + { + "--add-dir", sessionDir, + "--effort", effort, + $"Read the file {briefPath} first. It contains the task you must work on. " + + "After reading it, begin the session as your instructions describe.", + }; + } + + private static string BuildTaskBrief(TaskEntity task) { var title = task.Title?.Trim(); var description = task.Description?.Trim(); - var prompt = (string.IsNullOrEmpty(title), string.IsNullOrEmpty(description)) switch + return (string.IsNullOrEmpty(title), string.IsNullOrEmpty(description)) switch { (false, false) => $"{title}\n\n{description}", (false, true) => title!, (true, false) => description!, _ => string.Empty, }; - return string.IsNullOrEmpty(prompt) ? Array.Empty() : new[] { prompt }; } private async Task> FilterToInstalledSkillsAsync(IReadOnlyList requested, CancellationToken ct) diff --git a/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs index 3100cad3..fa71b9d6 100644 --- a/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Runner/InteractiveLaunchSpecServiceTests.cs @@ -1,3 +1,4 @@ +using ClaudeDo.Data; using ClaudeDo.Data.Git; using ClaudeDo.Data.Models; using ClaudeDo.Data.Repositories; @@ -25,6 +26,7 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable private readonly FakeSessionSkillSeeder _seeder = new(); private readonly FakeSessionSkillRegistry _registry = new(); private readonly List _gitFixtures = new(); + private readonly List _seededTaskIds = new(); private static bool GitAvailable => GitRepoFixture.IsGitAvailable(); @@ -54,6 +56,8 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable try { Directory.Delete(_tempDir, true); } catch { /* best effort */ } foreach (var d in _mergeHelperSessionDirs) try { Directory.Delete(d, true); } catch { /* best effort */ } + foreach (var id in _seededTaskIds) + try { Directory.Delete(Path.Combine(Paths.AppDataRoot(), "task-sessions", id), true); } catch { /* best effort */ } } private sealed class FakeSessionSkillRegistry : ISessionSkillRegistry @@ -92,6 +96,8 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable Id = taskId, ListId = listId, Title = title, Status = status, CreatedAt = DateTime.UtcNow, SessionSkills = sessionSkillsJson, }); + _seededTaskIds.Add(taskId); // a fresh-task ConPTY spec may write a real brief.md under + // ~/.todo-app/task-sessions/ -- clean it up on dispose } private async Task SeedWorktreeAsync(string taskId, WorktreeState state, string? path = null) @@ -185,8 +191,11 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable Assert.True(Directory.Exists(wtRow.Path)); Assert.Equal(wtRow.Path, spec.Cwd); - Assert.Equal(new[] { "T" }, ArgsAfterEffort(spec)); // fresh session seeds the task title as the prompt Assert.Equal(_claudeStubPath, spec.Exe); + + var (_, briefPath, kickoff) = ParseFreshTaskArgs(spec); + Assert.Contains(briefPath, kickoff); // fresh session points claude at the brief file, not a raw prompt + Assert.Equal("T", File.ReadAllText(briefPath)); // brief file holds the task title } [Theory] @@ -262,7 +271,9 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable var svc = BuildService(); var spec = await svc.BuildForTaskAsync(taskId, CancellationToken.None); - Assert.Equal(new[] { "T" }, ArgsAfterEffort(spec)); // fresh: seeds the task title as the prompt + var (_, briefPath, kickoff) = ParseFreshTaskArgs(spec); // fresh: no --resume, brief file holds the title + Assert.Contains(briefPath, kickoff); + Assert.Equal("T", File.ReadAllText(briefPath)); Assert.Equal(_worktreeDir, spec.Cwd); } @@ -278,7 +289,9 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable var svc = BuildService(); var spec = await svc.BuildForTaskAsync(taskId, CancellationToken.None); - Assert.Equal(new[] { "T" }, ArgsAfterEffort(spec)); // fresh: seeds the task title as the prompt + var (_, briefPath, kickoff) = ParseFreshTaskArgs(spec); // fresh: brief file holds the title + Assert.Contains(briefPath, kickoff); + Assert.Equal("T", File.ReadAllText(briefPath)); } [Fact] @@ -298,7 +311,59 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable var svc = BuildService(); var spec = await svc.BuildForTaskAsync(taskId, CancellationToken.None); - Assert.Equal(new[] { "T\n\nDo the thing" }, ArgsAfterEffort(spec)); + var (_, briefPath, kickoff) = ParseFreshTaskArgs(spec); + Assert.Contains(briefPath, kickoff); + Assert.Equal("T\n\nDo the thing", File.ReadAllText(briefPath)); + } + + [Fact] + public async Task BuildForTaskAsync_FreshTask_DashInDescription_DoesNotAppearInArgs() + { + var listId = await SeedListAsync(); + var taskId = Guid.NewGuid().ToString(); + await SeedTaskAsync(taskId, listId, TaskStatus.Idle, title: "Worker: OAuth-Usage-Client"); + await SeedWorktreeAsync(taskId, WorktreeState.Active); + var description = "WorkerHub.PreviewMerge(taskId, targetBranch) -> MergePreviewDto\n" + + "run with --abort -Seconds 5 -m -A and `backticks` \"quotes\""; + using (var ctx = _db.CreateContext()) + { + var t = await ctx.Tasks.FindAsync(taskId); + t!.Description = description; + await ctx.SaveChangesAsync(); + } + + var svc = BuildService(); + var spec = await svc.BuildForTaskAsync(taskId, CancellationToken.None); + + var (_, briefPath, kickoff) = ParseFreshTaskArgs(spec); + Assert.Contains(briefPath, kickoff); + Assert.DoesNotContain('\n', kickoff); + + // No arg element carries any fragment of the title/description -- it only ever + // reaches the session via the brief file. + foreach (var arg in spec.Args) + { + Assert.DoesNotContain("->", arg); + Assert.DoesNotContain("--abort", arg); + Assert.DoesNotContain("OAuth-Usage-Client", arg); + } + + var briefContent = File.ReadAllText(briefPath); + Assert.Equal($"Worker: OAuth-Usage-Client\n\n{description}", briefContent); + } + + [Fact] + public async Task BuildForTaskAsync_FreshTask_NoTitleNoDescription_NoPositionalArg() + { + var listId = await SeedListAsync(); + var taskId = Guid.NewGuid().ToString(); + await SeedTaskAsync(taskId, listId, TaskStatus.Idle, title: ""); + await SeedWorktreeAsync(taskId, WorktreeState.Active); + + var svc = BuildService(); + var spec = await svc.BuildForTaskAsync(taskId, CancellationToken.None); + + Assert.Empty(ArgsAfterEffort(spec)); // no brief -> no --add-dir, no positional arg } [Fact] @@ -360,6 +425,25 @@ public sealed class InteractiveLaunchSpecServiceTests : IDisposable return args.Skip(2).ToArray(); } + /// A fresh (never-resumed) task with a title/description produces + /// `--add-dir --effort ` -- the brief travels via the file at + /// /brief.md, never as a raw CLI argument. Asserts that exact shape and returns + /// (sessionDir, briefPath, kickoff) 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) ParseFreshTaskArgs(LaunchSpec spec) + { + var args = spec.Args.ToList(); + 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]); + var kickoff = args[4]; + + var briefPath = Path.Combine(sessionDir, "brief.md"); + return (sessionDir, briefPath, kickoff); + } + private readonly List _mergeHelperSessionDirs = new(); /// The session dir is the value right after --add-dir; register it for cleanup