From 611778098206396e817e62fc044999f36d06bacc Mon Sep 17 00:00:00 2001 From: Ramon Nogueira Date: Tue, 23 Jun 2026 14:06:53 -0400 Subject: [PATCH] feat(open-swe): let any org member post to a thread, with attribution (#1594) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(dashboard): let any org member post to a thread, with attribution Posting to an Agents chat thread from the web UI was restricted to the thread owner. Open it to any authenticated org member (login is already org-gated by OAuth) on both write paths — the queued follow-up (send_dashboard_message) and the idle-thread run.start (_enrich_run_start_command). Non-owner messages are prefixed with the poster's verified GitHub login (@login:) so the agent and owner can tell who sent them. Thread management (cancel/delete/resolve) stays owner-only, and the UI now shows the composer to non-owners. * fix(dashboard): keep non-run.start commands owner-only Non-owner posting is allowed only via the attributed run.start path. Other write commands (e.g. input.respond) carry unattributed user input, so the commands proxy keeps them owner-only instead of readable-by-any-org-member. * docs(e2e): drop per-test details from the E2E README --- agent/dashboard/thread_api.py | 56 +++++- tests/e2e/README.md | 28 +-- tests/e2e/tests/dashboard.spec.ts | 24 ++- tests/test_dashboard_thread_api.py | 194 ++++++++++++++++--- ui/src/components/agents/AgentThreadView.tsx | 69 +++---- 5 files changed, 279 insertions(+), 92 deletions(-) diff --git a/agent/dashboard/thread_api.py b/agent/dashboard/thread_api.py index 793d87de..10a91b1c 100644 --- a/agent/dashboard/thread_api.py +++ b/agent/dashboard/thread_api.py @@ -248,6 +248,18 @@ def _assert_thread_owner(metadata: dict[str, Any], login: str, email: str | None raise HTTPException(404, "thread not found") +def _attribution_prefix(metadata: dict[str, Any], login: str, email: str | None) -> str: + """Attribution prefix for a message; empty when the poster owns the thread. + + Teammates can post into any surfaced-source thread (read access is already + org-gated). Their messages are tagged with the verified session login so the + agent and the thread owner can tell who sent them. + """ + if _user_owns_thread(metadata, login, email): + return "" + return f"@{login}: " + + def _thread_is_readable(metadata: dict[str, Any]) -> bool: """Any surfaced-source thread is readable by authenticated users. @@ -1006,6 +1018,28 @@ def _command_message_content(params: dict[str, Any]) -> Any: return last.get("content") if isinstance(last, dict) else None +def _set_command_last_message_content(params: dict[str, Any], content: Any) -> None: + run_input = params.get("input") + if not isinstance(run_input, dict): + return + messages = run_input.get("messages") + if not isinstance(messages, list) or not messages: + return + last = messages[-1] + if isinstance(last, dict): + last["content"] = content + + +def _prefix_message_content(content: Any, prefix: str) -> Any: + if not prefix: + return content + if isinstance(content, str): + return f"{prefix}{content}" + if isinstance(content, list): + return [{"type": "text", "text": prefix.rstrip()}, *content] + return content + + def _command_prompt_text(content: Any) -> str: if isinstance(content, str): return content.strip() @@ -1062,6 +1096,7 @@ async def _enrich_run_start_command( metadata: dict[str, Any], thread_busy: bool = False, creating: bool = False, + email: str | None = None, ) -> dict[str, Any]: if command.get("method") != "run.start": return command @@ -1114,6 +1149,9 @@ async def _enrich_run_start_command( overrides["agent_effort"] = chosen_effort else: _validate_command_images(content, model_id=chosen_model or _metadata_model_id(metadata)) + prefix = _attribution_prefix(metadata, login, email) + if prefix: + _set_command_last_message_content(params, _prefix_message_content(content, prefix)) metadata_update: dict[str, Any] = {} if chosen_model and chosen_effort: overrides["agent_model_id"] = chosen_model @@ -1159,9 +1197,9 @@ async def send_dashboard_message( raise HTTPException(404, "thread not found") from exc metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {} - _assert_thread_owner(metadata, login, email) + _assert_thread_readable(metadata) - prompt = body.content.strip() + prompt = f"{_attribution_prefix(metadata, login, email)}{body.content.strip()}" now_ms = _now_ms() chosen_model, chosen_effort = _normalize_model_choice(body.model_id, body.effort) metadata_update: dict[str, Any] = {"source": _DASHBOARD_SOURCE, "updated_at_ms": now_ms} @@ -1426,9 +1464,11 @@ async def proxy_dashboard_thread_commands( # The dashboard mints the thread id client-side and submits straight away, # so the very first ``run.start`` may target a thread that doesn't exist # yet. That command lazily creates + stamps + owns the thread (in - # ``_enrich_run_start_command``); any other command against a missing - # thread — or a command from a non-owner against an existing thread — is a - # 404. + # ``_enrich_run_start_command``); any other command against a missing thread + # is a 404. On an existing thread, ``run.start`` (the posting path) is open + # to any org member and attributed in ``_enrich_run_start_command``; every + # other write command carries unattributed input (e.g. ``input.respond``), + # so it stays owner-only. method = parsed.get("method") try: thread = await langgraph_client().threads.get(thread_id) @@ -1444,7 +1484,10 @@ async def proxy_dashboard_thread_commands( thread_busy = False else: metadata = thread.get("metadata") if isinstance(thread.get("metadata"), dict) else {} - _assert_thread_owner(metadata, login, email) + if method == "run.start": + _assert_thread_readable(metadata) + else: + _assert_thread_owner(metadata, login, email) metadata_run_status = metadata.get("latest_run_status") thread_busy = _thread_is_busy(thread) or metadata_run_status in {"pending", "running"} @@ -1458,6 +1501,7 @@ async def proxy_dashboard_thread_commands( metadata=metadata, thread_busy=thread_busy, creating=creating, + email=email, ) outgoing = json.dumps(enriched).encode() diff --git a/tests/e2e/README.md b/tests/e2e/README.md index 3f581cb4..7edfbc51 100644 --- a/tests/e2e/README.md +++ b/tests/e2e/README.md @@ -42,29 +42,15 @@ so what Playwright asserts on is exactly what the real agent produced. - `static/{slack,github}.html` — the mock Slack/GitHub UIs (external SaaS we can't run locally). The dashboard is **not** mocked — it's the real `ui/` app. - `global-setup.ts` — builds the real `ui/` SPA (once) so the harness can serve it. -- `tests/full_flow.spec.ts` — Slack → implement → PR → reply. -- `tests/dashboard.spec.ts` — the Slack → web handoff (below). -## Slack → web handoff (dashboard.spec.ts) — the REAL ui/ app +## The dashboard — the real `ui/` app -After the Slack run, the bot posts an "Open in Web" link -(`DASHBOARD_BASE_URL/agents/{thread_id}`). The test clicks that real link, which -loads the **actual built `ui/` React app** — served same-origin from the harness -so the session cookie and `/dashboard/api/*` calls work without CORS. The signed -session cookie is real (minted via `/control/login`), so the per-user -authorization is genuine: - -- **Same user** (session email = the Slack triggerer = thread owner): the real - `AgentThreadView` shows the transcript (incl. the PR link), the `AgentPromptBar` - composer is present, and submitting a follow-up streams a new agent reply into - the same thread. -- **Different user** (any other org login): the same transcript renders, but the - real UI shows **no composer** (`AgentThreadView` gates it on `thread.isOwner`). - -Ownership is by `github_login` / `triggering_user_email` on the thread metadata; -`GET /dashboard/api/threads/{id}` returns `isOwner`, which the real UI uses to -gate the composer. The only extra fake here is the OAuth-token store (an external -credential); the authorization logic itself is real. +The dashboard is **not** mocked. The bot's "Open in Web" link +(`DASHBOARD_BASE_URL/agents/{thread_id}`) loads the **actual built `ui/` React +app** — served same-origin from the harness so the session cookie and +`/dashboard/api/*` calls work without CORS. The signed session cookie is real +(minted via `/control/login`), so per-user authorization is genuine; the only +extra fake is the OAuth-token store (an external credential). The UI is built by `global-setup.ts` with `VITE_DASHBOARD_API_BASE_URL` pointed at the harness. It builds once; set `E2E_FORCE_UI_BUILD=1` to rebuild (e.g. after a diff --git a/tests/e2e/tests/dashboard.spec.ts b/tests/e2e/tests/dashboard.spec.ts index b478458b..e1aa72c1 100644 --- a/tests/e2e/tests/dashboard.spec.ts +++ b/tests/e2e/tests/dashboard.spec.ts @@ -56,15 +56,29 @@ test.describe("Slack → web handoff (real dashboard UI)", () => { await expect(page.getByRole("link", { name: "Add greet() helper" }).first()).toBeVisible(); }); - test("a DIFFERENT user sees the thread read-only (no composer)", async ({ page }) => { + test("a DIFFERENT user can post, and their message is attributed", async ({ page }) => { await loginAs(page, OTHER_USER); await openThreadViaSlackLink(page); // The same thread + transcript is visible… await expectTranscriptVisible(page); - // …but a non-owner gets no composer. - await expect(page.getByPlaceholder("Add a follow up")).toHaveCount(0); - await expect(page.getByPlaceholder("Send the first message")).toHaveCount(0); - await expect(page.getByLabel("Send message")).toHaveCount(0); + + // …and a non-owner now gets a composer too (owner-only restriction removed). + const composer = page.getByPlaceholder(/Add a follow up|Send the first message/); + await expect(composer).toBeVisible(); + + // Posting starts a new run — the agent's follow-up reply streams in. + await composer.fill("Can you also add a docstring?"); + await composer.press("Enter"); + await expect(page.getByText(/anything else you'd like changed/)).toBeVisible(); + + // The non-owner's message is tagged server-side with their GitHub login, so + // the owner can tell who sent it. Visible once the transcript re-hydrates. + await expect(async () => { + await page.reload(); + await expect( + page.getByText(new RegExp(`@${OTHER_USER.login}`)).first(), + ).toBeVisible({ timeout: 8000 }); + }).toPass({ timeout: 60000 }); }); }); diff --git a/tests/test_dashboard_thread_api.py b/tests/test_dashboard_thread_api.py index 34cc1820..d8e05bb9 100644 --- a/tests/test_dashboard_thread_api.py +++ b/tests/test_dashboard_thread_api.py @@ -324,26 +324,83 @@ async def test_proxy_commands_lazily_creates_missing_thread_only_for_run_start( assert exc_info.value.status_code == 404 -async def test_proxy_commands_run_start_by_non_owner_is_rejected(monkeypatch) -> None: - class OwnedThreads: - async def get(self, thread_id: str) -> dict[str, object]: - return { - "thread_id": thread_id, - "metadata": {"source": "dashboard", "github_login": "owner"}, - } +async def test_enrich_run_start_command_attributes_non_owner_message(monkeypatch) -> None: + class FakeThreads: + async def update(self, *, thread_id: str, metadata: dict[str, object]) -> None: + pass - class OwnedClient: - threads = OwnedThreads() + class FakeClient: + threads = FakeThreads() - monkeypatch.setattr(thread_api, "langgraph_client", lambda: OwnedClient()) + async def fake_get_profile(login: str) -> dict[str, object]: + return {} - # An existing thread owned by someone else is never lazily re-created — a - # run.start from a non-owner is a 404, not a takeover. - with pytest.raises(HTTPException) as exc_info: - await thread_api.proxy_dashboard_thread_commands( - "tid", "intruder", b'{"method": "run.start"}' - ) - assert exc_info.value.status_code == 404 + async def fake_ensure_token(login: str) -> None: + pass + + async def fake_resolve_email(login: str, profile: dict[str, object]) -> str: + return f"{login}@example.com" + + monkeypatch.setattr(thread_api, "langgraph_client", lambda: FakeClient()) + monkeypatch.setattr(thread_api, "get_profile", fake_get_profile) + monkeypatch.setattr(thread_api, "_ensure_dashboard_github_token", fake_ensure_token) + monkeypatch.setattr(thread_api, "_resolve_run_email", fake_resolve_email) + + command = { + "method": "run.start", + "params": {"input": {"messages": [{"role": "user", "content": "fix the bug"}]}}, + } + + enriched = await thread_api._enrich_run_start_command( + "tid", + "teammate", + command, + metadata={"source": "dashboard", "github_login": "owner"}, + email="teammate@example.com", + ) + + # A non-owner's message is forwarded but tagged with their login. + last = enriched["params"]["input"]["messages"][-1] + assert last["content"] == "@teammate: fix the bug" + + +async def test_enrich_run_start_command_does_not_attribute_owner_message(monkeypatch) -> None: + class FakeThreads: + async def update(self, *, thread_id: str, metadata: dict[str, object]) -> None: + pass + + class FakeClient: + threads = FakeThreads() + + async def fake_get_profile(login: str) -> dict[str, object]: + return {} + + async def fake_ensure_token(login: str) -> None: + pass + + async def fake_resolve_email(login: str, profile: dict[str, object]) -> str: + return f"{login}@example.com" + + monkeypatch.setattr(thread_api, "langgraph_client", lambda: FakeClient()) + monkeypatch.setattr(thread_api, "get_profile", fake_get_profile) + monkeypatch.setattr(thread_api, "_ensure_dashboard_github_token", fake_ensure_token) + monkeypatch.setattr(thread_api, "_resolve_run_email", fake_resolve_email) + + command = { + "method": "run.start", + "params": {"input": {"messages": [{"role": "user", "content": "fix the bug"}]}}, + } + + enriched = await thread_api._enrich_run_start_command( + "tid", + "owner", + command, + metadata={"source": "dashboard", "github_login": "owner"}, + email="owner@example.com", + ) + + last = enriched["params"]["input"]["messages"][-1] + assert last["content"] == "fix the bug" async def test_enrich_run_start_command_allowlists_client_configurable(monkeypatch) -> None: @@ -431,8 +488,31 @@ async def test_proxy_commands_rejects_non_object_body(monkeypatch) -> None: assert exc_info.value.status_code == 400 -async def test_proxy_endpoints_enforce_thread_ownership(monkeypatch) -> None: - """Write endpoints (commands, run_cancel) still require thread ownership.""" +async def test_proxy_commands_non_run_start_by_non_owner_is_rejected(monkeypatch) -> None: + """Non-owners may only post via the attributed run.start path; other write + commands (e.g. input.respond) carry unattributed input and stay owner-only.""" + + class OwnedThreads: + async def get(self, thread_id: str) -> dict[str, object]: + return { + "thread_id": thread_id, + "metadata": {"source": "dashboard", "github_login": "owner"}, + } + + class OwnedClient: + threads = OwnedThreads() + + monkeypatch.setattr(thread_api, "langgraph_client", lambda: OwnedClient()) + + with pytest.raises(HTTPException) as exc_info: + await thread_api.proxy_dashboard_thread_commands( + "tid", "intruder", b'{"method": "input.respond"}' + ) + assert exc_info.value.status_code == 404 + + +async def test_run_cancel_enforces_thread_ownership(monkeypatch) -> None: + """Cancelling a run still requires thread ownership (it is not "posting").""" class FakeThreads: async def get(self, thread_id: str) -> dict[str, object]: @@ -447,10 +527,6 @@ async def test_proxy_endpoints_enforce_thread_ownership(monkeypatch) -> None: monkeypatch.setattr(thread_api, "langgraph_client", lambda: FakeClient()) - with pytest.raises(HTTPException) as exc_info: - await thread_api.proxy_dashboard_thread_commands("tid", "intruder", b"{}") - assert exc_info.value.status_code == 404 - with pytest.raises(HTTPException) as exc_info: await thread_api.proxy_dashboard_thread_run_cancel("tid", "run-1", "intruder") assert exc_info.value.status_code == 404 @@ -559,6 +635,78 @@ async def test_send_dashboard_message_returns_502_when_activity_unknown(monkeypa assert exc_info.value.status_code == 502 +async def test_send_dashboard_message_attributes_non_owner(monkeypatch) -> None: + captured: dict[str, object] = {} + + class FakeThreads: + async def get(self, thread_id: str) -> dict[str, object]: + return { + "thread_id": "tid", + "metadata": {"source": "dashboard", "github_login": "owner"}, + } + + async def update(self, *, thread_id: str, metadata: dict[str, object]) -> None: + pass + + class FakeClient: + threads = FakeThreads() + + async def active(thread_id: str) -> bool: + return True + + async def fake_queue(thread_id: str, payload: dict[str, object]) -> bool: + captured["payload"] = payload + return True + + monkeypatch.setattr(thread_api, "langgraph_client", lambda: FakeClient()) + monkeypatch.setattr(thread_api, "get_thread_active_status", active) + monkeypatch.setattr(thread_api, "queue_message_for_thread", fake_queue) + + await thread_api.send_dashboard_message( + "tid", + "teammate", + thread_api.ThreadMessageBody(content="ship it"), + ) + + assert captured["payload"]["text"] == "@teammate: ship it" + + +async def test_send_dashboard_message_does_not_attribute_owner(monkeypatch) -> None: + captured: dict[str, object] = {} + + class FakeThreads: + async def get(self, thread_id: str) -> dict[str, object]: + return { + "thread_id": "tid", + "metadata": {"source": "dashboard", "github_login": "owner"}, + } + + async def update(self, *, thread_id: str, metadata: dict[str, object]) -> None: + pass + + class FakeClient: + threads = FakeThreads() + + async def active(thread_id: str) -> bool: + return True + + async def fake_queue(thread_id: str, payload: dict[str, object]) -> bool: + captured["payload"] = payload + return True + + monkeypatch.setattr(thread_api, "langgraph_client", lambda: FakeClient()) + monkeypatch.setattr(thread_api, "get_thread_active_status", active) + monkeypatch.setattr(thread_api, "queue_message_for_thread", fake_queue) + + await thread_api.send_dashboard_message( + "tid", + "owner", + thread_api.ThreadMessageBody(content="ship it"), + ) + + assert captured["payload"]["text"] == "ship it" + + def test_thread_summary_exposes_resolved_state() -> None: summary = thread_api._thread_summary( { diff --git a/ui/src/components/agents/AgentThreadView.tsx b/ui/src/components/agents/AgentThreadView.tsx index 9c519d5f..9397e7ef 100644 --- a/ui/src/components/agents/AgentThreadView.tsx +++ b/ui/src/components/agents/AgentThreadView.tsx @@ -24,7 +24,6 @@ export function AgentThreadView({ thread }: AgentThreadViewProps) { const sendMessage = useSubmitAgentMessage(thread.id) const stream = useAgentThreadStream() const isMobile = useIsMobile() - const isReadOnly = thread.isOwner === false const { models, defaultSelection } = useModelOptions() const threadSelection = useMemo(() => { @@ -78,42 +77,10 @@ export function AgentThreadView({ thread }: AgentThreadViewProps) { settingUpSandbox={settingUpSandbox} contentWidthClass="max-w-3xl" /> - {!isReadOnly && ( -
-
- - sendMessage.mutateAsync({ - content, - images, - model_id: activeSelection?.modelId ?? null, - effort: activeSelection?.effort ?? null, - }) - } - models={models} - selection={activeSelection} - onSelectionChange={setSelection} - /> -
-
- )} - - ) : isHydrating ? ( -
-

Loading conversation…

-
- ) : ( -
-

- This thread has no messages yet. -

- {!isReadOnly && ( -
+
+
@@ -129,7 +96,35 @@ export function AgentThreadView({ thread }: AgentThreadViewProps) { onSelectionChange={setSelection} />
- )} +
+
+ ) : isHydrating ? ( +
+

Loading conversation…

+
+ ) : ( +
+

+ This thread has no messages yet. +

+
+ + sendMessage.mutateAsync({ + content, + images, + model_id: activeSelection?.modelId ?? null, + effort: activeSelection?.effort ?? null, + }) + } + models={models} + selection={activeSelection} + onSelectionChange={setSelection} + /> +
)}