feat: added PR fallback routing and followup comment context propagation
- Routed PR conversation and review-comment webhooks to `issue_key(repo, pr_number)` when origin mapping was missing. - Added PR-origin context via `origin` in persona outputs for `followup_comment` and `directive`. - Updated directive and follow-up prompt templates to use `origin.description` when origin issue is unavailable.
This commit is contained in:
+11
-18
@@ -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",
|
||||
|
||||
@@ -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),
|
||||
},
|
||||
)
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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}})
|
||||
|
||||
|
||||
+12
-10
@@ -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]
|
||||
|
||||
+167
-8
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user