From 8e39f62122805b461370e29f5c021940c77273b4 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Wed, 17 Jun 2026 14:12:04 -0700 Subject: [PATCH] feat: activate PR babysitting UI toggles for autofix and trigger mode (#1561) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: activate PR babysitting UI toggles for autofix and trigger mode Remove the "coming soon" gating on the Autofix Mode, Autofix Severity Threshold, and Trigger Mode controls in the review settings page so admins can enable CI auto-fix and review-comment resolution on PRs that Open SWE opens. The backend (ci_autofix.py, webapp.py webhook routing) was already fully wired — only the UI was disabled. Co-authored-by: open-swe[bot] * feat: simplify autofix to on/off toggle, remove severity threshold Replace the four-level AutofixMode (off/low/medium/high) and the autofix_severity_threshold setting with a single boolean autofix_enabled toggle. The severity threshold was leftover from the reviewer finding-severity model and does not apply to CI autofix; the agent should fix any failing CI and resolve any comments on PRs it opens. Co-authored-by: open-swe[bot] * feat: move autofix toggle to per-user profile, remove team-level setting The autofix toggle is now per-user (auto_fix_ci in the user profile) instead of team-level (admin-only). This uses the existing auto_fix_ci field that was already in ProfileUpdate but never wired up. Changes: - ci_autofix.py: check per-user auto_fix_ci profile flag after resolving the agent thread's github_login, instead of checking team-level autofix_enabled before knowing the PR - webapp.py: removed early is_autofix_enabled() webhook gates; the per-user check now happens in ci_autofix.py once the thread is found - team_settings.py: removed autofix_enabled field, is_autofix_enabled() - cloud-agents.tsx: enabled the auto_fix_ci toggle (was comingSoon) - review.tsx: removed the admin-level autofix switch - Updated tests and AGENTS.md The agent graph (not the reviewer) is what gets dispatched - this was already correct in ci_autofix.py line 223: client.runs.create( thread_id, "agent", ...). Co-authored-by: open-swe[bot] * feat: batch PR babysitting events Remove the leftover trigger-mode gate from PR babysitting and batch new CI/review events while an agent run is already active so the running agent can handle the latest PR state before finishing. Also moves review-feedback permission checks behind the per-user opt-out and applies the auto-fix profile gate to merge-conflict babysitting. Co-authored-by: open-swe[bot] * fix: consume batched babysitting events Teach the agent queue middleware to turn pending PR babysitting metadata into an injected instruction for the active run, so batched CI/review events are not dropped while still avoiding duplicate run creation. Co-authored-by: open-swe[bot] * fix: address review findings in PR babysitting batching - Route batched events through the LangGraph store (read in-process by the message-queue middleware) instead of a per-model-call threads.get on every agent thread. - Only record an attempt / mark the head SHA handled on a real dispatch, not on a batch, so an event isn't permanently dropped if the in-flight run ends before consuming it. - Carry the reviewer's comment through batched review feedback instead of replacing it with a generic re-check nudge. --------- Co-authored-by: open-swe[bot] --- AGENTS.md | 2 +- agent/ci_autofix.py | 180 +++++++++++++++++------- agent/dashboard/autofix_state.py | 2 +- agent/dashboard/team_settings.py | 48 +------ agent/middleware/check_message_queue.py | 72 +++++++--- agent/webapp.py | 31 +--- tests/test_autofix_state.py | 41 +----- tests/test_autofix_webhook.py | 18 +-- tests/test_check_message_queue.py | 47 ++++++- tests/test_ci_autofix.py | 131 +++++++++-------- ui/src/lib/api.ts | 6 - ui/src/routes/cloud-agents.tsx | 4 +- ui/src/routes/review.tsx | 112 +-------------- 13 files changed, 312 insertions(+), 382 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6e52f5c3..11f2f656 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -34,7 +34,7 @@ make format # ruff format + ruff check --fix The FastAPI app is `agent.webapp:app`. -CI auto-fix ("PR babysitting") lives in `agent/ci_autofix.py`: when a CI check fails (webhook `check_run` / `check_suite` / `workflow_run` / `status`) or a reviewer leaves actionable feedback on a PR Open SWE opened, it locates the originating agent thread (by `pr_url` metadata) and dispatches a confidence-gated fix run on the `agent` graph. Gated by the team `autofix_mode` / `trigger_mode` settings, the enabled-repos opt-in, and a per-PR `@open-swe autofix on|off` toggle (`agent/dashboard/autofix_state.py`). Skip-rules (base-branch failures, human commits, dedupe, loop cap) all live in `ci_autofix.py`. +CI auto-fix ("PR babysitting") lives in `agent/ci_autofix.py`: when a CI check fails (webhook `check_run` / `check_suite` / `workflow_run` / `status`) or a reviewer leaves actionable feedback on a PR Open SWE opened, it locates the originating agent thread (by `pr_url` metadata) and dispatches a confidence-gated fix run on the `agent` graph. Gated by the per-user `auto_fix_ci` profile flag, the enabled-repos opt-in, and a per-PR `@open-swe autofix on|off` toggle (`agent/dashboard/autofix_state.py`). Skip-rules (base-branch failures, human commits, same-head dedupe, batching while runs are active, loop cap) all live in `ci_autofix.py`. ## Architecture diff --git a/agent/ci_autofix.py b/agent/ci_autofix.py index aa40903c..4a20f329 100644 --- a/agent/ci_autofix.py +++ b/agent/ci_autofix.py @@ -11,8 +11,8 @@ loop-capping live in one place. Skip-rules mirror Cursor/Claude Code: * Only PRs Open SWE authored (an agent thread with this ``pr_url`` exists). * Skip failures inherited from the base branch. * Skip when the latest commit was authored by a human (don't fight pushes). -* Dedupe per (head SHA + failing-check set); cap total attempts. -* Honor team ``autofix_mode`` / ``trigger_mode`` and the per-PR opt-out. +* Dedupe per head SHA; cap total attempts. +* Honor the per-user ``auto_fix_ci`` profile flag and the per-PR opt-out. """ from __future__ import annotations @@ -22,9 +22,9 @@ from typing import Any from langgraph_sdk import get_client +from .dashboard.agent_overrides import load_profile, resolve_login_from_email_async from .dashboard.autofix_state import is_pr_autofix_disabled from .dashboard.enabled_repos import is_review_repo_enabled -from .dashboard.team_settings import get_autofix_settings from .reviewer_findings import REVIEWER_THREAD_KIND from .utils.dashboard_links import dashboard_thread_url from .utils.github_app import get_github_app_installation_token @@ -32,6 +32,7 @@ from .utils.github_checks import post_autofix_status_check from .utils.github_ci import ( fetch_open_pr_for_branch, fetch_pr, + has_repo_write_permission, head_commit_author_login, list_failing_check_runs, list_failing_statuses, @@ -41,7 +42,6 @@ from .utils.github_org_membership import INTERNAL_BOT_LOGINS from .utils.thread_ops import ( is_thread_active, langgraph_client, - queue_message_for_thread, ) logger = logging.getLogger(__name__) @@ -51,10 +51,27 @@ logger = logging.getLogger(__name__) MAX_AUTOFIX_ATTEMPTS = 10 # Keep the dedupe list bounded on thread metadata. _MAX_HANDLED_KEYS = 30 +# Store location for batched auto-fix events, consumed by the message-queue middleware. +_PENDING_AUTOFIX_NS = "autofix" +_PENDING_AUTOFIX_KEY = "pending_event" -def _dedupe_key(head_sha: str, failing_names: list[str]) -> str: - return f"{head_sha}:" + ",".join(sorted(failing_names)) +def _dedupe_key(head_sha: str) -> str: + return head_sha + + +async def _user_autofix_enabled(github_login: str, user_email: str = "") -> bool: + """Check the per-user ``auto_fix_ci`` profile flag (defaults to True).""" + login = github_login.strip() if isinstance(github_login, str) else "" + if not login and user_email: + login = await resolve_login_from_email_async(user_email) + if not login: + return True + profile = await load_profile(login) + if not isinstance(profile, dict): + return True + value = profile.get("auto_fix_ci") + return value if isinstance(value, bool) else True async def find_agent_thread_for_pr(pr_url: str) -> tuple[str, dict[str, Any]] | None: @@ -124,7 +141,10 @@ def _build_ci_fix_prompt( "need, then stop.\n" "4. Never force-push. Never weaken or delete test assertions just to go " "green unless the behavior change is intentional and correct.\n" - "5. After you push, CI re-runs automatically — you don't need to merge." + "5. Before finishing, re-check the PR's latest CI status and review " + "comments. Address any newly failed checks or unhandled actionable " + "comments that arrived while you were working.\n" + "6. After you push, CI re-runs automatically — you don't need to merge." ) @@ -149,19 +169,24 @@ def _build_review_feedback_prompt( "existing branch.\n" "2. If the comment is ambiguous, opinion-based, or needs a design " "decision, reply on the PR asking for clarification instead of guessing.\n" - "3. Never force-push. Reply to the reviewer on GitHub to explain what " + "3. Before finishing, re-check the PR's latest review comments and CI " + "status. Address any newly arrived actionable comments or failed checks " + "that are clear and deterministic.\n" + "4. Never force-push. Reply to the reviewer on GitHub to explain what " "you changed." ) -async def _thread_autofix_state(metadata: dict[str, Any]) -> tuple[int, list[str], str]: +async def _thread_autofix_state(metadata: dict[str, Any]) -> tuple[int, list[str], str, str]: attempts = metadata.get("autofix_attempts") attempts = attempts if isinstance(attempts, int) and attempts >= 0 else 0 handled = metadata.get("autofix_handled") handled = [h for h in handled if isinstance(h, str)] if isinstance(handled, list) else [] github_login = metadata.get("github_login") github_login = github_login if isinstance(github_login, str) else "" - return attempts, handled, github_login + user_email = metadata.get("triggering_user_email") + user_email = user_email if isinstance(user_email, str) else "" + return attempts, handled, github_login, user_email async def _record_attempt( @@ -212,11 +237,44 @@ def _run_configurable( return configurable -async def _dispatch_or_queue(thread_id: str, prompt: str, *, configurable: dict[str, Any]) -> str: +async def _mark_pending_autofix_event(thread_id: str, reason: str, detail: str = "") -> None: + """Record a batched auto-fix event in the store the message-queue middleware reads. + + Uses the same store namespace mechanism as ``queue_message_for_thread`` so the + in-flight run picks it up in-process at its next ``before_model`` step — no + per-step thread fetch. ``detail`` (e.g. a reviewer's comment) is accumulated so + specifics aren't lost when several events batch against one busy run. + """ + client = langgraph_client() + namespace = (_PENDING_AUTOFIX_NS, thread_id) + try: + details: list[str] = [] + try: + existing = await client.store.get_item(namespace, _PENDING_AUTOFIX_KEY) + if existing and existing.get("value"): + prior = existing["value"].get("details") + if isinstance(prior, list): + details = [d for d in prior if isinstance(d, str)] + except Exception: # noqa: BLE001 + logger.debug("No existing pending auto-fix event for thread %s", thread_id) + if detail and detail not in details: + details.append(detail) + await client.store.put_item( + namespace, _PENDING_AUTOFIX_KEY, {"reason": reason, "details": details} + ) + except Exception: # noqa: BLE001 + logger.debug( + "Failed to record pending auto-fix event for thread %s", thread_id, exc_info=True + ) + + +async def _dispatch_or_batch( + thread_id: str, prompt: str, *, configurable: dict[str, Any], reason: str, detail: str = "" +) -> str: if await is_thread_active(thread_id): - logger.info("Agent thread %s busy; queuing auto-fix message", thread_id) - await queue_message_for_thread(thread_id, prompt) - return "queued" + logger.info("Agent thread %s busy; batching auto-fix event %s", thread_id, reason) + await _mark_pending_autofix_event(thread_id, reason, detail) + return "batched" client = langgraph_client() await client.runs.create( thread_id, @@ -247,9 +305,6 @@ async def handle_ci_failure( if not owner or not repo: return "missing_repo" - settings = await get_autofix_settings() - if settings["autofix_mode"] == "off": - return "autofix_disabled_team" if not await is_review_repo_enabled(owner, repo): return "repo_not_enabled" @@ -284,12 +339,11 @@ async def handle_ci_failure( return "no_agent_thread" thread_id, metadata = found - attempts, handled, github_login = await _thread_autofix_state(metadata) + attempts, handled, github_login, user_email = await _thread_autofix_state(metadata) + + if not await _user_autofix_enabled(github_login, user_email): + return "autofix_disabled_user" - if settings["trigger_mode"] == "manual": - return "trigger_manual" - if settings["trigger_mode"] == "once_per_pr" and attempts >= 1: - return "once_per_pr_done" if attempts >= MAX_AUTOFIX_ATTEMPTS: await post_autofix_status_check( owner=owner, @@ -321,8 +375,7 @@ async def handle_ci_failure( if not actionable: return "all_failing_on_base" - failing_names = [c.get("name", "") for c in actionable] - dedupe_key = _dedupe_key(head_sha, failing_names) + dedupe_key = _dedupe_key(head_sha) if dedupe_key in handled: return "already_handled" @@ -351,28 +404,34 @@ async def handle_ci_failure( head_sha=head_sha, failing_checks=actionable, ) - result = await _dispatch_or_queue( + result = await _dispatch_or_batch( thread_id, prompt, configurable=_run_configurable( metadata, repo_config={"owner": owner, "name": repo}, pr_number=pr_number ), + reason="ci_failure", ) - await _record_attempt( - thread_id, attempts=attempts, handled=handled, dedupe_key=dedupe_key, head_sha=head_sha - ) - await post_autofix_status_check( - owner=owner, - repo=repo, - head_sha=head_sha, - token=token, - title=f"Auto-fixing {len(actionable)} failing check(s)", - summary=( - "Open SWE is investigating the failing checks and will push a fix if " - "the cause is clear. Track progress in the linked run." - ), - details_url=dashboard_thread_url(thread_id), - ) + if result == "dispatched": + # Only burn an attempt / mark the SHA handled on a real dispatch. A batched + # event is just a nudge to the in-flight run; if that run ends before + # consuming it, leaving the SHA un-handled lets a later webhook or the sweep + # re-dispatch instead of silently dropping the failure. + await _record_attempt( + thread_id, attempts=attempts, handled=handled, dedupe_key=dedupe_key, head_sha=head_sha + ) + await post_autofix_status_check( + owner=owner, + repo=repo, + head_sha=head_sha, + token=token, + title=f"Auto-fixing {len(actionable)} failing check(s)", + summary=( + "Open SWE is investigating the failing checks and will push a fix if " + "the cause is clear. Track progress in the linked run." + ), + details_url=dashboard_thread_url(thread_id), + ) return result @@ -392,11 +451,6 @@ async def handle_review_feedback( if not owner or not repo or not pr_url: return "missing_repo" - settings = await get_autofix_settings() - if settings["autofix_mode"] == "off": - return "autofix_disabled_team" - if settings["trigger_mode"] == "manual": - return "trigger_manual" if not await is_review_repo_enabled(owner, repo): return "repo_not_enabled" if await is_pr_autofix_disabled(owner, repo, pr_number): @@ -407,6 +461,24 @@ async def handle_review_feedback( return "no_agent_thread" thread_id, metadata = found + _, _, github_login, user_email = await _thread_autofix_state(metadata) + if not await _user_autofix_enabled(github_login, user_email): + return "autofix_disabled_user" + + if token is None: + token = await get_github_app_installation_token() + if not token: + return "no_token" + if not await has_repo_write_permission(owner=owner, repo=repo, username=reviewer, token=token): + logger.info( + "Skipping auto-fix review feedback on %s/%s#%s: %s lacks write access", + owner, + repo, + pr_number, + reviewer or "", + ) + return "reviewer_no_write_permission" + prompt = _build_review_feedback_prompt( owner=owner, repo=repo, @@ -415,12 +487,16 @@ async def handle_review_feedback( reviewer=reviewer, body=body, ) - return await _dispatch_or_queue( + return await _dispatch_or_batch( thread_id, prompt, configurable=_run_configurable( metadata, repo_config={"owner": owner, "name": repo}, pr_number=pr_number ), + reason="review_feedback", + detail=f"Reviewer {reviewer or 'unknown'} commented: {body.strip()}" + if body.strip() + else "", ) @@ -431,7 +507,7 @@ async def sweep_open_prs() -> dict[str, int]: only path that can react to base-branch merge conflicts (GitHub emits no webhook for those). """ - counts = {"scanned": 0, "dispatched": 0, "queued": 0, "conflicts": 0} + counts = {"scanned": 0, "dispatched": 0, "batched": 0, "conflicts": 0} token = await get_github_app_installation_token() if not token: logger.warning("CI monitor sweep: no GitHub App token") @@ -487,8 +563,8 @@ async def sweep_open_prs() -> dict[str, int]: ) if result == "dispatched": counts["dispatched"] += 1 - elif result == "queued": - counts["queued"] += 1 + elif result == "batched": + counts["batched"] += 1 logger.info("CI monitor sweep complete: %s", counts) return counts @@ -503,6 +579,9 @@ async def _flag_merge_conflict( if found is None: return thread_id, metadata = found + _, _, github_login, user_email = await _thread_autofix_state(metadata) + if not await _user_autofix_enabled(github_login, user_email): + return if metadata.get("autofix_conflict_head") == head_sha: return prompt = ( @@ -512,12 +591,13 @@ async def _flag_merge_conflict( "conflict resolution is ambiguous, comment on the PR and ask before " "guessing. Never force-push over commits already on the remote." ) - await _dispatch_or_queue( + await _dispatch_or_batch( thread_id, prompt, configurable=_run_configurable( metadata, repo_config={"owner": owner, "name": repo}, pr_number=pr_number ), + reason="merge_conflict", ) try: await get_client().threads.update( diff --git a/agent/dashboard/autofix_state.py b/agent/dashboard/autofix_state.py index 605c7ca4..687e71cf 100644 --- a/agent/dashboard/autofix_state.py +++ b/agent/dashboard/autofix_state.py @@ -1,6 +1,6 @@ """Per-PR auto-fix opt-out, stored in the LangGraph Store. -Team-wide auto-fix is gated by :func:`agent.dashboard.team_settings.is_autofix_enabled`. +Auto-fix is gated by the per-user ``auto_fix_ci`` profile flag. On top of that, a single PR can be silenced with ``@open-swe autofix off`` (and re-enabled with ``@open-swe autofix on``), mirroring Cursor's ``@cursor autofix off`` per-PR control. The toggle lives here rather than on the diff --git a/agent/dashboard/team_settings.py b/agent/dashboard/team_settings.py index 47af5436..c9cb3fd0 100644 --- a/agent/dashboard/team_settings.py +++ b/agent/dashboard/team_settings.py @@ -27,21 +27,15 @@ logger = logging.getLogger(__name__) TEAM_SETTINGS_NAMESPACE: list[str] = ["team_settings"] TEAM_SETTINGS_KEY = "default" -TriggerMode = Literal["every_push", "once_per_pr", "manual"] -AutofixMode = Literal["off", "low", "medium", "high"] - # Cap the org-wide guidelines so a runaway value can't dominate the reviewer # prompt. Generous enough for a detailed policy, small enough to stay bounded. ORG_GUIDELINES_MAX_CHARS = 10_000 class TeamSettingsUpdate(BaseModel): - trigger_mode: TriggerMode = "every_push" review_draft_prs: bool = False pr_summaries: bool = True review_trace_links: bool = True - autofix_mode: AutofixMode = "off" - autofix_severity_threshold: AutofixMode = "medium" org_guidelines: str | None = None default_agent_model: str | None = None default_agent_reasoning_effort: str | None = None @@ -135,12 +129,9 @@ def _parse_repo(value: object) -> dict[str, str] | None: def _default_settings() -> dict[str, Any]: fallback_model, fallback_effort = default_model_pair() return { - "trigger_mode": "every_push", "review_draft_prs": False, "pr_summaries": True, "review_trace_links": True, - "autofix_mode": "off", - "autofix_severity_threshold": "medium", "org_guidelines": None, "default_agent_model": fallback_model, "default_agent_reasoning_effort": fallback_effort, @@ -178,21 +169,21 @@ async def get_team_settings() -> dict[str, Any]: # selection) still surface the hardcoded default instead of a null. overlay = {k: v for k, v in value.items() if v is not None} merged = {**defaults, **overlay} - # Drop obsolete trigger mode values so a legacy record doesn't surface a - # value the new TriggerMode literal would reject on the next PUT. - if merged.get("trigger_mode") not in {"every_push", "once_per_pr", "manual"}: - merged["trigger_mode"] = defaults["trigger_mode"] + for stale_field in ( + "trigger_mode", + "autofix_mode", + "autofix_severity_threshold", + "autofix_enabled", + ): + merged.pop(stale_field, None) return merged async def upsert_team_settings(update: TeamSettingsUpdate) -> dict[str, Any]: value: dict[str, Any] = { - "trigger_mode": update.trigger_mode, "review_draft_prs": update.review_draft_prs, "pr_summaries": update.pr_summaries, "review_trace_links": update.review_trace_links, - "autofix_mode": update.autofix_mode, - "autofix_severity_threshold": update.autofix_severity_threshold, "org_guidelines": update.org_guidelines, "default_agent_model": update.default_agent_model, "default_agent_reasoning_effort": update.default_agent_reasoning_effort, @@ -306,31 +297,6 @@ async def get_team_default_grouping_model() -> tuple[str, str]: ) -async def get_autofix_settings() -> dict[str, Any]: - """Return the team-wide auto-fix config: mode, severity threshold, trigger mode.""" - settings = await get_team_settings() - mode = settings.get("autofix_mode") - if mode not in {"off", "low", "medium", "high"}: - mode = "off" - threshold = settings.get("autofix_severity_threshold") - if threshold not in {"off", "low", "medium", "high"}: - threshold = "medium" - trigger = settings.get("trigger_mode") - if trigger not in {"every_push", "once_per_pr", "manual"}: - trigger = "every_push" - return { - "autofix_mode": mode, - "autofix_severity_threshold": threshold, - "trigger_mode": trigger, - } - - -async def is_autofix_enabled() -> bool: - """Return whether team-wide auto-fix is turned on (mode != ``off``).""" - settings = await get_autofix_settings() - return settings["autofix_mode"] != "off" - - 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() diff --git a/agent/middleware/check_message_queue.py b/agent/middleware/check_message_queue.py index 2c42ef53..a29d3d2c 100644 --- a/agent/middleware/check_message_queue.py +++ b/agent/middleware/check_message_queue.py @@ -14,6 +14,7 @@ import httpx from langchain.agents.middleware import AgentState, before_model from langgraph.config import get_config, get_store from langgraph.runtime import Runtime +from langgraph.store.base import BaseStore from langgraph_sdk import get_client from ..dashboard.options import model_supports_images @@ -91,6 +92,49 @@ def _is_dashboard_queued_message(content: object) -> bool: return isinstance(content, dict) and content.get("source") == "dashboard" +def _message_update(content_blocks: list[dict[str, Any]], thread_id: str) -> dict[str, Any] | None: + if not content_blocks: + return None + logger.info( + "Injected %d queued message block(s) into state for thread %s", + len(content_blocks), + thread_id, + ) + return {"messages": [{"role": "user", "content": content_blocks}]} + + +async def _consume_pending_autofix_event(store: BaseStore, thread_id: str) -> str | None: + """Pull and clear a batched PR-babysitting event from the store (no thread fetch).""" + namespace = ("autofix", thread_id) + try: + item = await store.aget(namespace, "pending_event") + except Exception: # noqa: BLE001 + logger.debug( + "Could not read pending auto-fix event for thread %s", thread_id, exc_info=True + ) + return None + if item is None or not item.value.get("reason"): + return None + try: + await store.adelete(namespace, "pending_event") + except Exception: # noqa: BLE001 + logger.debug( + "Could not clear pending auto-fix event for thread %s", thread_id, exc_info=True + ) + message = ( + "A PR babysitting event arrived while you were already working on this PR. " + "Do not start a separate run for that event. Before finishing, re-check the " + "PR's latest CI status and review comments, then address any newly failed " + "checks or actionable comments that are clear and deterministic." + ) + details = item.value.get("details") + if isinstance(details, list): + joined = "\n\n".join(d for d in details if isinstance(d, str) and d) + if joined: + message += "\n\nNewly arrived feedback to address:\n" + joined + return message + + @before_model(state_schema=LinearNotifyState) async def check_message_queue_before_model( # noqa: PLR0911 state: LinearNotifyState, # noqa: ARG001 @@ -122,16 +166,21 @@ async def check_message_queue_before_model( # noqa: PLR0911 if store is None: return None + content_blocks: list[dict[str, Any]] = [] + pending_autofix = await _consume_pending_autofix_event(store, thread_id) + if pending_autofix: + content_blocks.append({"type": "text", "text": pending_autofix}) + namespace = ("queue", thread_id) try: queued_item = await store.aget(namespace, "pending_messages") except Exception as e: # noqa: BLE001 logger.warning("Failed to get queued item: %s", e) - return None + return _message_update(content_blocks, thread_id) if queued_item is None: - return None + return _message_update(content_blocks, thread_id) queued_value = queued_item.value queued_messages = queued_value.get("messages", []) @@ -140,7 +189,7 @@ async def check_message_queue_before_model( # noqa: PLR0911 await store.adelete(namespace, "pending_messages") if not queued_messages: - return None + return _message_update(content_blocks, thread_id) logger.info( "Found %d queued message(s) for thread %s, injecting into state", @@ -157,7 +206,6 @@ async def check_message_queue_before_model( # noqa: PLR0911 if has_images: resolved_model_id = await _resolve_thread_model_id(thread_id) - content_blocks: list[dict[str, Any]] = [] for msg in queued_messages: content = msg.get("content") if _is_dashboard_queued_message(content): @@ -177,21 +225,7 @@ async def check_message_queue_before_model( # noqa: PLR0911 logger.debug("Queued message contains text content") content_blocks.append({"type": "text", "text": content}) - if not content_blocks: - return None - - new_message = { - "role": "user", - "content": content_blocks, - } - - logger.info( - "Injected %d queued message(s) into state for thread %s", - len(content_blocks), - thread_id, - ) - - return {"messages": [new_message]} # noqa: TRY300 + return _message_update(content_blocks, thread_id) # noqa: TRY300 except Exception: logger.exception("Error in check_message_queue_before_model") return None diff --git a/agent/webapp.py b/agent/webapp.py index 488c1095..a834cd88 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -35,7 +35,6 @@ from .dashboard.profiles import get_profile, get_valid_access_token, has_access_ from .dashboard.team_settings import ( get_team_default_repo, get_team_settings, - is_autofix_enabled, ) from .dashboard.user_mappings import ( email_for_login, @@ -72,7 +71,6 @@ from .utils.github_app import ( from .utils.github_checks import complete_review_check_run, create_review_check_run from .utils.github_ci import ( branch_from_check_payload, - has_repo_write_permission, head_sha_from_check_payload, is_failing_ci_payload, ) @@ -2752,20 +2750,6 @@ async def process_github_autofix_review(payload: dict[str, Any], event_type: str body = (comment.get("body") or "") if isinstance(comment, dict) else "" if not body.strip() or reviewer in INTERNAL_BOT_LOGINS: return - # Defense-in-depth beyond the author_association gate: confirm the reviewer - # actually has write access before dispatching a write-capable agent run. - token = await get_github_app_installation_token() - if not token or not await has_repo_write_permission( - owner=ref["owner"], repo=ref["name"], username=reviewer, token=token - ): - logger.info( - "Skipping auto-fix review feedback on %s/%s#%s: %s lacks write access", - ref["owner"], - ref["name"], - ref["number"], - reviewer or "", - ) - return result = await handle_review_feedback( repo_config={"owner": ref["owner"], "name": ref["name"]}, pr_number=ref["number"], @@ -3348,10 +3332,10 @@ async def github_webhook(request: Request, background_tasks: BackgroundTasks) -> return {"status": "accepted", "message": "Processing GitHub push for reviewer watch"} if event_type in _GH_CI_EVENTS: + if not is_failing_ci_payload(payload, event_type): + return {"status": "ignored", "reason": "CI event is not a completed failure"} if not await _is_repo_enabled_for_review(webhook_repo_config): return {"status": "ignored", "reason": "Repository not enabled for review"} - if not await is_autofix_enabled(): - return {"status": "ignored", "reason": "Auto-fix is disabled"} logger.info("Accepted GitHub %s webhook, scheduling CI auto-fix evaluation", event_type) background_tasks.add_task(process_github_ci_event, payload, event_type) return {"status": "accepted", "message": f"Processing GitHub {event_type} for auto-fix"} @@ -3432,12 +3416,11 @@ async def github_webhook(request: Request, background_tasks: BackgroundTasks) -> if _is_actionable_review_payload(payload, event_type) and await _is_repo_enabled_for_review( webhook_repo_config ): - if await is_autofix_enabled(): - gate_rejection = await _enforce_public_repo_org_gate(payload, event_type) - if gate_rejection is not None: - return gate_rejection - background_tasks.add_task(process_github_autofix_review, payload, event_type) - return {"status": "accepted", "message": "Processing auto-fix review feedback"} + gate_rejection = await _enforce_public_repo_org_gate(payload, event_type) + if gate_rejection is not None: + return gate_rejection + background_tasks.add_task(process_github_autofix_review, payload, event_type) + return {"status": "accepted", "message": "Processing auto-fix review feedback"} logger.debug( "Ignoring GitHub %s%s that does not mention @openswe or @open-swe", event_type, diff --git a/tests/test_autofix_state.py b/tests/test_autofix_state.py index 6a5b2311..e3fbad77 100644 --- a/tests/test_autofix_state.py +++ b/tests/test_autofix_state.py @@ -1,4 +1,4 @@ -"""Unit tests for per-PR auto-fix opt-out state and team settings accessor.""" +"""Unit tests for per-PR auto-fix opt-out state.""" from __future__ import annotations @@ -7,7 +7,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from agent.dashboard import autofix_state, team_settings +from agent.dashboard import autofix_state @pytest.mark.asyncio @@ -31,40 +31,3 @@ async def test_set_and_check_pr_disabled() -> None: assert await autofix_state.is_pr_autofix_disabled("o", "r", 5) is True await autofix_state.set_pr_autofix_disabled("o", "r", 5, False) assert await autofix_state.is_pr_autofix_disabled("o", "r", 5) is False - - -@pytest.mark.asyncio -async def test_get_autofix_settings_normalizes() -> None: - with patch.object( - team_settings, - "get_team_settings", - AsyncMock( - return_value={ - "autofix_mode": "bogus", - "autofix_severity_threshold": "high", - "trigger_mode": "weird", - } - ), - ): - settings = await team_settings.get_autofix_settings() - assert settings == { - "autofix_mode": "off", - "autofix_severity_threshold": "high", - "trigger_mode": "every_push", - } - - -@pytest.mark.asyncio -async def test_is_autofix_enabled() -> None: - with patch.object( - team_settings, - "get_team_settings", - AsyncMock(return_value={"autofix_mode": "high"}), - ): - assert await team_settings.is_autofix_enabled() is True - with patch.object( - team_settings, - "get_team_settings", - AsyncMock(return_value={"autofix_mode": "off"}), - ): - assert await team_settings.is_autofix_enabled() is False diff --git a/tests/test_autofix_webhook.py b/tests/test_autofix_webhook.py index 6181a536..c448fee7 100644 --- a/tests/test_autofix_webhook.py +++ b/tests/test_autofix_webhook.py @@ -164,30 +164,22 @@ async def test_autofix_review_dispatches_for_writer() -> None: "review": {"body": "rename to userId", "user": {"login": "alice"}}, } handle = AsyncMock(return_value="dispatched") - with ( - patch.object(webapp, "get_github_app_installation_token", AsyncMock(return_value="tok")), - patch.object(webapp, "has_repo_write_permission", AsyncMock(return_value=True)), - patch.object(webapp, "handle_review_feedback", handle), - ): + with patch.object(webapp, "handle_review_feedback", handle): await webapp.process_github_autofix_review(payload, "pull_request_review") handle.assert_awaited_once() @pytest.mark.asyncio -async def test_autofix_review_skips_non_writer() -> None: +async def test_autofix_review_delegates_permission_check_to_core() -> None: payload = { "repository": {"owner": {"login": "o"}, "name": "r"}, "pull_request": {"number": 9, "html_url": "https://github.com/o/r/pull/9"}, "review": {"body": "inject code", "user": {"login": "attacker"}}, } - handle = AsyncMock() - with ( - patch.object(webapp, "get_github_app_installation_token", AsyncMock(return_value="tok")), - patch.object(webapp, "has_repo_write_permission", AsyncMock(return_value=False)), - patch.object(webapp, "handle_review_feedback", handle), - ): + handle = AsyncMock(return_value="reviewer_no_write_permission") + with patch.object(webapp, "handle_review_feedback", handle): await webapp.process_github_autofix_review(payload, "pull_request_review") - handle.assert_not_called() + handle.assert_awaited_once() def test_ci_events_supported() -> None: diff --git a/tests/test_check_message_queue.py b/tests/test_check_message_queue.py index cb767017..56262ef3 100644 --- a/tests/test_check_message_queue.py +++ b/tests/test_check_message_queue.py @@ -18,12 +18,13 @@ class _QueuedItem: class _FakeStore: - def __init__(self, value: dict[str, Any]) -> None: - self.value = value + def __init__(self, items: dict[tuple[tuple[str, ...], str], dict[str, Any]]) -> None: + self.items = items self.deleted: list[tuple[tuple[str, ...], str]] = [] - async def aget(self, namespace: tuple[str, ...], key: str) -> _QueuedItem: - return _QueuedItem(self.value) + async def aget(self, namespace: tuple[str, ...], key: str) -> _QueuedItem | None: + value = self.items.get((namespace, key)) + return _QueuedItem(value) if value is not None else None async def adelete(self, namespace: tuple[str, ...], key: str) -> None: self.deleted.append((namespace, key)) @@ -33,9 +34,11 @@ class _FakeStore: async def test_check_message_queue_injects_dashboard_handoff_instruction() -> None: store = _FakeStore( { - "messages": [ - {"content": {"text": "continue in web", "source": "dashboard"}}, - ] + (("queue", "thread-1"), "pending_messages"): { + "messages": [ + {"content": {"text": "continue in web", "source": "dashboard"}}, + ] + } } ) @@ -56,6 +59,36 @@ async def test_check_message_queue_injects_dashboard_handoff_instruction() -> No assert store.deleted == [(("queue", "thread-1"), "pending_messages")] +@pytest.mark.asyncio +async def test_check_message_queue_injects_pending_autofix_event() -> None: + store = _FakeStore( + { + (("autofix", "thread-1"), "pending_event"): { + "reason": "review_feedback", + "details": ["Reviewer alice commented: rename to userId"], + } + } + ) + + with ( + patch( + "agent.middleware.check_message_queue.get_config", + return_value={"configurable": {"thread_id": "thread-1"}}, + ), + patch("agent.middleware.check_message_queue.get_store", return_value=store), + ): + result = await check_message_queue_before_model.abefore_model({}, MagicMock()) + + assert result is not None + message = result["messages"][0] + assert message["role"] == "user" + text = message["content"][0]["text"] + assert "PR babysitting event arrived" in text + # The reviewer's actual comment is carried through, not dropped for a generic nudge. + assert "rename to userId" in text + assert (("autofix", "thread-1"), "pending_event") in store.deleted + + @pytest.mark.asyncio async def test_build_blocks_skips_images_for_text_only_model() -> None: payload = { diff --git a/tests/test_ci_autofix.py b/tests/test_ci_autofix.py index a72e7970..12a4b91c 100644 --- a/tests/test_ci_autofix.py +++ b/tests/test_ci_autofix.py @@ -21,8 +21,11 @@ _PR = { def happy(monkeypatch: pytest.MonkeyPatch) -> dict[str, Any]: """Patch every dependency of handle_ci_failure to a happy-path default.""" runs_create = AsyncMock() + store_put = AsyncMock() lg_client = MagicMock() lg_client.runs.create = runs_create + lg_client.store.get_item = AsyncMock(return_value=None) + lg_client.store.put_item = store_put threads_update = AsyncMock() store_client = MagicMock() store_client.threads.update = threads_update @@ -31,20 +34,10 @@ def happy(monkeypatch: pytest.MonkeyPatch) -> dict[str, Any]: "runs_create": runs_create, "threads_update": threads_update, "status_check": AsyncMock(return_value=True), - "queue": AsyncMock(return_value=True), + "store_put": store_put, } - monkeypatch.setattr( - ci_autofix, - "get_autofix_settings", - AsyncMock( - return_value={ - "autofix_mode": "high", - "autofix_severity_threshold": "medium", - "trigger_mode": "every_push", - } - ), - ) + monkeypatch.setattr(ci_autofix, "_user_autofix_enabled", AsyncMock(return_value=True)) monkeypatch.setattr(ci_autofix, "is_review_repo_enabled", AsyncMock(return_value=True)) monkeypatch.setattr( ci_autofix, "get_github_app_installation_token", AsyncMock(return_value="tok") @@ -66,7 +59,6 @@ def happy(monkeypatch: pytest.MonkeyPatch) -> dict[str, Any]: ci_autofix, "head_commit_author_login", AsyncMock(return_value="open-swe[bot]") ) monkeypatch.setattr(ci_autofix, "is_thread_active", AsyncMock(return_value=False)) - monkeypatch.setattr(ci_autofix, "queue_message_for_thread", mocks["queue"]) monkeypatch.setattr(ci_autofix, "post_autofix_status_check", mocks["status_check"]) monkeypatch.setattr(ci_autofix, "langgraph_client", lambda: lg_client) monkeypatch.setattr(ci_autofix, "get_client", lambda: store_client) @@ -94,28 +86,21 @@ async def test_dispatch_happy_path(happy: dict[str, Any]) -> None: @pytest.mark.asyncio -async def test_queues_when_thread_busy(happy: dict[str, Any], monkeypatch) -> None: +async def test_batches_when_thread_busy(happy: dict[str, Any], monkeypatch) -> None: monkeypatch.setattr(ci_autofix, "is_thread_active", AsyncMock(return_value=True)) result = await _run() - assert result == "queued" - happy["queue"].assert_awaited_once() + assert result == "batched" + happy["store_put"].assert_awaited() happy["runs_create"].assert_not_called() + # A batched event must not burn an attempt or mark the SHA handled, so a later + # webhook/sweep can still dispatch if the in-flight run never consumes it. + happy["threads_update"].assert_not_awaited() @pytest.mark.asyncio -async def test_skip_team_disabled(happy: dict[str, Any], monkeypatch) -> None: - monkeypatch.setattr( - ci_autofix, - "get_autofix_settings", - AsyncMock( - return_value={ - "autofix_mode": "off", - "autofix_severity_threshold": "medium", - "trigger_mode": "every_push", - } - ), - ) - assert await _run() == "autofix_disabled_team" +async def test_skip_user_disabled(happy: dict[str, Any], monkeypatch) -> None: + monkeypatch.setattr(ci_autofix, "_user_autofix_enabled", AsyncMock(return_value=False)) + assert await _run() == "autofix_disabled_user" @pytest.mark.asyncio @@ -136,43 +121,6 @@ async def test_skip_no_agent_thread(happy: dict[str, Any], monkeypatch) -> None: assert await _run() == "no_agent_thread" -@pytest.mark.asyncio -async def test_skip_trigger_manual(happy: dict[str, Any], monkeypatch) -> None: - monkeypatch.setattr( - ci_autofix, - "get_autofix_settings", - AsyncMock( - return_value={ - "autofix_mode": "high", - "autofix_severity_threshold": "medium", - "trigger_mode": "manual", - } - ), - ) - assert await _run() == "trigger_manual" - - -@pytest.mark.asyncio -async def test_skip_once_per_pr_after_first(happy: dict[str, Any], monkeypatch) -> None: - monkeypatch.setattr( - ci_autofix, - "get_autofix_settings", - AsyncMock( - return_value={ - "autofix_mode": "high", - "autofix_severity_threshold": "medium", - "trigger_mode": "once_per_pr", - } - ), - ) - monkeypatch.setattr( - ci_autofix, - "find_agent_thread_for_pr", - AsyncMock(return_value=("t1", {"github_login": "alice", "autofix_attempts": 1})), - ) - assert await _run() == "once_per_pr_done" - - @pytest.mark.asyncio async def test_skip_max_attempts(happy: dict[str, Any], monkeypatch) -> None: monkeypatch.setattr( @@ -198,7 +146,7 @@ async def test_skip_all_failing_on_base(happy: dict[str, Any], monkeypatch) -> N @pytest.mark.asyncio async def test_skip_already_handled(happy: dict[str, Any], monkeypatch) -> None: - key = ci_autofix._dedupe_key("head1", ["lint"]) + key = ci_autofix._dedupe_key("head1") monkeypatch.setattr( ci_autofix, "find_agent_thread_for_pr", @@ -227,6 +175,55 @@ async def test_ci_read_failed(happy: dict[str, Any], monkeypatch) -> None: assert await _run(failing_checks=None) == "ci_read_failed" +@pytest.mark.asyncio +async def test_review_feedback_skips_user_disabled(happy: dict[str, Any], monkeypatch) -> None: + monkeypatch.setattr(ci_autofix, "_user_autofix_enabled", AsyncMock(return_value=False)) + assert ( + await ci_autofix.handle_review_feedback( + repo_config={"owner": "o", "name": "r"}, + pr_number=5, + pr_url="https://github.com/o/r/pull/5", + reviewer="alice", + body="fix this", + ) + == "autofix_disabled_user" + ) + happy["runs_create"].assert_not_called() + + +@pytest.mark.asyncio +async def test_review_feedback_batches_when_thread_busy(happy: dict[str, Any], monkeypatch) -> None: + monkeypatch.setattr(ci_autofix, "has_repo_write_permission", AsyncMock(return_value=True)) + monkeypatch.setattr(ci_autofix, "is_thread_active", AsyncMock(return_value=True)) + result = await ci_autofix.handle_review_feedback( + repo_config={"owner": "o", "name": "r"}, + pr_number=5, + pr_url="https://github.com/o/r/pull/5", + reviewer="alice", + body="fix this", + ) + assert result == "batched" + happy["runs_create"].assert_not_called() + + +@pytest.mark.asyncio +async def test_review_feedback_checks_write_permission_after_user_gate( + happy: dict[str, Any], monkeypatch +) -> None: + permission = AsyncMock(return_value=False) + monkeypatch.setattr(ci_autofix, "has_repo_write_permission", permission) + result = await ci_autofix.handle_review_feedback( + repo_config={"owner": "o", "name": "r"}, + pr_number=5, + pr_url="https://github.com/o/r/pull/5", + reviewer="alice", + body="fix this", + ) + assert result == "reviewer_no_write_permission" + permission.assert_awaited_once() + happy["runs_create"].assert_not_called() + + @pytest.mark.asyncio async def test_find_agent_thread_picks_agent_skips_reviewer(monkeypatch) -> None: client = MagicMock() diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index 72d87f41..c3c8d960 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -110,16 +110,10 @@ export interface ProfileUpdate { review_draft_prs?: boolean | null } -export type TriggerMode = "every_push" | "once_per_pr" | "manual" -export type AutofixMode = "off" | "low" | "medium" | "high" - export interface TeamSettings { - trigger_mode: TriggerMode review_draft_prs: boolean pr_summaries: boolean review_trace_links: boolean - autofix_mode: AutofixMode - autofix_severity_threshold: AutofixMode org_guidelines?: string | null default_agent_model?: string | null default_agent_reasoning_effort?: string | null diff --git a/ui/src/routes/cloud-agents.tsx b/ui/src/routes/cloud-agents.tsx index 1a83db84..1b2d35d3 100644 --- a/ui/src/routes/cloud-agents.tsx +++ b/ui/src/routes/cloud-agents.tsx @@ -294,13 +294,11 @@ function CloudAgentsPage() {
persist({ auto_fix_ci: v })} - disabled /> } /> diff --git a/ui/src/routes/review.tsx b/ui/src/routes/review.tsx index 851bcf15..672dc481 100644 --- a/ui/src/routes/review.tsx +++ b/ui/src/routes/review.tsx @@ -4,15 +4,8 @@ import { CaretRightIcon } from "@phosphor-icons/react"; import { useEffect, useMemo, useState } from "react"; import { IoLogoGithub } from "react-icons/io5"; -import type { AutofixMode, ReposPayload, TeamSettings, TriggerMode } from "@/lib/api"; +import type { ReposPayload, TeamSettings } from "@/lib/api"; import { AppShell, SettingsRow, SettingsSection } from "@/components/AppShell"; -import { - Select, - SelectContent, - SelectItem, - SelectTrigger, - SelectValue, -} from "@/components/ui/select"; import { Button } from "@/components/ui/button"; import { Skeleton } from "@/components/ui/skeleton"; import { Switch } from "@/components/ui/switch"; @@ -22,38 +15,10 @@ import { useSession } from "@/lib/session"; export const Route = createFileRoute("/review")({ component: ReviewPage }); -const TRIGGER_MODES: Array<{ value: TriggerMode; label: string; description: string }> = [ - { - value: "every_push", - label: "Every Push", - description: "Review on every push to the PR", - }, - { - value: "once_per_pr", - label: "Once Per PR", - description: "Review once when the PR is opened, skip subsequent pushes", - }, - { - value: "manual", - label: "Manual Only", - description: "Only review when '@open-swe review' is commented", - }, -]; - -const AUTOFIX_MODES: Array<{ value: AutofixMode; label: string }> = [ - { value: "off", label: "Off" }, - { value: "low", label: "Low" }, - { value: "medium", label: "Medium" }, - { value: "high", label: "High" }, -]; - const DEFAULT_SETTINGS: TeamSettings = { - trigger_mode: "every_push", review_draft_prs: false, pr_summaries: true, review_trace_links: true, - autofix_mode: "off", - autofix_severity_threshold: "medium", org_guidelines: null, default_agent_model: null, default_agent_reasoning_effort: null, @@ -111,10 +76,6 @@ function ReviewPage() { if (canEdit) save.mutate(next); }; - const triggerDescription = - TRIGGER_MODES.find((m) => m.value === current.trigger_mode)?.description ?? - "Open SWE Review will automatically review every push to a PR"; - const trimmedGuidelines = guidelinesDraft.trim(); const savedGuidelines = current.org_guidelines ?? ""; const guidelinesDirty = trimmedGuidelines !== savedGuidelines.trim(); @@ -178,29 +139,6 @@ function ReviewPage() {
- persist({ trigger_mode: v as TriggerMode })} - disabled - > - - - - - {TRIGGER_MODES.map((m) => ( - - {m.label} - - ))} - - - } - /> } /> - persist({ autofix_mode: v as AutofixMode })} - disabled - > - - - - - {AUTOFIX_MODES.map((m) => ( - - {m.label} - - ))} - - - } - /> - - persist({ autofix_severity_threshold: v as AutofixMode }) - } - disabled - > - - - - - {AUTOFIX_MODES.map((m) => ( - - {m.label} - - ))} - - - } - />