docs: update for task-numbers features (Slice 5)
Document task-number allocation in Data layer (TaskEntity.Number,
next_task_number counter, invariants, insert paths).
Update Worker docs to clarify TaskIdResolver wiring (#123 → GUID lookup),
Number in MCP payloads, and correct the 'Two hard conventions' statement
(only the first is test-enforced).
Bump external-mcp.md verified-against commit to 38af549 (Slice 4 merge)
and add new sections on task ID resolution and numbering.
Add Slice 4 visual verification items to open.md (row/detail number display,
worker-log messages).
Verified against:
- src/ClaudeDo.Data/TaskNumberAllocator.cs
- src/ClaudeDo.Data/Repositories/TaskRepository.cs (AddAsync line 20, CreateChildAsync line 276)
- src/ClaudeDo.Worker/External/TaskIdResolver.cs
- src/ClaudeDo.Worker/External/ExternalMcpService.cs (TaskDto/TaskRefDto DTOs)
- tests/ClaudeDo.Worker.Tests/External/ExternalMcpToolSchemaTests.cs (only first convention test-enforced)
This commit is contained in:
@@ -1,8 +1,8 @@
|
|||||||
# External MCP tool surface
|
# External MCP tool surface
|
||||||
|
|
||||||
> **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative.
|
> **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative.
|
||||||
> Last verified against commit `6a2a19c` (2026-08-10).
|
> Last verified against commit `38af549` (2026-08-11).
|
||||||
> Drift check: `git log --oneline 6a2a19c..HEAD -- src/ClaudeDo.Worker/External`
|
> Drift check: `git log --oneline 38af549..HEAD -- src/ClaudeDo.Worker/External`
|
||||||
> Stable structure only (no line numbers). See docs/explore-notes/README.md.
|
> 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
|
Covers `src/ClaudeDo.Worker/External/` — the always-on MCP tools ClaudeDo exposes to general
|
||||||
@@ -14,11 +14,34 @@ need no `--mcp-config`.
|
|||||||
and git/merge operations. They deliberately do **not** expose multi-turn control, planning
|
and git/merge operations. They deliberately do **not** expose multi-turn control, planning
|
||||||
session internals, or app-settings writes. Auth via an optional `X-ClaudeDo-Key` header.
|
session internals, or app-settings writes. Auth via an optional `X-ClaudeDo-Key` header.
|
||||||
|
|
||||||
## Hard conventions (enforced by tests)
|
## Task ID resolution and numbering
|
||||||
|
|
||||||
|
Every tool parameter accepting a task id — `taskId`, `parentId`, `taskIds` arrays, and similar —
|
||||||
|
is wired through `TaskIdResolver`, which resolves:
|
||||||
|
- `#123` (with or without the `#` prefix) → look up by `TaskEntity.Number`
|
||||||
|
- Bare GUID string → use as-is
|
||||||
|
- Unknown number → `InvalidOperationException` (`no task with number 123`)
|
||||||
|
|
||||||
|
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.
|
||||||
|
|
||||||
|
Every tool description carries a shared boilerplate clause (defined in `McpToolDocs.TaskNumberHint`)
|
||||||
|
instructing the agent to **refer to tasks as `#<number>` when reporting results to the user** —
|
||||||
|
without this the agent sees the number in every payload but never learns to speak it.
|
||||||
|
|
||||||
|
## Conventions
|
||||||
|
|
||||||
|
### Test-enforced
|
||||||
|
|
||||||
1. **Every optional/filter parameter needs a C# default value** (e.g. `string? status = null`).
|
1. **Every optional/filter parameter needs a C# default value** (e.g. `string? status = null`).
|
||||||
The MCP schema only marks a parameter optional when it has one — nullability alone does
|
The MCP schema only marks a parameter optional when it has one — nullability alone does
|
||||||
not do it. `ExternalMcpToolSchemaTests` guards this by reflection.
|
not do it. `ExternalMcpToolSchemaTests` guards this by reflection.
|
||||||
|
|
||||||
|
### Not test-enforced, but strongly observed
|
||||||
|
|
||||||
2. **No tool returns bare `Task` or a nullable payload directly.** An MCP client cannot tell
|
2. **No tool returns bare `Task` or a nullable payload directly.** An MCP client cannot tell
|
||||||
an empty/omitted response apart from a dropped one.
|
an empty/omitted response apart from a dropped one.
|
||||||
- *Write* tools return a small confirmation record — `{ ok/deleted/removed/reset/started:
|
- *Write* tools return a small confirmation record — `{ ok/deleted/removed/reset/started:
|
||||||
@@ -32,13 +55,15 @@ session internals, or app-settings writes. Auth via an optional `X-ClaudeDo-Key`
|
|||||||
`ListTasksResult`/`BatchGetTaskResult`, where exactly one of the lean (`TaskRefDto`) and
|
`ListTasksResult`/`BatchGetTaskResult`, where exactly one of the lean (`TaskRefDto`) and
|
||||||
full (`TaskDto`, incl. Description/Result) fields is populated per the flag — keeps a
|
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.
|
list of verbosely-described tasks from blowing past the response size limit by default.
|
||||||
|
|
||||||
3. **Description style is documented in `McpToolDocs`** (same folder) and shared boilerplate
|
3. **Description style is documented in `McpToolDocs`** (same folder) and shared boilerplate
|
||||||
lives there as `const` strings. Rules: the first sentence says what the tool does *and* when
|
lives there as `const` strings. Rules: the first sentence says what the tool does *and* when
|
||||||
to reach for it (MCP clients rank tools by that text, so the trigger must not sit behind
|
to reach for it (MCP clients rank tools by that text, so the trigger must not sit behind
|
||||||
return-shape prose); parameters are documented with `[Description]` **on the parameter**, not
|
return-shape prose); parameters are documented with `[Description]` **on the parameter**, not
|
||||||
in the tool description; result fields appear only where the caller must branch on them
|
in the tool description; result fields appear only where the caller must branch on them
|
||||||
before calling (`isEmpty`, `truncated`, `conflicts`, `available`); no design rationale or
|
before calling (`isEmpty`, `truncated`, `conflicts`, `available`); no design rationale or
|
||||||
"since this feature was introduced" history. Not test-enforced — review it in PRs.
|
"since this feature was introduced" history.
|
||||||
|
|
||||||
4. `ExternalMcpExceptionFilter.Wrap` is registered as a call-tool filter so
|
4. `ExternalMcpExceptionFilter.Wrap` is registered as a call-tool filter so
|
||||||
`InvalidOperationException` / `ArgumentException` messages survive as `McpException` —
|
`InvalidOperationException` / `ArgumentException` messages survive as `McpException` —
|
||||||
otherwise the SDK's catch-all replaces any non-`McpException` with a generic
|
otherwise the SDK's catch-all replaces any non-`McpException` with a generic
|
||||||
|
|||||||
@@ -81,6 +81,21 @@ verifiziert und deshalb hier entfernt. Es bleiben die Entscheidungen, die daran
|
|||||||
`dotnet test` invocation as the configured command) — only fast synthetic commands (`exit N`,
|
`dotnet test` invocation as the configured command) — only fast synthetic commands (`exit N`,
|
||||||
`ping` for timeout) were exercised.
|
`ping` for timeout) were exercised.
|
||||||
|
|
||||||
|
## Offene Verifikation (2026-08-11, Task numbers in UI)
|
||||||
|
|
||||||
|
Slice 4 der Task-Numbers-Features (UI-Display) ist gemerged (commit 38af549); Build + Tests grün,
|
||||||
|
aber **nicht visuell verifiziert**:
|
||||||
|
|
||||||
|
- **Task Row Display:** `TaskRowViewModel.Number` (Zeile 39, TaskRowViewModel.cs) zeigt die
|
||||||
|
Nummer als `#<number>` dimmed vor dem Titel in der Row an (`TaskRowView.axaml` Zeile 45+). Prüfen:
|
||||||
|
offene Task in der Übersicht hat sichtbar `#<number>` vor dem Titel.
|
||||||
|
- **Detail Pane Header:** `DetailsIslandViewModel.TaskIdBadge` (Zeile 77, DetailsIslandViewModel.cs)
|
||||||
|
zeigt jetzt `#<number>` statt des alten GUID-Präfix. Prüfen: Task-Detail-Chip zeigt
|
||||||
|
`#<number>`.
|
||||||
|
- **Worker Log Messages:** Geschäftsereignisse in `TaskRunner` (Zeile 199+), `TaskMergeService`
|
||||||
|
(Zeile 174+), und `TaskResetService` (Zeile 84+) prefixen Task-Titel mit `#<number>`. Prüfen:
|
||||||
|
Footer Worker-Log zeigt Task-Messages wie „`#42 finished`" statt nur dem GUID.
|
||||||
|
|
||||||
## Offene Verifikation (2026-08-06, Fix-Batch aus der Sichtprüfung)
|
## Offene Verifikation (2026-08-06, Fix-Batch aus der Sichtprüfung)
|
||||||
|
|
||||||
Fünf Findings der Sichtprüfung sind gefixt, Build + Tests grün, aber **noch nicht in der App
|
Fünf Findings der Sichtprüfung sind gefixt, Build + Tests grün, aber **noch nicht in der App
|
||||||
|
|||||||
@@ -4,10 +4,11 @@ Shared data layer: models, repositories, SQLite infrastructure, and git operatio
|
|||||||
|
|
||||||
## Models
|
## Models
|
||||||
|
|
||||||
- **TaskEntity** — Id, ListId, Title, Description, Status, PlanningPhase, BlockedByTaskId (FK to predecessor in a chain), DependsOnTaskId (FK to a user/MCP-declared predecessor, distinct from BlockedByTaskId), ScheduledFor, Result, ReviewFeedback, LogPath, timestamps, CommitType, Model / SystemPrompt / AgentPath / MaxTurns (nullable overrides), IsStarred, IsMyDay, IsManual, Notes, ParentTaskId, PlanningSessionId / PlanningSessionToken / PlanningFinalizedAt, CreatedBy, HandlerBaseCommit / HandlerHeadCommit, InteractiveSessionId.
|
- **TaskEntity** — Id, ListId, Title, Description, Status, PlanningPhase, BlockedByTaskId (FK to predecessor in a chain), DependsOnTaskId (FK to a user/MCP-declared predecessor, distinct from BlockedByTaskId), ScheduledFor, Result, ReviewFeedback, LogPath, timestamps, CommitType, Model / SystemPrompt / AgentPath / MaxTurns (nullable overrides), IsStarred, IsMyDay, IsManual, Notes, ParentTaskId, PlanningSessionId / PlanningSessionToken / PlanningFinalizedAt, CreatedBy, HandlerBaseCommit / HandlerHeadCommit, InteractiveSessionId, Number (global immutable human-readable alias).
|
||||||
- Status / PlanningPhase / BlockedByTaskId / DependsOnTaskId semantics + allowed transitions: `ClaudeDo.Worker/CLAUDE.md` → Status Model.
|
- Status / PlanningPhase / BlockedByTaskId / DependsOnTaskId semantics + allowed transitions: `ClaudeDo.Worker/CLAUDE.md` → Status Model.
|
||||||
- `HandlerBaseCommit`/`HandlerHeadCommit` = the review range for a **worktree-less "list handler" host task** ("Let Claude handle it"), which commits straight into the list's working dir instead of a per-task worktree. Everything that reads a task's diff falls back to this pair whenever `Worktree` is null → [conpty-sessions](../../docs/explore-notes/conpty-sessions.md).
|
- `HandlerBaseCommit`/`HandlerHeadCommit` = the review range for a **worktree-less "list handler" host task** ("Let Claude handle it"), which commits straight into the list's working dir instead of a per-task worktree. Everything that reads a task's diff falls back to this pair whenever `Worktree` is null → [conpty-sessions](../../docs/explore-notes/conpty-sessions.md).
|
||||||
- `InteractiveSessionId` = the claude session id an embedded ConPTY interactive task session runs under, persisted by `InteractiveLaunchSpecService` before launch so a closed/aborted session can be resumed → [conpty-sessions](../../docs/explore-notes/conpty-sessions.md).
|
- `InteractiveSessionId` = the claude session id an embedded ConPTY interactive task session runs under, persisted by `InteractiveLaunchSpecService` before launch so a closed/aborted session can be resumed → [conpty-sessions](../../docs/explore-notes/conpty-sessions.md).
|
||||||
|
- `Number` (INTEGER NOT NULL, unique index `idx_tasks_number`) — a global, monotonically increasing display alias for tasks. **Never reused**: deleting a task leaves a gap in numbering, ensuring that a task number that appears in a log or report always points to the same task (if it exists). Allocated by `TaskNumberAllocator.AddWithNumberAsync` on every insert via a persistent counter (`app_settings.next_task_number`). **Why not `MAX(number) + 1`:** deleting the highest-numbered task would free its number for reuse, breaking the immutability guarantee. The allocator uses an `UPDATE…RETURNING` statement to claim numbers atomically, retrying up to 5 times if a uniqueness collision occurs (the counter advances on each attempt, so retries always allocate fresh numbers). ⚠️ Tests use `EnsureCreated`, which bypasses migrations — the backfill needs a migration that runs `Migrate()` explicitly (not tested by default; see `worker-task-pipeline` notes).
|
||||||
- Legacy status values `Manual`/`Planning`/`Planned`/`Draft`/`Waiting` were retired; existing rows backfill via the `RetireLegacyTaskStatus` migration.
|
- Legacy status values `Manual`/`Planning`/`Planned`/`Draft`/`Waiting` were retired; existing rows backfill via the `RetireLegacyTaskStatus` migration.
|
||||||
- **ListEntity** — Id, Name, WorkingDir, DefaultCommitType, CreatedAt, IsManual (reminder list — tasks created here default to `IsManual`)
|
- **ListEntity** — Id, Name, WorkingDir, DefaultCommitType, CreatedAt, IsManual (reminder list — tasks created here default to `IsManual`)
|
||||||
- **ListConfigEntity** — ListId (PK, 1:1), Model, SystemPrompt, AgentPath, MaxTurns, SessionSkills, VerifyCommand (all nullable). `VerifyCommand` is an optional post-merge gate; null/blank = no gate → [review-merge](../../docs/explore-notes/review-merge.md).
|
- **ListConfigEntity** — ListId (PK, 1:1), Model, SystemPrompt, AgentPath, MaxTurns, SessionSkills, VerifyCommand (all nullable). `VerifyCommand` is an optional post-merge gate; null/blank = no gate → [review-merge](../../docs/explore-notes/review-merge.md).
|
||||||
@@ -33,6 +34,7 @@ Beyond the basics it carries:
|
|||||||
| `DailyPrepMaxTasks` | `daily_prep_max_tasks` | 5 | Hard cap on MyDay tasks the daily prep may place. |
|
| `DailyPrepMaxTasks` | `daily_prep_max_tasks` | 5 | Hard cap on MyDay tasks the daily prep may place. |
|
||||||
| `ReportExcludedPaths` | `report_excluded_paths` | null | JSON array of excluded path prefixes. |
|
| `ReportExcludedPaths` | `report_excluded_paths` | null | JSON array of excluded path prefixes. |
|
||||||
| `StandupWeekday` | `standup_weekday` | Wednesday | int `DayOfWeek`. |
|
| `StandupWeekday` | `standup_weekday` | Wednesday | int `DayOfWeek`. |
|
||||||
|
| `NextTaskNumber` | `next_task_number` | 1 | Persistent counter for `TaskEntity.Number` allocation. Incremented atomically in `TaskNumberAllocator.AddWithNumberAsync` on every task insert. **Monotonic only:** the counter itself advances across all tasks (no per-list numbering), and it never resets. A deleted task leaves a gap. |
|
||||||
|
|
||||||
All four usage percentages are clamped 0..100 by `AppSettingsRepository.UpdateAsync`.
|
All four usage percentages are clamped 0..100 by `AppSettingsRepository.UpdateAsync`.
|
||||||
Gate/throttle semantics → [usage-monitoring](../../docs/explore-notes/usage-monitoring.md).
|
Gate/throttle semantics → [usage-monitoring](../../docs/explore-notes/usage-monitoring.md).
|
||||||
@@ -48,7 +50,7 @@ Gate/throttle semantics → [usage-monitoring](../../docs/explore-notes/usage-mo
|
|||||||
All use EF Core LINQ via `ClaudeDoDbContext`. The atomic `Queued → Running` claim lives in the
|
All use EF Core LINQ via `ClaudeDoDbContext`. The atomic `Queued → Running` claim lives in the
|
||||||
Worker's `QueuePicker` (`FromSqlRaw`), **not** here.
|
Worker's `QueuePicker` (`FromSqlRaw`), **not** here.
|
||||||
|
|
||||||
- **TaskRepository** — CRUD, planning helpers (`CreateChildAsync`, `SetPlanningStartedAsync`, `DiscardPlanningAsync`, `UpdateChildAsync`), `UpdateAgentSettingsAsync`. ⚠️ Status-mutation primitives (`MarkRunningAsync`/`MarkDoneAsync`/`MarkFailedAsync`/`FlipAllRunningToFailedAsync`) are **`internal`** — only the Worker's `TaskStateService` may call them. `CreateChildAsync` produces children with `Status=Idle, PlanningPhase=None`.
|
- **TaskRepository** — CRUD, planning helpers (`CreateChildAsync`, `SetPlanningStartedAsync`, `DiscardPlanningAsync`, `UpdateChildAsync`), `UpdateAgentSettingsAsync`. ⚠️ Status-mutation primitives (`MarkRunningAsync`/`MarkDoneAsync`/`MarkFailedAsync`/`FlipAllRunningToFailedAsync`) are **`internal`** — only the Worker's `TaskStateService` may call them. `CreateChildAsync` produces children with `Status=Idle, PlanningPhase=None`. **Task-number allocation:** `AddAsync` (line 20) and `CreateChildAsync` (line 276) are the two sole task-creation paths; both call `TaskNumberAllocator.AddWithNumberAsync` to allocate a number atomically during the same transaction as the insert. Every other creation path (UI, MCP `add_task`, `batch_add_tasks`, Online Inbox sync, merge-helper handler tasks, planning children) routes through one of these two.
|
||||||
- **ListRepository** — CRUD, `GetConfigAsync`/`SetConfigAsync` (upsert)/`DeleteConfigAsync` for `list_config`
|
- **ListRepository** — CRUD, `GetConfigAsync`/`SetConfigAsync` (upsert)/`DeleteConfigAsync` for `list_config`
|
||||||
- **WorktreeRepository** — CRUD, `UpdateHeadAsync`, `SetStateAsync`, `SetMergedAsync` (atomically sets State=Merged **and** stamps MergeCommit in one update — the only writer of MergeCommit)
|
- **WorktreeRepository** — CRUD, `UpdateHeadAsync`, `SetStateAsync`, `SetMergedAsync` (atomically sets State=Merged **and** stamps MergeCommit in one update — the only writer of MergeCommit)
|
||||||
- **TaskAttachmentRepository** — `AddAsync`, `UpdateAsync`, `GetAsync(taskId, fileName)`, `ListByTaskIdAsync`, `DeleteAsync(taskId, fileName)`, `DeleteAllForTaskAsync`
|
- **TaskAttachmentRepository** — `AddAsync`, `UpdateAsync`, `GetAsync(taskId, fileName)`, `ListByTaskIdAsync`, `DeleteAsync(taskId, fileName)`, `DeleteAllForTaskAsync`
|
||||||
|
|||||||
@@ -50,7 +50,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.
|
- **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).
|
- **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`.
|
- **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`. **Two hard conventions** (both test-enforced): every optional parameter needs a C# default value, and no tool returns bare `Task`/a nullable payload. **Tool-description style** (not test-enforced) is documented in `External/McpToolDocs.cs`, which also holds the shared boilerplate clauses — read it before adding or editing a tool description. 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. 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 `#<number>` 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
|
## Status Model
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user