diff --git a/src/robomp/github_events.py b/src/robomp/github_events.py index 4d9dcadd2..3de98f35f 100644 --- a/src/robomp/github_events.py +++ b/src/robomp/github_events.py @@ -138,10 +138,10 @@ def route( """Decide whether and how to handle a webhook event. `resolve_issue_from_pr(repo, pr_number)` maps a PR number back to its - originating-issue key (e.g. `octo/widget#42`). User comments on PRs are - only actionable when that mapping exists, so they serialize on the same - inflight key as the issue's own events. PR lifecycle cleanup may still - fall back to a PR-scoped key when the origin row is gone. + originating-issue key (e.g. `octo/widget#42`). PR-derived events prefer + that key so follow-ups serialize with the original issue. If the mapping + is missing, the event is still actionable and falls back to the PR's own + issue key (`octo/widget#1080`). """ repo = _repo_full_name(payload) if repo is None or repo.lower() not in allowlist: @@ -154,12 +154,7 @@ def route( resolved = resolve_issue_from_pr(repo, pr_number) # type: ignore[arg-type] if resolved: return resolved - return f"{repo}#pr-{pr_number}" - - def _resolve_origin_issue_key(pr_number: int) -> str | None: - if resolve_issue_from_pr is None: - return None - return resolve_issue_from_pr(repo, pr_number) # type: ignore[arg-type] + return issue_key(repo, pr_number) # type: ignore[arg-type] def _reviewer_bot_login(user: Mapping[str, Any] | None) -> str | None: """Return the lowercased login if this user is a configured reviewer bot.""" @@ -226,11 +221,11 @@ def route( return RouteDecision("skip", None, repo, None, "comment missing issue number") if "pull_request" in issue: # Conversation comment on a PR. The PR number lives at issue.number - # on this payload type. Only mapped bot PR follow-ups are - # actionable; unknown PRs must not consume per-user quota. - key = _resolve_origin_issue_key(number) - if key is None: - return RouteDecision("skip", None, repo, None, f"PR #{number} is not mapped to an issue") + # on this payload type. Prefer the originating issue key when the + # DB has it, but do not drop bot-authored follow-ups just because + # the PR mapping was lost; the worker can recover from the PR + # branch or handle the PR directly. + key = _resolve_pr_key(number) login, assoc = _submitter_info(comment) return RouteDecision( "queue", @@ -267,9 +262,7 @@ def route( number = pr.get("number") if not isinstance(number, int): return RouteDecision("skip", None, repo, None, "PR missing number") - key = _resolve_origin_issue_key(number) - if key is None: - return RouteDecision("skip", None, repo, None, f"PR #{number} is not mapped to an issue") + key = _resolve_pr_key(number) login, assoc = _submitter_info(comment) return RouteDecision( "queue", diff --git a/src/robomp/persona.py b/src/robomp/persona.py index 67255c111..33a69ebd3 100644 --- a/src/robomp/persona.py +++ b/src/robomp/persona.py @@ -209,11 +209,17 @@ def _inbound_scope(issue: IssueInfo, pr_number: int | None) -> dict[str, Any]: `kind` field lets prompts say "PR" or "issue" without branching in the template engine. """ - if pr_number is not None and pr_number != issue.number: + if pr_number is not None: return {"kind": "PR", "number": pr_number} return {"kind": "issue", "number": issue.number} +def _origin_scope(issue: IssueInfo) -> dict[str, Any]: + if issue.is_pull_request: + return {"description": "originating issue unknown; handling this PR directly"} + return {"description": f"originating issue #{issue.number}"} + + def followup_comment( *, repo: RepoInfo, @@ -232,6 +238,7 @@ def followup_comment( "comment": comment, "state": {"pr_status": pr_status}, "inbound": _inbound_scope(issue, pr_number), + "origin": _origin_scope(issue), }, ) @@ -258,6 +265,7 @@ def directive( "thread": _render_thread(getattr(directive, "thread", ()) or ()), "state": {"pr_status": pr_status}, "inbound": _inbound_scope(issue, pr_number), + "origin": _origin_scope(issue), }, ) diff --git a/src/robomp/prompts/directive.md b/src/robomp/prompts/directive.md index 83a2f8fc2..990632b7d 100644 --- a/src/robomp/prompts/directive.md +++ b/src/robomp/prompts/directive.md @@ -1,9 +1,9 @@ # Directive on {{repo.full_name}}#{{inbound.number}} ({{inbound.kind}}) **@{{directive.author}}** posted an authoritative directive on this -{{inbound.kind}} thread (originating issue #{{issue.number}}). They're -either a maintainer who tagged you (`@bot`) or a configured reviewer bot -whose comments you treat as binding. Current PR state: +{{inbound.kind}} thread ({{origin.description}}). They're either a maintainer +who tagged you (`@bot`) or a configured reviewer bot whose comments you treat +as binding. Current PR state: `{{state.pr_status}}`. The directive overrides any prior plan or seed todos. diff --git a/src/robomp/prompts/followup_comment.md b/src/robomp/prompts/followup_comment.md index bb85fa4de..1596fa385 100644 --- a/src/robomp/prompts/followup_comment.md +++ b/src/robomp/prompts/followup_comment.md @@ -1,7 +1,7 @@ # Follow-up on {{repo.full_name}}#{{inbound.number}} ({{inbound.kind}}) -A new comment arrived on this {{inbound.kind}} thread (originating issue -#{{issue.number}}). Current PR state: `{{state.pr_status}}`. +A new comment arrived on this {{inbound.kind}} thread ({{origin.description}}). +Current PR state: `{{state.pr_status}}`. ## New comment by @{{comment.author}} ({{comment.created_at}}) diff --git a/tests/test_github_events.py b/tests/test_github_events.py index c2bf45e0a..0ec4391c9 100644 --- a/tests/test_github_events.py +++ b/tests/test_github_events.py @@ -167,8 +167,8 @@ def test_route_pr_conversation_uses_resolver_for_inflight_key() -> None: assert decision.issue_key == "octo/widget#42" -def test_route_pr_conversation_skips_when_resolver_misses() -> None: - """Unknown PR comments are not actionable and must not count as submissions.""" +def test_route_pr_conversation_falls_back_to_pr_key_when_resolver_misses() -> None: + """Unmapped PR comments still queue so the worker can recover from the PR branch.""" decision = route( "issue_comment", @@ -182,10 +182,10 @@ def test_route_pr_conversation_skips_when_resolver_misses() -> None: bot_login=BOT, resolve_issue_from_pr=lambda _r, _n: None, ) - assert not decision.should_queue - assert decision.submitter is None - assert decision.issue_key is None - assert "not mapped" in decision.reason + assert decision.should_queue + assert decision.task == "handle_pr_conversation" + assert decision.submitter == "alice" + assert decision.issue_key == "octo/widget#9" def test_route_review_only_for_bot_authored_pr() -> None: @@ -219,7 +219,7 @@ def test_route_review_only_for_bot_authored_pr() -> None: assert not not_ours.should_queue -def test_route_review_comment_skips_when_resolver_misses() -> None: +def test_route_review_comment_falls_back_to_pr_key_when_resolver_misses() -> None: decision = route( "pull_request_review_comment", { @@ -232,8 +232,10 @@ def test_route_review_comment_skips_when_resolver_misses() -> None: bot_login=BOT, resolve_issue_from_pr=lambda _r, _n: None, ) - assert not decision.should_queue - assert decision.submitter is None + assert decision.should_queue + assert decision.task == "handle_review" + assert decision.submitter == "alice" + assert decision.issue_key == "octo/widget#9" def test_route_pr_closed_only_when_merged_by_bot() -> None: @@ -262,7 +264,7 @@ def test_route_pr_closed_only_when_merged_by_bot() -> None: ) assert fallback.should_queue assert fallback.task == "cleanup_workspace" - assert fallback.issue_key == "octo/widget#pr-9" + assert fallback.issue_key == "octo/widget#9" assert fallback.submitter is None payload["pull_request"]["merged"] = False # type: ignore[index] diff --git a/tests/test_server.py b/tests/test_server.py index 278300c9c..a0b79681c 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -773,20 +773,20 @@ def test_webhook_rate_limits_unknown_submitter_at_default_cap(rate_limited_setti assert states == ["queued", "queued", "skipped"] -def test_webhook_unmapped_pr_comment_does_not_consume_submitter_budget( +def test_webhook_unmapped_pr_comment_queues_with_pr_key_and_counts_budget( rate_limited_settings: Settings, ) -> None: app = create_app(rate_limited_settings) with TestClient(app) as client: - skipped = _post_pr_issue_comment( + queued = _post_pr_issue_comment( client, delivery="pr-unmapped", user="stranger", pr_number=900, association="NONE", ) - assert skipped.status_code == 202 - assert skipped.json()["state"] == "skipped" + assert queued.status_code == 202 + assert queued.json()["state"] == "queued" states = [] for i in range(3): @@ -805,10 +805,9 @@ def test_webhook_unmapped_pr_comment_does_not_consume_submitter_budget( close_database() assert unmapped is not None - assert unmapped.issue_key is None - assert unmapped.last_error is not None - assert "not mapped" in unmapped.last_error - assert states == ["queued", "queued", "skipped"] + assert unmapped.issue_key == "octo/widget#900" + assert unmapped.last_error is None + assert states == ["queued", "skipped", "skipped"] def test_webhook_contributor_gets_higher_cap(rate_limited_settings: Settings) -> None: @@ -1403,6 +1402,166 @@ def stub_run_task(monkeypatch: pytest.MonkeyPatch) -> list[dict]: return captured +async def test_handle_pr_conversation_unmapped_bot_pr_uses_pr_branch( + settings: Settings, tmp_path: Path, stub_run_task, monkeypatch +) -> None: + from robomp import tasks + from robomp.github_client import IssueInfo, PullRequestInfo, RepoInfo + + sandbox = _RecordingSandbox(tmp_path) + db = get_database(settings.sqlite_path) + repo = RepoInfo( + full_name="octo/widget", default_branch="main", clone_url="https://github.com/octo/widget.git", private=False + ) + pr_issue = IssueInfo( + repo="octo/widget", + number=900, + title="Fix flaky parser", + body="PR body", + state="open", + author=settings.bot_login, + labels=(), + is_pull_request=True, + ) + pr_info = PullRequestInfo( + repo="octo/widget", + number=900, + html_url="https://github.com/octo/widget/pull/900", + head_ref="farm/abc12345/fix-flaky-parser", + base_ref="main", + state="open", + author=settings.bot_login, + head_repo="octo/widget", + ) + + async def _get_pull_request(self, repo_full: str, number: int): + assert repo_full == "octo/widget" + assert number == 900 + return pr_info + + async def _get_repo(self, repo_full: str): + assert repo_full == "octo/widget" + return repo + + async def _get_issue(self, repo_full: str, number: int): + assert repo_full == "octo/widget" + assert number == 900 + return pr_issue + + monkeypatch.setattr(GitHubClient, "get_pull_request", _get_pull_request) + monkeypatch.setattr(GitHubClient, "get_repo", _get_repo) + monkeypatch.setattr(GitHubClient, "get_issue", _get_issue) + + payload = { + "action": "created", + "issue": {"number": 900, "pull_request": {"url": "https://api.github.com/repos/octo/widget/pulls/900"}}, + "comment": {"user": {"login": "can1357"}, "body": "please fix", "id": 10, "created_at": "2026-05-15T00:00:00Z"}, + "repository": {"full_name": "octo/widget"}, + } + await tasks.handle_pr_conversation( + settings=settings, + db=db, + github=GitHubClient("t"), + git_transport=LocalGitTransport(token=None), + sandbox=sandbox, + payload=payload, + delivery_id="test-pr-direct", + ) + + assert len(stub_run_task) == 1 + call = stub_run_task[0] + assert call["task_kind"] == "handle_comment" + assert call["pr_number"] == 900 + assert call["inputs"].issue.is_pull_request is True + assert sandbox.ensure_calls[0]["number"] == 900 + assert sandbox.ensure_calls[0]["existing_branch"] == "farm/abc12345/fix-flaky-parser" + row = db.get_issue("octo/widget#900") + assert row is not None + assert row.pr_number == 900 + assert row.branch == "farm/abc12345/fix-flaky-parser" + close_database() + + +async def test_handle_pr_conversation_repairs_missing_pr_mapping_from_branch( + settings: Settings, tmp_path: Path, stub_run_task, monkeypatch +) -> None: + from robomp import tasks + from robomp.github_client import IssueInfo, PullRequestInfo, RepoInfo + + sandbox = _RecordingSandbox(tmp_path) + db = get_database(settings.sqlite_path) + branch = "farm/abc12345/fix-flaky-parser" + db.upsert_issue(key="octo/widget#42", repo="octo/widget", number=42, state="opened", branch=branch) + repo = RepoInfo( + full_name="octo/widget", default_branch="main", clone_url="https://github.com/octo/widget.git", private=False + ) + issue = IssueInfo( + repo="octo/widget", + number=42, + title="Parser is flaky", + body="issue body", + state="open", + author="alice", + labels=(), + is_pull_request=False, + ) + pr_info = PullRequestInfo( + repo="octo/widget", + number=900, + html_url="https://github.com/octo/widget/pull/900", + head_ref=branch, + base_ref="main", + state="open", + author=settings.bot_login, + head_repo="octo/widget", + ) + + async def _get_pull_request(self, repo_full: str, number: int): + assert repo_full == "octo/widget" + assert number == 900 + return pr_info + + async def _get_repo(self, repo_full: str): + assert repo_full == "octo/widget" + return repo + + async def _get_issue(self, repo_full: str, number: int): + assert repo_full == "octo/widget" + assert number == 42 + return issue + + monkeypatch.setattr(GitHubClient, "get_pull_request", _get_pull_request) + monkeypatch.setattr(GitHubClient, "get_repo", _get_repo) + monkeypatch.setattr(GitHubClient, "get_issue", _get_issue) + + payload = { + "action": "created", + "issue": {"number": 900, "pull_request": {"url": "https://api.github.com/repos/octo/widget/pulls/900"}}, + "comment": {"user": {"login": "can1357"}, "body": "please fix", "id": 11, "created_at": "2026-05-15T00:00:00Z"}, + "repository": {"full_name": "octo/widget"}, + } + await tasks.handle_pr_conversation( + settings=settings, + db=db, + github=GitHubClient("t"), + git_transport=LocalGitTransport(token=None), + sandbox=sandbox, + payload=payload, + delivery_id="test-pr-repair", + ) + + assert len(stub_run_task) == 1 + call = stub_run_task[0] + assert call["inputs"].issue.number == 42 + assert call["pr_number"] == 900 + assert sandbox.ensure_calls[0]["number"] == 42 + assert sandbox.ensure_calls[0]["existing_branch"] == branch + row = db.get_issue("octo/widget#42") + assert row is not None + assert row.pr_number == 900 + close_database() + + async def test_handle_comment_directive_bootstraps_untriaged_issue( settings: Settings, tmp_path: Path, stub_run_task, monkeypatch ) -> None: