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]