feat(agent): added implementation authorization gate for branch/PR tools

- Added `is_implementation_authorizer` check requiring OWNER or allowlisted maintainer to authorize implementation work.
- Blocked `gh_push_branch` and `gh_open_pr` for unclassified/enhancement/proposal issues without explicit directive authorization.
- Auto-allowed bug and documentation issues without requiring a directive.
- Propagated `authorizes_impl` flag through events, server, tasks, and worker bindings.
This commit is contained in:
can1357
2026-06-02 08:44:59 +02:00
parent 2ecb5fd9fa
commit f18eb90324
13 changed files with 252 additions and 7 deletions
+36
View File
@@ -5,6 +5,7 @@ import hmac
from robomp.github_events import (
extract_mention,
is_implementation_authorizer,
is_maintainer,
rate_limit_cap,
route,
@@ -545,6 +546,17 @@ def test_is_maintainer_rejects_contributor_and_none() -> None:
assert is_maintainer(None, "OWNER", maintainers=frozenset()) # association still wins
def test_is_implementation_authorizer_accepts_allowlist_and_owner() -> None:
assert is_implementation_authorizer("can1357", None, maintainers=frozenset({"can1357"}))
assert is_implementation_authorizer("Can1357", "NONE", maintainers=frozenset({"can1357"}))
assert is_implementation_authorizer("stranger", "OWNER", maintainers=frozenset())
def test_is_implementation_authorizer_rejects_non_owner_associations() -> None:
for assoc in ("MEMBER", "COLLABORATOR", "NONE", "CONTRIBUTOR", None):
assert not is_implementation_authorizer("stranger", assoc, maintainers=frozenset()), assoc
def test_route_directive_set_on_issue_comment_when_owner_mentions_bot() -> None:
decision = route(
"issue_comment",
@@ -565,6 +577,7 @@ def test_route_directive_set_on_issue_comment_when_owner_mentions_bot() -> None:
assert decision.directive is True
assert decision.directive_body == "please refactor X"
assert decision.directive_author == "can1357"
assert decision.directive_authorizes_impl is True
def test_route_directive_set_when_login_in_maintainers_list() -> None:
@@ -587,6 +600,28 @@ def test_route_directive_set_when_login_in_maintainers_list() -> None:
assert decision.directive is True
assert decision.directive_body == "do it"
assert decision.directive_author == "can1357"
assert decision.directive_authorizes_impl is True
def test_route_directive_from_collaborator_does_not_authorize_impl() -> None:
decision = route(
"issue_comment",
{
"action": "created",
"comment": {
"user": {"login": "oldschoola"},
"author_association": "COLLABORATOR",
"body": "@robomp-bot go ahead with the plan",
},
"issue": {"number": 9},
"repository": {"full_name": "octo/widget"},
},
allowlist=ALLOWLIST,
bot_login=BOT,
)
assert decision.directive is True
assert decision.directive_body == "go ahead with the plan"
assert decision.directive_authorizes_impl is False
def test_route_directive_unset_for_random_user_even_with_mention() -> None:
@@ -719,6 +754,7 @@ def test_route_reviewer_bot_review_comment_is_directive() -> None:
assert decision.directive is True
assert decision.directive_body == "This branch leaks memory."
assert decision.directive_author == "chatgpt-codex-connector"
assert decision.directive_authorizes_impl is False
def test_route_random_bot_still_skipped_when_not_in_reviewer_list() -> None:
+99
View File
@@ -350,6 +350,7 @@ def test_gh_post_comment_propagates_github_error(db: Database, tmp_path: Path) -
def test_gh_open_pr_requires_template_sections(db: Database, tmp_path: Path) -> None:
transport = httpx.MockTransport(lambda r: httpx.Response(500))
bindings, loop, t = _bindings(db, tmp_path, transport)
db.set_issue_classification(bindings.issue_key, "bug")
try:
tool = next(x for x in build(bindings) if x.name == "gh_open_pr")
with pytest.raises(RpcCommandError) as exc:
@@ -942,6 +943,90 @@ 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
) -> 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("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")
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")
open_pr = next(x for x in build(bindings) if x.name == "gh_open_pr")
with pytest.raises(RpcCommandError) as push_exc:
push.execute({}, _ctx())
with pytest.raises(RpcCommandError) as pr_exc:
open_pr.execute({"title": "fix: x", "body": "invalid"}, _ctx())
finally:
_stop_loop(loop, t)
for msg in (str(push_exc.value), str(pr_exc.value)):
assert "classified `proposal`" in msg
assert "OWNER or allowlisted maintainer" in msg
assert "gh_post_comment" in msg
assert calls == []
rows = db._conn.execute(
"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)
def test_impl_gate_allows_authorized_proposal_to_reach_pr_validation(db: Database, tmp_path: Path) -> 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")
bindings = replace(bindings, impl_authorized=True)
try:
tool = next(x for x in build(bindings) if x.name == "gh_open_pr")
with pytest.raises(RpcCommandError) as exc:
tool.execute({"title": "fix: x", "body": ""}, _ctx())
finally:
_stop_loop(loop, t)
msg = str(exc.value)
assert "requires a non-empty 'body'" in msg
assert "OWNER or allowlisted maintainer" not in msg
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")
try:
tool = next(x for x in build(bindings) if x.name == "gh_open_pr")
with pytest.raises(RpcCommandError) as exc:
tool.execute({"title": "fix: x", "body": ""}, _ctx())
finally:
_stop_loop(loop, t)
msg = str(exc.value)
assert "requires a non-empty 'body'" in msg
assert "OWNER or allowlisted maintainer" not in msg
def test_impl_gate_allows_existing_proposal_pr_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, "proposal")
db.set_issue_pr(bindings.issue_key, 7)
try:
tool = next(x for x in build(bindings) if x.name == "gh_open_pr")
with pytest.raises(RpcCommandError) as exc:
tool.execute({"title": "fix: x", "body": ""}, _ctx())
finally:
_stop_loop(loop, t)
msg = str(exc.value)
assert "requires a non-empty 'body'" in msg
assert "OWNER or allowlisted maintainer" not in msg
def test_classify_issue_on_pr_thread_is_noop(db: Database, tmp_path: Path) -> None:
"""On PR threads the tool must not hit GitHub and must not raise."""
calls: list[str] = []
@@ -1279,6 +1364,7 @@ def test_gh_push_branch_rejects_wrong_identity(db: Database, tmp_path: Path) ->
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_push_branch")
with pytest.raises(RpcCommandError) as exc:
tool.execute({}, _ctx())
@@ -1397,6 +1483,7 @@ def test_gh_open_pr_rejects_wrong_identity_before_push_or_pr(db: Database, tmp_p
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_open_pr")
body = "## Repro\nrepro\n\n## Cause\ncause\n\n## Fix\nfix\n\n## Verification\nran tests\n\nFixes #42\n"
with pytest.raises(RpcCommandError) as exc:
@@ -1517,6 +1604,7 @@ def test_gh_push_branch_rejects_invalid_identity_scan_range(db: Database, tmp_pa
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_push_branch")
with pytest.raises(RpcCommandError) as exc:
tool.execute({}, _ctx())
@@ -1542,6 +1630,7 @@ def test_gh_push_branch_rejects_invalid_identity_scan_range(db: Database, tmp_pa
def test_gh_open_pr_requires_closes_keyword(db: Database, tmp_path: Path) -> None:
"""gh_open_pr refuses if the body has the four sections but no Fixes/Closes/Resolves keyword."""
bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(lambda r: httpx.Response(500)))
db.set_issue_classification(bindings.issue_key, "bug")
try:
tool = next(x for x in build(bindings) if x.name == "gh_open_pr")
body = "## Repro\nrepro\n\n## Cause\ncause\n\n## Fix\nfix\n\n## Verification\nran tests\n"
@@ -1574,6 +1663,7 @@ def test_gh_open_pr_refuses_failed_bun_check_before_push_or_pr(
)
bindings, loop, t = _bindings(db, tmp_path, httpx.MockTransport(handler))
db.set_issue_classification(bindings.issue_key, "bug")
fakebin = tmp_path / "fakebin"
fakebin.mkdir()
fake_bun = fakebin / "bun"
@@ -1713,6 +1803,7 @@ def test_gh_push_branch_rejects_dirty_worktree(db: Database, tmp_path: Path) ->
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_push_branch")
with pytest.raises(RpcCommandError) as exc:
tool.execute({}, _ctx())
@@ -1860,6 +1951,7 @@ def test_gh_push_branch_runs_fix_and_check_before_pushing(
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_push_branch")
result = tool.execute({}, _ctx())
finally:
@@ -1993,6 +2085,7 @@ def test_gh_push_branch_force_with_lease_recovers_after_amend(db: Database, tmp_
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_push_branch")
tool.execute({}, _ctx())
@@ -2171,6 +2264,7 @@ def test_gh_push_branch_aborts_on_failed_bun_check(
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_push_branch")
with pytest.raises(RpcCommandError) as exc:
tool.execute({}, _ctx())
@@ -2326,6 +2420,7 @@ def test_gh_push_branch_skip_checks_bypasses_failing_bun_check(
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_push_branch")
result = tool.execute({"skip_checks": True}, _ctx())
finally:
@@ -2463,6 +2558,7 @@ def test_gh_push_branch_skip_checks_still_refuses_dirty_worktree(
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_push_branch")
with pytest.raises(RpcCommandError) as exc:
tool.execute({"skip_checks": True}, _ctx())
@@ -2629,6 +2725,7 @@ def test_gh_open_pr_runs_fix_then_check_and_commits_fixup(
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_open_pr")
body = "## Repro\nrepro\n\n## Cause\ncause\n\n## Fix\nfix\n\n## Verification\nran tests\n\nFixes #42\n"
result = tool.execute({"title": "fix: x", "body": body}, _ctx())
@@ -2802,6 +2899,7 @@ def test_gh_open_pr_refuses_dirty_worktree_before_fix(
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
push_tool = next(x for x in build(bindings) if x.name == "gh_push_branch")
with pytest.raises(RpcCommandError) as exc:
push_tool.execute({}, _ctx())
@@ -2985,6 +3083,7 @@ def test_gh_open_pr_skips_fix_when_no_script(db: Database, tmp_path: Path, monke
branch=ws.branch,
session_dir=str(ws.session_dir),
)
db.set_issue_classification(bindings.issue_key, "bug")
tool = next(x for x in build(bindings) if x.name == "gh_open_pr")
body = "## Repro\nrepro\n\n## Cause\ncause\n\n## Fix\nfix\n\n## Verification\nran tests\n\nFixes #42\n"
result = tool.execute({"title": "fix: x", "body": body}, _ctx())
+1 -1
View File
@@ -1500,7 +1500,7 @@ def test_webhook_directive_on_unknown_issue_is_queued_with_metadata(env) -> None
assert row is not None
assert row.state == "queued"
directive = row.payload.get("_robomp_directive")
assert directive == {"body": "please refactor X", "author": "can1357", "pragmas": []}
assert directive == {"body": "please refactor X", "author": "can1357", "pragmas": [], "authorizes_impl": True}
def test_webhook_maintainer_bypasses_rate_limit(
@@ -19,12 +19,14 @@ def test_directive_from_payload_parses_pragmas() -> None:
assert directive.body == "do the thing"
assert directive.author == "can1357"
assert directive.pragmas == (("model", "gpt"), ("thinking", "low"))
assert directive.authorizes_impl is False
def test_directive_from_payload_missing_pragmas_is_empty_tuple() -> None:
directive = _directive_from_payload({"_robomp_directive": {"body": "x", "author": "can1357"}})
assert directive is not None
assert directive.pragmas == ()
assert directive.authorizes_impl is False
def test_directive_from_payload_drops_malformed_pragma_entries() -> None:
@@ -46,6 +48,20 @@ def test_directive_from_payload_drops_malformed_pragma_entries() -> None:
assert directive.pragmas == (("model", "gpt"),)
def test_directive_from_payload_parses_implementation_authorization() -> None:
directive = _directive_from_payload(
{
"_robomp_directive": {
"body": "do the thing",
"author": "can1357",
"authorizes_impl": True,
}
}
)
assert directive is not None
assert directive.authorizes_impl is True
def test_directive_from_payload_returns_none_for_missing_directive() -> None:
assert _directive_from_payload({}) is None
assert _directive_from_payload({"_robomp_directive": "not-a-mapping"}) is None
+34
View File
@@ -157,6 +157,40 @@ def _patch_worker(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None:
)
@pytest.mark.asyncio
async def test_run_task_sets_impl_authorized_from_directive(
tmp_path: Path, settings: Settings, monkeypatch: pytest.MonkeyPatch
) -> None:
inputs, _bindings = _make_inputs(tmp_path, settings, session_has_jsonl=False)
captured: dict[str, bool] = {}
monkeypatch.setattr(worker, "_build_prompt", lambda *args, **kwargs: "prompt")
def fake_run_rpc_blocking(
_inputs: worker.TaskInputs,
*,
task_kind: str,
prompt: str,
loop: asyncio.AbstractEventLoop,
bindings: worker.ToolBindings,
directive: worker.DirectiveInfo | None = None,
) -> str:
del task_kind, prompt, loop, directive
captured["impl_authorized"] = bindings.impl_authorized
return "ok"
monkeypatch.setattr(worker, "_run_rpc_blocking", fake_run_rpc_blocking)
result = await worker.run_task(
task_kind="triage_issue",
inputs=inputs,
directive=worker.DirectiveInfo(body="go ahead", author="can1357", authorizes_impl=True),
)
assert result == "ok"
assert captured == {"impl_authorized": True}
@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)