diff --git a/python/robomp/.env.example b/python/robomp/.env.example index 2cf2ff218..7318f57b9 100644 --- a/python/robomp/.env.example +++ b/python/robomp/.env.example @@ -28,9 +28,10 @@ # configured in the GitHub webhook UI / settings. GITHUB_WEBHOOK_SECRET= -# Login name of the bot account whose PAT is in GITHUB_TOKEN (or whichever -# account the gh-proxy authenticates as). Used to skip webhook events authored -# by the bot itself. +# GitHub login handle for the bot account whose PAT is in GITHUB_TOKEN (or +# whichever account the gh-proxy authenticates as). Prefer the lowercase bare +# mention handle (`roboomp`, not `@roboomp` or `roboomp[bot]`); startup +# normalizes common forms, including uppercase and surrounding whitespace. ROBOMP_BOT_LOGIN= # Commit identity for branches the bot pushes. The email is what reviewers and @@ -44,6 +45,11 @@ ROBOMP_GIT_AUTHOR_EMAIL= # Comma-separated owner/repo entries the bot is allowed to act on. ROBOMP_REPO_ALLOWLIST= +# Optional comma-separated GitHub logins whose @ROBOMP_BOT_LOGIN comments may +# authorize implementation work in addition to repo OWNER. `@` and case do not +# matter; use the webhook actor login, not a display name. +ROBOMP_MAINTAINER_LOGINS= + # ============================================================================= # ### gh-proxy mode (RECOMMENDED, default in docker compose) ### diff --git a/python/robomp/AGENTS.md b/python/robomp/AGENTS.md index 893516792..cdae5be51 100644 --- a/python/robomp/AGENTS.md +++ b/python/robomp/AGENTS.md @@ -109,7 +109,7 @@ Lint + format: TypeScript via Biome (config in `biome.json`), Python via Ruff (c - **Package manager**: `pip` only. No poetry / uv / pdm files; don't introduce one. - **Task runner**: `bun` (root `package.json` `scripts`). Always reach for an existing `bun run` recipe before invoking `docker compose` or `pytest` directly. - **Container runtime**: Docker Compose v2. The image embeds Bun 1.3.14 + a rustup launcher and exposes `omp` via a `/usr/local/bin/omp` shim; `ROBOMP_OMP_COMMAND=omp` should not need changing. -- **Required env** (set in `.env`, see `.env.example`): `GITHUB_WEBHOOK_SECRET`, `ROBOMP_BOT_LOGIN`, `ROBOMP_GIT_AUTHOR_NAME`, `ROBOMP_GIT_AUTHOR_EMAIL`, `ROBOMP_REPO_ALLOWLIST`, plus model knobs (`ROBOMP_MODEL`, `ROBOMP_THINKING`, optional `ROBOMP_PROVIDER`) and rate-limit / concurrency / timeout overrides. **GitHub auth is mode-exclusive**: either set `ROBOMP_GH_PROXY_URL` + `ROBOMP_GH_PROXY_HMAC_KEY` (gh-proxy mode; PAT lives only in the sidecar container — the bundled compose default), or set `GITHUB_TOKEN` directly (single-process PAT mode). `Settings._validate_proxy_or_pat` rejects a `.env` that sets both. +- **Required env** (set in `.env`, see `.env.example`): `GITHUB_WEBHOOK_SECRET`, `ROBOMP_BOT_LOGIN`, `ROBOMP_GIT_AUTHOR_NAME`, `ROBOMP_GIT_AUTHOR_EMAIL`, `ROBOMP_REPO_ALLOWLIST`, plus model knobs (`ROBOMP_MODEL`, `ROBOMP_THINKING`, optional `ROBOMP_PROVIDER`) and rate-limit / concurrency / timeout overrides. Set `ROBOMP_BOT_LOGIN` to the lowercase mention handle (`roboomp` in production, no leading `@` or `[bot]`; config normalizes common variants). `ROBOMP_MAINTAINER_LOGINS` is optional comma-separated logins (`@` optional, case-insensitive) for non-owner implementation authorizers. **GitHub auth is mode-exclusive**: either set `ROBOMP_GH_PROXY_URL` + `ROBOMP_GH_PROXY_HMAC_KEY` (gh-proxy mode; PAT lives only in the sidecar container — the bundled compose default), or set `GITHUB_TOKEN` directly (single-process PAT mode). `Settings._validate_proxy_or_pat` rejects a `.env` that sets both. - **PI_ROOT resolution**: roboomp lives inside the oh-my-pi monorepo at `python/robomp/`. `bun run pi:image` builds the parent monorepo (`../..`) as its docker build context to produce `oh-my-pi/pi:dev`; `docker-compose.yml` extends that image via `PI_BASE` and mounts the same parent path read-only at `/work/pi` for the orchestrator to see live source. Override `PI_ROOT` only when pointing the build/mount at a different oh-my-pi checkout. Inside the container the path is always `/work/pi`. Build invalidation stays bounded: Python-only edits in roboomp never trigger a natives recompile. - **Forbidden**: no docker-in-docker, no extra service containers, no new background workers outside `WorkerPool`. The container itself is the isolation boundary; per-issue isolation is the git worktree. diff --git a/python/robomp/README.md b/python/robomp/README.md index 4628ccc6b..3307e4ba8 100644 --- a/python/robomp/README.md +++ b/python/robomp/README.md @@ -182,7 +182,7 @@ The integration test spawns a real `omp --mode rpc` against an |---|---| | `401 invalid signature` | `GITHUB_WEBHOOK_SECRET` mismatch with the repo webhook config. | | Container exits with `PI_ROOT … missing` | `/work/pi` mount empty inside the container; on the host either run `docker compose` from `python/robomp/` so `PI_ROOT` defaults to `../..`, or export `PI_ROOT` to a valid oh-my-pi checkout. | -| `git push: Authentication required` | Bot PAT lacks push, or `ROBOMP_BOT_LOGIN` ≠ PAT's account. | +| `git push: Authentication required` | Bot PAT lacks push, or `ROBOMP_BOT_LOGIN` does not identify the PAT account's mention handle (production: `roboomp`, no `@`/`[bot]`). | | `refusing to push: commit author identity mismatch` | Some commit not authored as `ROBOMP_GIT_AUTHOR_*`. The error lists the offending shas; `git commit --amend --reset-author --no-edit`. | | `refusing to push: working tree is dirty` | Uncommitted agent edits. Or just call `gh_open_pr`, which auto-commits `bun run fix` output. | | `bun check failed before PR creation` | Fix the reported failure and retry `gh_open_pr`. | diff --git a/python/robomp/src/config.py b/python/robomp/src/config.py index 97c14bf8b..48f4fdc16 100644 --- a/python/robomp/src/config.py +++ b/python/robomp/src/config.py @@ -129,7 +129,7 @@ class Settings(BaseSettings): rate_limit_default: int = Field(3, alias="ROBOMP_RATE_LIMIT_DEFAULT") rate_limit_contributor: int = Field(10, alias="ROBOMP_RATE_LIMIT_CONTRIBUTOR") rate_limit_unlimited_raw: str = Field("", alias="ROBOMP_RATE_LIMIT_UNLIMITED") - # Logins (comma-separated, `@` prefix optional) whose `@bot_login` + # Logins (comma-separated, `@` prefix optional, case-insensitive) whose `@bot_login` # mentions are treated as authoritative directives. These accounts also # bypass rate limiting regardless of `author_association`. maintainer_logins_raw: str = Field("", alias="ROBOMP_MAINTAINER_LOGINS") @@ -163,7 +163,9 @@ class Settings(BaseSettings): @field_validator("bot_login", mode="after") @classmethod def _require_bot_login(cls, value: str) -> str: - cleaned = value.strip().removeprefix("@") + cleaned = value.strip().removeprefix("@").lower() + if cleaned.endswith("[bot]"): + cleaned = cleaned[:-5] if not cleaned: raise ValueError("ROBOMP_BOT_LOGIN must be a non-empty GitHub login") return cleaned diff --git a/python/robomp/src/github_events.py b/python/robomp/src/github_events.py index a4fa57c08..d37925743 100644 --- a/python/robomp/src/github_events.py +++ b/python/robomp/src/github_events.py @@ -56,10 +56,43 @@ def _repo_full_name(payload: Mapping[str, Any]) -> str | None: return None -def _login_matches_repo_owner(login: str | None, repo: str | None) -> bool: - """Return whether `login` is the personal-account owner in `owner/repo`.""" +def _normalize_bot_login(login: str | None) -> str: + if not isinstance(login, str): + return "" + cleaned = login.strip().removeprefix("@") + if cleaned.lower().endswith("[bot]"): + cleaned = cleaned[:-5] + return cleaned.lower() + + +def _login_matches_bot(login: str | None, bot_login: str) -> bool: + normalized_login = _normalize_bot_login(login) + return bool(normalized_login) and normalized_login == _normalize_bot_login(bot_login) + + +def _login_matches_personal_repo_owner( + login: str | None, + repository: Mapping[str, Any] | None, + repo: str | None, +) -> bool: + """Return whether `login` owns this personal-account repository.""" if not isinstance(login, str) or not login: return False + owner_login: str | None = None + owner_type: str | None = None + if isinstance(repository, Mapping): + owner = repository.get("owner") + if isinstance(owner, Mapping): + raw_login = owner.get("login") + if isinstance(raw_login, str) and raw_login: + owner_login = raw_login + raw_type = owner.get("type") + if isinstance(raw_type, str) and raw_type: + owner_type = raw_type + if owner_type is not None and owner_type.lower() == "organization": + return False + if owner_login: + return login.lower() == owner_login.lower() if not isinstance(repo, str): return False owner, sep, _name = repo.partition("/") @@ -68,6 +101,19 @@ def _login_matches_repo_owner(login: str | None, repo: str | None) -> bool: return login.lower() == owner.lower() +def _effective_association( + login: str | None, + association: str | None, + repository: Mapping[str, Any] | None, + repo: str | None, +) -> str | None: + if association: + return association + if _login_matches_personal_repo_owner(login, repository, repo): + return "OWNER" + return association + + PrIssueResolver = Callable[[str, int], str | None] | None @@ -77,9 +123,9 @@ def _is_bot_account(user: Mapping[str, Any] | None, bot_login: str) -> bool: login = str(user.get("login") or "") if not login: return False - if login == bot_login: + if _login_matches_bot(login, bot_login): return True - if login.endswith("[bot]"): + if login.lower().endswith("[bot]"): return True if str(user.get("type") or "") == "Bot": return True @@ -108,7 +154,7 @@ def extract_mention(body: str | None, bot_login: str) -> str | None: """ if not isinstance(body, str) or not body: return None - login = bot_login.strip() + login = _normalize_bot_login(bot_login) if not login: return None pattern = re.compile( @@ -233,14 +279,13 @@ def route( "directive_pragmas": pragmas, "directive_authorizes_impl": False, } - repo_owner_matches = _login_matches_repo_owner(login, repo) - if not repo_owner_matches and not is_maintainer(login, assoc, maintainers=maintainers): + if not is_maintainer(login, assoc, maintainers=maintainers): return {} stripped = extract_mention(body, bot_login) if stripped is None: return {} cleaned, pragmas = parse_pragmas(stripped) - authorizes_impl = repo_owner_matches or is_implementation_authorizer(login, assoc, maintainers=maintainers) + authorizes_impl = is_implementation_authorizer(login, assoc, maintainers=maintainers) return { "directive": True, "directive_body": cleaned, @@ -283,9 +328,10 @@ def route( # amend-and-push workflow. key = _resolve_pr_key(number) login, assoc = _submitter_info(comment) + assoc = _effective_association(login, assoc, payload.get("repository"), repo) issue_user_raw = issue.get("user") issue_user = issue_user_raw if isinstance(issue_user_raw, Mapping) else {} - if str(issue_user.get("login") or "") == bot_login: + if _login_matches_bot(str(issue_user.get("login") or ""), bot_login): return RouteDecision( "queue", "handle_pr_conversation", @@ -299,6 +345,7 @@ def route( return RouteDecision("skip", None, repo, issue_key(repo, number), "incoming PR comments ignored") key = issue_key(repo, number) login, assoc = _submitter_info(comment) + assoc = _effective_association(login, assoc, payload.get("repository"), repo) return RouteDecision( "queue", "handle_comment", @@ -343,13 +390,14 @@ def route( return RouteDecision("skip", None, repo, None, "bot/self review comment") pr = payload.get("pull_request") or {} pr_user = pr.get("user") or {} - if str(pr_user.get("login") or "") != bot_login: + if not _login_matches_bot(str(pr_user.get("login") or ""), bot_login): return RouteDecision("skip", None, repo, None, "PR not authored by bot") number = pr.get("number") if not isinstance(number, int): return RouteDecision("skip", None, repo, None, "PR missing number") key = _resolve_pr_key(number) login, assoc = _submitter_info(comment) + assoc = _effective_association(login, assoc, payload.get("repository"), repo) return RouteDecision( "queue", "handle_review", diff --git a/python/robomp/tests/test_config.py b/python/robomp/tests/test_config.py index 6761f7998..9a60349c1 100644 --- a/python/robomp/tests/test_config.py +++ b/python/robomp/tests/test_config.py @@ -93,13 +93,55 @@ def test_blank_bot_login_rejected(monkeypatch: pytest.MonkeyPatch, env: dict[str Settings() # type: ignore[call-arg] -def test_bot_login_strips_at_prefix(monkeypatch: pytest.MonkeyPatch, env: dict[str, str]) -> None: - monkeypatch.setenv("ROBOMP_BOT_LOGIN", " @roboomp ") +@pytest.mark.parametrize( + "raw_login", + [ + "roboomp", + " @roboomp ", + " @ROBOOMP ", + "roboomp[bot]", + "@roboomp[bot]", + " @ROBOOMP[BOT] ", + ], +) +def test_bot_login_normalizes_mention_case_and_app_suffix( + monkeypatch: pytest.MonkeyPatch, env: dict[str, str], raw_login: str +) -> None: + monkeypatch.setenv("ROBOMP_BOT_LOGIN", raw_login) reset_settings_cache() cfg = Settings() # type: ignore[call-arg] assert cfg.bot_login == "roboomp" +def test_maintainer_logins_normalize_csv_entries( + monkeypatch: pytest.MonkeyPatch, env: dict[str, str] +) -> None: + monkeypatch.setenv("ROBOMP_MAINTAINER_LOGINS", " can1357, @ROBOOMP , @Alice[bot] ,, ") + reset_settings_cache() + cfg = Settings() # type: ignore[call-arg] + assert cfg.maintainer_logins == frozenset({"can1357", "roboomp", "alice[bot]"}) + + +@pytest.mark.parametrize( + ("raw_login", "expected"), + [ + ("roboomp", "roboomp"), + (" @roboomp ", "roboomp"), + (" @ROBOOMP ", "roboomp"), + ("roboomp[bot]", "roboomp[bot]"), + ("@roboomp[bot]", "roboomp[bot]"), + (" @ROBOOMP[BOT] ", "roboomp[bot]"), + ], +) +def test_maintainer_logins_common_entry_forms( + monkeypatch: pytest.MonkeyPatch, env: dict[str, str], raw_login: str, expected: str +) -> None: + monkeypatch.setenv("ROBOMP_MAINTAINER_LOGINS", raw_login) + reset_settings_cache() + cfg = Settings() # type: ignore[call-arg] + assert cfg.maintainer_logins == frozenset({expected}) + + def test_model_pool_single(env: dict[str, str]) -> None: cfg = Settings() # type: ignore[call-arg] assert cfg.model_pool == (cfg.model,) diff --git a/python/robomp/tests/test_github_events.py b/python/robomp/tests/test_github_events.py index d90494806..52250a9dc 100644 --- a/python/robomp/tests/test_github_events.py +++ b/python/robomp/tests/test_github_events.py @@ -2,6 +2,7 @@ from __future__ import annotations import hashlib import hmac +import pytest from robomp.github_events import ( extract_mention, @@ -143,6 +144,23 @@ def test_route_pr_conversation_uses_handle_pr_conversation() -> None: assert decision.task == "handle_pr_conversation" +def test_route_pr_conversation_normalizes_bot_author_suffix() -> None: + decision = route( + "issue_comment", + { + "action": "created", + "comment": {"user": {"login": "alice"}, "body": "looks good"}, + "issue": {"number": 9, "user": {"login": f"{BOT}[bot]"}, "pull_request": {"url": "x"}}, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=f"@{BOT}[bot]", + resolve_issue_from_pr=lambda _r, _n: "octo/widget#42", + ) + assert decision.should_queue + assert decision.task == "handle_pr_conversation" + + def test_route_pr_conversation_uses_resolver_for_inflight_key() -> None: """PR-derived events MUST serialize on the originating issue's key.""" @@ -511,6 +529,11 @@ def test_extract_mention_returns_body_minus_mention() -> None: assert extract_mention("@robomp-bot do X", "robomp-bot") == "do X" +@pytest.mark.parametrize("configured_login", ["@roboomp", "roboomp[bot]", "@roboomp[bot]"]) +def test_extract_mention_accepts_prefixed_or_app_bot_login(configured_login: str) -> None: + assert extract_mention("@roboomp go ahead", configured_login) == "go ahead" + + def test_extract_mention_returns_none_without_mention() -> None: assert extract_mention("hello there", "robomp-bot") is None assert extract_mention(None, "robomp-bot") is None @@ -614,7 +637,7 @@ def test_route_directive_authorizes_personal_repo_owner_without_author_associati "body": "@robomp-bot go ahead and push", }, "issue": {"number": 9}, - "repository": {"full_name": "can1357/widget"}, + "repository": {"full_name": "can1357/widget", "owner": {"login": "can1357", "type": "User"}}, }, allowlist=frozenset({"can1357/widget"}), bot_login=BOT, @@ -623,6 +646,26 @@ def test_route_directive_authorizes_personal_repo_owner_without_author_associati assert decision.directive_body == "go ahead and push" assert decision.directive_author == "can1357" assert decision.directive_authorizes_impl is True + assert decision.association == "OWNER" + + +def test_route_directive_does_not_authorize_org_owner_name_without_author_association() -> None: + decision = route( + "issue_comment", + { + "action": "created", + "comment": { + "user": {"login": "octo"}, + "body": "@robomp-bot go ahead and push", + }, + "issue": {"number": 9}, + "repository": {"full_name": "octo/widget", "owner": {"login": "octo", "type": "Organization"}}, + }, + allowlist=ALLOWLIST, + bot_login=BOT, + ) + assert decision.directive is False + assert decision.directive_authorizes_impl is False def test_route_directive_from_collaborator_does_not_authorize_impl() -> None: @@ -730,6 +773,29 @@ def test_route_directive_set_on_review_comment() -> None: assert decision.directive_body == "use a generator here" +def test_route_review_comment_normalizes_bot_author_suffix() -> None: + decision = route( + "pull_request_review_comment", + { + "action": "created", + "comment": { + "user": {"login": "can1357"}, + "author_association": "OWNER", + "body": "@robomp-bot use a generator here", + }, + "pull_request": {"number": 50, "user": {"login": f"{BOT}[bot]"}}, + "repository": {"full_name": "octo/widget"}, + }, + allowlist=ALLOWLIST, + bot_login=f"@{BOT}[bot]", + resolve_issue_from_pr=lambda _r, _n: "octo/widget#42", + ) + assert decision.should_queue + assert decision.task == "handle_review" + assert decision.directive is True + assert decision.directive_body == "use a generator here" + + # ---------- reviewer bots ---------- diff --git a/python/robomp/tests/test_host_tools.py b/python/robomp/tests/test_host_tools.py index 590364ec7..5c11441ca 100644 --- a/python/robomp/tests/test_host_tools.py +++ b/python/robomp/tests/test_host_tools.py @@ -1234,6 +1234,32 @@ def test_impl_gate_allows_authorized_proposal_to_reach_pr_validation(db: Databas assert "OWNER or allowlisted maintainer" not in msg +def test_impl_gate_allows_authorized_proposal_push_to_reach_repo_commands( + db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + from dataclasses import replace + + calls: list[list[str] | tuple[str, ...]] = [] + + def record_repo_command(_bindings: ToolBindings, cmd: list[str] | tuple[str, ...], *, timeout: float | None = None): + del timeout + calls.append(cmd) + raise RuntimeError("authorized proposal reached gh_push_branch repo command") + + bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500))) + db.set_issue_classification(bindings.issue_key, "proposal") + bindings = replace(bindings, impl_authorized=True) + monkeypatch.setattr(host_tools, "_run_repo_command", record_repo_command) + try: + tool = next(x for x in build(bindings) if x.name == "gh_push_branch") + with pytest.raises(RuntimeError, match="authorized proposal reached gh_push_branch repo command"): + tool.execute({}, _ctx()) + finally: + _stop_loop(loop, t) + + assert calls + + def test_impl_gate_allows_bug_without_directive_to_reach_pr_validation(db: Database, tmp_path: Path) -> None: bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500))) db.set_issue_classification(bindings.issue_key, "bug") diff --git a/python/robomp/tests/test_server.py b/python/robomp/tests/test_server.py index 52ee1e17f..6c04d1511 100644 --- a/python/robomp/tests/test_server.py +++ b/python/robomp/tests/test_server.py @@ -1536,6 +1536,43 @@ def test_webhook_directive_on_unknown_issue_is_queued_with_metadata(env) -> None assert directive == {"body": "please refactor X", "author": "can1357", "pragmas": [], "authorizes_impl": True} +def test_webhook_directive_authorizes_deployed_app_login_without_author_association( + monkeypatch: pytest.MonkeyPatch, env +) -> None: + monkeypatch.setenv("ROBOMP_BOT_LOGIN", "@roboomp[bot]") + monkeypatch.setenv("ROBOMP_REPO_ALLOWLIST", "can1357/widget") + reset_settings_cache() + cfg = Settings() # type: ignore[call-arg] + cfg.ensure_paths() + app = create_app(cfg) + payload = { + "action": "created", + "comment": { + "user": {"login": "can1357"}, + "body": "@roboomp go ahead", + }, + "issue": {"number": 3196}, + "repository": { + "full_name": "can1357/widget", + "owner": {"login": "can1357", "type": "User"}, + }, + } + raw = json.dumps(payload).encode() + with TestClient(app) as client: + resp = client.post( + "/webhook/github", + content=raw, + headers=_signed_headers("test-webhook-secret", raw, event="issue_comment", delivery="dir-app-login"), + ) + assert resp.status_code == 202 + assert resp.json()["state"] == "queued" + row = get_database(cfg.sqlite_path).get_event("dir-app-login") + close_database() + assert row is not None + directive = row.payload.get("_robomp_directive") + assert directive == {"body": "go ahead", "author": "can1357", "pragmas": [], "authorizes_impl": True} + + def test_webhook_maintainer_bypasses_rate_limit( rate_limited_settings: Settings, monkeypatch: pytest.MonkeyPatch, diff --git a/python/robomp/tests/test_tasks_directive.py b/python/robomp/tests/test_tasks_directive.py index dc3ac5fcb..73f1a3fad 100644 --- a/python/robomp/tests/test_tasks_directive.py +++ b/python/robomp/tests/test_tasks_directive.py @@ -2,7 +2,11 @@ from __future__ import annotations +from types import SimpleNamespace + import pytest +from robomp import tasks +from robomp.github_client import IssueInfo, RepoInfo from robomp.tasks import _attach_thread, _directive_from_payload from robomp.worker import DirectiveInfo @@ -85,3 +89,179 @@ async def test_attach_thread_preserves_authorizes_impl(monkeypatch: pytest.Monke assert hydrated.body == "test body" assert hydrated.author == "test_author" assert hydrated.authorizes_impl is True + + + +def _payload_with_directive(*, issue_number: int, body: str = "@robomp-bot ship it") -> dict[str, object]: + return { + "repository": { + "full_name": "octo/widget", + "default_branch": "main", + "clone_url": "https://x/octo/widget.git", + "private": False, + }, + "issue": { + "number": issue_number, + "title": "proposal", + "body": "issue body", + "state": "open", + "user": {"login": "alice"}, + "labels": [{"name": "proposal"}], + }, + "comment": { + "id": 99, + "body": body, + "created_at": "2026-01-01T00:00:00Z", + "user": {"login": "owner"}, + }, + "_robomp_directive": { + "body": body, + "author": "owner", + "authorizes_impl": True, + }, + } + + +def _workspace(tmp_path): + repo_dir = tmp_path / "repo" + session_dir = tmp_path / "session" + repo_dir.mkdir(exist_ok=True) + session_dir.mkdir(exist_ok=True) + return SimpleNamespace(root=tmp_path, repo_dir=repo_dir, session_dir=session_dir, branch="robomp/issue-42") + + +async def test_handle_comment_preserves_authorizes_impl_to_run_task( + db, tmp_path, settings, monkeypatch: pytest.MonkeyPatch +) -> None: + workspace = _workspace(tmp_path) + captured: dict[str, object] = {} + + async def fake_attach_thread( + _github: object, + directive: DirectiveInfo | None, + _repo: str, + _number: int, + *, + is_pr: bool, + ) -> DirectiveInfo | None: + assert directive is not None + assert is_pr is False + captured["attached_authorizes_impl"] = directive.authorizes_impl + return directive + + async def fake_run_task( + *, + task_kind: str, + inputs: object, + directive: DirectiveInfo | None = None, + **_kwargs: object, + ) -> None: + del inputs + captured["task_kind"] = task_kind + captured["run_task_authorizes_impl"] = directive.authorizes_impl if directive is not None else None + + monkeypatch.setattr(tasks, "_attach_thread", fake_attach_thread) + monkeypatch.setattr(tasks, "run_task", fake_run_task) + + await tasks.handle_comment( + settings=settings, + db=db, + github=SimpleNamespace(), + sandbox=SimpleNamespace(natives_cache=None, ensure_workspace=lambda **_kwargs: workspace), + git_transport=SimpleNamespace(), + payload=_payload_with_directive(issue_number=42), + delivery_id="d-comment", + ) + + assert captured == { + "attached_authorizes_impl": True, + "task_kind": "triage_issue", + "run_task_authorizes_impl": True, + } + + +async def test_handle_pr_conversation_preserves_authorizes_impl_to_run_task( + db, tmp_path, settings, monkeypatch: pytest.MonkeyPatch +) -> None: + workspace = _workspace(tmp_path) + db.upsert_issue( + key="octo/widget#42", + repo="octo/widget", + number=42, + state="opened", + branch=workspace.branch, + session_dir=str(workspace.session_dir), + pr_number=7, + ) + captured: dict[str, object] = {} + + class FakeGitHub: + async def get_repo(self, repo: str) -> RepoInfo: + assert repo == "octo/widget" + return RepoInfo(full_name=repo, default_branch="main", clone_url="https://x/octo/widget.git", private=False) + + async def get_issue(self, repo: str, number: int) -> IssueInfo: + assert repo == "octo/widget" + assert number == 42 + return IssueInfo( + repo=repo, + number=number, + title="proposal", + body="issue body", + state="open", + author="alice", + labels=("proposal",), + is_pull_request=False, + ) + + async def fake_attach_thread( + _github: object, + directive: DirectiveInfo | None, + repo: str, + number: int, + *, + is_pr: bool, + ) -> DirectiveInfo | None: + assert directive is not None + assert repo == "octo/widget" + assert number == 7 + assert is_pr is True + captured["attached_authorizes_impl"] = directive.authorizes_impl + return directive + + async def fake_run_task( + *, + task_kind: str, + inputs: object, + pr_number: int | None = None, + directive: DirectiveInfo | None = None, + **_kwargs: object, + ) -> None: + del inputs + captured["task_kind"] = task_kind + captured["pr_number"] = pr_number + captured["run_task_authorizes_impl"] = directive.authorizes_impl if directive is not None else None + + monkeypatch.setattr(tasks, "_attach_thread", fake_attach_thread) + monkeypatch.setattr(tasks, "run_task", fake_run_task) + + payload = _payload_with_directive(issue_number=7) + issue_payload = payload["issue"] + assert isinstance(issue_payload, dict) + issue_payload["pull_request"] = {"url": "https://api.github.com/repos/octo/widget/pulls/7"} + await tasks.handle_pr_conversation( + settings=settings, + db=db, + github=FakeGitHub(), + sandbox=SimpleNamespace(natives_cache=None, ensure_workspace=lambda **_kwargs: workspace), + git_transport=SimpleNamespace(), + payload=payload, + delivery_id="d-pr-comment", + ) + + assert captured == { + "attached_authorizes_impl": True, + "task_kind": "handle_comment", + "pr_number": 7, + "run_task_authorizes_impl": True, + } \ No newline at end of file diff --git a/python/robomp/tests/test_worker.py b/python/robomp/tests/test_worker.py index f2de9e17d..3094a5a7d 100644 --- a/python/robomp/tests/test_worker.py +++ b/python/robomp/tests/test_worker.py @@ -192,6 +192,32 @@ async def test_run_task_sets_impl_authorized_from_directive( assert captured == {"impl_authorized": True} +@pytest.mark.asyncio +async def test_run_task_preserves_impl_authorized_when_resuming( + tmp_path: Path, settings: Settings, monkeypatch: pytest.MonkeyPatch +) -> None: + inputs, _bindings = _make_inputs(tmp_path, settings, session_has_jsonl=True) + captured: dict[str, bool] = {} + + monkeypatch.setattr(worker, "_build_prompt", lambda *args, **kwargs: "prompt") + + def capture_build(bindings: worker.ToolBindings) -> tuple: + captured["impl_authorized"] = bindings.impl_authorized + return () + + monkeypatch.setattr(worker.host_tools, "build", capture_build) + + result = await worker.run_task( + task_kind="handle_comment", + inputs=inputs, + directive=worker.DirectiveInfo(body="go ahead", author="can1357", authorizes_impl=True), + ) + + assert result == "ok" + assert captured == {"impl_authorized": True} + assert _FakeRpcClient.instances[0].kwargs["extra_args"] == ("--continue",) + + @pytest.mark.asyncio async def test_run_rpc_passes_continue_when_session_jsonl_present(tmp_path: Path, settings: Settings) -> None: inputs, bindings = _make_inputs(tmp_path, settings, session_has_jsonl=True)