From d57324683a45eb47007782c2d5925673ef294d4e Mon Sep 17 00:00:00 2001 From: captainsafia Date: Thu, 4 Jun 2026 16:30:56 +0000 Subject: [PATCH 1/5] fix: allowlist slack feedback bot for issue triage Co-Authored-By: Oz --- core/routing.py | 24 +++++++++++++++++++++--- tests/test_routing.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/core/routing.py b/core/routing.py index d549cb3..55e79e4 100644 --- a/core/routing.py +++ b/core/routing.py @@ -46,7 +46,9 @@ 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 + they come from the allowlisted internal Slack feedback bot, whose + reports still need labels for external contributors. - ``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 @@ -99,6 +101,11 @@ READY_TO_SPEC_LABEL = "ready-to-spec" READY_TO_IMPLEMENT_LABEL = "ready-to-implement" AUTO_IMPLEMENT_LABEL = "auto-implement" +ISSUE_TRIAGE_BOT_AUTHOR_ALLOWLIST = frozenset( + { + "warp-dev-github-integration[bot]", + } +) OZ_AGENT_MENTION = "@oz-agent" OZ_REVIEW_COMMAND = "/oz-review" @@ -170,6 +177,14 @@ def _is_bot(actor: Any) -> bool: return bool(login) and login.endswith("[bot]") +def _is_issue_triage_allowlisted_bot(actor: Any) -> bool: + """Return True when *actor* is an automation account allowed to trigger issue triage.""" + return ( + _is_bot(actor) + and _login(actor).lower() in ISSUE_TRIAGE_BOT_AUTHOR_ALLOWLIST + ) + + def _route_issue_comment(payload: dict[str, Any]) -> RouteDecision: action = str(payload.get("action") or "").strip() if action != "created": @@ -266,7 +281,9 @@ def _route_issues(payload: dict[str, Any]) -> RouteDecision: ``ready-to-implement``, etc.) — for example because they were 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. + any state changes that landed while the issue was closed. The + internal Slack feedback bot is allowlisted through the bot-author + drop because its 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 +318,8 @@ 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): return RouteDecision(None, "issue authored by automation user") return RouteDecision( WORKFLOW_TRIAGE_NEW_ISSUES, "issues.opened triggers triage" diff --git a/tests/test_routing.py b/tests/test_routing.py index 2951194..4beb3f4 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -108,6 +108,38 @@ def test_issues_opened_for_bot_author_is_dropped(self) -> None: ) self.assertIsNone(decision.workflow) + def test_issues_opened_for_slack_feedback_bot_routes_to_triage(self) -> None: + decision = route_event( + "issues", + { + "action": "opened", + "issue": _issue( + user={ + "login": "warp-dev-github-integration[bot]", + "type": "Bot", + } + ), + }, + ) + self.assertEqual(decision.workflow, WORKFLOW_TRIAGE_NEW_ISSUES) + + def test_issues_opened_for_slack_feedback_bot_login_is_case_insensitive( + self, + ) -> None: + decision = route_event( + "issues", + { + "action": "opened", + "issue": _issue( + user={ + "login": "Warp-Dev-GitHub-Integration[Bot]", + "type": "Bot", + } + ), + }, + ) + self.assertEqual(decision.workflow, WORKFLOW_TRIAGE_NEW_ISSUES) + def test_issues_opened_with_auto_implement_from_bot_routes_to_create_implementation( self, ) -> None: From 966c057306b8b5cc53117c5381d7141175b59e9f Mon Sep 17 00:00:00 2001 From: captainsafia Date: Fri, 5 Jun 2026 17:01:11 +0000 Subject: [PATCH 2/5] fix: configure triage bot author allowlist Co-Authored-By: Oz --- .github/oz/config.yml | 3 + api/webhook.py | 103 +++++++++++++++++++++++++++++- core/routing.py | 44 ++++++++----- docs/onboarding.md | 6 ++ oz/workflow_config.py | 111 ++++++++++++++++++++++++++------- tests/test_routing.py | 27 +++++++- tests/test_webhook_dispatch.py | 80 ++++++++++++++++++++++++ tests/test_workflow_config.py | 72 +++++++++++++++++++++ 8 files changed, 405 insertions(+), 41 deletions(-) create mode 100644 tests/test_workflow_config.py 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..6800c61 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, @@ -59,6 +59,45 @@ _DELIVERY_HEADER = "x-github-delivery" +def _normalize_login_allowlist(values: Iterable[str] | None) -> frozenset[str]: + return frozenset( + value.strip().removeprefix("@").lower() + for value in values or [] + if isinstance(value, str) and value.strip() + ) + + +def _actor_login(actor: Any) -> str: + if isinstance(actor, dict): + login = actor.get("login") + else: + login = getattr(actor, "login", None) + return str(login or "").strip() + + +def _actor_is_bot(actor: Any) -> bool: + user_type = "" + if isinstance(actor, dict): + user_type = str(actor.get("type") or "").strip().lower() + else: + user_type = str(getattr(actor, "type", "") or "").strip().lower() + if user_type == "bot": + return True + login = _actor_login(actor).lower() + return bool(login) and login.endswith("[bot]") + + +def _needs_triage_bot_author_allowlist(event: str, payload: Mapping[str, Any]) -> bool: + 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 _actor_is_bot(issue.get("user")) + + @dataclass(frozen=True) class WebhookResponse: """Structured response surfaced by :func:`process_webhook_request`.""" @@ -130,6 +169,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 +210,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 = _normalize_login_allowlist( + triage_bot_author_allowlist + ) + if ( + triage_bot_author_allowlist_loader is not None + and _needs_triage_bot_author_allowlist(event, payload) + ): + try: + route_triage_bot_author_allowlist = _normalize_login_allowlist( + 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 +393,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) @@ -355,9 +427,15 @@ def _build_runtime_wiring(*, body: bytes) -> dict[str, Any]: from api.cron import build_state_store from core.builders import build_builder_registry from core.github_app import fetch_installation_token + from oz.triage import decode_repo_text_file from oz.oz_client import ( # type: ignore[import-not-found] build_agent_config, ) + from oz.workflow_config import ( # type: ignore[import-not-found] + CONFIG_RELATIVE_PATH, + load_triage_workflow_config, + load_triage_workflow_config_from_text, + ) from workflows.announce_ready_issue import ( # type: ignore[import-not-found] apply_announce_ready_issue_sync, ) @@ -490,6 +568,26 @@ 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 "/" in full_name: + repo_handle = _client_for_payload().get_repo(full_name) + config_text = decode_repo_text_file( + 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 + workflow_root = _Path(__file__).resolve().parents[1] + return load_triage_workflow_config( + workflow_root + ).bot_author_allowlist + return { "builder_registry": builder_registry, "runner": runner, @@ -497,6 +595,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 55e79e4..7c31834 100644 --- a/core/routing.py +++ b/core/routing.py @@ -47,8 +47,7 @@ ``triage-new-issues`` regardless of existing lifecycle labels (``ready-to-spec`` / ``ready-to-implement`` issues still get a triage pass). Automation-authored opened issues are skipped unless - they come from the allowlisted internal Slack feedback bot, whose - reports still need labels for external contributors. + 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 @@ -101,11 +100,6 @@ READY_TO_SPEC_LABEL = "ready-to-spec" READY_TO_IMPLEMENT_LABEL = "ready-to-implement" AUTO_IMPLEMENT_LABEL = "auto-implement" -ISSUE_TRIAGE_BOT_AUTHOR_ALLOWLIST = frozenset( - { - "warp-dev-github-integration[bot]", - } -) OZ_AGENT_MENTION = "@oz-agent" OZ_REVIEW_COMMAND = "/oz-review" @@ -177,11 +171,14 @@ def _is_bot(actor: Any) -> bool: return bool(login) and login.endswith("[bot]") -def _is_issue_triage_allowlisted_bot(actor: Any) -> bool: +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 ISSUE_TRIAGE_BOT_AUTHOR_ALLOWLIST + and _login(actor).lower() in bot_author_allowlist ) @@ -267,7 +264,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: @@ -281,9 +282,9 @@ def _route_issues(payload: dict[str, Any]) -> RouteDecision: ``ready-to-implement``, etc.) — for example because they were 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. The - internal Slack feedback bot is allowlisted through the bot-author - drop because its generated issues still need triage labels. + any state changes that landed while the issue was closed. + Configured bot authors are allowlisted through the bot-author + drop 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 @@ -319,7 +320,10 @@ def _route_issues(payload: dict[str, Any]) -> RouteDecision: "auto-implement label on newly opened issue", ) issue_author = issue.get("user") - if _is_bot(issue_author) and not _is_issue_triage_allowlisted_bot(issue_author): + 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" @@ -481,7 +485,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 @@ -490,6 +499,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") 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..315e2d9 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,41 @@ 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, + ) diff --git a/tests/test_routing.py b/tests/test_routing.py index 4beb3f4..0941f44 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -108,7 +108,24 @@ def test_issues_opened_for_bot_author_is_dropped(self) -> None: ) self.assertIsNone(decision.workflow) - def test_issues_opened_for_slack_feedback_bot_routes_to_triage(self) -> None: + 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", { @@ -120,10 +137,13 @@ def test_issues_opened_for_slack_feedback_bot_routes_to_triage(self) -> None: } ), }, + triage_bot_author_allowlist=frozenset( + {"warp-dev-github-integration[bot]"} + ), ) self.assertEqual(decision.workflow, WORKFLOW_TRIAGE_NEW_ISSUES) - def test_issues_opened_for_slack_feedback_bot_login_is_case_insensitive( + def test_issues_opened_for_configured_bot_author_login_is_case_insensitive( self, ) -> None: decision = route_event( @@ -137,6 +157,9 @@ def test_issues_opened_for_slack_feedback_bot_login_is_case_insensitive( } ), }, + triage_bot_author_allowlist=frozenset( + {"warp-dev-github-integration[bot]"} + ), ) self.assertEqual(decision.workflow, WORKFLOW_TRIAGE_NEW_ISSUES) 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..46a7356 --- /dev/null +++ b/tests/test_workflow_config.py @@ -0,0 +1,72 @@ +from __future__ import annotations + +import unittest + +from . import conftest # noqa: F401 + +try: + import yaml # noqa: F401 +except ModuleNotFoundError: + _HAS_YAML = False +else: + _HAS_YAML = True + + +@unittest.skipUnless(_HAS_YAML, "PyYAML is not installed") +class TriageWorkflowConfigTest(unittest.TestCase): + def _load_config(self, text: str): + from oz.workflow_config import load_triage_workflow_config_from_text + + 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]", + "", + ] + ), + ) + + +if __name__ == "__main__": + unittest.main() From 566710f56da73bfbf1888d997fc33da4104641ed Mon Sep 17 00:00:00 2001 From: lucieleblanc Date: Fri, 5 Jun 2026 17:39:13 +0000 Subject: [PATCH 3/5] refactor: consolidate bot-detection, extract config loader, remove PyYAML skip - Move duplicated bot-detection logic (_actor_login, _actor_is_bot, _needs_triage_bot_author_allowlist) from api/webhook.py into core/routing.py as the shared needs_triage_bot_author_allowlist helper, eliminating drift risk between pre-routing and routing. - Extract load_triage_bot_author_allowlist(repo_handle, fallback_workspace) into oz/workflow_config.py so the webhook loader closure is a thin call-through. The helper is independently testable with an optional repo_text_fetcher parameter. - Add LoadTriageBotAuthorAllowlistTest covering: consuming repo config present (uses it), repo config missing (falls back to bundled config), and repo config malformed (raises RuntimeError). - Add test_rejects_malformed_yaml to TriageWorkflowConfigTest for explicit malformed-config coverage. - Remove the PyYAML try/except skip guard from test_workflow_config.py since PyYAML is a runtime dependency in requirements.txt and should fail loudly when missing. Co-Authored-By: Oz --- api/webhook.py | 60 +++++++---------------------------- core/routing.py | 20 +++++++++++- oz/workflow_config.py | 36 +++++++++++++++++++++ tests/test_workflow_config.py | 60 +++++++++++++++++++++++++++++------ 4 files changed, 118 insertions(+), 58 deletions(-) diff --git a/api/webhook.py b/api/webhook.py index 6800c61..74c4539 100644 --- a/api/webhook.py +++ b/api/webhook.py @@ -41,6 +41,7 @@ RouteDecision, WORKFLOW_ANNOUNCE_READY_ISSUE, WORKFLOW_PLAN_APPROVED, + needs_triage_bot_author_allowlist, route_event, ) from core.signatures import ( @@ -67,36 +68,6 @@ def _normalize_login_allowlist(values: Iterable[str] | None) -> frozenset[str]: ) -def _actor_login(actor: Any) -> str: - if isinstance(actor, dict): - login = actor.get("login") - else: - login = getattr(actor, "login", None) - return str(login or "").strip() - - -def _actor_is_bot(actor: Any) -> bool: - user_type = "" - if isinstance(actor, dict): - user_type = str(actor.get("type") or "").strip().lower() - else: - user_type = str(getattr(actor, "type", "") or "").strip().lower() - if user_type == "bot": - return True - login = _actor_login(actor).lower() - return bool(login) and login.endswith("[bot]") - - -def _needs_triage_bot_author_allowlist(event: str, payload: Mapping[str, Any]) -> bool: - 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 _actor_is_bot(issue.get("user")) - @dataclass(frozen=True) class WebhookResponse: @@ -215,7 +186,7 @@ def process_webhook_request( ) if ( triage_bot_author_allowlist_loader is not None - and _needs_triage_bot_author_allowlist(event, payload) + and needs_triage_bot_author_allowlist(event, payload) ): try: route_triage_bot_author_allowlist = _normalize_login_allowlist( @@ -427,14 +398,11 @@ def _build_runtime_wiring(*, body: bytes) -> dict[str, Any]: from api.cron import build_state_store from core.builders import build_builder_registry from core.github_app import fetch_installation_token - from oz.triage import decode_repo_text_file from oz.oz_client import ( # type: ignore[import-not-found] build_agent_config, ) from oz.workflow_config import ( # type: ignore[import-not-found] - CONFIG_RELATIVE_PATH, - load_triage_workflow_config, - load_triage_workflow_config_from_text, + load_triage_bot_author_allowlist, ) from workflows.announce_ready_issue import ( # type: ignore[import-not-found] apply_announce_ready_issue_sync, @@ -572,21 +540,17 @@ def triage_bot_author_allowlist_loader(payload: Mapping[str, Any]) -> frozenset[ full_name = str( (payload.get("repository") or {}).get("full_name") or "" ) - if "/" in full_name: - repo_handle = _client_for_payload().get_repo(full_name) - config_text = decode_repo_text_file( - repo_handle, - str(CONFIG_RELATIVE_PATH), + if "/" not in full_name: + raise RuntimeError( + "webhook payload is missing repository.full_name; " + "cannot load triage bot author allowlist" ) - if config_text is not None: - return load_triage_workflow_config_from_text( - config_text, - config_path=CONFIG_RELATIVE_PATH, - ).bot_author_allowlist + repo_handle = _client_for_payload().get_repo(full_name) workflow_root = _Path(__file__).resolve().parents[1] - return load_triage_workflow_config( - workflow_root - ).bot_author_allowlist + return load_triage_bot_author_allowlist( + repo_handle, + fallback_workspace=workflow_root, + ) return { "builder_registry": builder_registry, diff --git a/core/routing.py b/core/routing.py index 7c31834..2734d5b 100644 --- a/core/routing.py +++ b/core/routing.py @@ -77,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 @@ -182,6 +182,23 @@ def _is_issue_triage_allowlisted_bot( ) +def needs_triage_bot_author_allowlist(event: str, payload: Mapping[str, Any]) -> bool: + """Return True when *event*/*payload* is a bot-authored ``issues.opened``. + + The webhook pre-routing path uses this to decide whether the + runtime config loader needs to be invoked before calling + :func:`route_event`. + """ + 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": @@ -534,5 +551,6 @@ def route_event( "WORKFLOW_TRIAGE_NEW_ISSUES", "WORKFLOW_VERIFY_PR_COMMENT", "has_oz_review_command", + "needs_triage_bot_author_allowlist", "route_event", ] diff --git a/oz/workflow_config.py b/oz/workflow_config.py index 315e2d9..baf2236 100644 --- a/oz/workflow_config.py +++ b/oz/workflow_config.py @@ -316,3 +316,39 @@ def load_triage_workflow_config_from_text( 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_workflow_config.py b/tests/test_workflow_config.py index 46a7356..31cc9db 100644 --- a/tests/test_workflow_config.py +++ b/tests/test_workflow_config.py @@ -1,22 +1,19 @@ from __future__ import annotations import unittest +from pathlib import Path +from unittest.mock import MagicMock from . import conftest # noqa: F401 -try: - import yaml # noqa: F401 -except ModuleNotFoundError: - _HAS_YAML = False -else: - _HAS_YAML = True +from oz.workflow_config import ( + load_triage_bot_author_allowlist, + load_triage_workflow_config_from_text, +) -@unittest.skipUnless(_HAS_YAML, "PyYAML is not installed") class TriageWorkflowConfigTest(unittest.TestCase): def _load_config(self, text: str): - from oz.workflow_config import load_triage_workflow_config_from_text - return load_triage_workflow_config_from_text(text) def test_defaults_bot_author_allowlist_to_empty(self) -> None: @@ -67,6 +64,51 @@ def test_rejects_non_list_bot_author_allowlist(self) -> None: ), ) + 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() From 79e67c04466720e3f22fd6770b16c5a9baae0579 Mon Sep 17 00:00:00 2001 From: lucieleblanc Date: Fri, 5 Jun 2026 14:08:26 -0400 Subject: [PATCH 4/5] clean up --- core/routing.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/core/routing.py b/core/routing.py index 2734d5b..5d0ec6d 100644 --- a/core/routing.py +++ b/core/routing.py @@ -183,12 +183,7 @@ def _is_issue_triage_allowlisted_bot( def needs_triage_bot_author_allowlist(event: str, payload: Mapping[str, Any]) -> bool: - """Return True when *event*/*payload* is a bot-authored ``issues.opened``. - - The webhook pre-routing path uses this to decide whether the - runtime config loader needs to be invoked before calling - :func:`route_event`. - """ + """Return True when *event*/*payload* is a bot-authored ``issues.opened``.""" if event != "issues": return False if str(payload.get("action") or "").strip() != "opened": @@ -300,8 +295,8 @@ def _route_issues( 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-author - drop because some generated issues still need triage labels. + 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 From f5482a46fdaa42430fdd702b16e406451f8733cd Mon Sep 17 00:00:00 2001 From: lucieleblanc Date: Fri, 5 Jun 2026 16:17:47 -0400 Subject: [PATCH 5/5] remove duplicate normalization --- api/webhook.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/api/webhook.py b/api/webhook.py index 74c4539..8f83e62 100644 --- a/api/webhook.py +++ b/api/webhook.py @@ -60,13 +60,6 @@ _DELIVERY_HEADER = "x-github-delivery" -def _normalize_login_allowlist(values: Iterable[str] | None) -> frozenset[str]: - return frozenset( - value.strip().removeprefix("@").lower() - for value in values or [] - if isinstance(value, str) and value.strip() - ) - @dataclass(frozen=True) @@ -181,15 +174,15 @@ def process_webhook_request( body={"error": "webhook payload must be a JSON object"}, ) - route_triage_bot_author_allowlist = _normalize_login_allowlist( - triage_bot_author_allowlist + 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 = _normalize_login_allowlist( + route_triage_bot_author_allowlist = frozenset( triage_bot_author_allowlist_loader(payload) ) except Exception as exc: