diff --git a/docs/explore-notes/external-mcp.md b/docs/explore-notes/external-mcp.md index 76c1a243..7a59a0ca 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 `6a2a19c` (2026-08-10). -> Drift check: `git log --oneline 6a2a19c..HEAD -- src/ClaudeDo.Worker/External` +> Last verified against commit `38af549` (2026-08-11). +> Drift check: `git log --oneline 38af549..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 @@ -14,11 +14,34 @@ need no `--mcp-config`. 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. -## 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 `#` 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`). The MCP schema only marks a parameter optional when it has one — nullability alone does 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 an empty/omitted response apart from a dropped one. - *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 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. **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 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 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 - "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 `InvalidOperationException` / `ArgumentException` messages survive as `McpException` — otherwise the SDK's catch-all replaces any non-`McpException` with a generic diff --git a/docs/open.md b/docs/open.md index a5241c32..366d660f 100644 --- a/docs/open.md +++ b/docs/open.md @@ -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`, `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 `#` dimmed vor dem Titel in der Row an (`TaskRowView.axaml` Zeile 45+). Prüfen: + offene Task in der Übersicht hat sichtbar `#` vor dem Titel. +- **Detail Pane Header:** `DetailsIslandViewModel.TaskIdBadge` (Zeile 77, DetailsIslandViewModel.cs) + zeigt jetzt `#` statt des alten GUID-Präfix. Prüfen: Task-Detail-Chip zeigt + `#`. +- **Worker Log Messages:** Geschäftsereignisse in `TaskRunner` (Zeile 199+), `TaskMergeService` + (Zeile 174+), und `TaskResetService` (Zeile 84+) prefixen Task-Titel mit `#`. 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) Fünf Findings der Sichtprüfung sind gefixt, Build + Tests grün, aber **noch nicht in der App diff --git a/src/ClaudeDo.Data/CLAUDE.md b/src/ClaudeDo.Data/CLAUDE.md index 5a3e627d..09c1d99c 100644 --- a/src/ClaudeDo.Data/CLAUDE.md +++ b/src/ClaudeDo.Data/CLAUDE.md @@ -4,10 +4,11 @@ Shared data layer: models, repositories, SQLite infrastructure, and git operatio ## 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. - `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). + - `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. - **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). @@ -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. | | `ReportExcludedPaths` | `report_excluded_paths` | null | JSON array of excluded path prefixes. | | `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`. 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 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` - **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` diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index 4d3fbca9..0d4fdd1e 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -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. - **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`. **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 `#` 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