diff --git a/coworker/server/app.py b/coworker/server/app.py index 2f558085..f63599c1 100644 --- a/coworker/server/app.py +++ b/coworker/server/app.py @@ -164,6 +164,7 @@ from ..providers import AssistantTurn from .. import toolchain from ..teams.model import AuthorityError as TeamsAuthorityError from ..teams.model import BoardError as TeamsBoardError +from ..teams.model import BoardNotFoundError as TeamsBoardNotFoundError from .manager import SessionManager @@ -842,6 +843,8 @@ def create_app(manager: SessionManager) -> FastAPI: ) try: return handler(actor) + except TeamsBoardNotFoundError as error: + return JSONResponse({"error": str(error)}, status_code=404) except TeamsAuthorityError as error: return JSONResponse({"error": str(error)}, status_code=403) except (TeamsBoardError, ValueError) as error: @@ -873,7 +876,8 @@ def create_app(manager: SessionManager) -> FastAPI: @app.get("/v1/board/item") def board_get_item(request: Request, space: str, id: int): return _board( - request, lambda actor: manager.team_store.get_item(space, int(id)) + request, + lambda actor: manager.team_store.get_item(space, int(id), actor=actor), ) @app.post("/v1/board/items") diff --git a/coworker/server/manager.py b/coworker/server/manager.py index 1486d906..40285768 100644 --- a/coworker/server/manager.py +++ b/coworker/server/manager.py @@ -1697,7 +1697,9 @@ class SessionManager: if space is None: return {"error": "no board for this session"} try: - item = self.team_store.get_item(space, int(item_id)) + item = self.team_store.get_item( + space, int(item_id), actor=self._user_actor() + ) except TeamsBoardError as error: return {"error": str(error)} timeline: list[dict[str, Any]] = [] @@ -2295,7 +2297,9 @@ class SessionManager: # item's ASSIGNEE — a filer merely hears about it. def _holds(event) -> bool: try: - item = self.team_store.get_item(team.space, int(event["item_id"])) + item = self.team_store.get_item( + team.space, int(event["item_id"]), actor=self._user_actor() + ) except Exception: return False return item["assignee"] == actor @@ -2388,7 +2392,9 @@ class SessionManager: item = None if item_id is not None: try: - item = self.team_store.get_item(team.space, int(item_id)) + item = self.team_store.get_item( + team.space, int(item_id), actor=self._user_actor() + ) except Exception: item = None title = f"#{item_id} {item['title']}" if item else f"#{item_id}" diff --git a/coworker/teams/__init__.py b/coworker/teams/__init__.py index 96320fd6..fd0806a4 100644 --- a/coworker/teams/__init__.py +++ b/coworker/teams/__init__.py @@ -7,6 +7,7 @@ from .model import ( Actor, AuthorityError, BoardError, + BoardNotFoundError, ChainError, ItemState, Role, @@ -18,6 +19,7 @@ __all__ = [ "Actor", "AuthorityError", "BoardError", + "BoardNotFoundError", "ChainError", "ItemState", "JournalStore", diff --git a/coworker/teams/dialect.py b/coworker/teams/dialect.py index 1bdb4d78..b96a01d8 100644 --- a/coworker/teams/dialect.py +++ b/coworker/teams/dialect.py @@ -149,7 +149,7 @@ class LocalDialect: return self.store.list_items(space, self.actor, state=state, assignee=assignee) def get_item(self, space: str, item_id: int) -> dict[str, Any]: - return self.store.get_item(space, item_id) + return self.store.get_item(space, item_id, actor=self.actor) def create_item( self, diff --git a/coworker/teams/model.py b/coworker/teams/model.py index 2228e661..3f89a34b 100644 --- a/coworker/teams/model.py +++ b/coworker/teams/model.py @@ -85,6 +85,10 @@ class BoardError(Exception): """A verb call the board refuses — illegal transition, missing item, bad input.""" +class BoardNotFoundError(BoardError): + """A requested board object is missing or is not visible to the actor.""" + + class AuthorityError(BoardError): """The actor's role does not permit this verb on this item.""" diff --git a/coworker/teams/store.py b/coworker/teams/store.py index 876347bf..547a1933 100644 --- a/coworker/teams/store.py +++ b/coworker/teams/store.py @@ -38,6 +38,7 @@ from .model import ( Actor, AuthorityError, BoardError, + BoardNotFoundError, ChainError, ItemState, Role, @@ -469,7 +470,16 @@ class TeamStore: ) with self._lock: if parent is not None: - parent_item = self._item(space, parent) + try: + parent_item = self._item(space, parent) + except BoardError: + raise BoardNotFoundError( + f"no visible item #{parent} in space {space!r}" + ) from None + if not self._item_visible_to(space, actor, parent_item): + raise BoardNotFoundError( + f"no visible item #{parent} in space {space!r}" + ) if case is None: case = parent_item["case_id"] or None item_id = self._next_item_id(space) @@ -489,7 +499,7 @@ class TeamStore: ) if self.journal is not None and case: self.journal.ensure_case(case, actor.id) - return self.get_item(space, item_id, seq=event["seq"]) + return self.get_item(space, item_id, actor=actor, seq=event["seq"]) def list_items( self, @@ -499,8 +509,11 @@ class TeamStore: state: Optional[str] = None, assignee: Optional[str] = None, ) -> list[dict[str, Any]]: - """Items in a space. Workers see only their slice: assigned items plus items - directly linked to those.""" + """Return actor-visible items in a space. + + A worker's slice contains items it owns or created, their direct links, + and—while claims are open—the open, unassigned claim pool. + """ where = ["space = ?"] params: list[Any] = [space] if state: @@ -517,32 +530,50 @@ class TeamStore: params, ).fetchall() items = [_row_to_item(row) for row in rows] + worker_slice = None + claims_open = None if actor.role == Role.WORKER: - visible = self._worker_slice(space, actor.id) - # On an open-claims board the claimable pool is visible too — a - # pull queue nobody can see is not a queue (drill-caught: an - # external worker with no assignment saw an empty board). Under - # lead-only policy workers can't act on it, so it stays hidden. + worker_slice = self._worker_slice(space, actor.id) claims_open = self.policy(space)["claims"] == "open" - items = [ - item - for item in items - if item["id"] in visible - or ( - claims_open - and item["state"] == ItemState.OPEN.value - and not item["assignee"] - ) - ] + items = [ + item + for item in items + if self._item_visible_to( + space, + actor, + item, + worker_slice=worker_slice, + claims_open=claims_open, + ) + ] for item in items: item["links"] = self._links_of(space, item["id"]) return items def get_item( - self, space: str, item_id: int, *, seq: Optional[int] = None + self, + space: str, + item_id: int, + *, + actor: Actor, + seq: Optional[int] = None, ) -> dict[str, Any]: + """Return one actor-visible item. + + The actor is required because detail reads enforce the same worker scope as + list reads. Missing and hidden items deliberately share one error contract. + """ with self._lock: - item = self._item(space, item_id) + try: + item = self._item(space, item_id) + except BoardError: + raise BoardNotFoundError( + f"no visible item #{item_id} in space {space!r}" + ) from None + if not self._item_visible_to(space, actor, item): + raise BoardNotFoundError( + f"no visible item #{item_id} in space {space!r}" + ) item["links"] = self._links_of(space, item_id) item["comments"] = self.comments(space, item_id) if seq is not None: @@ -587,7 +618,7 @@ class TeamStore: }, taint=taint, ) - return self.get_item(space, item_id, seq=event["seq"]) + return self.get_item(space, item_id, actor=actor, seq=event["seq"]) def comment( self, @@ -652,7 +683,7 @@ class TeamStore: assignee=assignee, previous=item["assignee"] or "", ) - return self.get_item(space, item_id, seq=event["seq"]) + return self.get_item(space, item_id, actor=actor, seq=event["seq"]) def claim(self, space: str, actor: Actor, item_id: int) -> dict[str, Any]: """Self-assign an open, unassigned item. Nobody stamps a claim — the store @@ -694,7 +725,7 @@ class TeamStore: assignee=actor.id, previous="", ) - return self.get_item(space, item_id, seq=event["seq"]) + return self.get_item(space, item_id, actor=actor, seq=event["seq"]) def policy(self, space: str) -> dict[str, Any]: with self._lock: @@ -897,6 +928,30 @@ class TeamStore: out.add(row["src"]) return out + def _item_visible_to( + self, + space: str, + actor: Actor, + item: dict[str, Any], + *, + worker_slice: Optional[set[int]] = None, + claims_open: Optional[bool] = None, + ) -> bool: + """Whether an actor may read one item through any board surface.""" + if actor.role != Role.WORKER: + return True + if worker_slice is None: + worker_slice = self._worker_slice(space, actor.id) + if item["id"] in worker_slice: + return True + if claims_open is None: + claims_open = self.policy(space)["claims"] == "open" + return ( + claims_open + and item["state"] == ItemState.OPEN.value + and not item["assignee"] + ) + def _links_of(self, space: str, item_id: int) -> list[dict[str, Any]]: rows = self._conn.execute( "SELECT src, kind, dst FROM team_links WHERE space = ?" diff --git a/tests/test_team_board.py b/tests/test_team_board.py index 42ce7e2d..66cedcf4 100644 --- a/tests/test_team_board.py +++ b/tests/test_team_board.py @@ -3,6 +3,7 @@ import pytest from coworker.teams import Actor, AuthorityError, BoardError, Role, TeamStore +from coworker.teams.dialect import LocalDialect from coworker.teams.tools import board_tools USER = Actor(id="user", role=Role.USER) @@ -67,6 +68,23 @@ def test_child_inherits_parent_case(store): assert {"kind": "parent", "item": parent["id"]} in child["links"] +def test_worker_cannot_expand_its_slice_through_a_hidden_parent(store): + hidden = assigned_item(store, assignee="worker-2") + before = store.event_count(SPACE) + + with pytest.raises(BoardError): + store.create_item( + SPACE, + WORKER, + title="Bridge into another worker's task", + criteria="must not be created", + parent=hidden, + ) + + assert store.event_count(SPACE) == before + assert store.list_items(SPACE, WORKER) == [] + + # ------------------------------------------------------------------- transitions def test_full_happy_path(store): @@ -127,7 +145,7 @@ def test_rework_loop(store): store.transition(SPACE, WORKER, item_id, "in_progress") store.transition(SPACE, WORKER, item_id, "review") store.transition(SPACE, LEAD, item_id, "in_progress", comment="criteria 2 unmet") - item = store.get_item(SPACE, item_id) + item = store.get_item(SPACE, item_id, actor=LEAD) assert item["state"] == "in_progress" assert item["comments"][-1]["body"] == "criteria 2 unmet" @@ -161,9 +179,9 @@ def test_blocked_by_shows_on_the_other_side(store): a = store.create_item(SPACE, LEAD, title="A", criteria="c") b = store.create_item(SPACE, LEAD, title="B", criteria="c") store.link(SPACE, LEAD, a["id"], "blocks", b["id"]) - assert {"kind": "blocked_by", "item": a["id"]} in store.get_item(SPACE, b["id"])[ - "links" - ] + assert {"kind": "blocked_by", "item": a["id"]} in store.get_item( + SPACE, b["id"], actor=LEAD + )["links"] def test_artifact_refs_accumulate_on_the_item(store): @@ -174,10 +192,10 @@ def test_artifact_refs_accumulate_on_the_item(store): SPACE, WORKER, item_id, "review", comment="done", refs=["branch:fix/acl", "report:posture.html"], ) - item = store.get_item(SPACE, item_id) + item = store.get_item(SPACE, item_id, actor=WORKER) assert item["refs"] == ["branch:fix/acl", "report:posture.html"] # deduped, ordered store.rebuild(SPACE) - assert store.get_item(SPACE, item_id)["refs"] == item["refs"] + assert store.get_item(SPACE, item_id, actor=WORKER)["refs"] == item["refs"] # ------------------------------------------------------------- worker visibility @@ -194,6 +212,36 @@ def test_worker_sees_only_its_slice(store): assert len(store.list_items(SPACE, USER)) == 3 +def test_worker_item_detail_matches_list_visibility(store): + mine = assigned_item(store, assignee="worker-1") + theirs = assigned_item(store, assignee="worker-2") + claimable = store.create_item(SPACE, LEAD, title="Available", criteria="c") + linked = store.create_item(SPACE, LEAD, title="Dependency", criteria="c") + store.link(SPACE, LEAD, linked["id"], "blocks", mine) + worker = LocalDialect(store, journal=None, actor=WORKER) + + assert worker.get_item(SPACE, mine)["id"] == mine + assert worker.get_item(SPACE, linked["id"])["id"] == linked["id"] + assert worker.get_item(SPACE, claimable["id"])["id"] == claimable["id"] + assert store.get_item(SPACE, mine, actor=WORKER)["id"] == mine + + with pytest.raises(BoardError): + worker.get_item(SPACE, theirs) + with pytest.raises(BoardError): + store.get_item(SPACE, theirs, actor=WORKER) + + store.claim(SPACE, OTHER, claimable["id"]) + with pytest.raises(BoardError): + worker.get_item(SPACE, claimable["id"]) + + held = store.create_item(SPACE, LEAD, title="Held", criteria="c") + store.set_policy(SPACE, LEAD, claims="lead-only") + with pytest.raises(BoardError): + worker.get_item(SPACE, held["id"]) + + assert worker.get_item(SPACE, mine)["id"] == mine + + def test_worker_comments_only_on_its_slice(store): theirs = assigned_item(store, assignee="worker-2") with pytest.raises(AuthorityError, match="assigned"): diff --git a/tests/test_team_open_surface.py b/tests/test_team_open_surface.py index ad7b5d40..2ed8994d 100644 --- a/tests/test_team_open_surface.py +++ b/tests/test_team_open_surface.py @@ -248,6 +248,166 @@ def test_token_binds_identity_and_store_enforces_authority(api): assert bad.status_code == 403 or bad.status_code == 400 +def test_board_item_reads_hide_foreign_worker_items(api): + client, manager, _ = api + lead_token = _tokens(manager).mint("lead-1", "lead") + nia_token = _tokens(manager).mint("nia", "worker") + webb_token = _tokens(manager).mint("webb", "worker") + lead = {"Authorization": f"Bearer {lead_token}"} + nia = {"Authorization": f"Bearer {nia_token}"} + webb = {"Authorization": f"Bearer {webb_token}"} + + def create(title, *, description=""): + response = client.post( + "/v1/board/items", + headers=lead, + json={ + "space": "proj", + "title": title, + "description": description, + "criteria": "done", + }, + ) + assert response.status_code == 200 + return response.json() + + created = create( + "Webb private investigation", description="credential rotation details" + ) + assigned_response = client.post( + "/v1/board/items/assign", + headers=lead, + json={"space": "proj", "id": created["id"], "assignee": "webb"}, + ) + assert assigned_response.status_code == 200 + + mine = create("Nia task") + assert client.post( + "/v1/board/items/assign", + headers=lead, + json={"space": "proj", "id": mine["id"], "assignee": "nia"}, + ).status_code == 200 + claimable = create("Available") + linked = create("Linked dependency") + assert client.post( + "/v1/board/link", + headers=lead, + json={ + "space": "proj", + "src": linked["id"], + "kind": "blocks", + "dst": mine["id"], + }, + ).status_code == 200 + + denied = client.get( + "/v1/board/item", + headers=nia, + params={"space": "proj", "id": created["id"]}, + ) + assert denied.status_code == 404 + assert "Webb private investigation" not in denied.text + assert "credential rotation details" not in denied.text + + for item_id in (mine["id"], claimable["id"], linked["id"]): + assert client.get( + "/v1/board/item", + headers=nia, + params={"space": "proj", "id": item_id}, + ).status_code == 200 + + assert client.post( + "/v1/board/items/claim", + headers=webb, + json={"space": "proj", "id": claimable["id"]}, + ).status_code == 200 + assert client.get( + "/v1/board/item", + headers=nia, + params={"space": "proj", "id": claimable["id"]}, + ).status_code == 404 + + held = create("Held") + assert client.post( + "/v1/board/policy", + headers=lead, + json={"space": "proj", "claims": "lead-only"}, + ).status_code == 200 + assert client.get( + "/v1/board/item", + headers=nia, + params={"space": "proj", "id": held["id"]}, + ).status_code == 404 + assert client.get( + "/v1/board/item", + headers=nia, + params={"space": "proj", "id": mine["id"]}, + ).status_code == 200 + + allowed = client.get( + "/v1/board/item", + headers=lead, + params={"space": "proj", "id": created["id"]}, + ) + assert allowed.status_code == 200 + assert allowed.json()["title"] == "Webb private investigation" + + +def test_board_item_reads_return_not_found_for_missing_items(api): + client, manager, _ = api + nia_token = _tokens(manager).mint("nia", "worker") + lead_token = _tokens(manager).mint("lead-1", "lead") + user_token = _tokens(manager).mint("user", "user") + nia = {"Authorization": f"Bearer {nia_token}"} + lead = {"Authorization": f"Bearer {lead_token}"} + user = {"Authorization": f"Bearer {user_token}"} + + for headers in (nia, lead, user): + missing = client.get( + "/v1/board/item", + headers=headers, + params={"space": "proj", "id": 999}, + ) + assert missing.status_code == 404 + + +def test_board_item_create_hides_missing_and_foreign_parents(api): + client, manager, _ = api + lead_token = _tokens(manager).mint("lead-1", "lead") + nia_token = _tokens(manager).mint("nia", "worker") + lead = {"Authorization": f"Bearer {lead_token}"} + nia = {"Authorization": f"Bearer {nia_token}"} + + hidden_response = client.post( + "/v1/board/items", + headers=lead, + json={"space": "proj", "title": "Webb task", "criteria": "done"}, + ) + assert hidden_response.status_code == 200 + hidden = hidden_response.json() + assert client.post( + "/v1/board/items/assign", + headers=lead, + json={"space": "proj", "id": hidden["id"], "assignee": "webb"}, + ).status_code == 200 + before = manager.team_store.event_count("proj") + + for parent in (hidden["id"], 999): + denied = client.post( + "/v1/board/items", + headers=nia, + json={ + "space": "proj", + "title": "Probe", + "criteria": "must not be created", + "parent": parent, + }, + ) + assert denied.status_code == 404 + + assert manager.team_store.event_count("proj") == before + + def test_remote_dialect_round_trip(api): client, manager, app = api lead_token = _tokens(manager).mint("lead-1", "lead") @@ -288,6 +448,13 @@ def test_remote_dialect_round_trip(api): with pytest.raises(BoardError, match="only open items"): nia.claim("proj", item["id"]) + foreign = lead.create_item( + "proj", title="Webb-only task", criteria="visible only to webb" + ) + lead.assign("proj", foreign["id"], "webb") + with pytest.raises(BoardError, match="no visible item"): + nia.get_item("proj", foreign["id"]) + # policy flip over the wire blocks the next worker claim second = lead.create_item("proj", title="Held back", criteria="c") lead.set_policy("proj", claims="lead-only") @@ -477,10 +644,33 @@ def test_mcp_worker_loop_through_call_tool(tmp_path): call("board_claim", {"item": item["id"]}) call("board_move", {"item": item["id"], "to": "in_progress"}) + payload = json.loads(call("board_show", {"item": item["id"]})[0].text) + assert (payload["id"], payload["assignee"]) == (item["id"], "nia") shown = lead_dialect.get_item("proj", item["id"]) assert (shown["assignee"], shown["state"]) == ("nia", "in_progress") +def test_mcp_board_show_does_not_return_a_foreign_item(tmp_path): + import anyio + + from coworker.teams.mcp_server import build + + lead = local_dialect(tmp_path, actor="lead-1", role="lead") + foreign = lead.create_item( + "proj", title="Private Webb task", criteria="not visible to nia" + ) + lead.assign("proj", foreign["id"], "webb") + worker = build(LocalDialect(lead.store, lead.journal, NIA), space="proj") + + result = anyio.run( + lambda: worker.call_tool("board_show", {"item": foreign["id"]}) + ) + payload = json.loads(result[0].text) + + assert "error" in payload + assert "Private Webb task" not in payload["error"] + + # ------------------------------------------------------------------ CLI @@ -497,6 +687,10 @@ def test_cli_headless_flow(tmp_path, capsys): ["board", "claim", "1", *space_args, "--actor", "nia", "--role", "worker"] ) == 0 assert "claimed #1" in capsys.readouterr().out + assert main( + ["board", "show", "1", *space_args, "--actor", "nia", "--role", "worker"] + ) == 0 + assert "CLI item" in capsys.readouterr().out assert main(["board", "list", *space_args, "--json"]) == 0 items = json.loads(capsys.readouterr().out) assert [(i["id"], i["assignee"]) for i in items] == [(1, "nia")] @@ -518,6 +712,27 @@ def test_cli_headless_flow(tmp_path, capsys): assert "found it" in capsys.readouterr().out +def test_cli_worker_cannot_show_a_foreign_item(tmp_path, capsys): + from coworker.teams.cli import main + + space_args = ["--db", str(tmp_path), "--space", "proj"] + lead_args = [*space_args, "--actor", "lead-1", "--role", "lead"] + worker_args = [*space_args, "--actor", "nia", "--role", "worker"] + + assert main( + ["board", "create", "Private Webb task", "--criteria", "c", *lead_args] + ) == 0 + capsys.readouterr() + assert main(["board", "assign", "1", "webb", *lead_args]) == 0 + capsys.readouterr() + + assert main(["board", "show", "1", *worker_args]) == 1 + output = capsys.readouterr() + assert output.out == "" + assert "no visible item" in output.err + assert "Private Webb task" not in output.err + + def test_cli_token_mint_and_list(tmp_path, capsys): from coworker.teams.cli import main diff --git a/tests/test_team_store.py b/tests/test_team_store.py index 2269ca88..b7e04bcc 100644 --- a/tests/test_team_store.py +++ b/tests/test_team_store.py @@ -95,7 +95,10 @@ def test_rebuild_reproduces_the_projection(store): assert after[0]["state"] == "in_progress" assert after[0]["assignee"] == "worker-1" # created_ts comes from the event, so replay is deterministic - assert store.get_item("proj", item["id"])["created_ts"] == item["created_ts"] + assert ( + store.get_item("proj", item["id"], actor=USER)["created_ts"] + == item["created_ts"] + ) def test_taint_travels_with_the_record(store):