From 2f56d7546ae1ffaa0eb7ae062ee5d012854dad8b Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Fri, 26 Jun 2026 11:43:38 -0700 Subject: [PATCH] fix: omit plan link in PR description when no plan exists (#1618) Plan links in PR descriptions were always built from the thread id, so runs that never produced a plan linked to an empty plan-review page. Now the plan content store is consulted first; the link is only added when a plan with non-empty markdown actually exists. A transient store failure degrades gracefully (no link) rather than blocking PR creation. Co-authored-by: open-swe[bot] --- agent/tools/open_pull_request.py | 12 +++++-- tests/test_open_pull_request.py | 56 ++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/agent/tools/open_pull_request.py b/agent/tools/open_pull_request.py index fd1ad05b..dd12b945 100644 --- a/agent/tools/open_pull_request.py +++ b/agent/tools/open_pull_request.py @@ -11,6 +11,7 @@ from langgraph.config import get_config from langgraph_sdk import get_client from ..dashboard.agent_usage import record_agent_pr_usage +from ..dashboard.plan_store import get_plan_content from ..utils.dashboard_links import dashboard_plan_url from ..utils.github_app import get_github_app_installation_token from ..utils.github_comments import derive_pr_state @@ -169,10 +170,17 @@ async def _record_pr_telemetry( ) -def _plan_reference_line(configurable: dict[str, Any]) -> str | None: +async def _plan_reference_line(configurable: dict[str, Any]) -> str | None: thread_id = configurable.get("thread_id") if not isinstance(thread_id, str): return None + try: + plan = await get_plan_content(thread_id) + except Exception: + logger.debug("Failed to look up plan content for %s", thread_id, exc_info=True) + return None + if not plan or not str(plan.get("markdown", "")).strip(): + return None plan_url = dashboard_plan_url(thread_id) if not plan_url: return None @@ -224,7 +232,7 @@ async def _maybe_append_references( if not isinstance(configurable, dict): configurable = {} lines: list[str] = [] - plan_line = _plan_reference_line(configurable) + plan_line = await _plan_reference_line(configurable) if plan_line: lines.append(plan_line) try: diff --git a/tests/test_open_pull_request.py b/tests/test_open_pull_request.py index 1ccbec57..ca89a0c8 100644 --- a/tests/test_open_pull_request.py +++ b/tests/test_open_pull_request.py @@ -257,6 +257,10 @@ def _stub_token(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(opr, "_resolve_pr_author_token", lambda: _coro(("tok", "user"))) +def _stub_plan(monkeypatch: pytest.MonkeyPatch, plan: dict[str, Any] | None) -> None: + monkeypatch.setattr(opr, "get_plan_content", lambda *_a, **_k: _coro(plan)) + + def test_appends_slack_reference_for_private_repo(monkeypatch: pytest.MonkeyPatch) -> None: _set_config( monkeypatch, @@ -288,6 +292,7 @@ def test_appends_plan_reference_from_thread_id(monkeypatch: pytest.MonkeyPatch) monkeypatch.setenv("DASHBOARD_BASE_URL", "https://dashboard.example") _set_config(monkeypatch, {"source": "dashboard", "thread_id": "thread-1"}) _stub_token(monkeypatch) + _stub_plan(monkeypatch, {"markdown": "# Plan\n- step 1", "status": "ready"}) client = _FakeClient(post=_FakeResponse(201, {"html_url": "u", "number": 1, "user": {}})) _install_client(monkeypatch, client) @@ -300,6 +305,55 @@ def test_appends_plan_reference_from_thread_id(monkeypatch: pytest.MonkeyPatch) assert client.get_calls == [] +def test_omits_plan_reference_when_no_plan_exists(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("DASHBOARD_BASE_URL", "https://dashboard.example") + _set_config(monkeypatch, {"source": "dashboard", "thread_id": "thread-1"}) + _stub_token(monkeypatch) + _stub_plan(monkeypatch, None) + + client = _FakeClient(post=_FakeResponse(201, {"html_url": "u", "number": 1, "user": {}})) + _install_client(monkeypatch, client) + + _open_with_body("body") + + assert client.post_calls[0]["json"]["body"] == "body" + assert client.get_calls == [] + + +def test_omits_plan_reference_when_plan_markdown_empty(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("DASHBOARD_BASE_URL", "https://dashboard.example") + _set_config(monkeypatch, {"source": "dashboard", "thread_id": "thread-1"}) + _stub_token(monkeypatch) + _stub_plan(monkeypatch, {"markdown": " \n ", "status": "ready"}) + + client = _FakeClient(post=_FakeResponse(201, {"html_url": "u", "number": 1, "user": {}})) + _install_client(monkeypatch, client) + + _open_with_body("body") + + assert client.post_calls[0]["json"]["body"] == "body" + assert client.get_calls == [] + + +def test_omits_plan_reference_when_store_lookup_fails(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("DASHBOARD_BASE_URL", "https://dashboard.example") + _set_config(monkeypatch, {"source": "dashboard", "thread_id": "thread-1"}) + _stub_token(monkeypatch) + + async def fail_plan(*_a: Any, **_k: Any) -> Any: + raise RuntimeError("store down") + + monkeypatch.setattr(opr, "get_plan_content", fail_plan) + + client = _FakeClient(post=_FakeResponse(201, {"html_url": "u", "number": 1, "user": {}})) + _install_client(monkeypatch, client) + + _open_with_body("body") + + assert client.post_calls[0]["json"]["body"] == "body" + assert client.get_calls == [] + + def test_plan_reference_survives_source_reference_failure( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -313,6 +367,7 @@ def test_plan_reference_survives_source_reference_failure( }, ) _stub_token(monkeypatch) + _stub_plan(monkeypatch, {"markdown": "# Plan\n- step 1", "status": "ready"}) async def fail_permalink(*_args: Any, **_kwargs: Any) -> str: raise RuntimeError("slack failed") @@ -365,6 +420,7 @@ def test_public_repo_appends_plan_but_not_source_reference( }, ) _stub_token(monkeypatch) + _stub_plan(monkeypatch, {"markdown": "# Plan\n- step 1", "status": "ready"}) monkeypatch.setattr( opr, "get_slack_permalink", lambda *_a, **_k: _coro("https://slack.example/p1") )