From f8c48e2ed746d84e639b76a574ff2bdf74384842 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Thu, 6 Aug 2026 11:25:32 +0200 Subject: [PATCH] fix(worker): make list_tasks/batch_get_tasks lean by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit list_tasks on a list of ~100 verbosely-described tasks could return 390k+ characters in one call, blowing past the caller's token limit. Both tools now default to lean TaskRefDto references (no Description/Result) and take an includeDescription flag to opt back into the full TaskDto payload — same flag-alongside-nullable-payload idiom already used by BatchGetTaskResult/TaskConfigResult. get_task is unchanged. --- docs/explore-notes/external-mcp.md | 16 +++++- src/ClaudeDo.Worker/External/BatchMcpTools.cs | 30 ++++++++--- .../External/ExternalMcpService.cs | 33 ++++++++++-- .../External/BatchMcpToolsTests.cs | 44 ++++++++++++++-- .../External/ExternalMcpServiceTests.cs | 50 ++++++++++++++++--- 5 files changed, 151 insertions(+), 22 deletions(-) diff --git a/docs/explore-notes/external-mcp.md b/docs/explore-notes/external-mcp.md index bc28c2f6..38ace536 100644 --- a/docs/explore-notes/external-mcp.md +++ b/docs/explore-notes/external-mcp.md @@ -1,8 +1,8 @@ # External MCP tool surface > **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative. -> Last verified against commit `bdee731` (2026-08-05). -> Drift check: `git log --oneline bdee731..HEAD -- src/ClaudeDo.Worker/External` +> Last verified against commit `86f962e` (2026-08-06). +> Drift check: `git log --oneline 86f962e..HEAD -- src/ClaudeDo.Worker/External` > Stable structure only (no line numbers). See docs/explore-notes/README.md. Covers `src/ClaudeDo.Worker/External/` — the always-on MCP tools ClaudeDo exposes to general @@ -27,6 +27,11 @@ session internals, or app-settings writes. Auth via an optional `X-ClaudeDo-Key` the resulting config so the caller can see which fields were set vs. cleared to null). - *Read* tools that may have nothing to return use an explicit `Found` / `Available` flag alongside the nullable payload (`TaskConfigResult`, `BatchGetTaskResult`, `TaskLogResult`). + - The same flag-alongside-nullable-payload idiom also covers "which of two shapes did you + get": `ListTasks`/`BatchGetTasks` take `includeDescription` (default `false`) and return + `ListTasksResult`/`BatchGetTaskResult`, where exactly one of the lean (`TaskRefDto`) and + full (`TaskDto`, incl. Description/Result) fields is populated per the flag — keeps a + list of verbosely-described tasks from blowing past the response size limit by default. 3. `ExternalMcpExceptionFilter.Wrap` is registered as a call-tool filter so `InvalidOperationException` / `ArgumentException` messages survive as `McpException` — otherwise the SDK's catch-all replaces any non-`McpException` with a generic @@ -62,6 +67,11 @@ Daily prep: `GetDailyPrepCandidates`, `SetMyDay`. ## Per-tool behaviour worth knowing +**`ListTasks`** — `includeDescription=false` (default) returns lean `TaskRefDto` references in +`tasks` (`tasksFull` null); `includeDescription=true` returns full `TaskDto`s (incl. +Description/Result) in `tasksFull` instead (`tasks` null). Filtering by `createdBy`/`status` +happens before the lean/full projection either way. + **`UpdateTaskStatus`** accepts `Idle` / `Queued` / `Cancelled` / `Done` only. - `Cancelled` goes through `TaskStateService.CancelAsync(..., allowFromIdle: true)` — the **only** caller that opts into cancelling from `Idle`. `PlanningChainCoordinator` relies on @@ -102,6 +112,8 @@ directory is shared with other concurrent sessions. **Sequential**, because the scoped `DbContext` is not thread-safe. Merge/review stay single-task. Every tool returns a per-item result array (`{ id/index, ok, error?, … }`) — a failing item never aborts the rest — and rejects batches over **100 items**. +`BatchGetTasks` mirrors `ListTasks`'s `includeDescription` flag (default `false`): a found item's +`BatchGetTaskResult` carries `task` (lean) or `taskFull` (full), never both. **`GetTaskLog`** — latest run's log, tail-capped at 256 KB. diff --git a/src/ClaudeDo.Worker/External/BatchMcpTools.cs b/src/ClaudeDo.Worker/External/BatchMcpTools.cs index 7c8fb8bd..86413dc5 100644 --- a/src/ClaudeDo.Worker/External/BatchMcpTools.cs +++ b/src/ClaudeDo.Worker/External/BatchMcpTools.cs @@ -6,7 +6,10 @@ namespace ClaudeDo.Worker.External; public sealed record BatchAddTaskInput(string Title, string? Description = null, string? Model = null); public sealed record BatchSetMyDayInput(string TaskId, bool IsMyDay, int? SortOrder = null); -public sealed record BatchGetTaskResult(string Id, bool Found, TaskDto? Task, string? Error); +// task is populated when found and includeDescription=false (the default, lean reference); +// taskFull is populated when found and includeDescription=true (full task incl. +// Description/Result). Both are null when found=false. +public sealed record BatchGetTaskResult(string Id, bool Found, TaskRefDto? Task, TaskDto? TaskFull, string? Error); public sealed record BatchAddTaskResult(int Index, string Title, bool Ok, TaskRefDto? Task, string? Error); public sealed record BatchTaskResult(string TaskId, bool Ok, string? Error); public sealed record BatchCancelResult(string TaskId, bool Ok, bool Cancelled, string? Error); @@ -30,10 +33,13 @@ public sealed class BatchMcpTools [McpServerTool, Description( "Fetch a snapshot of many tasks in one call (overview / polling a fan-out). " + - "Returns one result per id: { id, found, task, error }. A missing id is found=false " + - "(not an error); error is only set for an unexpected failure. Max 100 ids.")] + "Returns one result per id: { id, found, task, taskFull, error }. " + + "includeDescription=false (default): found tasks come back in `task` (lean reference, no " + + "Description/Result). includeDescription=true: found tasks come back in `taskFull` (incl. " + + "Description/Result) instead. A missing id is found=false (not an error; task and taskFull both null); " + + "error is only set for an unexpected failure. Max 100 ids.")] public async Task> BatchGetTasks( - string[] taskIds, CancellationToken cancellationToken) + string[] taskIds, bool includeDescription = false, CancellationToken cancellationToken = default) { EnsureWithinCap(taskIds, nameof(taskIds)); @@ -42,17 +48,25 @@ public sealed class BatchMcpTools { try { - var task = await _svc.GetTask(id, cancellationToken); - results.Add(new BatchGetTaskResult(id, true, task, null)); + if (includeDescription) + { + var task = await _svc.GetTask(id, cancellationToken); + results.Add(new BatchGetTaskResult(id, true, null, task, null)); + } + else + { + var taskRef = await _svc.GetTaskRefAsync(id, cancellationToken); + results.Add(new BatchGetTaskResult(id, true, taskRef, null, null)); + } } catch (OperationCanceledException) { throw; } catch (InvalidOperationException) { - results.Add(new BatchGetTaskResult(id, false, null, null)); + results.Add(new BatchGetTaskResult(id, false, null, null, null)); } catch (Exception ex) { - results.Add(new BatchGetTaskResult(id, false, null, ex.Message)); + results.Add(new BatchGetTaskResult(id, false, null, null, ex.Message)); } } return results; diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index 962859a6..38e1891f 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -47,6 +47,15 @@ public sealed record TaskRefDto( int SortOrder, bool IsMyDay); +// tasks is populated when includeDescription=false (the default): lean references, no +// Description/Result. tasksFull is populated when includeDescription=true: full tasks incl. +// Description/Result. Exactly one of the two is non-null per the includeDescription flag — +// same "flag alongside nullable payload" idiom as BatchGetTaskResult. +public sealed record ListTasksResult( + bool IncludeDescription, + IReadOnlyList? Tasks, + IReadOnlyList? TasksFull); + public sealed record WorktreeInfoDto( string Path, string Branch, string HeadCommit, string BaseCommit, int Ahead, int Behind, bool IsDirty, string? MergeCommit = null); @@ -139,11 +148,17 @@ public sealed class ExternalMcpService [McpServerTool, Description( "List tasks in a given list. Optionally filter by creator (createdBy) and/or status. " + - "Valid status values: Idle, Queued, Running, WaitingForReview, WaitingForChildren, Done, Failed, Cancelled.")] - public async Task> ListTasks( + "Valid status values: Idle, Queued, Running, WaitingForReview, WaitingForChildren, Done, Failed, Cancelled. " + + "includeDescription=false (default): returns lean task references in `tasks` (no Description/Result) — " + + "use this unless you actually need the description text, since a list of verbosely-described tasks can " + + "otherwise blow past the response size limit. " + + "includeDescription=true: returns full tasks (incl. Description/Result) in `tasksFull` instead; `tasks` is " + + "null in that case.")] + public async Task ListTasks( string listId, string? createdBy = null, string? status = null, + bool includeDescription = false, CancellationToken cancellationToken = default) { TaskStatus? statusFilter = null; @@ -162,7 +177,10 @@ public sealed class ExternalMcpService if (statusFilter is not null) query = query.Where(t => t.Status == statusFilter); - return query.Select(ToDto).ToList(); + var filtered = query.ToList(); + return includeDescription + ? new ListTasksResult(true, null, filtered.Select(ToDto).ToList()) + : new ListTasksResult(false, filtered.Select(ToRefDto).ToList(), null); } [McpServerTool, Description( @@ -177,6 +195,15 @@ public sealed class ExternalMcpService return ToDto(task); } + // Lean counterpart to GetTask, used internally by BatchGetTasks' default (includeDescription=false) + // path. Not an MCP tool itself — GetTask's own behavior stays untouched. + internal async Task GetTaskRefAsync(string taskId, CancellationToken cancellationToken) + { + var task = await _tasks.GetByIdAsync(taskId, cancellationToken) + ?? throw new InvalidOperationException($"Task {taskId} not found."); + return ToRefDto(task); + } + [McpServerTool, Description( "Create a new task in the given list. Set queueImmediately=true to enqueue it for agent execution. " + "Set model to the cheapest model that can do the task well — 'haiku' for trivial/mechanical work, " + diff --git a/tests/ClaudeDo.Worker.Tests/External/BatchMcpToolsTests.cs b/tests/ClaudeDo.Worker.Tests/External/BatchMcpToolsTests.cs index ee7b8830..9c2bb18b 100644 --- a/tests/ClaudeDo.Worker.Tests/External/BatchMcpToolsTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/BatchMcpToolsTests.cs @@ -149,17 +149,55 @@ public sealed class BatchMcpToolsTests : IDisposable var task = await SeedTaskAsync(listId); var sut = BuildSut(); - var results = await sut.BatchGetTasks(new[] { task.Id, "nope" }, CancellationToken.None); + var results = await sut.BatchGetTasks(new[] { task.Id, "nope" }, cancellationToken: CancellationToken.None); var found = results.Single(r => r.Id == task.Id); var missing = results.Single(r => r.Id == "nope"); Assert.True(found.Found); Assert.NotNull(found.Task); + Assert.Null(found.TaskFull); Assert.False(missing.Found); Assert.Null(missing.Task); + Assert.Null(missing.TaskFull); Assert.Null(missing.Error); } + [Fact] + public async Task BatchGetTasks_Default_ReturnsLeanTask_NoTaskFull() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId); + task.Description = "a description that should not come back by default"; + await _tasks.UpdateAsync(task); + var sut = BuildSut(); + + var results = await sut.BatchGetTasks(new[] { task.Id }, cancellationToken: CancellationToken.None); + + var found = results.Single(r => r.Id == task.Id); + Assert.True(found.Found); + Assert.NotNull(found.Task); + Assert.Equal(task.Id, found.Task!.Id); + Assert.Null(found.TaskFull); + } + + [Fact] + public async Task BatchGetTasks_IncludeDescriptionTrue_ReturnsTaskFull() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId); + task.Description = "the full description"; + await _tasks.UpdateAsync(task); + var sut = BuildSut(); + + var results = await sut.BatchGetTasks(new[] { task.Id }, includeDescription: true, cancellationToken: CancellationToken.None); + + var found = results.Single(r => r.Id == task.Id); + Assert.True(found.Found); + Assert.Null(found.Task); + Assert.NotNull(found.TaskFull); + Assert.Equal("the full description", found.TaskFull!.Description); + } + [Fact] public async Task BatchDeleteTasks_RunningTask_ReportedNotOk_OthersDeleted() { @@ -212,7 +250,7 @@ public sealed class BatchMcpToolsTests : IDisposable { var sut = BuildSut(); await Assert.ThrowsAsync( - () => sut.BatchGetTasks(Array.Empty(), CancellationToken.None)); + () => sut.BatchGetTasks(Array.Empty(), cancellationToken: CancellationToken.None)); } [Fact] @@ -222,7 +260,7 @@ public sealed class BatchMcpToolsTests : IDisposable var ids = Enumerable.Range(0, 101).Select(i => i.ToString()).ToArray(); var ex = await Assert.ThrowsAsync( - () => sut.BatchGetTasks(ids, CancellationToken.None)); + () => sut.BatchGetTasks(ids, cancellationToken: CancellationToken.None)); Assert.Contains("max", ex.Message, StringComparison.OrdinalIgnoreCase); } } diff --git a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs index b3644b30..a80efb0b 100644 --- a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs @@ -947,10 +947,11 @@ public sealed class ExternalMcpServiceTests : IDisposable await SeedTaskAsync(listId, "idle", TaskStatus.Idle); var sut = NewService(); - var result = await sut.ListTasks(listId, null, "WaitingForReview", CancellationToken.None); + var result = await sut.ListTasks(listId, null, "WaitingForReview", cancellationToken: CancellationToken.None); - Assert.Single(result); - Assert.Equal("WaitingForReview", result[0].Status); + Assert.NotNull(result.Tasks); + Assert.Single(result.Tasks!); + Assert.Equal("WaitingForReview", result.Tasks![0].Status); } [Fact] @@ -961,10 +962,47 @@ public sealed class ExternalMcpServiceTests : IDisposable await SeedTaskAsync(listId, "done", TaskStatus.Done); var sut = NewService(); - var result = await sut.ListTasks(listId, null, "WaitingForChildren", CancellationToken.None); + var result = await sut.ListTasks(listId, null, "WaitingForChildren", cancellationToken: CancellationToken.None); - Assert.Single(result); - Assert.Equal("WaitingForChildren", result[0].Status); + Assert.NotNull(result.Tasks); + Assert.Single(result.Tasks!); + Assert.Equal("WaitingForChildren", result.Tasks![0].Status); + } + + [Fact] + public async Task ListTasks_Default_ReturnsLeanReferences_NoDescriptionOrTasksFull() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId, "with desc"); + task.Description = "a long description that should not come back by default"; + await _tasks.UpdateAsync(task); + var sut = NewService(); + + var result = await sut.ListTasks(listId, cancellationToken: CancellationToken.None); + + Assert.False(result.IncludeDescription); + Assert.NotNull(result.Tasks); + Assert.Null(result.TasksFull); + Assert.Single(result.Tasks!); + Assert.Equal(task.Id, result.Tasks![0].Id); + } + + [Fact] + public async Task ListTasks_IncludeDescriptionTrue_ReturnsFullTasks() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId, "with desc"); + task.Description = "the full description"; + await _tasks.UpdateAsync(task); + var sut = NewService(); + + var result = await sut.ListTasks(listId, includeDescription: true, cancellationToken: CancellationToken.None); + + Assert.True(result.IncludeDescription); + Assert.Null(result.Tasks); + Assert.NotNull(result.TasksFull); + Assert.Single(result.TasksFull!); + Assert.Equal("the full description", result.TasksFull![0].Description); } // ── MergeTask allowWaitingForReview ───────────────────────────────────────