diff --git a/python/robomp/.env.example b/python/robomp/.env.example index 7318f57b9..692206eec 100644 --- a/python/robomp/.env.example +++ b/python/robomp/.env.example @@ -46,8 +46,8 @@ ROBOMP_GIT_AUTHOR_EMAIL= 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. +# authorize implementation work in addition to repo OWNER. Prefer lowercase +# bare logins; startup ignores leading `@`, case, whitespace, and `[bot]`. ROBOMP_MAINTAINER_LOGINS= diff --git a/python/robomp/AGENTS.md b/python/robomp/AGENTS.md index cdae5be51..d0d2ec519 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. 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. +- **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 bare logins (`@`/`[bot]` 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/src/config.py b/python/robomp/src/config.py index 48f4fdc16..6b645f90c 100644 --- a/python/robomp/src/config.py +++ b/python/robomp/src/config.py @@ -305,7 +305,10 @@ class Settings(BaseSettings): @property def maintainer_logins(self) -> frozenset[str]: - items = [piece.strip().lstrip("@").lower() for piece in self.maintainer_logins_raw.split(",")] + items = [ + piece.strip().lstrip("@").lower().removesuffix("[bot]") + for piece in self.maintainer_logins_raw.split(",") + ] return frozenset(item for item in items if item) def allows(self, full_name: str) -> bool: diff --git a/python/robomp/src/db.py b/python/robomp/src/db.py index a016b501d..73045501c 100644 --- a/python/robomp/src/db.py +++ b/python/robomp/src/db.py @@ -607,6 +607,26 @@ class Database: last_error=row["last_error"], ) + def has_authorized_impl_event(self, issue_key: str) -> bool: + """Return whether a non-skipped event on this issue carried implementation authorization.""" + with self._lock: + rows = self._conn.execute( + """ + SELECT payload_json + FROM events + WHERE issue_key = ? + AND state <> 'skipped' + ORDER BY received_at DESC + """, + (issue_key,), + ).fetchall() + for row in rows: + payload = json.loads(row["payload_json"]) + directive = payload.get("_robomp_directive") + if isinstance(directive, dict) and directive.get("authorizes_impl") is True: + return True + return False + def requeue_event( self, delivery_id: str, diff --git a/python/robomp/src/github_events.py b/python/robomp/src/github_events.py index d37925743..8aea67c36 100644 --- a/python/robomp/src/github_events.py +++ b/python/robomp/src/github_events.py @@ -89,16 +89,11 @@ def _login_matches_personal_repo_owner( 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": + if owner_type is None or owner_type.lower() != "user": return False - if owner_login: - return login.lower() == owner_login.lower() - if not isinstance(repo, str): + if not owner_login: return False - owner, sep, _name = repo.partition("/") - if not sep or not owner: - return False - return login.lower() == owner.lower() + return login.lower() == owner_login.lower() def _effective_association( @@ -158,7 +153,7 @@ def extract_mention(body: str | None, bot_login: str) -> str | None: if not login: return None pattern = re.compile( - rf"(? None: + assert extract_mention("@roboomp[bot] go ahead", "roboomp[bot]") == "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 diff --git a/python/robomp/tests/test_host_tools.py b/python/robomp/tests/test_host_tools.py index 5c11441ca..9d4ab2b1e 100644 --- a/python/robomp/tests/test_host_tools.py +++ b/python/robomp/tests/test_host_tools.py @@ -1181,8 +1181,9 @@ def test_review_mode_rejects_push_and_open_pr_before_repo_commands(db: Database, assert calls == [] -def test_impl_gate_rejects_unauthorized_proposal_before_repo_commands( - db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +@pytest.mark.parametrize("classification", ["enhancement", "proposal"]) +def test_impl_gate_rejects_unauthorized_non_auto_classification_before_repo_commands( + db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, classification: str ) -> None: calls: list[list[str] | tuple[str, ...]] = [] @@ -1192,7 +1193,7 @@ def test_impl_gate_rejects_unauthorized_proposal_before_repo_commands( raise AssertionError("repo command must not run before implementation authorization") bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500))) - db.set_issue_classification(bindings.issue_key, "proposal") + db.set_issue_classification(bindings.issue_key, classification) monkeypatch.setattr(host_tools, "_run_repo_command", record_repo_command) try: push = next(x for x in build(bindings) if x.name == "gh_push_branch") @@ -1205,7 +1206,7 @@ def test_impl_gate_rejects_unauthorized_proposal_before_repo_commands( _stop_loop(loop, t) for msg in (str(push_exc.value), str(pr_exc.value)): - assert "classified `proposal`" in msg + assert f"classified `{classification}`" in msg assert "OWNER or allowlisted maintainer" in msg assert "gh_post_comment" in msg assert calls == [] @@ -1213,14 +1214,17 @@ def test_impl_gate_rejects_unauthorized_proposal_before_repo_commands( "SELECT tool, error FROM tool_calls WHERE tool IN ('gh_push_branch', 'gh_open_pr') ORDER BY id" ).fetchall() assert [row["tool"] for row in rows] == ["gh_push_branch", "gh_open_pr"] - assert all("classified `proposal`" in row["error"] for row in rows) + assert all(f"classified `{classification}`" in row["error"] for row in rows) -def test_impl_gate_allows_authorized_proposal_to_reach_pr_validation(db: Database, tmp_path: Path) -> None: +@pytest.mark.parametrize("classification", ["enhancement", "proposal"]) +def test_impl_gate_allows_authorized_non_auto_classification_to_reach_pr_validation( + db: Database, tmp_path: Path, classification: str +) -> None: from dataclasses import replace bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500))) - db.set_issue_classification(bindings.issue_key, "proposal") + db.set_issue_classification(bindings.issue_key, classification) bindings = replace(bindings, impl_authorized=True) try: tool = next(x for x in build(bindings) if x.name == "gh_open_pr") @@ -1234,8 +1238,9 @@ 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 +@pytest.mark.parametrize("classification", ["enhancement", "proposal"]) +def test_impl_gate_allows_authorized_non_auto_classification_push_to_reach_repo_commands( + db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, classification: str ) -> None: from dataclasses import replace @@ -1244,15 +1249,15 @@ def test_impl_gate_allows_authorized_proposal_push_to_reach_repo_commands( 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") + raise RuntimeError("authorized non-auto issue 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") + db.set_issue_classification(bindings.issue_key, classification) 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"): + with pytest.raises(RuntimeError, match="authorized non-auto issue reached gh_push_branch repo command"): tool.execute({}, _ctx()) finally: _stop_loop(loop, t) @@ -1260,6 +1265,80 @@ def test_impl_gate_allows_authorized_proposal_push_to_reach_repo_commands( assert calls +def test_impl_gate_allows_later_authorized_event_to_reach_repo_commands( + db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + 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("later authorized event 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, "enhancement") + db.record_event( + delivery_id="auth-event", + event_type="issue_comment", + repo=bindings.issue.repo, + issue_key=bindings.issue_key, + payload={ + "_robomp_directive": { + "body": "go ahead", + "author": "can1357", + "pragmas": [], + "authorizes_impl": 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="later authorized event reached gh_push_branch repo command"): + tool.execute({}, _ctx()) + finally: + _stop_loop(loop, t) + + assert calls + + +def test_impl_gate_ignores_skipped_authorized_event(db: Database, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + 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 AssertionError("skipped authorization must not reach repo command") + + bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda _r: httpx.Response(500))) + db.set_issue_classification(bindings.issue_key, "enhancement") + db.record_event( + delivery_id="skipped-auth-event", + event_type="issue_comment", + repo=bindings.issue.repo, + issue_key=bindings.issue_key, + payload={ + "_robomp_directive": { + "body": "go ahead", + "author": "can1357", + "pragmas": [], + "authorizes_impl": True, + } + }, + state="skipped", + ) + 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(RpcCommandError) as exc: + tool.execute({}, _ctx()) + finally: + _stop_loop(loop, t) + + assert "OWNER or allowlisted maintainer" in str(exc.value) + 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 6c04d1511..f4c492b5b 100644 --- a/python/robomp/tests/test_server.py +++ b/python/robomp/tests/test_server.py @@ -1598,8 +1598,16 @@ def test_webhook_maintainer_bypasses_rate_limit( ) assert resp.status_code == 202 states.append(resp.json()["state"]) + directive_event = get_database(cfg.sqlite_path).get_event("m-3") close_database() assert states == ["queued"] * 4, states + assert directive_event is not None + assert directive_event.payload.get("_robomp_directive") == { + "body": "do X", + "author": "can1357", + "pragmas": [], + "authorizes_impl": True, + } # -------- handler-level: bootstrap + reopen ----------------------------