From 826fb248579e4645d326dbef5a6239a8a25b58ac Mon Sep 17 00:00:00 2001 From: praxagent Date: Sun, 26 Jul 2026 23:38:17 +0000 Subject: [PATCH] fix(mcp): credit the agent on the board, let it tidy up, and survive a restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things found by connecting a real client and using it. THE BOARD SAID A HUMAN DID IT Cards filed over MCP were credited to the person reading the board. Credentials now carry a display label — "Claude Code" by default, overridable so Codex users do not see the wrong tool — and every mutation passes it as the author, editor or comment actor. An unlabelled key falls back to its own name rather than to "human": a key identifier on a card is ugly, crediting the wrong party is worse. A COMMENT THAT SILENTLY VANISHED comment_on_task sent {comment, author}. The Library API reads {text, actor}. So comments were accepted, reported as successful, and arrived EMPTY and unattributed — a silent wrong result, which is worse than an error because nothing tells you to look. There was a test asserting the broken shape. It checked what our code sent rather than what the API reads, so it passed for as long as the feature was broken. Rewritten against the contract, with a note saying so, because that is the more useful lesson than the bug. delete_task added: an agent that files a card should be able to withdraw it. Requires task.write, respects space scope, refreshes the board like any other mutation, and its description says to only remove cards it created or was asked to — a tool the agent can see is a tool it will try. A TAB THAT NEVER CAME BACK Reported as a WebSocket 502. The 502 itself was transient (tailscale serve answers 502 while the upstream restarts, which is every deploy) — but the reason a tab stayed dead is ours: backoff was 1000 * 2^attempts with NO ceiling and a hard stop at 10. Attempt 7 waits 2 minutes, attempt 9 waits 8.5, then it gives up permanently and tells you to refresh. - Capped at 30s with jitter. Backing off protects the server; waiting minutes to notice it came back protects nobody, and lockstep retries from several tabs hit a server that is still coming up. - Never gives up. A server down now is usually back inside a minute, and a tab left open overnight should be alive in the morning. "Refresh the page" is the app admitting it stopped trying. - Refetch on a reconnect that followed a drop. Nothing replays missed events, so a recovered tab was showing pre-gap state — the dangerous kind of stale, because it looks fine. This is the same class as the no-polling Kanban. 294 backend + 37 frontend tests green, tsc clean. --- frontend/src/hooks/useWebSocket.ts | 45 ++++++++++--- src/teamwork/agent_auth.py | 16 +++++ src/teamwork/agent_registry.py | 10 ++- src/teamwork/mcp_server.py | 34 ++++++++-- src/teamwork/routers/mcp_admin.py | 5 +- tests/test_agent_registry.py | 40 +++++++++++ tests/test_mcp_server.py | 105 +++++++++++++++++++++++++++-- 7 files changed, 234 insertions(+), 21 deletions(-) diff --git a/frontend/src/hooks/useWebSocket.ts b/frontend/src/hooks/useWebSocket.ts index 9e82c0f..d54750f 100644 --- a/frontend/src/hooks/useWebSocket.ts +++ b/frontend/src/hooks/useWebSocket.ts @@ -15,8 +15,17 @@ class WebSocketManager { private subscribedProjects: Set = new Set(); private subscribedChannels: Set = new Set(); private reconnectAttempts = 0; - private maxReconnectAttempts = 10; private baseReconnectDelay = 1000; + // Backoff is capped rather than doubling forever. Uncapped, attempt 9 waited + // 8.5 minutes — so a tab sat dead long after a 20-second server restart had + // finished. Backing off protects the server; waiting minutes to notice it + // came back protects nobody. + private maxReconnectDelay = 30_000; + // Whether this socket has ever been connected and then dropped. Used to tell + // "first load" from "we were away and may have missed things". + private hadConnection = false; + /** Called after a reconnect that followed a drop, so the UI can catch up. */ + onResync: (() => void) | null = null; connect() { if (this.ws?.readyState === WebSocket.OPEN) { @@ -27,13 +36,20 @@ class WebSocketManager { this.ws.onopen = () => { console.log('WebSocket connected'); + const wasDropped = this.reconnectAttempts > 0 || this.hadConnection; if (this.reconnectAttempts > 0) { toast.success('Reconnected to server'); } this.reconnectAttempts = 0; + this.hadConnection = true; // Re-subscribe to previous subscriptions this.subscribedProjects.forEach((id) => this.subscribeToProject(id)); this.subscribedChannels.forEach((id) => this.subscribeToChannel(id)); + // Events that happened while we were away are gone — nothing replays + // them. Reconnecting without refetching leaves the page confidently + // showing state from before the gap, which is worse than showing that + // the connection dropped, because it looks fine. + if (wasDropped) this.onResync?.(); }; this.ws.onmessage = (event) => { @@ -59,17 +75,21 @@ class WebSocketManager { } private scheduleReconnect() { - if (this.reconnectAttempts >= this.maxReconnectAttempts) { - console.error('Max reconnect attempts reached'); - toast.error('Unable to reconnect. Please refresh the page.'); - return; - } - - const delay = this.baseReconnectDelay * Math.pow(2, this.reconnectAttempts); + // Deliberately never gives up. The old code stopped after 10 tries and told + // the user to refresh — but a server that is down now is usually back in + // under a minute (a deploy), and a tab left open overnight should be alive + // in the morning. "Refresh the page" is the app admitting it stopped trying. + const delay = Math.min( + this.baseReconnectDelay * Math.pow(2, this.reconnectAttempts), + this.maxReconnectDelay, + ); + // Jitter so several open tabs do not retry in lockstep against a server + // that is still coming up. + const jittered = delay * (0.5 + Math.random() * 0.5); this.reconnectTimeout = window.setTimeout(() => { this.reconnectAttempts++; this.connect(); - }, delay); + }, jittered); } disconnect() { @@ -129,6 +149,13 @@ export function useWebSocket() { const setAgentTyping = useUIStore((state) => state.setAgentTyping); useEffect(() => { + // Anything that changed while the socket was down was never delivered — + // nothing replays missed events. Refetching on reconnect is what stops a + // recovered tab from confidently showing pre-gap state, which is the + // dangerous version of stale: it looks fine. + wsManager.onResync = () => { + queryClient.invalidateQueries(); + }; wsManager.connect(); const removeHandler = wsManager.addHandler((event) => { diff --git a/src/teamwork/agent_auth.py b/src/teamwork/agent_auth.py index 06ecd03..9d39edd 100644 --- a/src/teamwork/agent_auth.py +++ b/src/teamwork/agent_auth.py @@ -93,6 +93,10 @@ class AgentClient: #: "task.write" over the REST API is a different decision from letting an #: arbitrary MCP client connect as it. Off unless the registry says so. mcp: bool = False + # What the board should SAY this agent is. The registry name is a key + # identifier ("mcp-project-a"); a person reading a Kanban card wants + # "Claude Code". Falls back to the name so there is always something. + label: str = "" allow: frozenset[str] = field(default_factory=lambda: frozenset({ALL_CAPABILITIES})) legacy: bool = False # the shared workspace-wide key public_key: str | None = None # Ed25519 (base64/hex) — enables signing @@ -102,6 +106,17 @@ class AgentClient: # distinct state from both "may do X" and "may not do X". gated: frozenset[str] = field(default_factory=frozenset) + @property + def display_name(self) -> str: + """What a person reading the board should see. + + Board attribution is not bookkeeping — it is the whole reason a shared + board is worth keeping. When every writer had to be "human" or "prax", + an agent's cards were credited to the person reading them, and you could + not tell your own work from an agent's at a glance. + """ + return self.label or self.name + def needs_approval(self, capability: str) -> bool: if ALL_CAPABILITIES in self.gated or capability in self.gated: return True @@ -203,6 +218,7 @@ def load_clients(path: str | None = None, legacy_key: str | None = None) -> list gated=frozenset(_parse_allow(entry["gated"])) if entry.get("gated") else frozenset(), mcp=bool(entry.get("mcp", False)), + label=(entry.get("label") or "").strip(), spaces=frozenset( s.strip() for s in ( entry["spaces"].split(",") diff --git a/src/teamwork/agent_registry.py b/src/teamwork/agent_registry.py index 806e88a..f0cbe71 100644 --- a/src/teamwork/agent_registry.py +++ b/src/teamwork/agent_registry.py @@ -43,6 +43,11 @@ # refused that anyway, since channels do not belong to a space. DEFAULT_ALLOW = ["task.write", "activity.write"] +# What the board shows for work this key does. The registry name is an +# identifier ("mcp-project-a"); a person reading a card wants to see the tool +# that wrote it. Overridable per grant for anything that is not Claude Code. +DEFAULT_LABEL = "Claude Code" + class RegistryError(Exception): """The registry could not be read or written.""" @@ -98,7 +103,8 @@ def client_name_for_space(space: str) -> str: def grant_space(space: str, *, name: str | None = None, - allow: list[str] | None = None) -> dict[str, Any]: + allow: list[str] | None = None, + label: str | None = None) -> dict[str, Any]: """Mint a key scoped to one space and record it. Returns the plaintext token exactly once. Re-granting an already-granted @@ -126,6 +132,7 @@ def grant_space(space: str, *, name: str | None = None, "mcp": True, "spaces": [space], "allow": list(allow or DEFAULT_ALLOW), + "label": (label or DEFAULT_LABEL).strip() or DEFAULT_LABEL, } rotated = False @@ -146,6 +153,7 @@ def grant_space(space: str, *, name: str | None = None, "token": token, # the only time this value exists outside memory "rotated": rotated, "allow": entry["allow"], + "label": entry["label"], "path": str(path), } diff --git a/src/teamwork/mcp_server.py b/src/teamwork/mcp_server.py index 97c45da..97c9455 100644 --- a/src/teamwork/mcp_server.py +++ b/src/teamwork/mcp_server.py @@ -60,6 +60,7 @@ def __init__(self, message: str, code: int = -32000): "create_task": CAP_TASK_WRITE, "update_task": CAP_TASK_WRITE, "comment_on_task": CAP_TASK_WRITE, + "delete_task": CAP_TASK_WRITE, "list_notebooks": "", "create_notebook": CAP_ACTIVITY_WRITE, "create_note": CAP_ACTIVITY_WRITE, @@ -71,7 +72,7 @@ def __init__(self, message: str, code: int = -32000): # Tools that change something. A caller watching the UI should see the result # without reloading, so these announce themselves; reads say nothing. MUTATING_TOOLS = frozenset({ - "create_task", "update_task", "comment_on_task", + "create_task", "update_task", "comment_on_task", "delete_task", "create_notebook", "create_note", "update_note", }) @@ -146,6 +147,19 @@ def tool_definitions() -> list[dict[str, Any]]: "required": ["space", "task_id", "comment"], }, }, + { + "name": "delete_task", + "description": ( + "Delete a Kanban task. Use this only for a card you created " + "yourself, or when the human explicitly asks — deleting " + "someone else's card destroys work you cannot see the value of." + ), + "inputSchema": { + "type": "object", + "properties": {"space": space, "task_id": {"type": "string"}}, + "required": ["space", "task_id"], + }, + }, { "name": "list_notebooks", "description": "List notebooks in a space.", @@ -389,7 +403,10 @@ async def _dispatch_library(tool: str, arguments: dict[str, Any], space, title = _require(arguments, "space", "title") body = {"title": title, "description": arguments.get("description", ""), - "assignees": arguments.get("assignees") or []} + "assignees": arguments.get("assignees") or [], + # Say who actually filed it. Without this the board credits the + # person reading it, which defeats the point of the board. + "author": client.display_name} if arguments.get("status"): body["column"] = arguments["status"] return await _prax("POST", f"/spaces/{space}/tasks", json=body) @@ -398,6 +415,8 @@ async def _dispatch_library(tool: str, arguments: dict[str, Any], space, task_id = _require(arguments, "space", "task_id") body = {k: arguments[k] for k in ("title", "description") if arguments.get(k) is not None} + if body: + body["editor"] = client.display_name result: dict[str, Any] = {} if body: result = await _prax("PATCH", f"/spaces/{space}/tasks/{task_id}", json=body) @@ -405,7 +424,8 @@ async def _dispatch_library(tool: str, arguments: dict[str, Any], # Column changes go through /move — the Kanban records a transition, # not just a field edit, and the activity log depends on it. result = await _prax("PATCH", f"/spaces/{space}/tasks/{task_id}/move", - json={"column": arguments["status"]}) + json={"column": arguments["status"], + "editor": client.display_name}) if not result: raise McpError("nothing to update: give at least one of title, " "description or status") @@ -414,7 +434,11 @@ async def _dispatch_library(tool: str, arguments: dict[str, Any], if tool == "comment_on_task": space, task_id, comment = _require(arguments, "space", "task_id", "comment") return await _prax("POST", f"/spaces/{space}/tasks/{task_id}/comment", - json={"comment": comment, "author": client.name}) + json={"text": comment, "actor": client.display_name}) + + if tool == "delete_task": + space, task_id = _require(arguments, "space", "task_id") + return await _prax("DELETE", f"/spaces/{space}/tasks/{task_id}") if tool == "list_notebooks": (space,) = _require(arguments, "space") @@ -438,7 +462,7 @@ async def _dispatch_library(tool: str, arguments: dict[str, Any], arguments, "space", "notebook", "title", "content") return await _prax("POST", "/notes", json={ "project": space, "notebook": notebook, "title": title, - "content": content, "author": client.name}) + "content": content, "author": client.display_name}) if tool == "read_note": space, notebook, note = _require(arguments, "space", "notebook", "note") diff --git a/src/teamwork/routers/mcp_admin.py b/src/teamwork/routers/mcp_admin.py index 157689c..2c41d67 100644 --- a/src/teamwork/routers/mcp_admin.py +++ b/src/teamwork/routers/mcp_admin.py @@ -78,7 +78,8 @@ async def mcp_status(request: Request, space: str | None = None) -> dict: @router.post("/spaces/{space}/enable") -async def enable_for_space(space: str, request: Request) -> dict: +async def enable_for_space(space: str, request: Request, + label: str | None = None) -> dict: """Mint a key scoped to this space and record it. This exists so nobody has to hand-author a credential file. Re-enabling an @@ -87,7 +88,7 @@ async def enable_for_space(space: str, request: Request) -> dict: dishonest answer to "I lost my token". """ try: - result = agent_registry.grant_space(space) + result = agent_registry.grant_space(space, label=label) except agent_registry.RegistryError as exc: raise HTTPException(status_code=400, detail=str(exc)) from exc diff --git a/tests/test_agent_registry.py b/tests/test_agent_registry.py index 26b2076..4245253 100644 --- a/tests/test_agent_registry.py +++ b/tests/test_agent_registry.py @@ -183,3 +183,43 @@ def test_the_writer_and_reader_resolve_to_the_same_file(monkeypatch, tmp_path): reg.grant_space("project-a") assert reg.registry_path().exists() assert reg.granted_spaces() == {"project-a"} + + +# ── What the board shows ───────────────────────────────────────────────────── + +def test_a_grant_carries_a_human_readable_label(registry): + """The registry name identifies the key; the label is what a person reads. + + A card filed by an agent used to be credited to "human", so you could not + tell your own work from an agent's — which is most of what a shared board is + for. + """ + from teamwork.agent_auth import load_clients, resolve_client + + result = reg.grant_space("project-a") + assert result["label"] == "Claude Code" + + client = resolve_client(result["token"], load_clients(str(registry))) + assert client.display_name == "Claude Code" + assert client.name == "mcp-project-a", "the identifier is unchanged" + + +def test_the_label_can_name_a_different_agent(registry): + # Codex users should not see "Claude Code" on their cards. + from teamwork.agent_auth import load_clients, resolve_client + + result = reg.grant_space("project-a", label="Codex") + client = resolve_client(result["token"], load_clients(str(registry))) + assert client.display_name == "Codex" + + +def test_display_name_falls_back_to_the_key_name(registry): + """A hand-authored entry without a label still shows something.""" + import json + + from teamwork.agent_auth import load_clients, resolve_client + + registry.parent.mkdir(parents=True, exist_ok=True) + registry.write_text(json.dumps([{"name": "hand-rolled", "token": "t", "mcp": True}])) + client = resolve_client("t", load_clients(str(registry))) + assert client.display_name == "hand-rolled" diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 71d77dc..ccbf912 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -269,9 +269,10 @@ async def test_create_task_posts_to_the_named_space(prax): @pytest.mark.asyncio async def test_status_change_goes_through_move_not_a_field_edit(prax): # The Kanban records a transition; a plain PATCH would lose it. - await _call(client(allow=frozenset({CAP_TASK_WRITE})), "update_task", - {"space": "proj-a", "task_id": "t1", "status": "done"}) - assert prax.calls == [("PATCH", "/spaces/proj-a/tasks/t1/move", {"column": "done"})] + await _call(client(label="Claude Code", allow=frozenset({CAP_TASK_WRITE})), + "update_task", {"space": "proj-a", "task_id": "t1", "status": "done"}) + assert prax.calls == [("PATCH", "/spaces/proj-a/tasks/t1/move", + {"column": "done", "editor": "Claude Code"})] @pytest.mark.asyncio @@ -284,11 +285,16 @@ async def test_update_task_with_nothing_to_change_is_refused(prax): @pytest.mark.asyncio async def test_a_comment_is_attributed_to_the_calling_key(prax): # Not to a generic "agent" — the board should say which one wrote it. + # + # This test used to assert `comment`/`author`, which is what the code sent + # and NOT what the Library API reads (`text`/`actor`). It passed the whole + # time the feature was broken, because it checked the implementation instead + # of the contract. The field names below are the API's. await _call(client(name="codex", allow=frozenset({CAP_TASK_WRITE})), "comment_on_task", {"space": "proj-a", "task_id": "t1", "comment": "picked this up"}) _, _, body = prax.calls[0] - assert body == {"comment": "picked this up", "author": "codex"} + assert body == {"text": "picked this up", "actor": "codex"} @pytest.mark.asyncio @@ -480,3 +486,94 @@ async def on_change(**kw): on_change=on_change) assert result == {"ok": True} assert prax.calls, "the write still went through" + + +# ── Attribution: the board must say who actually did it ────────────────────── + +@pytest.mark.asyncio +async def test_a_card_is_credited_to_the_agent_not_the_human(prax): + """The bug this fixes: agent-filed cards said author=human. + + If you cannot tell your own cards from an agent's, the board stops being a + picture of the work and becomes a pile. + """ + await _call(client(label="Claude Code", allow=frozenset({CAP_TASK_WRITE})), + "create_task", {"space": "proj-a", "title": "Ship it"}) + _, _, body = prax.calls[0] + assert body["author"] == "Claude Code" + + +@pytest.mark.asyncio +async def test_a_move_records_who_moved_it(prax): + await _call(client(label="Claude Code", allow=frozenset({CAP_TASK_WRITE})), + "update_task", {"space": "proj-a", "task_id": "t1", "status": "doing"}) + _, path, body = prax.calls[0] + assert path.endswith("/move") + assert body == {"column": "doing", "editor": "Claude Code"} + + +@pytest.mark.asyncio +async def test_a_comment_uses_the_field_names_the_api_actually_reads(prax): + """These were `comment`/`author`; the API reads `text`/`actor`. + + The call succeeded and the comment arrived empty and unattributed — a + silent wrong result, which is worse than an error. + """ + await _call(client(label="Claude Code", allow=frozenset({CAP_TASK_WRITE})), + "comment_on_task", + {"space": "proj-a", "task_id": "t1", "comment": "picked this up"}) + _, _, body = prax.calls[0] + assert body == {"text": "picked this up", "actor": "Claude Code"} + + +@pytest.mark.asyncio +async def test_an_unlabelled_key_still_says_something(prax): + # Falls back to the credential name rather than to "human". + await _call(client(name="mcp-proj-a", allow=frozenset({CAP_TASK_WRITE})), + "create_task", {"space": "proj-a", "title": "x"}) + assert prax.calls[0][2]["author"] == "mcp-proj-a" + + +# ── Deleting ───────────────────────────────────────────────────────────────── + +@pytest.mark.asyncio +async def test_delete_task_removes_the_card(prax): + await _call(client(allow=frozenset({CAP_TASK_WRITE})), "delete_task", + {"space": "proj-a", "task_id": "t1"}) + assert prax.calls == [("DELETE", "/spaces/proj-a/tasks/t1", {})] + + +@pytest.mark.asyncio +async def test_delete_needs_write_like_any_other_mutation(prax): + with pytest.raises(McpError, match="task.write"): + await _call(client(allow=frozenset()), "delete_task", + {"space": "proj-a", "task_id": "t1"}) + assert prax.calls == [] + + +@pytest.mark.asyncio +async def test_delete_respects_space_scope(prax): + with pytest.raises(McpError): + await _call(client(spaces=frozenset({"proj-a"}), + allow=frozenset({CAP_TASK_WRITE})), + "delete_task", {"space": "other", "task_id": "t1"}) + assert prax.calls == [] + + +@pytest.mark.asyncio +async def test_a_delete_refreshes_the_board(prax): + seen = [] + + async def on_change(**kw): + seen.append(kw) + + await _call(client(allow=frozenset({CAP_TASK_WRITE})), "delete_task", + {"space": "proj-a", "task_id": "t1"}, on_change=on_change) + assert seen == [{"space": "proj-a", "tool": "delete_task"}] + + +def test_delete_warns_against_touching_someone_elses_card(): + """A tool the agent can see is a tool it will try.""" + desc = next(t["description"] for t in tool_definitions() + if t["name"] == "delete_task") + assert "you created yourself" in desc or "created yourself" in desc