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} - - ))} - - - } - />