diff --git a/.github/oz/config.yml b/.github/oz/config.yml index 07e997a..cbf919a 100644 --- a/.github/oz/config.yml +++ b/.github/oz/config.yml @@ -1,3 +1,6 @@ version: 1 self_improvement: base_branch: auto +triage: + bot_author_allowlist: + - warp-dev-github-integration[bot] diff --git a/api/webhook.py b/api/webhook.py index 48318c2..8f83e62 100644 --- a/api/webhook.py +++ b/api/webhook.py @@ -28,7 +28,7 @@ import os from dataclasses import dataclass from http.server import BaseHTTPRequestHandler -from typing import Any, Callable, Mapping +from typing import Any, Callable, Iterable, Mapping from core.dispatch import ( DispatchRequest, @@ -41,6 +41,7 @@ RouteDecision, WORKFLOW_ANNOUNCE_READY_ISSUE, WORKFLOW_PLAN_APPROVED, + needs_triage_bot_author_allowlist, route_event, ) from core.signatures import ( @@ -59,6 +60,8 @@ _DELIVERY_HEADER = "x-github-delivery" + + @dataclass(frozen=True) class WebhookResponse: """Structured response surfaced by :func:`process_webhook_request`.""" @@ -130,6 +133,8 @@ def process_webhook_request( store: StateStore | None = None, sync_plan_approved: Callable[[Mapping[str, Any]], dict[str, Any] | None] | None = None, sync_announce_ready_issue: Callable[[Mapping[str, Any]], dict[str, Any]] | None = None, + triage_bot_author_allowlist: Iterable[str] | None = None, + triage_bot_author_allowlist_loader: Callable[[Mapping[str, Any]], Iterable[str]] | None = None, ) -> WebhookResponse: """Validate a webhook delivery and dispatch the cloud agent run. @@ -169,7 +174,35 @@ def process_webhook_request( body={"error": "webhook payload must be a JSON object"}, ) - decision: RouteDecision = route_event(event, payload) + route_triage_bot_author_allowlist: frozenset[str] = frozenset( + triage_bot_author_allowlist or () + ) + if ( + triage_bot_author_allowlist_loader is not None + and needs_triage_bot_author_allowlist(event, payload) + ): + try: + route_triage_bot_author_allowlist = frozenset( + triage_bot_author_allowlist_loader(payload) + ) + except Exception as exc: + logger.exception("Failed to load triage bot author allowlist") + return WebhookResponse( + status=500, + body={ + "event": event, + "workflow": None, + "reason": "failed to load triage bot author allowlist", + "delivery": delivery_id or "", + "error": f"route config failed: {exc}", + }, + ) + + decision: RouteDecision = route_event( + event, + payload, + triage_bot_author_allowlist=route_triage_bot_author_allowlist, + ) base_body: dict[str, Any] = { "event": event, "workflow": decision.workflow, @@ -324,6 +357,9 @@ def do_POST(self) -> None: # noqa: N802 - signature comes from BaseHTTPRequestH store=wiring["store"], sync_plan_approved=wiring["sync_plan_approved"], sync_announce_ready_issue=wiring["sync_announce_ready_issue"], + triage_bot_author_allowlist_loader=wiring[ + "triage_bot_author_allowlist_loader" + ], ) self._respond(response.status, response.body) @@ -358,6 +394,9 @@ def _build_runtime_wiring(*, body: bytes) -> dict[str, Any]: from oz.oz_client import ( # type: ignore[import-not-found] build_agent_config, ) + from oz.workflow_config import ( # type: ignore[import-not-found] + load_triage_bot_author_allowlist, + ) from workflows.announce_ready_issue import ( # type: ignore[import-not-found] apply_announce_ready_issue_sync, ) @@ -490,6 +529,22 @@ def sync_announce_ready_issue( repo_handle, payload=payload ) + def triage_bot_author_allowlist_loader(payload: Mapping[str, Any]) -> frozenset[str]: + full_name = str( + (payload.get("repository") or {}).get("full_name") or "" + ) + if "/" not in full_name: + raise RuntimeError( + "webhook payload is missing repository.full_name; " + "cannot load triage bot author allowlist" + ) + repo_handle = _client_for_payload().get_repo(full_name) + workflow_root = _Path(__file__).resolve().parents[1] + return load_triage_bot_author_allowlist( + repo_handle, + fallback_workspace=workflow_root, + ) + return { "builder_registry": builder_registry, "runner": runner, @@ -497,6 +552,7 @@ def sync_announce_ready_issue( "store": build_state_store(), "sync_plan_approved": sync_plan_approved, "sync_announce_ready_issue": sync_announce_ready_issue, + "triage_bot_author_allowlist_loader": triage_bot_author_allowlist_loader, } diff --git a/core/routing.py b/core/routing.py index d549cb3..5d0ec6d 100644 --- a/core/routing.py +++ b/core/routing.py @@ -46,7 +46,8 @@ the normal bot-author drop. Other opened issues route to ``triage-new-issues`` regardless of existing lifecycle labels (``ready-to-spec`` / ``ready-to-implement`` issues still get a - triage pass). + triage pass). Automation-authored opened issues are skipped unless + the author is present in the configured triage bot author allowlist. - ``assigned`` routes to ``create-spec-from-issue`` or ``create-implementation-from-issue`` when the assignee being added is ``oz-agent`` and the issue carries the matching @@ -76,7 +77,7 @@ import re from dataclasses import dataclass -from typing import Any +from typing import Any, Mapping # Workflow identifiers the dispatcher knows how to handle. These strings # are used as state-store keys and as ``RouteDecision.workflow`` values @@ -170,6 +171,29 @@ def _is_bot(actor: Any) -> bool: return bool(login) and login.endswith("[bot]") +def _is_issue_triage_allowlisted_bot( + actor: Any, + bot_author_allowlist: frozenset[str], +) -> bool: + """Return True when *actor* is an automation account allowed to trigger issue triage.""" + return ( + _is_bot(actor) + and _login(actor).lower() in bot_author_allowlist + ) + + +def needs_triage_bot_author_allowlist(event: str, payload: Mapping[str, Any]) -> bool: + """Return True when *event*/*payload* is a bot-authored ``issues.opened``.""" + if event != "issues": + return False + if str(payload.get("action") or "").strip() != "opened": + return False + issue = payload.get("issue") or {} + if not isinstance(issue, dict) or issue.get("pull_request"): + return False + return _is_bot(issue.get("user")) + + def _route_issue_comment(payload: dict[str, Any]) -> RouteDecision: action = str(payload.get("action") or "").strip() if action != "created": @@ -252,7 +276,11 @@ def _route_plain_issue_comment( ) -def _route_issues(payload: dict[str, Any]) -> RouteDecision: +def _route_issues( + payload: dict[str, Any], + *, + bot_author_allowlist: frozenset[str] = frozenset(), +) -> RouteDecision: """Route an ``issues`` webhook event. Three actions are routed: @@ -267,6 +295,8 @@ def _route_issues(payload: dict[str, Any]) -> RouteDecision: imported from another repo or re-opened — still get a triage pass so the bot can post a fresh progress comment and pick up any state changes that landed while the issue was closed. + Configured bot authors are allowlisted through the bot check + because some generated issues still need triage labels. - ``assigned`` triggers ``create-spec-from-issue`` or ``create-implementation-from-issue`` when the assignee being added is ``oz-agent`` itself and the issue carries the @@ -301,7 +331,11 @@ def _route_issues(payload: dict[str, Any]) -> RouteDecision: WORKFLOW_CREATE_IMPLEMENTATION_FROM_ISSUE, "auto-implement label on newly opened issue", ) - if _is_bot(issue.get("user")): + issue_author = issue.get("user") + if _is_bot(issue_author) and not _is_issue_triage_allowlisted_bot( + issue_author, + bot_author_allowlist, + ): return RouteDecision(None, "issue authored by automation user") return RouteDecision( WORKFLOW_TRIAGE_NEW_ISSUES, "issues.opened triggers triage" @@ -463,7 +497,12 @@ def _route_pull_request_review(payload: dict[str, Any]) -> RouteDecision: } -def route_event(event: str, payload: dict[str, Any]) -> RouteDecision: +def route_event( + event: str, + payload: dict[str, Any], + *, + triage_bot_author_allowlist: frozenset[str] | None = None, +) -> RouteDecision: """Decide which workflow (if any) handles *event* + *payload*. The router never raises on unknown events or malformed payloads; it @@ -472,6 +511,11 @@ def route_event(event: str, payload: dict[str, Any]) -> RouteDecision: """ if not isinstance(payload, dict): return RouteDecision(None, "non-object webhook payload") + if event == "issues": + return _route_issues( + payload, + bot_author_allowlist=triage_bot_author_allowlist or frozenset(), + ) handler = _EVENT_HANDLERS.get(event) if handler is None: return RouteDecision(None, f"event {event!r} not handled") @@ -502,5 +546,6 @@ def route_event(event: str, payload: dict[str, Any]) -> RouteDecision: "WORKFLOW_TRIAGE_NEW_ISSUES", "WORKFLOW_VERIFY_PR_COMMENT", "has_oz_review_command", + "needs_triage_bot_author_allowlist", "route_event", ] diff --git a/docs/onboarding.md b/docs/onboarding.md index 0d1c86d..3735e6a 100644 --- a/docs/onboarding.md +++ b/docs/onboarding.md @@ -65,8 +65,14 @@ self_improvement: triage: prior_triage_labels: - triaged + bot_author_allowlist: + - trusted-intake[bot] ``` +`triage.bot_author_allowlist` lets a repository opt specific automation +accounts back into `issues.opened` triage. Other bot-authored issues are +skipped by default. + ## 4. Bootstrap triage configuration (optional) Run the [`bootstrap-issue-config`](../.agents/skills/bootstrap-issue-config/SKILL.md) skill against your repository to seed `.github/issue-triage/config.json` and `.github/STAKEHOLDERS` with sensible defaults derived from your existing labels and CODEOWNERS. diff --git a/oz/workflow_config.py b/oz/workflow_config.py index 685e351..baf2236 100644 --- a/oz/workflow_config.py +++ b/oz/workflow_config.py @@ -19,12 +19,16 @@ class SelfImprovementConfig: reviewers: list[str] | None base_branch: str | None + + @dataclass(frozen=True) class TriageWorkflowConfig: prior_triage_labels: frozenset[str] + bot_author_allowlist: frozenset[str] _DEFAULT_PRIOR_TRIAGE_LABELS: frozenset[str] = frozenset({"triaged"}) +_DEFAULT_BOT_AUTHOR_ALLOWLIST: frozenset[str] = frozenset() def resolve_repo_config_path(workspace_root: Path) -> Path | None: @@ -105,6 +109,45 @@ def _parse_label_list( return frozenset(labels) +def _parse_login_list( + raw_value: Any, + *, + config_path: Path, + source: str, +) -> frozenset[str]: + if not isinstance(raw_value, list): + raise _fail(config_path, f"{source} must be a list of GitHub logins.") + logins: set[str] = set() + for item in raw_value: + if not isinstance(item, str): + raise _fail(config_path, f"{source} entries must be strings.") + value = item.strip().removeprefix("@").lower() + if not value: + raise _fail(config_path, f"{source} entries must not be blank.") + if any(char.isspace() for char in value): + raise _fail(config_path, f"Invalid GitHub login {value!r} in {source}.") + logins.add(value) + return frozenset(logins) + + +def _parse_workflow_config_text(config_path: Path, text: str) -> dict[str, Any]: + try: + raw_data = yaml.safe_load(text) + except yaml.YAMLError as exc: + raise _fail(config_path, "Invalid YAML in .github/oz/config.yml.") from exc + + if raw_data is None: + raw_data = {} + if not isinstance(raw_data, dict): + raise _fail(config_path, "The config root must be a YAML mapping.") + + version = raw_data.get("version") + if version != 1: + raise _fail(config_path, "Unsupported config version; expected version: 1.") + + return raw_data + + def _load_raw_workflow_config( workspace_root: Path, *, @@ -120,22 +163,11 @@ def _load_raw_workflow_config( return CONFIG_RELATIVE_PATH, {"version": 1} try: - raw_data = yaml.safe_load(config_path.read_text(encoding="utf-8")) - except yaml.YAMLError as exc: - raise _fail(config_path, "Invalid YAML in .github/oz/config.yml.") from exc + text = config_path.read_text(encoding="utf-8") except OSError as exc: raise _fail(config_path, "Unable to read .github/oz/config.yml.") from exc - if raw_data is None: - raw_data = {} - if not isinstance(raw_data, dict): - raise _fail(config_path, "The config root must be a YAML mapping.") - - version = raw_data.get("version") - if version != 1: - raise _fail(config_path, "Unsupported config version; expected version: 1.") - - return config_path, raw_data + return config_path, _parse_workflow_config_text(config_path, text) def _parse_env_reviewers(config_path: Path) -> list[str] | None: @@ -216,13 +248,11 @@ def load_self_improvement_config(workspace_root: Path) -> SelfImprovementConfig: return SelfImprovementConfig(reviewers=reviewers, base_branch=base_branch) -def load_triage_workflow_config(workspace_root: Path) -> TriageWorkflowConfig: - """Load the optional triage workflow settings from `.github/oz/config.yml`.""" - config_path, raw_data = _load_raw_workflow_config( - workspace_root, - require_exists=False, - ) - +def _parse_triage_workflow_config( + *, + config_path: Path, + raw_data: dict[str, Any], +) -> TriageWorkflowConfig: triage = raw_data.get("triage") if triage is None: triage = {} @@ -232,7 +262,7 @@ def load_triage_workflow_config(workspace_root: Path) -> TriageWorkflowConfig: unknown_keys = sorted( key for key in triage.keys() - if key not in {"prior_triage_labels"} + if key not in {"prior_triage_labels", "bot_author_allowlist"} ) if unknown_keys: raise _fail( @@ -248,4 +278,77 @@ def load_triage_workflow_config(workspace_root: Path) -> TriageWorkflowConfig: source="triage.prior_triage_labels", ) - return TriageWorkflowConfig(prior_triage_labels=prior_triage_labels) + bot_author_allowlist = _DEFAULT_BOT_AUTHOR_ALLOWLIST + if "bot_author_allowlist" in triage: + bot_author_allowlist = _parse_login_list( + triage["bot_author_allowlist"], + config_path=config_path, + source="triage.bot_author_allowlist", + ) + + return TriageWorkflowConfig( + prior_triage_labels=prior_triage_labels, + bot_author_allowlist=bot_author_allowlist, + ) + + +def load_triage_workflow_config(workspace_root: Path) -> TriageWorkflowConfig: + """Load the optional triage workflow settings from `.github/oz/config.yml`.""" + config_path, raw_data = _load_raw_workflow_config( + workspace_root, + require_exists=False, + ) + return _parse_triage_workflow_config( + config_path=config_path, + raw_data=raw_data, + ) + + +def load_triage_workflow_config_from_text( + text: str, + *, + config_path: Path | str = CONFIG_RELATIVE_PATH, +) -> TriageWorkflowConfig: + """Load triage workflow settings from API-fetched `.github/oz/config.yml` text.""" + resolved_config_path = Path(config_path) + raw_data = _parse_workflow_config_text(resolved_config_path, text) + return _parse_triage_workflow_config( + config_path=resolved_config_path, + raw_data=raw_data, + ) + + +def load_triage_bot_author_allowlist( + repo_handle: Any, + fallback_workspace: Path, + *, + repo_text_fetcher: Any | None = None, +) -> frozenset[str]: + """Load ``triage.bot_author_allowlist`` for a webhook delivery. + + Tries the consuming repository's ``.github/oz/config.yml`` via the + GitHub API first. Falls back to the bundled config on disk at + *fallback_workspace* when the consuming repo does not have one. + Raises on malformed config so the webhook can surface the error + rather than silently falling back to an empty allowlist. + + *repo_text_fetcher* is an optional callable ``(repo_handle, path) -> str | None`` + used to read a text file from the repo. Defaults to + ``oz.triage.decode_repo_text_file`` when not provided. + """ + if repo_text_fetcher is None: + from .triage import decode_repo_text_file + repo_text_fetcher = decode_repo_text_file + + config_text = repo_text_fetcher( + repo_handle, + str(CONFIG_RELATIVE_PATH), + ) + if config_text is not None: + return load_triage_workflow_config_from_text( + config_text, + config_path=CONFIG_RELATIVE_PATH, + ).bot_author_allowlist + return load_triage_workflow_config( + fallback_workspace, + ).bot_author_allowlist diff --git a/tests/test_routing.py b/tests/test_routing.py index 2951194..0941f44 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -108,6 +108,61 @@ def test_issues_opened_for_bot_author_is_dropped(self) -> None: ) self.assertIsNone(decision.workflow) + def test_issues_opened_for_slack_feedback_bot_without_allowlist_is_dropped( + self, + ) -> None: + decision = route_event( + "issues", + { + "action": "opened", + "issue": _issue( + user={ + "login": "warp-dev-github-integration[bot]", + "type": "Bot", + } + ), + }, + ) + self.assertIsNone(decision.workflow) + + def test_issues_opened_for_configured_bot_author_routes_to_triage(self) -> None: + decision = route_event( + "issues", + { + "action": "opened", + "issue": _issue( + user={ + "login": "warp-dev-github-integration[bot]", + "type": "Bot", + } + ), + }, + triage_bot_author_allowlist=frozenset( + {"warp-dev-github-integration[bot]"} + ), + ) + self.assertEqual(decision.workflow, WORKFLOW_TRIAGE_NEW_ISSUES) + + def test_issues_opened_for_configured_bot_author_login_is_case_insensitive( + self, + ) -> None: + decision = route_event( + "issues", + { + "action": "opened", + "issue": _issue( + user={ + "login": "Warp-Dev-GitHub-Integration[Bot]", + "type": "Bot", + } + ), + }, + triage_bot_author_allowlist=frozenset( + {"warp-dev-github-integration[bot]"} + ), + ) + self.assertEqual(decision.workflow, WORKFLOW_TRIAGE_NEW_ISSUES) + def test_issues_opened_with_auto_implement_from_bot_routes_to_create_implementation( self, ) -> None: diff --git a/tests/test_webhook_dispatch.py b/tests/test_webhook_dispatch.py index b350ed2..d5d9f28 100644 --- a/tests/test_webhook_dispatch.py +++ b/tests/test_webhook_dispatch.py @@ -25,6 +25,7 @@ WORKFLOW_ANNOUNCE_READY_ISSUE, WORKFLOW_PLAN_APPROVED, WORKFLOW_REVIEW_PR, + WORKFLOW_TRIAGE_NEW_ISSUES, ) from core.signatures import expected_signature from core.state import InMemoryStateStore @@ -234,6 +235,85 @@ def exploding_builder(payload: Mapping[str, Any]) -> DispatchRequest: +class IssueTriageRouteConfigTest(unittest.TestCase): + def _payload(self, *, login: str = "warp-dev-github-integration[bot]") -> dict[str, Any]: + return { + "action": "opened", + "repository": {"full_name": "acme/widgets"}, + "installation": {"id": 1234}, + "issue": { + "number": 42, + "labels": [], + "assignees": [], + "user": {"login": login, "type": "Bot"}, + }, + } + + def test_configured_bot_author_allowlist_routes_to_triage(self) -> None: + body, signature = _signed_envelope(self._payload()) + store = InMemoryStateStore() + + def builder(payload: Mapping[str, Any]) -> DispatchRequest: + return DispatchRequest( + workflow=WORKFLOW_TRIAGE_NEW_ISSUES, + repo="acme/widgets", + installation_id=1234, + config_name=WORKFLOW_TRIAGE_NEW_ISSUES, + title="Triage issue #42", + skill_name="triage-issue", + prompt="prompt body", + payload_subset={"issue_number": 42}, + ) + + loader = MagicMock(return_value=["warp-dev-github-integration[bot]"]) + runner = MagicMock(return_value=SimpleNamespace(run_id="oz-run-triage")) + + response = process_webhook_request( + body=body, + signature_header=signature, + event_header="issues", + delivery_id="delivery-triage-bot", + secret=_SECRET, + builder_registry={WORKFLOW_TRIAGE_NEW_ISSUES: builder}, + runner=runner, + config_factory=lambda name, role: {"environment_id": "env", "name": name}, + store=store, + triage_bot_author_allowlist_loader=loader, + ) + + self.assertEqual(response.status, 202) + self.assertEqual(response.body["workflow"], WORKFLOW_TRIAGE_NEW_ISSUES) + self.assertTrue(response.body["dispatched"]) + self.assertEqual(response.body["run_id"], "oz-run-triage") + loader.assert_called_once() + runner.assert_called_once() + + def test_unconfigured_bot_author_is_skipped_before_dispatch(self) -> None: + body, signature = _signed_envelope(self._payload(login="dependabot[bot]")) + runner = MagicMock(side_effect=AssertionError("should not dispatch")) + loader = MagicMock(return_value=[]) + + response = process_webhook_request( + body=body, + signature_header=signature, + event_header="issues", + delivery_id="delivery-triage-dependabot", + secret=_SECRET, + builder_registry={}, + runner=runner, + config_factory=lambda name, role: {}, + store=InMemoryStateStore(), + triage_bot_author_allowlist_loader=loader, + ) + + self.assertEqual(response.status, 202) + self.assertIsNone(response.body["workflow"]) + self.assertEqual(response.body["reason"], "issue authored by automation user") + loader.assert_called_once() + runner.assert_not_called() + + + class SynchronousPlanApprovedPathTest(unittest.TestCase): def _payload(self) -> dict[str, Any]: return { diff --git a/tests/test_workflow_config.py b/tests/test_workflow_config.py new file mode 100644 index 0000000..31cc9db --- /dev/null +++ b/tests/test_workflow_config.py @@ -0,0 +1,114 @@ +from __future__ import annotations + +import unittest +from pathlib import Path +from unittest.mock import MagicMock + +from . import conftest # noqa: F401 + +from oz.workflow_config import ( + load_triage_bot_author_allowlist, + load_triage_workflow_config_from_text, +) + + +class TriageWorkflowConfigTest(unittest.TestCase): + def _load_config(self, text: str): + return load_triage_workflow_config_from_text(text) + + def test_defaults_bot_author_allowlist_to_empty(self) -> None: + config = self._load_config( + "version: 1\n", + ) + self.assertEqual(config.prior_triage_labels, frozenset({"triaged"})) + self.assertEqual(config.bot_author_allowlist, frozenset()) + + def test_parses_bot_author_allowlist(self) -> None: + config = self._load_config( + "\n".join( + [ + "version: 1", + "triage:", + " prior_triage_labels:", + " - triaged", + " bot_author_allowlist:", + " - warp-dev-github-integration[bot]", + " - '@Trusted-Intake[Bot]'", + "", + ] + ), + ) + self.assertEqual( + config.bot_author_allowlist, + frozenset( + { + "warp-dev-github-integration[bot]", + "trusted-intake[bot]", + } + ), + ) + + def test_rejects_non_list_bot_author_allowlist(self) -> None: + with self.assertRaisesRegex( + RuntimeError, + "triage.bot_author_allowlist must be a list", + ): + self._load_config( + "\n".join( + [ + "version: 1", + "triage:", + " bot_author_allowlist: warp-dev-github-integration[bot]", + "", + ] + ), + ) + + def test_rejects_malformed_yaml(self) -> None: + with self.assertRaisesRegex(RuntimeError, "Invalid YAML"): + self._load_config("version: 1\ntriage: [") + + +class LoadTriageBotAuthorAllowlistTest(unittest.TestCase): + def _call(self, *, config_text: str | None, fallback_workspace: Path) -> frozenset[str]: + repo = MagicMock() + repo.full_name = "acme/widgets" + fetcher = MagicMock(return_value=config_text) + return load_triage_bot_author_allowlist( + repo, + fallback_workspace=fallback_workspace, + repo_text_fetcher=fetcher, + ) + + def test_uses_consuming_repo_config_when_present(self) -> None: + config_text = "\n".join([ + "version: 1", + "triage:", + " bot_author_allowlist:", + " - custom-bot[bot]", + "", + ]) + result = self._call( + config_text=config_text, + fallback_workspace=Path("/nonexistent"), + ) + self.assertEqual(result, frozenset({"custom-bot[bot]"})) + + def test_falls_back_to_bundled_config_when_repo_config_missing(self) -> None: + repo_root = Path(__file__).resolve().parent.parent + result = self._call( + config_text=None, + fallback_workspace=repo_root, + ) + self.assertIn("warp-dev-github-integration[bot]", result) + + def test_raises_on_malformed_repo_config(self) -> None: + with self.assertRaises(RuntimeError): + self._call( + config_text="version: 1\ntriage: [", + fallback_workspace=Path("/nonexistent"), + ) + + +if __name__ == "__main__": + unittest.main()