mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-07 15:09:09 +00:00
fix: include reviewer trace links (#1351)
* fix: include reviewer trace links Co-authored-by: Johannes du Plessis <51395795+johannes117@users.noreply.github.com> * feat: move reviewer trace-link toggle to dashboard Replace the OPEN_SWE_REVIEW_TRACE_LINK_ENABLED env var with a team-level 'Trace Links' toggle in the Open SWE Review dashboard tab. The toggle is read per-publish via get_team_review_trace_links_enabled(); the per-run review_trace_link_enabled config override still forces it off. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> Co-authored-by: Johannes du Plessis <51395795+johannes117@users.noreply.github.com> Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
This commit is contained in:
parent
fee209601b
commit
d8d3794649
6 changed files with 162 additions and 21 deletions
|
|
@ -34,6 +34,7 @@ class TeamSettingsUpdate(BaseModel):
|
||||||
trigger_mode: TriggerMode = "every_push"
|
trigger_mode: TriggerMode = "every_push"
|
||||||
review_draft_prs: bool = False
|
review_draft_prs: bool = False
|
||||||
pr_summaries: bool = True
|
pr_summaries: bool = True
|
||||||
|
review_trace_links: bool = True
|
||||||
autofix_mode: AutofixMode = "off"
|
autofix_mode: AutofixMode = "off"
|
||||||
autofix_severity_threshold: AutofixMode = "medium"
|
autofix_severity_threshold: AutofixMode = "medium"
|
||||||
default_agent_model: str | None = None
|
default_agent_model: str | None = None
|
||||||
|
|
@ -87,6 +88,7 @@ def _default_settings() -> dict[str, Any]:
|
||||||
"trigger_mode": "every_push",
|
"trigger_mode": "every_push",
|
||||||
"review_draft_prs": False,
|
"review_draft_prs": False,
|
||||||
"pr_summaries": True,
|
"pr_summaries": True,
|
||||||
|
"review_trace_links": True,
|
||||||
"autofix_mode": "off",
|
"autofix_mode": "off",
|
||||||
"autofix_severity_threshold": "medium",
|
"autofix_severity_threshold": "medium",
|
||||||
"default_agent_model": fallback_model,
|
"default_agent_model": fallback_model,
|
||||||
|
|
@ -129,6 +131,7 @@ async def upsert_team_settings(update: TeamSettingsUpdate) -> dict[str, Any]:
|
||||||
"trigger_mode": update.trigger_mode,
|
"trigger_mode": update.trigger_mode,
|
||||||
"review_draft_prs": update.review_draft_prs,
|
"review_draft_prs": update.review_draft_prs,
|
||||||
"pr_summaries": update.pr_summaries,
|
"pr_summaries": update.pr_summaries,
|
||||||
|
"review_trace_links": update.review_trace_links,
|
||||||
"autofix_mode": update.autofix_mode,
|
"autofix_mode": update.autofix_mode,
|
||||||
"autofix_severity_threshold": update.autofix_severity_threshold,
|
"autofix_severity_threshold": update.autofix_severity_threshold,
|
||||||
"default_agent_model": update.default_agent_model,
|
"default_agent_model": update.default_agent_model,
|
||||||
|
|
@ -166,6 +169,12 @@ async def get_team_default_model(
|
||||||
return _resolve_default_pair(model, effort)
|
return _resolve_default_pair(model, effort)
|
||||||
|
|
||||||
|
|
||||||
|
async def get_team_review_trace_links_enabled() -> bool:
|
||||||
|
"""Return whether GitHub review bodies should include a LangSmith trace link."""
|
||||||
|
settings = await get_team_settings()
|
||||||
|
return bool(settings.get("review_trace_links", True))
|
||||||
|
|
||||||
|
|
||||||
async def get_team_default_subagent_model(
|
async def get_team_default_subagent_model(
|
||||||
role: Literal["agent", "reviewer"],
|
role: Literal["agent", "reviewer"],
|
||||||
) -> tuple[str, str]:
|
) -> tuple[str, str]:
|
||||||
|
|
|
||||||
|
|
@ -213,14 +213,8 @@ def render_inline_comment_payload(finding: Finding) -> dict[str, Any] | None:
|
||||||
return payload
|
return payload
|
||||||
|
|
||||||
|
|
||||||
def render_review_body(*, pr_number: int, surfaced_count: int) -> str:
|
def render_review_body(*, pr_number: int, surfaced_count: int, trace_url: str | None = None) -> str:
|
||||||
"""Compose the top-level review body.
|
"""Compose the top-level review body."""
|
||||||
|
|
||||||
Two fixed shapes — no agent prose:
|
|
||||||
|
|
||||||
- 0 surfaced findings: a single "no issues" line.
|
|
||||||
- N surfaced findings: a single "found N potential issue(s)" line.
|
|
||||||
"""
|
|
||||||
if surfaced_count == 0:
|
if surfaced_count == 0:
|
||||||
headline = (
|
headline = (
|
||||||
"## ✅ Open SWE Review: No issues found\n\n"
|
"## ✅ Open SWE Review: No issues found\n\n"
|
||||||
|
|
@ -229,7 +223,12 @@ def render_review_body(*, pr_number: int, surfaced_count: int) -> str:
|
||||||
else:
|
else:
|
||||||
issue_word = "issue" if surfaced_count == 1 else "issues"
|
issue_word = "issue" if surfaced_count == 1 else "issues"
|
||||||
headline = f"**Open SWE Review** found {surfaced_count} potential {issue_word}."
|
headline = f"**Open SWE Review** found {surfaced_count} potential {issue_word}."
|
||||||
return f"{headline}\n\n<!-- open-swe-reviewer pr={pr_number} -->"
|
|
||||||
|
parts = [headline]
|
||||||
|
if trace_url:
|
||||||
|
parts.append(f"[View Open SWE trace]({trace_url})")
|
||||||
|
parts.append(f"<!-- open-swe-reviewer pr={pr_number} -->")
|
||||||
|
return "\n\n".join(parts)
|
||||||
|
|
||||||
|
|
||||||
async def post_pull_request_review(
|
async def post_pull_request_review(
|
||||||
|
|
@ -553,12 +552,20 @@ async def fetch_review_thread_id_for_comment(
|
||||||
)
|
)
|
||||||
return None
|
return None
|
||||||
data = response.json()
|
data = response.json()
|
||||||
threads = (
|
if not isinstance(data, dict):
|
||||||
data.get("data", {})
|
return None
|
||||||
.get("repository", {})
|
data_root = data.get("data")
|
||||||
.get("pullRequest", {})
|
if not isinstance(data_root, dict):
|
||||||
.get("reviewThreads", {})
|
return None
|
||||||
)
|
repository = data_root.get("repository")
|
||||||
|
if not isinstance(repository, dict):
|
||||||
|
return None
|
||||||
|
pull_request = repository.get("pullRequest")
|
||||||
|
if not isinstance(pull_request, dict):
|
||||||
|
return None
|
||||||
|
threads = pull_request.get("reviewThreads")
|
||||||
|
if not isinstance(threads, dict):
|
||||||
|
return None
|
||||||
for thread in threads.get("nodes", []) or []:
|
for thread in threads.get("nodes", []) or []:
|
||||||
comment_ids = {
|
comment_ids = {
|
||||||
c.get("databaseId") for c in (thread.get("comments", {}).get("nodes") or [])
|
c.get("databaseId") for c in (thread.get("comments", {}).get("nodes") or [])
|
||||||
|
|
|
||||||
|
|
@ -7,6 +7,7 @@ from typing import Any
|
||||||
|
|
||||||
from langgraph.config import get_config
|
from langgraph.config import get_config
|
||||||
|
|
||||||
|
from ..dashboard.team_settings import get_team_review_trace_links_enabled
|
||||||
from ..reviewer_diff import compute_diff_line_set, fetch_pr_diff, is_range_in_diff
|
from ..reviewer_diff import compute_diff_line_set, fetch_pr_diff, is_range_in_diff
|
||||||
from ..reviewer_findings import (
|
from ..reviewer_findings import (
|
||||||
Finding,
|
Finding,
|
||||||
|
|
@ -40,6 +41,7 @@ from ..utils.github_token import (
|
||||||
get_github_token,
|
get_github_token,
|
||||||
invalidate_cached_github_token,
|
invalidate_cached_github_token,
|
||||||
)
|
)
|
||||||
|
from ..utils.langsmith import get_langsmith_trace_url
|
||||||
from ..utils.slack import post_slack_thread_reply
|
from ..utils.slack import post_slack_thread_reply
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -73,11 +75,12 @@ def publish_review(
|
||||||
return {"success": False, "error": f"Invalid severity_threshold: {severity_threshold}"}
|
return {"success": False, "error": f"Invalid severity_threshold: {severity_threshold}"}
|
||||||
|
|
||||||
config = get_config()
|
config = get_config()
|
||||||
configurable = config.get("configurable", {}) if isinstance(config, dict) else {}
|
raw_configurable = config.get("configurable", {}) if isinstance(config, dict) else {}
|
||||||
repo_config = configurable.get("repo") if isinstance(configurable, dict) else None
|
configurable = raw_configurable if isinstance(raw_configurable, dict) else {}
|
||||||
pr_number = configurable.get("pr_number") if isinstance(configurable, dict) else None
|
repo_config = configurable.get("repo")
|
||||||
head_sha = configurable.get("head_sha") if isinstance(configurable, dict) else None
|
pr_number = configurable.get("pr_number")
|
||||||
is_re_review = bool(configurable.get("re_review")) if isinstance(configurable, dict) else False
|
head_sha = configurable.get("head_sha")
|
||||||
|
is_re_review = bool(configurable.get("re_review"))
|
||||||
|
|
||||||
if (
|
if (
|
||||||
not isinstance(repo_config, dict)
|
not isinstance(repo_config, dict)
|
||||||
|
|
@ -115,6 +118,7 @@ def publish_review(
|
||||||
cap=cap,
|
cap=cap,
|
||||||
is_re_review=is_re_review,
|
is_re_review=is_re_review,
|
||||||
langgraph_run_id=_current_run_id(config),
|
langgraph_run_id=_current_run_id(config),
|
||||||
|
trace_link_config_override=configurable.get("review_trace_link_enabled"),
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
except GitHubAuthError as exc:
|
except GitHubAuthError as exc:
|
||||||
|
|
@ -135,6 +139,16 @@ def _cast_severity(value: str) -> Severity:
|
||||||
return value # type: ignore[return-value]
|
return value # type: ignore[return-value]
|
||||||
|
|
||||||
|
|
||||||
|
async def _resolve_review_trace_url(thread_id: str, config_override: object) -> str | None:
|
||||||
|
if config_override is False:
|
||||||
|
return None
|
||||||
|
if not await get_team_review_trace_links_enabled():
|
||||||
|
return None
|
||||||
|
if not thread_id:
|
||||||
|
return None
|
||||||
|
return get_langsmith_trace_url(thread_id)
|
||||||
|
|
||||||
|
|
||||||
def _is_reviewer_eval_mode(configurable: dict[str, Any]) -> bool:
|
def _is_reviewer_eval_mode(configurable: dict[str, Any]) -> bool:
|
||||||
return configurable.get("reviewer_eval") is True or configurable.get("eval") is True
|
return configurable.get("reviewer_eval") is True or configurable.get("eval") is True
|
||||||
|
|
||||||
|
|
@ -184,8 +198,10 @@ async def _publish_review_async(
|
||||||
cap: int,
|
cap: int,
|
||||||
is_re_review: bool,
|
is_re_review: bool,
|
||||||
langgraph_run_id: str | None = None,
|
langgraph_run_id: str | None = None,
|
||||||
|
trace_link_config_override: object = None,
|
||||||
) -> dict[str, Any]:
|
) -> dict[str, Any]:
|
||||||
thread_id = get_thread_id_from_runtime()
|
thread_id = get_thread_id_from_runtime()
|
||||||
|
review_trace_url = await _resolve_review_trace_url(thread_id, trace_link_config_override)
|
||||||
findings = await _backfill_findings_from_pr_threads(
|
findings = await _backfill_findings_from_pr_threads(
|
||||||
thread_id=thread_id,
|
thread_id=thread_id,
|
||||||
owner=owner,
|
owner=owner,
|
||||||
|
|
@ -245,6 +261,7 @@ async def _publish_review_async(
|
||||||
review_body = render_review_body(
|
review_body = render_review_body(
|
||||||
pr_number=pr_number,
|
pr_number=pr_number,
|
||||||
surfaced_count=len(inline_comments),
|
surfaced_count=len(inline_comments),
|
||||||
|
trace_url=review_trace_url,
|
||||||
)
|
)
|
||||||
|
|
||||||
review_response = await post_pull_request_review(
|
review_response = await post_pull_request_review(
|
||||||
|
|
@ -274,7 +291,11 @@ async def _publish_review_async(
|
||||||
)
|
)
|
||||||
if dropped_ids and valid_with_payload:
|
if dropped_ids and valid_with_payload:
|
||||||
retry_inline = [p for _, p in valid_with_payload]
|
retry_inline = [p for _, p in valid_with_payload]
|
||||||
retry_body = render_review_body(pr_number=pr_number, surfaced_count=len(retry_inline))
|
retry_body = render_review_body(
|
||||||
|
pr_number=pr_number,
|
||||||
|
surfaced_count=len(retry_inline),
|
||||||
|
trace_url=review_trace_url,
|
||||||
|
)
|
||||||
retry_response = await post_pull_request_review(
|
retry_response = await post_pull_request_review(
|
||||||
owner=owner,
|
owner=owner,
|
||||||
repo=repo,
|
repo=repo,
|
||||||
|
|
|
||||||
|
|
@ -184,6 +184,16 @@ def test_render_review_body_no_findings_message() -> None:
|
||||||
assert "<!-- open-swe-reviewer pr=99 -->" in body
|
assert "<!-- open-swe-reviewer pr=99 -->" in body
|
||||||
|
|
||||||
|
|
||||||
|
def test_render_review_body_includes_trace_link_when_provided() -> None:
|
||||||
|
body = render_review_body(
|
||||||
|
pr_number=123,
|
||||||
|
surfaced_count=0,
|
||||||
|
trace_url="https://smith.langchain.com/o/t/project/p/t/thread-id",
|
||||||
|
)
|
||||||
|
assert "[View Open SWE trace](https://smith.langchain.com/o/t/project/p/t/thread-id)" in body
|
||||||
|
assert body.endswith("<!-- open-swe-reviewer pr=123 -->")
|
||||||
|
|
||||||
|
|
||||||
def test_publish_review_eval_mode_does_not_call_github() -> None:
|
def test_publish_review_eval_mode_does_not_call_github() -> None:
|
||||||
from agent.tools.publish_review import publish_review
|
from agent.tools.publish_review import publish_review
|
||||||
|
|
||||||
|
|
@ -223,6 +233,87 @@ def test_publish_review_eval_mode_does_not_call_github() -> None:
|
||||||
set_meta.assert_awaited_once_with("tid", last_reviewed_sha="sha")
|
set_meta.assert_awaited_once_with("tid", last_reviewed_sha="sha")
|
||||||
|
|
||||||
|
|
||||||
|
def test_publish_review_forwards_trace_link_config_override() -> None:
|
||||||
|
from agent.tools.publish_review import publish_review
|
||||||
|
|
||||||
|
publish_async = AsyncMock(return_value={"success": True})
|
||||||
|
with (
|
||||||
|
patch(
|
||||||
|
"agent.tools.publish_review.get_config",
|
||||||
|
return_value={
|
||||||
|
"configurable": {
|
||||||
|
"thread_id": "reviewer-thread-id",
|
||||||
|
"repo": {"owner": "o", "name": "r"},
|
||||||
|
"pr_number": 7,
|
||||||
|
"head_sha": "sha",
|
||||||
|
"review_trace_link_enabled": False,
|
||||||
|
},
|
||||||
|
"metadata": {},
|
||||||
|
},
|
||||||
|
),
|
||||||
|
patch("agent.tools.publish_review.get_github_token", return_value="token"),
|
||||||
|
patch("agent.tools.publish_review._publish_review_async", publish_async),
|
||||||
|
):
|
||||||
|
result = publish_review()
|
||||||
|
|
||||||
|
assert result == {"success": True}
|
||||||
|
assert publish_async.call_args.kwargs["trace_link_config_override"] is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_resolve_review_trace_url_enabled_by_team_setting() -> None:
|
||||||
|
from agent.tools.publish_review import _resolve_review_trace_url
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch(
|
||||||
|
"agent.tools.publish_review.get_team_review_trace_links_enabled",
|
||||||
|
AsyncMock(return_value=True),
|
||||||
|
),
|
||||||
|
patch(
|
||||||
|
"agent.tools.publish_review.get_langsmith_trace_url",
|
||||||
|
return_value="https://smith/t",
|
||||||
|
),
|
||||||
|
):
|
||||||
|
url = await _resolve_review_trace_url("reviewer-thread-id", None)
|
||||||
|
|
||||||
|
assert url == "https://smith/t"
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_resolve_review_trace_url_disabled_by_team_setting() -> None:
|
||||||
|
from agent.tools.publish_review import _resolve_review_trace_url
|
||||||
|
|
||||||
|
trace_url = MagicMock(return_value="https://smith/t")
|
||||||
|
with (
|
||||||
|
patch(
|
||||||
|
"agent.tools.publish_review.get_team_review_trace_links_enabled",
|
||||||
|
AsyncMock(return_value=False),
|
||||||
|
),
|
||||||
|
patch("agent.tools.publish_review.get_langsmith_trace_url", trace_url),
|
||||||
|
):
|
||||||
|
url = await _resolve_review_trace_url("reviewer-thread-id", None)
|
||||||
|
|
||||||
|
assert url is None
|
||||||
|
trace_url.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_resolve_review_trace_url_config_override_skips_team_lookup() -> None:
|
||||||
|
from agent.tools.publish_review import _resolve_review_trace_url
|
||||||
|
|
||||||
|
team_lookup = AsyncMock(return_value=True)
|
||||||
|
trace_url = MagicMock(return_value="https://smith/t")
|
||||||
|
with (
|
||||||
|
patch("agent.tools.publish_review.get_team_review_trace_links_enabled", team_lookup),
|
||||||
|
patch("agent.tools.publish_review.get_langsmith_trace_url", trace_url),
|
||||||
|
):
|
||||||
|
url = await _resolve_review_trace_url("reviewer-thread-id", False)
|
||||||
|
|
||||||
|
assert url is None
|
||||||
|
team_lookup.assert_not_called()
|
||||||
|
trace_url.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_resolve_review_thread_returns_true_on_success() -> None:
|
async def test_resolve_review_thread_returns_true_on_success() -> None:
|
||||||
response = MagicMock()
|
response = MagicMock()
|
||||||
|
|
|
||||||
|
|
@ -97,6 +97,7 @@ export interface TeamSettings {
|
||||||
trigger_mode: TriggerMode;
|
trigger_mode: TriggerMode;
|
||||||
review_draft_prs: boolean;
|
review_draft_prs: boolean;
|
||||||
pr_summaries: boolean;
|
pr_summaries: boolean;
|
||||||
|
review_trace_links: boolean;
|
||||||
autofix_mode: AutofixMode;
|
autofix_mode: AutofixMode;
|
||||||
autofix_severity_threshold: AutofixMode;
|
autofix_severity_threshold: AutofixMode;
|
||||||
default_agent_model?: string | null;
|
default_agent_model?: string | null;
|
||||||
|
|
|
||||||
|
|
@ -48,6 +48,7 @@ const DEFAULT_SETTINGS: TeamSettings = {
|
||||||
trigger_mode: "every_push",
|
trigger_mode: "every_push",
|
||||||
review_draft_prs: false,
|
review_draft_prs: false,
|
||||||
pr_summaries: true,
|
pr_summaries: true,
|
||||||
|
review_trace_links: true,
|
||||||
autofix_mode: "off",
|
autofix_mode: "off",
|
||||||
autofix_severity_threshold: "medium",
|
autofix_severity_threshold: "medium",
|
||||||
default_agent_model: null,
|
default_agent_model: null,
|
||||||
|
|
@ -176,6 +177,17 @@ function ReviewPage() {
|
||||||
/>
|
/>
|
||||||
}
|
}
|
||||||
/>
|
/>
|
||||||
|
<SettingsRow
|
||||||
|
label="Trace Links"
|
||||||
|
description="Include a LangSmith trace link in each review comment. Only members of your LangSmith workspace can open it."
|
||||||
|
control={
|
||||||
|
<Switch
|
||||||
|
checked={current.review_trace_links}
|
||||||
|
onCheckedChange={(v) => persist({ review_trace_links: v })}
|
||||||
|
disabled={!canEdit}
|
||||||
|
/>
|
||||||
|
}
|
||||||
|
/>
|
||||||
<SettingsRow
|
<SettingsRow
|
||||||
label="Autofix Mode"
|
label="Autofix Mode"
|
||||||
description="When enabled, the reviewer will propose fixes. Billed at plan rates."
|
description="When enabled, the reviewer will propose fixes. Billed at plan rates."
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue