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