diff --git a/docs/reference/mcp-tools/tasks.md b/docs/reference/mcp-tools/tasks.md index 8c90f53..8fa474d 100644 --- a/docs/reference/mcp-tools/tasks.md +++ b/docs/reference/mcp-tools/tasks.md @@ -93,7 +93,7 @@ lease and refuses if someone else holds it); to close a task prefer | `title` | str? | `null` | New short label for the work. | | `description` | str? | `null` | New full description. Replaces the existing text; it is not appended to. | | `type` | `bug` \| `feature` \| `task` \| `chore` \| `epic`? | `null` | ``bug`` \| ``feature`` \| ``task`` \| ``chore`` \| ``epic``. | -| `status` | `open` \| `in_progress` \| `blocked` \| `closed`? | `null` | ``open`` \| ``in_progress`` \| ``blocked`` \| ``closed``. Two values have
side effects: ``in_progress`` stamps a fresh ``claimed_at`` lease, and
``closed`` stamps ``closed_at`` **and unblocks this task's dependents**,
exactly as ``task_close`` does. Prefer ``task_claim`` / ``task_close``
for those two transitions — they carry the ownership checks this does
not. | +| `status` | `open` \| `in_progress` \| `blocked` \| `closed`? | `null` | ``open`` \| ``in_progress`` \| ``blocked`` \| ``closed``. Two values have
side effects: ``in_progress`` stamps a fresh ``claimed_at`` lease, and
``closed`` stamps ``closed_at`` **and unblocks this task's dependents**,
exactly as ``task_close`` does. Prefer ``task_claim`` / ``task_close``
for those two transitions — they carry the ownership checks this does
not.
Setting ``in_progress`` with no ``assignee`` on a task that has none
recorded defaults it via ``_claim_holder`` — the same identity
``task_claim`` would use — rather than leaving
``(in_progress, claimed_at set, assignee null)`` reachable: that
combination is a live lease with nobody named on it, which
``task_claim`` correctly refuses and correctly cannot say who holds.
A task that already has an assignee keeps it; this only fills a gap,
never reassigns. | | `priority` | `p0` \| `p1` \| `p2` \| `p3`? | `null` | ``p0`` (highest) … ``p3``. Drives ``task_ready`` ordering. | | `repo` | str? | `null` | Canonical repo URI to (re)assign this task to. Pass an explicit value to
correct tasks that were created without proper repo context. | | `assignee` | str? | `null` | Holder identity to reassign the task to. Prefer ``task_claim`` to take a
task for yourself — it checks nobody else holds it, which this does not. | @@ -102,6 +102,7 @@ lease and refuses if someone else holds it); to close a task prefer | `external_uri` | str? | `null` | Reference URI — e.g. the GitHub issue or PR this task tracks. | | `symbol_refs` | list[str]? | `null` | Code-graph symbol ids (``repo#path::Name``) this task concerns.
Replaces the existing list. | | `tags` | list[str]? | `null` | Free-form tags. Replaces the existing list rather than merging into it. | +| `session_id` | str? | `null` | The calling agent session's id, used only to qualify the ``assignee``
this defaults when ``status="in_progress"`` leaves one to fill in —
see ``task_claim``'s ``session_id`` for why a deployed caller needs to
pass this explicitly (no shared environment to infer it from). Has no
effect when an explicit ``assignee`` is given, or when one already
exists on the task. Appended after every other parameter, not placed
near ``assignee``, so inserting it cannot shift what an existing
positional caller's later arguments bind to. | ## `task_claim` diff --git a/mcp/servers/witan/CHANGELOG.md b/mcp/servers/witan/CHANGELOG.md index 747368b..1a596af 100644 --- a/mcp/servers/witan/CHANGELOG.md +++ b/mcp/servers/witan/CHANGELOG.md @@ -6,6 +6,52 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/) (pre-1.0: a MINOR bump may include breaking changes). +## [0.29.1] - 2026-08-24 + +### Fixed + +- **`task_update(status="in_progress")` could stamp a lease with nobody named + as holder.** It set `claimed_at` — deliberately, so "started" has one + representation rather than reading as instantly and permanently free — but + never touched `assignee`, so a caller that used it instead of `task_claim` + to mark work started (a common substitution: "update the status" reads as + the obvious verb, and the docstring only *preferred* `task_claim` rather + than refusing) left the task at `(in_progress, claimed_at set, assignee + null)`. `task_claim` correctly refuses that state and correctly cannot say + who holds it — which is exactly right once reached, but the state should + not have been reachable through the normal tool surface in the first + place. Live-checked against the deployed graph: several tasks across + unrelated projects are in it right now, so this was not a one-off. + + `task_update` now defaults a missing `assignee` the same way `task_claim` + does — via `_claim_holder`, qualified by a new `session_id` parameter for + a deployed caller the same way `task_claim`'s already is — but only to + fill a genuine gap: a task that already has an assignee keeps it, so + marking someone else's task `in_progress` to log status does not silently + reassign it. The fill is resolved by `_update_task` itself, against the + read it takes immediately before writing — not from a value `task_update` + precomputed off an earlier, separate read — so a real `task_claim` landing + in between cannot be silently overwritten by a stale default. Existing + rows already in the broken state are left to their lease rather than + swept — a `readiness.status_pickable` task without a live lease is + reclaimable regardless of `assignee`, and backfilling one for another + actor's already-in-progress work would mean guessing an identity there is + no way to know correctly. + + A blank `assignee` is now normalised to "not provided" rather than written + through. `_claim_holder` had always read one that way (`if assignee:`) + while the write path tested `is not None`, so the two disagreed and an + explicit `assignee=""` slipped past both halves of the fix above — the + blank was written AND the default declined to fill it, reconstructing the + same unnameable-holder state through the very parameter meant to prevent + it. Worse, on a task that already had a holder the blank silently + *cleared* it: a live lease whose owner was erased by a call that named + nobody. Blank now means "leave the assignee alone", so it fills a gap like + any other missing value and never clears one; whitespace counts as blank, + since a holder is displayed to humans and matched by `_holder_matches` and + `" "` names nobody either. Clearing an assignee is still deliberately not + offered here. + ## [0.29.0] - 2026-08-24 ### Fixed diff --git a/mcp/servers/witan/pyproject.toml b/mcp/servers/witan/pyproject.toml index f58ceb4..9da250d 100644 --- a/mcp/servers/witan/pyproject.toml +++ b/mcp/servers/witan/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "witan-council" -version = "0.29.0" +version = "0.29.1" description = "witan — agent memory, planning, and collaboration graph (work-coordination layer + umbrella CLI)" readme = "README.md" license = "BSD-3-Clause" @@ -213,7 +213,7 @@ packages = ["witan"] "schema" = "schema" [tool.bumpversion] -current_version = "0.29.0" +current_version = "0.29.1" allow_dirty = true [[tool.bumpversion.files]] diff --git a/mcp/servers/witan/tests/test_tasks.py b/mcp/servers/witan/tests/test_tasks.py index b31b0d6..54a0615 100644 --- a/mcp/servers/witan/tests/test_tasks.py +++ b/mcp/servers/witan/tests/test_tasks.py @@ -411,6 +411,155 @@ def test_task_update_to_in_progress_stamps_claimed_at(server): assert node["claimed_at"] is not None +@requires_omnigraph +def test_task_update_to_in_progress_defaults_a_missing_assignee(server): + # tk-task-update-can-still-manufacture-an-unnameable--9ba86a: this used to + # stamp claimed_at without ever setting assignee, reaching exactly the + # state task_claim correctly refuses and correctly cannot name a holder + # for — a live lease with nobody on record. + from witan import server as srv + + t = server.task_create(title="marked started, no assignee given", description="x") + server.task_update(t["slug"], status="in_progress") + node = server.task_get(t["slug"]) + assert node["assignee"] == srv._current_author() + + +@requires_omnigraph +def test_task_update_to_in_progress_does_not_clobber_an_existing_assignee(server): + # Only fills a GAP. A colleague marking someone else's task in_progress to + # log status must not silently reassign it to themselves. + t = server.task_create(title="already held", description="x") + server.task_claim(t["slug"], assignee="original-holder") + + server.task_update(t["slug"], status="in_progress") + + assert server.task_get(t["slug"])["assignee"] == "original-holder" + + +@requires_omnigraph +def test_task_update_to_in_progress_respects_an_explicit_assignee(server): + t = server.task_create(title="explicit assignee", description="x") + server.task_update(t["slug"], status="in_progress", assignee="explicit-holder") + assert server.task_get(t["slug"])["assignee"] == "explicit-holder" + + +@requires_omnigraph +def test_task_update_to_in_progress_treats_a_blank_assignee_as_missing(server): + # The gap the first fix left, caught by Copilot as a suppressed review + # comment on #283 — which files no thread, so it did not show up in the + # unresolved count and was nearly missed. + # + # `_claim_holder` reads a blank assignee as missing (`if assignee:`) while + # the write path tested `is not None`, so an explicit "" was written + # straight through AND skipped the default: claimed_at stamped, no + # nameable holder. Exactly the state this task exists to make + # unrepresentable, reachable through the parameter meant to prevent it. + from witan import server as srv + + t = server.task_create(title="blank assignee, marked started", description="x") + server.task_update(t["slug"], status="in_progress", assignee="") + node = server.task_get(t["slug"]) + assert node["assignee"] == srv._current_author() + assert node["claimed_at"] + + +@requires_omnigraph +def test_task_update_to_in_progress_treats_a_whitespace_assignee_as_missing(server): + # Whitespace names nobody either, and a holder is both shown to humans and + # matched by _holder_matches, so " " must not survive as an identity. + from witan import server as srv + + t = server.task_create(title="whitespace assignee", description="x") + server.task_update(t["slug"], status="in_progress", assignee=" ") + assert server.task_get(t["slug"])["assignee"] == srv._current_author() + + +@requires_omnigraph +def test_task_update_blank_assignee_does_not_clear_an_existing_holder(server): + # Blank means "not provided", not "unassign". A task that already has a + # holder keeps it — the same gap-filling-only rule the non-blank path + # follows, and the reason this normalises to None rather than erroring. + t = server.task_create(title="held, blank passed", description="x") + server.task_claim(t["slug"], assignee="original-holder") + + server.task_update(t["slug"], status="in_progress", assignee="") + + assert server.task_get(t["slug"])["assignee"] == "original-holder" + + +@requires_omnigraph +def test_task_update_defaulted_assignee_is_qualified_by_session_id(server): + # Same qualification task_claim applies — see + # test_caller_supplied_session_id_beats_the_server_environment — so a + # deployed caller marking its own work in_progress via task_update, + # without a session in the server's own environment to fall back to, + # still gets a holder two of its own parallel sessions cannot collide on. + from witan import server as srv + + t = server.task_create(title="deployed caller", description="x") + server.task_update( + t["slug"], + status="in_progress", + session_id="cccccccc-9999-0000-1111-222222222222", + ) + node = server.task_get(t["slug"]) + assert node["assignee"] == f"{srv._current_author()}#cccccccc" + + +@requires_omnigraph +def test_update_task_default_if_missing_does_not_override_a_real_assignee(server): + """`_update_task`'s `default_if_missing` checks the row's CURRENT value — + read by this same call, not decided earlier by the caller (see its + docstring: a caller that precomputed the value off its own separate read + could hand back a stale default that clobbers a real assignment made in + between). A task already claimed for real must keep that assignee even + when a `default_if_missing["assignee"]` factory is supplied. + """ + from witan import server as srv + + t = server.task_create(title="already claimed", description="x") + server.task_claim(t["slug"], assignee="real-claimant") + + srv._update_task( + t["slug"], + {"status": "in_progress"}, + default_if_missing={"assignee": lambda: "stale-default"}, + ) + + assert server.task_get(t["slug"])["assignee"] == "real-claimant" + + +@requires_omnigraph +def test_update_task_default_if_missing_fills_a_genuine_gap(server): + from witan import server as srv + + t = server.task_create(title="never claimed", description="x") + + srv._update_task( + t["slug"], + {"status": "in_progress"}, + default_if_missing={"assignee": lambda: "filled-in"}, + ) + + assert server.task_get(t["slug"])["assignee"] == "filled-in" + + +@requires_omnigraph +def test_update_task_default_if_missing_never_overrides_an_explicit_change(server): + from witan import server as srv + + t = server.task_create(title="explicit wins", description="x") + + srv._update_task( + t["slug"], + {"status": "in_progress", "assignee": "explicit"}, + default_if_missing={"assignee": lambda: "should-not-be-used"}, + ) + + assert server.task_get(t["slug"])["assignee"] == "explicit" + + @requires_omnigraph def test_unleased_recent_in_progress_is_not_ready_or_claimable(server): """A task moved to in_progress with no assignee on record (e.g. a legacy row diff --git a/mcp/servers/witan/witan/server.py b/mcp/servers/witan/witan/server.py index f115c83..c47f527 100644 --- a/mcp/servers/witan/witan/server.py +++ b/mcp/servers/witan/witan/server.py @@ -15,7 +15,7 @@ import time import uuid from collections import Counter -from collections.abc import Iterable, Iterator +from collections.abc import Callable, Iterable, Iterator from contextlib import contextmanager from datetime import datetime, timedelta, timezone from pathlib import Path @@ -5085,6 +5085,7 @@ def _update_task( surface_conflict: bool = False, conditional: bool = False, extra_steps: list[_Step] | None = None, + default_if_missing: dict[str, Callable[[], object]] | None = None, ) -> tuple[dict | None, str | None]: """Read a task, merge ``changes`` over its mutable fields, write it back. @@ -5133,6 +5134,22 @@ def _update_task( A missing task still writes NOTHING, extras included — the early return below happens before any step is issued. + + ``default_if_missing`` fills a field ONLY from the read this call itself + just took, never from a value a caller precomputed earlier. That + distinction is the whole point: ``task_update``'s own gap-fill for + ``assignee`` used to read the row, decide off THAT snapshot, then hand + the decided value back here as an ordinary ``changes`` entry — a second, + avoidable read-then-decide sitting in front of the read-then-decide this + function already does, doubling the window in which something else + (``task_claim``, most obviously) could set the field for real between + the two reads. `merged` below is built from `current`, read moments + before the write goes out, so resolving here — not before calling this + function — closes that avoidable half of the gap. It does NOT make the + write itself compare-and-swap: an unconditional ``task_update`` call can + still race its own write the same way every other field here already + can (see the CAS paragraph above); this only removes the extra, earlier + window this helper does not need to have. """ if conditional and extra_steps: # Refused rather than silently downgraded. `change_many` composes the @@ -5146,6 +5163,10 @@ def _update_task( if not rows: return None, None current = rows[0] + if default_if_missing: + for field, factory in default_if_missing.items(): + if field not in changes and not current.get(field): + changes = {**changes, field: factory()} merged = { "slug": slug, "title": changes.get("title", current.get("title")), @@ -5420,6 +5441,7 @@ def task_update( external_uri: str | None = None, symbol_refs: list[str] | None = None, tags: list[str] | None = None, + session_id: str | None = None, ) -> dict | None: """ Update a task's mutable fields. Only non-null arguments are applied. @@ -5447,6 +5469,15 @@ def task_update( exactly as ``task_close`` does. Prefer ``task_claim`` / ``task_close`` for those two transitions — they carry the ownership checks this does not. + + Setting ``in_progress`` with no ``assignee`` on a task that has none + recorded defaults it via ``_claim_holder`` — the same identity + ``task_claim`` would use — rather than leaving + ``(in_progress, claimed_at set, assignee null)`` reachable: that + combination is a live lease with nobody named on it, which + ``task_claim`` correctly refuses and correctly cannot say who holds. + A task that already has an assignee keeps it; this only fills a gap, + never reassigns. priority: ``p0`` (highest) … ``p3``. Drives ``task_ready`` ordering. repo: @@ -5473,7 +5504,36 @@ def task_update( Replaces the existing list. tags: Free-form tags. Replaces the existing list rather than merging into it. - """ + session_id: + The calling agent session's id, used only to qualify the ``assignee`` + this defaults when ``status="in_progress"`` leaves one to fill in — + see ``task_claim``'s ``session_id`` for why a deployed caller needs to + pass this explicitly (no shared environment to infer it from). Has no + effect when an explicit ``assignee`` is given, or when one already + exists on the task. Appended after every other parameter, not placed + near ``assignee``, so inserting it cannot shift what an existing + positional caller's later arguments bind to. + """ + # A blank ``assignee`` is "not provided", not "provided as nothing". + # ``_claim_holder`` already reads it that way (``if assignee:``), and + # without this the two disagree: the ``is not None`` test below would write + # the blank straight through while the ``in_progress`` default further down + # declined to fill it, reconstructing the exact + # ``(in_progress, claimed_at set, no nameable holder)`` state this surface + # exists to make unrepresentable — see the ``status`` parameter's docstring. + # Normalised once, here, so every later branch agrees on what "provided" + # means rather than each re-deciding. Whitespace counts as blank because a + # holder is displayed to humans and matched by ``_holder_matches``; " " + # names nobody either. + # + # Not an error: the caller's intent (leave the assignee alone) is + # unambiguous and already expressible, so refusing would only turn a + # harmless call into a failed one. Clearing an assignee is deliberately + # still not offered here — the docstring calls this "reassign to", and + # nothing has asked for an unassign path. + if assignee is not None and not assignee.strip(): + assignee = None + changes: dict = {} if title is not None: changes["title"] = title @@ -5495,6 +5555,17 @@ def task_update( changes["symbol_refs"] = symbol_refs if tags is not None: changes["tags"] = tags + # tk-task-update-can-still-manufacture-an-unnameable--9ba86a: a lease with + # no named holder is unrepresentable through this surface, not just + # discouraged. Resolved inside `_update_task`, against the read IT takes + # immediately before writing — not from a value decided here off an + # earlier, separate read — so a `task_claim` landing in between cannot be + # silently overwritten by a stale default; see `_update_task`'s own + # `default_if_missing` docstring for why that distinction matters. Only + # fills a GAP either way: a task that already has an assignee keeps it + # unless `assignee` was passed explicitly above, so a colleague marking a + # task in_progress to log status does not silently reassign it. + default_if_missing: dict[str, Callable[[], object]] = {} if status is not None: changes["status"] = status if status == "closed": @@ -5504,6 +5575,8 @@ def task_update( # representation (see readiness.status_pickable's updated_at fallback # for stores/rows written before this existed). changes["claimed_at"] = now_iso() + if assignee is None: + default_if_missing["assignee"] = lambda: _claim_holder(None, session_id) # The parent is one edit in two encodings — the `parent_slug` field and the # ParentOf edge — so it is one commit, not three. It used to update the @@ -5517,7 +5590,12 @@ def task_update( ("mutations.gq", "link_parent_of", {"from": parent, "to": slug}) ) - updated, _new_commit = _update_task(slug, changes, extra_steps=extra_steps) + updated, _new_commit = _update_task( + slug, + changes, + extra_steps=extra_steps, + default_if_missing=default_if_missing or None, + ) # Closing here must unblock dependents too, matching task_close. if status == "closed" and updated is not None: diff --git a/uv.lock b/uv.lock index 8364350..d70a00d 100644 --- a/uv.lock +++ b/uv.lock @@ -2413,7 +2413,7 @@ test = [ [[package]] name = "witan-council" -version = "0.29.0" +version = "0.29.1" source = { editable = "mcp/servers/witan" } dependencies = [ { name = "agent-config-kit" },