From 44ec89b0e659527b0a76fa15cbf059a9b8921d72 Mon Sep 17 00:00:00 2001 From: Mika Kuns Date: Wed, 30 Sep 2026 10:49:07 +0200 Subject: [PATCH] chore(claude-do): Worktrees sollten nach dem Task benannt werden MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Durch die GUID namen kann ein User die Worktrees keinem Task zuordnen, und in Rider Müllen mir die Worktrees die BRanches voll. Also sollten wir einfach den Task titel oder Nummer stattdessen nehmen ClaudeDo-Task: 63f08d944d3341c081267158ef4bbb3e --- docs/explore-notes/external-mcp.md | 4 +- src/ClaudeDo.Worker/CLAUDE.md | 6 +-- src/ClaudeDo.Worker/Runner/WorktreeManager.cs | 31 +++++++++---- .../Runner/WorktreeManagerTests.cs | 45 ++++++++++++++++--- 4 files changed, 68 insertions(+), 18 deletions(-) diff --git a/docs/explore-notes/external-mcp.md b/docs/explore-notes/external-mcp.md index 6ab450ba..ad593bb0 100644 --- a/docs/explore-notes/external-mcp.md +++ b/docs/explore-notes/external-mcp.md @@ -25,8 +25,8 @@ is wired through `TaskIdResolver`, which resolves: This is **not** ambiguous: a GUID is never all-digits, so a pure-integer parameter is always a number, not a partial GUID. All task-returning tools (`GetTask`, `ListTasks`, `AddTask`, `BatchGetTasks`, etc.) stamp the resolved task's `Number` in the DTO — both `TaskDto` and the -lean `TaskRefDto` carry an `int Number` field. **Branch names and worktree paths** (`claudedo/{id}`) -continue to use the GUID; the number is a display alias, never the identity. +lean `TaskRefDto` carry an `int Number` field. **Task worktree branches and folders** are `claudedo/{number}-{title-slug}`; +otherwise the number is a display alias, never the identity. Every tool description carries a shared boilerplate clause (defined in `McpToolDocs.TaskNumberHint`) instructing the agent to **refer to tasks as `#` when reporting results to the user** — diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index 2603b18b..8ba4789f 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -55,7 +55,7 @@ subfolder within their area; the namespace stays the area namespace. - **RunCancellationRegistry** — taskId → running-run CTS. Lets `TaskStateService.CancelAsync` kill a cancelled task's process without a DI cycle. - **OverrideSlotService** — owns `RunNow` / `ContinueTask`; goes through `TaskStateService.StartRunningAsync` (caller-driven, serialized by slot lock). - **StaleTaskRecovery** — startup-only; calls `TaskStateService.RecoverStaleRunningAsync` to flip orphaned `Running` rows to `Failed`. -- **External/*** — always-on MCP tools for general Claude sessions, scoped to *starting* and *observing* sessions (no multi-turn, planning internals, or app-settings writes). Auth via optional `X-ClaudeDo-Key`. **Task-ID resolution:** every tool parameter that accepts a task id (`taskId`, `parentId`, `taskIds`, etc.) is wired through `TaskIdResolver`, which accepts both `#123` (short number-based handle) and bare GUIDs. A bare all-digits string is never a GUID and is unambiguous. Branch names and worktree paths continue to use the GUID (`claudedo/{id}`) — the `Number` is a display alias, never identity. Every task-returning tool stamps the task's `Number` in its DTO payload and the MCP tool descriptions carry a shared clause instructing the agent to **refer to tasks as `#` when reporting to the user** (otherwise the number sits unused in the payload). **Hard convention (test-enforced):** every optional/filter parameter needs a C# default value — the MCP schema only marks a parameter optional when one exists; `ExternalMcpToolSchemaTests` guards this. **Other conventions (not test-enforced):** no tool returns bare `Task`/a nullable payload directly (write tools return a confirmation record like `RunTaskNowResult`; read tools use an explicit `Found`/`Available` flag); tool descriptions follow rules documented in `External/McpToolDocs.cs`. Full tool inventory + per-tool behaviour → [external-mcp](../../docs/explore-notes/external-mcp.md). +- **External/*** — always-on MCP tools for general Claude sessions, scoped to *starting* and *observing* sessions (no multi-turn, planning internals, or app-settings writes). Auth via optional `X-ClaudeDo-Key`. **Task-ID resolution:** every tool parameter that accepts a task id (`taskId`, `parentId`, `taskIds`, etc.) is wired through `TaskIdResolver`, which accepts both `#123` (short number-based handle) and bare GUIDs. A bare all-digits string is never a GUID and is unambiguous. Task worktree branches and folders are named `{number}-{title-slug}` (`claudedo/{number}-{title-slug}`; stored in the `worktrees` row, never re-derived) — the `Number` is otherwise a display alias, never identity. Every task-returning tool stamps the task's `Number` in its DTO payload and the MCP tool descriptions carry a shared clause instructing the agent to **refer to tasks as `#` when reporting to the user** (otherwise the number sits unused in the payload). **Hard convention (test-enforced):** every optional/filter parameter needs a C# default value — the MCP schema only marks a parameter optional when one exists; `ExternalMcpToolSchemaTests` guards this. **Other conventions (not test-enforced):** no tool returns bare `Task`/a nullable payload directly (write tools return a confirmation record like `RunTaskNowResult`; read tools use an explicit `Found`/`Available` flag); tool descriptions follow rules documented in `External/McpToolDocs.cs`. Full tool inventory + per-tool behaviour → [external-mcp](../../docs/explore-notes/external-mcp.md). ## Status Model @@ -130,7 +130,7 @@ Full flow, invariants, and model/effort/max-turns resolution (including the low- - **ClaudeProcess** — spawns `claude -p --output-format stream-json --verbose --permission-mode auto` (or whatever app settings specify). Prompt via stdin, NDJSON from stdout. CancellationToken kills the process tree. Resolves `_cfg.ClaudeBin` via `ExecutableResolver` (`ClaudeDo.Data`, shared with `ClaudeCliPreflight` and the Installer's checks) before spawning — a `.cmd`/`.bat` shim (e.g. an npm-installed `claude`) is launched through `cmd.exe /c` since `UseShellExecute = false` can't exec a shim directly; a resolved `.exe` starts exactly as before. Throws if nothing resolves. *(Not yet on `main` — see `Environment Checks` in `ClaudeDo.Installer/CLAUDE.md`.)* - **ClaudeArgsBuilder** — `--model`, `--effort`, `--max-turns`, `--append-system-prompt`, `--agents`, `--json-schema`, `--resume` - **StreamAnalyzer** — parses NDJSON; extracts session_id, token counts, turn counts, result text, structured output. Replaced MessageParser. -- **WorktreeManager** — worktrees on `claudedo/{id}` branches (full task GUID, dashes stripped — a prefix isn't enough to rule out collisions); commits with semantic messages, updates DB with head commit + diff stats +- **WorktreeManager** — worktrees on `claudedo/{number}-{title-slug}` branches, same leaf as the folder (`NameFor`; the unique task number rules out collisions, slug capped at 40 chars); commits with semantic messages, updates DB with head commit + diff stats - **CommitMessageBuilder** — `{commitType}(slug): title\n\ndescription\n\nClaudeDo-Task: taskId`; `BuildMerge` is the merge-commit variant (`{commitType}(slug): merge title` + trailer). **Every merge caller passes a blank commit message on purpose** — `TaskMergeService` fills in `BuildMerge` from the task's commit type and its list's name, which is the only place that knows both. Don't reintroduce a caller-side literal. - **TaskResetService** — discards a failed task's worktree, resets the row to Idle, preserves run history - **AgentFileService** — manages `~/.claudeDo/agents/*.md`; list/refresh via SignalR @@ -309,6 +309,6 @@ distinguish the two singletons). The token never touches `worker.config.json` or - The worker runs standalone — start it separately from the UI. Loopback only (127.0.0.1). - `--permission-mode auto` by default; legacy `bypassPermissions` settings map to `auto` at dispatch time. `acceptEdits`, `plan`, `default` pass through unchanged. -- Worktree branches follow `claudedo/{id}`. +- Task worktree branches follow `claudedo/{number}-{title-slug}`; planning worktrees stay `claudedo/planning/{guid}` (their path is re-derived from the id on resume). - **Both MCP servers run `Stateless = true`** (`Program.cs`). No `Mcp-Session-Id`, so a worker restart can't 404 a session that outlives it, and a 2026-07-28 client isn't refused back to the initialize handshake. The trade: **no** sampling, elicitation, resource subscriptions, unsolicited notifications, or legacy SSE endpoint — don't reach for those. In-tool progress notifications ride the POST's own response stream and work in both modes. All three consumers (`TaskRunner`, `PlanningSessionManager`, the installer's `RegisterMcpStep`) already use `type: "http"`. - **An MCP tool that can run longer than ~5s reports progress.** Staying silent lets the MCP client abort after 300s idle while the worker keeps working — the caller sees an abort even though the operation is still running. `Lifecycle/ProgressReporter` is the one implementation (elapsed-time reporting via `RunAsync`, per-item `i/n` via `ReportItem`) — thread an `IProgress? progress = null` parameter through instead of writing another polling loop. diff --git a/src/ClaudeDo.Worker/Runner/WorktreeManager.cs b/src/ClaudeDo.Worker/Runner/WorktreeManager.cs index 26fb6cb2..9fe83a29 100644 --- a/src/ClaudeDo.Worker/Runner/WorktreeManager.cs +++ b/src/ClaudeDo.Worker/Runner/WorktreeManager.cs @@ -38,6 +38,14 @@ public sealed class WorktreeManager private Task BroadcastCreating(string taskId) => _broadcaster?.OperationProgress(taskId, PhaseCreatingWorktree, 0, 0) ?? Task.CompletedTask; + /// Branch leaf and folder name for a task worktree: {number}-{title-slug}, unique via the task number. + public static string NameFor(TaskEntity task) + { + var titleSlug = CommitMessageBuilder.ToSlug(task.Title); + if (titleSlug.Length > 40) titleSlug = titleSlug[..40].TrimEnd('-'); + return titleSlug.Length == 0 ? task.Number.ToString() : $"{task.Number}-{titleSlug}"; + } + public async Task CreateAsync(TaskEntity task, ListEntity list, CancellationToken ct) { var workingDir = list.WorkingDir @@ -51,10 +59,7 @@ public sealed class WorktreeManager _logger.LogInformation("Repository at {Dir} had no commits; created an empty bootstrap commit", workingDir); var baseCommit = await ResolveBaseCommitAsync(task, workingDir, ct); - // Use the full task id (dashes stripped) in the branch name so - // two GUIDs sharing an 8-char prefix cannot collide on the same branch. - var idForBranch = task.Id.Replace("-", ""); - var branchName = $"claudedo/{idForBranch}"; + var name = NameFor(task); var slug = CommitMessageBuilder.ToSlug(list.Name); string strategy; @@ -68,11 +73,21 @@ public sealed class WorktreeManager : _cfg.CentralWorktreeRoot; } - var worktreePath = strategy.Equals("central", StringComparison.OrdinalIgnoreCase) - ? Path.Combine(centralRoot ?? _cfg.CentralWorktreeRoot, slug, task.Id) - : Path.Combine(WorktreeRootResolver.ResolveSiblingRoot(workingDir), ".claudedo-worktrees", slug, task.Id); + var parentDir = strategy.Equals("central", StringComparison.OrdinalIgnoreCase) + ? Path.Combine(centralRoot ?? _cfg.CentralWorktreeRoot, slug) + : Path.Combine(WorktreeRootResolver.ResolveSiblingRoot(workingDir), ".claudedo-worktrees", slug); - worktreePath = Path.GetFullPath(worktreePath); + // Numbers restart after a DB reset: a taken name may be another task's unmerged branch, + // which the self-heal below would force-delete — the id suffix keeps it untouched. + if (Directory.Exists(Path.Combine(parentDir, name)) + || (await _git.ListLocalBranchesAsync(workingDir, ct)).Contains($"claudedo/{name}")) + { + var id = task.Id.Replace("-", ""); + name = $"{name}-{id[..Math.Min(8, id.Length)]}"; + } + + var branchName = $"claudedo/{name}"; + var worktreePath = Path.GetFullPath(Path.Combine(parentDir, name)); WorktreeRootResolver.EnsureOutsideWorkingDir(worktreePath, workingDir); // Ensure parent directory exists. diff --git a/tests/ClaudeDo.Worker.Tests/Runner/WorktreeManagerTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/WorktreeManagerTests.cs index 90c964b2..eec0c552 100644 --- a/tests/ClaudeDo.Worker.Tests/Runner/WorktreeManagerTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Runner/WorktreeManagerTests.cs @@ -65,7 +65,9 @@ public class WorktreeManagerTests : IDisposable Assert.NotNull(ctx); Assert.True(Directory.Exists(ctx.WorktreePath)); - Assert.Equal($"claudedo/{task.Id.Replace("-", "")}", ctx.BranchName); + // StartsWith: a folder leaked into the shared %TEMP% root by another test forces the id suffix. + Assert.StartsWith($"claudedo/{task.Number}-test-task", ctx.BranchName); + Assert.StartsWith($"{task.Number}-test-task", Path.GetFileName(ctx.WorktreePath)); Assert.Equal(repo.BaseCommit, ctx.BaseCommit); using var readCtx = db.CreateContext(); @@ -77,6 +79,15 @@ public class WorktreeManagerTests : IDisposable Assert.Null(row.HeadCommit); } + [Theory] + [InlineData("Fix: Löschen des Tasks!", "7-fix-lschen-des-tasks")] + [InlineData("äöü", "7")] + [InlineData("a very long title that goes well beyond the forty char cap", "7-a-very-long-title-that-goes-well-beyond")] + public void NameFor_NumberPlusCappedTitleSlug(string title, string expected) + { + Assert.Equal(expected, WorktreeManager.NameFor(new TaskEntity { Id = "x", ListId = "y", Title = title, Number = 7, CreatedAt = DateTime.UtcNow })); + } + [Fact] public async Task CreateAsync_BroadcastsCreatingWorktreePhase() { @@ -107,6 +118,28 @@ public class WorktreeManagerTests : IDisposable Assert.Equal(WorktreeManager.PhaseCreatingWorktree, (string?)progressCalls[0].Args[1]); } + [Fact] + public async Task CreateAsync_NameTakenByForeignBranch_FallsBackToIdSuffixAndLeavesBranchIntact() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var repo = CreateRepo(); + var (task, list) = MakeEntities(repo.RepoDir); + var (mgr, _) = await CreateManagerAsync(task, list); + + // A leftover from an earlier DB whose task happened to get the same number and title. + var foreign = $"claudedo/{WorktreeManager.NameFor(task)}"; + GitRepoFixture.RunGit(repo.RepoDir, "branch", foreign, repo.BaseCommit); + + var ctx = await mgr.CreateAsync(task, list, CancellationToken.None); + _worktreeCleanups.Add((repo.RepoDir, ctx.WorktreePath)); + + var suffixed = $"{WorktreeManager.NameFor(task)}-{task.Id.Replace("-", "")[..8]}"; + Assert.Equal($"claudedo/{suffixed}", ctx.BranchName); + Assert.Equal(suffixed, Path.GetFileName(ctx.WorktreePath)); + Assert.False(string.IsNullOrWhiteSpace(GitRepoFixture.RunGit(repo.RepoDir, "branch", "--list", foreign))); + } + [Fact] public async Task CreateAsync_SelfHealPath_BroadcastsCreatingWorktreePhaseForEachSection() { @@ -122,9 +155,11 @@ public class WorktreeManagerTests : IDisposable await new TaskRepository(seedCtx).AddAsync(task); } - // Pre-create the branch so `git worktree add -b` hits the "already exists" self-heal path. - var branchName = $"claudedo/{task.Id.Replace("-", "")}"; + // Occupy the plain name (-> id-suffixed fallback) and the fallback too, so + // `git worktree add -b` hits the "already exists" self-heal path. + var branchName = $"claudedo/{WorktreeManager.NameFor(task)}"; GitRepoFixture.RunGit(repo.RepoDir, "branch", branchName, repo.BaseCommit); + GitRepoFixture.RunGit(repo.RepoDir, "branch", $"{branchName}-{task.Id.Replace("-", "")[..8]}", repo.BaseCommit); var fakeHub = new CapturingHubContext(); var broadcaster = new HubBroadcaster(fakeHub); @@ -276,13 +311,13 @@ public class WorktreeManagerTests : IDisposable await Assert.ThrowsAnyAsync( () => mgr.CreateAsync(task, list, CancellationToken.None)); - var branchName = $"claudedo/{task.Id.Replace("-", "")}"; + var branchName = $"claudedo/{WorktreeManager.NameFor(task)}"; var branchList = GitRepoFixture.RunGit(repo.RepoDir, "branch", "--list", branchName); Assert.True(string.IsNullOrWhiteSpace(branchList), $"orphaned branch {branchName} should be cleaned up, got: {branchList}"); var worktreeList = GitRepoFixture.RunGit(repo.RepoDir, "worktree", "list"); - Assert.DoesNotContain(task.Id, worktreeList); + Assert.DoesNotContain(WorktreeManager.NameFor(task), worktreeList); } [Fact]