diff --git a/src/clayde/github.py b/src/clayde/github.py index 8860951..3ab73ee 100644 --- a/src/clayde/github.py +++ b/src/clayde/github.py @@ -57,7 +57,12 @@ def get_default_branch(g: Github, owner: str, repo: str) -> str: def get_assigned_issues(g: Github) -> list: - """Return all open issues assigned to the authenticated user.""" + """Return all open issues AND PRs assigned to the authenticated user. + + GitHub models PRs as issues — each item that is a PR will have + ``html_url`` containing ``/pull/``. Use ``is_pull_request_item()`` to + distinguish them. + """ try: return list(g.get_user().get_issues(filter="assigned", state="open")) except GithubException as e: @@ -65,6 +70,11 @@ def get_assigned_issues(g: Github) -> list: return [] +def is_pull_request_item(item) -> bool: + """Return True if an item from get_assigned_issues() is a pull request.""" + return "/pull/" in item.html_url + + def find_open_pr(g: Github, owner: str, repo: str, branch_name: str) -> str | None: """Return the HTML URL of an open PR for the given branch, or None.""" pulls = list(_get_repo(g, owner, repo).get_pulls( diff --git a/src/clayde/orchestrator.py b/src/clayde/orchestrator.py index 209d060..e9cbaef 100644 --- a/src/clayde/orchestrator.py +++ b/src/clayde/orchestrator.py @@ -37,13 +37,14 @@ get_pr_review_comments, get_pr_reviews, is_blocked, + is_pull_request_item, issue_ref, parse_issue_url, parse_pr_url, ) -from clayde.safety import get_new_visible_comments, has_visible_content +from clayde.safety import filter_pr_reviews, get_new_visible_comments, has_visible_content from clayde.state import get_issue_state, load_state, save_state, update_issue_state -from clayde.tasks import work +from clayde.tasks import pr_work, work from clayde.telemetry import get_tracer, init_tracer log = logging.getLogger("clayde.orchestrator") @@ -173,6 +174,95 @@ def _handle_issue(g: Github, issue: Issue, url: str) -> None: log.info("[%s] Cycle complete", label) +def _handle_standalone_pr(g: Github, url: str) -> None: + """Handle an assigned PR that has no parent tracked issue. + + Checks for new review activity from whitelisted users and invokes Claude + to address it. State is keyed by the PR url. + """ + tracer = get_tracer() + with tracer.start_as_current_span("clayde.handle_standalone_pr", attributes={"pr.url": url}) as span: + owner, repo, pr_number = parse_pr_url(url) + ref = issue_ref(owner, repo, pr_number) + + pr_state = get_issue_state(url) + in_progress = pr_state.get("in_progress", False) + last_seen_at = _parse_timestamp(pr_state.get("last_seen_at")) + + # Check for new review activity from whitelisted users + has_new_review_activity = False + try: + reviews = get_pr_reviews(g, owner, repo, pr_number) + review_comments = get_pr_review_comments(g, owner, repo, pr_number) + github_username = get_settings().github_username + visible_reviews = filter_pr_reviews(reviews, github_username) + + if last_seen_at is not None: + new_reviews = [r for r in visible_reviews if r.submitted_at > last_seen_at] + else: + new_reviews = list(visible_reviews) + + if new_reviews: + new_review_ids = {r.id for r in new_reviews} + has_inline = any( + rc.pull_request_review_id in new_review_ids + for rc in review_comments + ) + has_bodies = any(r.body and r.body.strip() for r in new_reviews) + if has_inline or has_bodies: + has_new_review_activity = True + else: + # Pure approval with no comments — update timestamp only + log.info("[%s] Pure PR approval — updating last_seen_at", ref) + update_issue_state(url, {"last_seen_at": _now_utc()}) + span.set_attribute("pr.skip_reason", "pure_approval") + return + except Exception as e: + log.warning("[%s] Failed to check PR reviews: %s", ref, e) + + should_invoke = in_progress or has_new_review_activity + + if not should_invoke: + log.info("[%s] No new review activity — skipping", ref) + span.set_attribute("pr.skip_reason", "no_new_activity") + return + + # Mark in_progress before invoking Claude + update_issue_state(url, {"in_progress": True, "owner": owner, "repo": repo, "number": pr_number}) + + log.info("[%s] New review activity — invoking PR work task", ref) + try: + pr_work.run(url) + except (UsageLimitError, InvocationTimeoutError) as e: + log.warning("[%s] Usage/timeout limit — will retry next cycle: %s", ref, e) + span.set_attribute("pr.status", "retry") + return + except Exception as e: + log.error("[%s] ERROR in PR work task: %s", ref, e) + span.set_status(StatusCode.ERROR, str(e)) + span.record_exception(e) + update_issue_state(url, {"in_progress": False}) + return + + update_issue_state(url, {"in_progress": False, "last_seen_at": _now_utc()}) + span.set_attribute("pr.status", "completed") + log.info("[%s] PR cycle complete", ref) + + +def _is_pr_tracked_as_issue(pr_url: str, issues_state: dict) -> bool: + """Return True if pr_url is already tracked as the child PR of a known issue. + + When Clayde opens a PR while working on an issue, the PR URL is stored + under the *issue* state entry as ``pr_url``. In that case the issue + handler owns review activity for the PR, so the standalone-PR handler + should skip it. + """ + return any( + ist.get("pr_url") == pr_url + for ist in issues_state.values() + ) + + def _prune_closed_issues(g: Github, issues_state: dict) -> None: """Remove closed issues from state to prevent stale entries accumulating.""" to_prune = [] @@ -240,17 +330,26 @@ def main(): issues_state = load_state().get("issues", {}) if not assigned: - log.info("No assigned issues. Going back to sleep.") + log.info("No assigned work items. Going back to sleep.") provider.force_flush() return - processed = 0 - for issue in assigned: - url = issue.html_url - processed += 1 - _handle_issue(g, issue, url) - - tick_span.set_attribute("issues.processed", processed) + issues_count = 0 + prs_count = 0 + for item in assigned: + url = item.html_url + if is_pull_request_item(item): + # Only handle PRs that are NOT already tracked as children of a + # known issue (those are handled via _handle_issue). + if not _is_pr_tracked_as_issue(url, issues_state): + prs_count += 1 + _handle_standalone_pr(g, url) + else: + issues_count += 1 + _handle_issue(g, item, url) + + tick_span.set_attribute("issues.processed", issues_count) + tick_span.set_attribute("prs.processed", prs_count) provider.force_flush() diff --git a/src/clayde/prompts/pr_work.j2 b/src/clayde/prompts/pr_work.j2 new file mode 100644 index 0000000..f0d8dc1 --- /dev/null +++ b/src/clayde/prompts/pr_work.j2 @@ -0,0 +1,34 @@ +You are Clayde, an autonomous software agent. You have been assigned to pull request #{{ number }}. + +PR: {{ owner }}/{{ repo }}#{{ number }}: {{ title }} +PR URL: {{ pr_url }} +PR BRANCH: {{ branch_name }} + +PR DESCRIPTION: +{{ body }} + +PR REVIEWS: +{{ review_text }} + +REPOSITORY ON DISK: {{ repo_path }} + +--- + +Address the review feedback on this pull request. The PR is already open on branch `{{ branch_name }}`. + +Steps: +1. Ensure you are on branch `{{ branch_name }}` (check out or pull it as needed). +2. Read the review comments carefully and implement all requested changes. +3. Run the test suite to confirm nothing is broken. +4. Commit your changes with a clear message. +5. Push: git push origin {{ branch_name }} + +Do NOT open a new pull request — the existing PR {{ pr_url }} will automatically update when you push to `{{ branch_name }}`. + +After completing your work, provide a short summary of what you did. + +IMPORTANT: Your final response must be ONLY a raw JSON object — nothing else. +Do not include any text before or after the JSON. Do not wrap it in markdown code fences. +Your entire response must be parseable by json.loads(). + +{"summary": ""} diff --git a/src/clayde/safety.py b/src/clayde/safety.py index 8c7d99b..5a259c3 100644 --- a/src/clayde/safety.py +++ b/src/clayde/safety.py @@ -57,6 +57,19 @@ def get_new_visible_comments(comments: list, last_seen_at: datetime | None) -> l ] +def filter_pr_reviews(reviews: list, github_username: str) -> list: + """Return only PR reviews from whitelisted users, excluding the bot's own. + + A review is visible if the reviewer is in the whitelist and is not the + authenticated bot account. + """ + whitelist = get_settings().whitelisted_users_list + return [ + r for r in reviews + if r.user.login in whitelist and r.user.login != github_username + ] + + def has_visible_content(issue, comments: list) -> bool: """Return True if there is any visible content (issue body or comments). diff --git a/src/clayde/tasks/pr_work.py b/src/clayde/tasks/pr_work.py new file mode 100644 index 0000000..929c452 --- /dev/null +++ b/src/clayde/tasks/pr_work.py @@ -0,0 +1,125 @@ +"""PR work task — address review comments on a standalone assigned PR. + +A "standalone PR" is a pull request assigned to Clayde that has no originating +issue in the plan → implement → PR lifecycle. State is keyed by the PR URL +rather than an issue URL. +""" + +import logging + +from clayde.claude import format_cost_line, invoke_claude +from clayde.config import get_github_client, get_settings +from clayde.git import ensure_repo +from clayde.github import ( + get_default_branch, + get_pr_review_comments, + get_pr_reviews, + issue_ref, + parse_pr_url, + post_comment, +) +from clayde.prompts import render_template +from clayde.responses import WorkResponse, parse_response +from clayde.safety import filter_pr_reviews +from clayde.state import get_issue_state, update_issue_state +from clayde.telemetry import get_tracer + +log = logging.getLogger("clayde.tasks.pr_work") + + +def _format_reviews(reviews: list, review_comments: list) -> str: + """Format PR reviews and inline comments into text for the prompt.""" + parts = [] + for review in reviews: + header = f"Review by @{review.user.login} (state: {review.state}):" + inline = [rc for rc in review_comments if rc.pull_request_review_id == review.id] + has_body = review.body and review.body.strip() + if has_body or inline: + parts.append(f"{header}\n{review.body}" if has_body else header) + for rc in inline: + file_info = f" File: {rc.path}" + if hasattr(rc, "line") and rc.line: + file_info += f", line {rc.line}" + parts.append(f"{file_info}\n {rc.body}") + return "\n---\n".join(parts) or "(none)" + + +def run(pr_url: str) -> None: + """Fetch PR context and invoke Claude to address review comments. + + State is keyed by *pr_url* (not an issue URL). Raises UsageLimitError or + InvocationTimeoutError on rate/timeout limits so the orchestrator can + leave in_progress=True for automatic retry. + """ + tracer = get_tracer() + with tracer.start_as_current_span("clayde.task.pr_work") as span: + g = get_github_client() + owner, repo, pr_number = parse_pr_url(pr_url) + ref = issue_ref(owner, repo, pr_number) + span.set_attribute("pr.number", pr_number) + span.set_attribute("pr.owner", owner) + span.set_attribute("pr.repo", repo) + + repo_obj = g.get_repo(f"{owner}/{repo}") + pr = repo_obj.get_pull(pr_number) + title = pr.title + body = pr.body or "(empty)" + branch_name = pr.head.ref + default_branch = get_default_branch(g, owner, repo) + + # Fetch and whitelist-filter reviews + settings = get_settings() + github_username = settings.github_username + reviews = get_pr_reviews(g, owner, repo, pr_number) + review_comments = get_pr_review_comments(g, owner, repo, pr_number) + visible_reviews = filter_pr_reviews(reviews, github_username) + review_text = _format_reviews(visible_reviews, review_comments) + + # Persist metadata before invoking Claude + update_issue_state(pr_url, { + "owner": owner, + "repo": repo, + "number": pr_number, + "pr_title": title, + "branch_name": branch_name, + "is_standalone_pr": True, + }) + + repo_path = ensure_repo(owner, repo, default_branch) + + prompt = render_template( + "pr_work.j2", + number=pr_number, + title=title, + owner=owner, + repo=repo, + body=body, + review_text=review_text, + repo_path=repo_path, + branch_name=branch_name, + pr_url=pr_url, + default_branch=default_branch, + ) + + log.info("[%s: %s] Invoking Claude for PR review", ref, title) + + # UsageLimitError/InvocationTimeoutError propagate to the orchestrator + result = invoke_claude(prompt, repo_path) + + span.set_attribute("pr_work.output_length", len(result.output or "")) + + # Parse summary (best-effort; fall back to raw output snippet) + summary = None + try: + parsed = parse_response(result.output, WorkResponse) + summary = parsed.summary + except ValueError: + log.warning("[%s: %s] Failed to parse PR work response JSON — using raw output", + ref, title) + summary = (result.output or "").strip()[:500] or None + + if summary: + post_comment(g, owner, repo, pr_number, + f"{summary}{format_cost_line(result.cost_eur)}") + + log.info("[%s: %s] PR work complete", ref, title) diff --git a/tests/test_github.py b/tests/test_github.py index 4267bdf..0eac3c2 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -20,6 +20,7 @@ get_pr_review_comments, get_pr_reviews, is_blocked, + is_pull_request_item, parse_issue_url, parse_pr_url, post_comment, @@ -54,6 +55,23 @@ def test_invalid_url_raises(self): parse_pr_url("https://github.com/alice/repo/issues/1") +class TestIsPullRequestItem: + def test_returns_true_for_pr_url(self): + item = MagicMock() + item.html_url = "https://github.com/o/r/pull/80" + assert is_pull_request_item(item) is True + + def test_returns_false_for_issue_url(self): + item = MagicMock() + item.html_url = "https://github.com/o/r/issues/1" + assert is_pull_request_item(item) is False + + def test_returns_false_for_arbitrary_url(self): + item = MagicMock() + item.html_url = "https://github.com/o/r" + assert is_pull_request_item(item) is False + + class TestFetchIssue: def test_calls_correct_api(self): g = MagicMock() diff --git a/tests/test_orchestrator.py b/tests/test_orchestrator.py index 86f6732..50a8063 100644 --- a/tests/test_orchestrator.py +++ b/tests/test_orchestrator.py @@ -7,6 +7,8 @@ from clayde.orchestrator import ( _handle_issue, + _handle_standalone_pr, + _is_pr_tracked_as_issue, _now_utc, _parse_timestamp, _prune_closed_issues, @@ -355,6 +357,237 @@ def test_prunes_multiple_closed_in_one_save(self): assert url2 not in saved["issues"] +class TestIsPrTrackedAsIssue: + def test_returns_true_when_pr_url_matches_issue_state(self): + issues_state = { + "https://github.com/o/r/issues/1": { + "pr_url": "https://github.com/o/r/pull/5" + } + } + assert _is_pr_tracked_as_issue("https://github.com/o/r/pull/5", issues_state) + + def test_returns_false_when_no_match(self): + issues_state = { + "https://github.com/o/r/issues/1": { + "pr_url": "https://github.com/o/r/pull/5" + } + } + assert not _is_pr_tracked_as_issue("https://github.com/o/r/pull/99", issues_state) + + def test_returns_false_for_empty_state(self): + assert not _is_pr_tracked_as_issue("https://github.com/o/r/pull/5", {}) + + def test_returns_false_when_state_has_no_pr_url(self): + issues_state = { + "https://github.com/o/r/issues/1": {"owner": "o", "repo": "r", "number": 1} + } + assert not _is_pr_tracked_as_issue("https://github.com/o/r/pull/5", issues_state) + + +class TestMainDispatch: + """Tests that main() correctly dispatches issues vs standalone PRs.""" + + def test_dispatches_issue_to_handle_issue(self): + item = MagicMock() + item.html_url = "https://github.com/o/r/issues/1" + with patch("clayde.orchestrator.get_settings", return_value=_mock_settings(enabled=True)), \ + patch("clayde.orchestrator.setup_logging"), \ + patch("clayde.orchestrator.init_tracer"), \ + patch("clayde.orchestrator.is_claude_available", return_value=True), \ + patch("clayde.orchestrator.get_github_client"), \ + patch("clayde.orchestrator.get_assigned_issues", return_value=[item]), \ + patch("clayde.orchestrator.load_state", return_value={"issues": {}}), \ + patch("clayde.orchestrator._prune_closed_issues"), \ + patch("clayde.orchestrator.is_pull_request_item", return_value=False), \ + patch("clayde.orchestrator._handle_issue") as mock_issue, \ + patch("clayde.orchestrator._handle_standalone_pr") as mock_pr: + main() + mock_issue.assert_called_once() + mock_pr.assert_not_called() + + def test_dispatches_standalone_pr_to_handle_standalone_pr(self): + item = MagicMock() + item.html_url = "https://github.com/o/r/pull/80" + with patch("clayde.orchestrator.get_settings", return_value=_mock_settings(enabled=True)), \ + patch("clayde.orchestrator.setup_logging"), \ + patch("clayde.orchestrator.init_tracer"), \ + patch("clayde.orchestrator.is_claude_available", return_value=True), \ + patch("clayde.orchestrator.get_github_client"), \ + patch("clayde.orchestrator.get_assigned_issues", return_value=[item]), \ + patch("clayde.orchestrator.load_state", return_value={"issues": {}}), \ + patch("clayde.orchestrator._prune_closed_issues"), \ + patch("clayde.orchestrator.is_pull_request_item", return_value=True), \ + patch("clayde.orchestrator._is_pr_tracked_as_issue", return_value=False), \ + patch("clayde.orchestrator._handle_issue") as mock_issue, \ + patch("clayde.orchestrator._handle_standalone_pr") as mock_pr: + main() + mock_issue.assert_not_called() + mock_pr.assert_called_once() + + def test_skips_pr_already_tracked_as_issue(self): + item = MagicMock() + item.html_url = "https://github.com/o/r/pull/5" + with patch("clayde.orchestrator.get_settings", return_value=_mock_settings(enabled=True)), \ + patch("clayde.orchestrator.setup_logging"), \ + patch("clayde.orchestrator.init_tracer"), \ + patch("clayde.orchestrator.is_claude_available", return_value=True), \ + patch("clayde.orchestrator.get_github_client"), \ + patch("clayde.orchestrator.get_assigned_issues", return_value=[item]), \ + patch("clayde.orchestrator.load_state", return_value={"issues": {}}), \ + patch("clayde.orchestrator._prune_closed_issues"), \ + patch("clayde.orchestrator.is_pull_request_item", return_value=True), \ + patch("clayde.orchestrator._is_pr_tracked_as_issue", return_value=True), \ + patch("clayde.orchestrator._handle_issue") as mock_issue, \ + patch("clayde.orchestrator._handle_standalone_pr") as mock_pr: + main() + mock_issue.assert_not_called() + mock_pr.assert_not_called() + + +class TestHandleStandalonePr: + PR_URL = "https://github.com/o/r/pull/80" + + def _mock_settings(self, username="ClaydeCode"): + s = MagicMock() + s.github_username = username + return s + + def test_skips_when_no_review_activity(self): + ts = "2024-01-01T12:00:00+00:00" + with patch("clayde.orchestrator.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.orchestrator.get_issue_state", return_value={"last_seen_at": ts}), \ + patch("clayde.orchestrator.get_pr_reviews", return_value=[]), \ + patch("clayde.orchestrator.get_pr_review_comments", return_value=[]), \ + patch("clayde.orchestrator.filter_pr_reviews", return_value=[]), \ + patch("clayde.orchestrator.get_settings", return_value=self._mock_settings()), \ + patch("clayde.orchestrator.pr_work") as mock_pr_work: + _handle_standalone_pr(MagicMock(), self.PR_URL) + mock_pr_work.run.assert_not_called() + + def test_invokes_pr_work_on_new_review(self): + ts = "2024-01-01T12:00:00+00:00" + review = MagicMock() + review.submitted_at = datetime(2024, 2, 1, tzinfo=timezone.utc) + review.body = "Please fix this" + review.id = 1 + with patch("clayde.orchestrator.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.orchestrator.get_issue_state", return_value={"last_seen_at": ts}), \ + patch("clayde.orchestrator.get_pr_reviews", return_value=[review]), \ + patch("clayde.orchestrator.get_pr_review_comments", return_value=[]), \ + patch("clayde.orchestrator.filter_pr_reviews", return_value=[review]), \ + patch("clayde.orchestrator.get_settings", return_value=self._mock_settings()), \ + patch("clayde.orchestrator.update_issue_state"), \ + patch("clayde.orchestrator.pr_work") as mock_pr_work: + _handle_standalone_pr(MagicMock(), self.PR_URL) + mock_pr_work.run.assert_called_once_with(self.PR_URL) + + def test_invokes_on_first_time_with_reviews(self): + """No last_seen_at + reviews present → invoke.""" + review = MagicMock() + review.body = "Add tests" + review.id = 42 + with patch("clayde.orchestrator.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.orchestrator.get_issue_state", return_value={}), \ + patch("clayde.orchestrator.get_pr_reviews", return_value=[review]), \ + patch("clayde.orchestrator.get_pr_review_comments", return_value=[]), \ + patch("clayde.orchestrator.filter_pr_reviews", return_value=[review]), \ + patch("clayde.orchestrator.get_settings", return_value=self._mock_settings()), \ + patch("clayde.orchestrator.update_issue_state"), \ + patch("clayde.orchestrator.pr_work") as mock_pr_work: + _handle_standalone_pr(MagicMock(), self.PR_URL) + mock_pr_work.run.assert_called_once_with(self.PR_URL) + + def test_skips_on_first_time_with_no_reviews(self): + """No last_seen_at + no reviews → skip (nothing to address yet).""" + with patch("clayde.orchestrator.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.orchestrator.get_issue_state", return_value={}), \ + patch("clayde.orchestrator.get_pr_reviews", return_value=[]), \ + patch("clayde.orchestrator.get_pr_review_comments", return_value=[]), \ + patch("clayde.orchestrator.filter_pr_reviews", return_value=[]), \ + patch("clayde.orchestrator.get_settings", return_value=self._mock_settings()), \ + patch("clayde.orchestrator.pr_work") as mock_pr_work: + _handle_standalone_pr(MagicMock(), self.PR_URL) + mock_pr_work.run.assert_not_called() + + def test_retries_when_in_progress(self): + """in_progress=True → always invoke (crash recovery).""" + ts = "2024-01-01T12:00:00+00:00" + with patch("clayde.orchestrator.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.orchestrator.get_issue_state", + return_value={"last_seen_at": ts, "in_progress": True}), \ + patch("clayde.orchestrator.get_pr_reviews", return_value=[]), \ + patch("clayde.orchestrator.get_pr_review_comments", return_value=[]), \ + patch("clayde.orchestrator.filter_pr_reviews", return_value=[]), \ + patch("clayde.orchestrator.get_settings", return_value=self._mock_settings()), \ + patch("clayde.orchestrator.update_issue_state"), \ + patch("clayde.orchestrator.pr_work") as mock_pr_work: + _handle_standalone_pr(MagicMock(), self.PR_URL) + mock_pr_work.run.assert_called_once_with(self.PR_URL) + + def test_pure_approval_updates_timestamp_only(self): + """Review with no body/comments but approval state → update last_seen_at, no Claude.""" + ts = "2024-01-01T12:00:00+00:00" + review = MagicMock() + review.submitted_at = datetime(2024, 2, 1, tzinfo=timezone.utc) + review.body = "" + review.id = 1 + calls = [] + with patch("clayde.orchestrator.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.orchestrator.get_issue_state", return_value={"last_seen_at": ts}), \ + patch("clayde.orchestrator.get_pr_reviews", return_value=[review]), \ + patch("clayde.orchestrator.get_pr_review_comments", return_value=[]), \ + patch("clayde.orchestrator.filter_pr_reviews", return_value=[review]), \ + patch("clayde.orchestrator.get_settings", return_value=self._mock_settings()), \ + patch("clayde.orchestrator.update_issue_state", + side_effect=lambda url, upd: calls.append(upd)), \ + patch("clayde.orchestrator.pr_work") as mock_pr_work: + _handle_standalone_pr(MagicMock(), self.PR_URL) + mock_pr_work.run.assert_not_called() + assert any("last_seen_at" in c for c in calls) + + def test_clears_in_progress_on_unexpected_error(self): + ts = "2024-01-01T12:00:00+00:00" + review = MagicMock() + review.submitted_at = datetime(2024, 2, 1, tzinfo=timezone.utc) + review.body = "Fix it" + review.id = 1 + calls = [] + with patch("clayde.orchestrator.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.orchestrator.get_issue_state", return_value={"last_seen_at": ts}), \ + patch("clayde.orchestrator.get_pr_reviews", return_value=[review]), \ + patch("clayde.orchestrator.get_pr_review_comments", return_value=[]), \ + patch("clayde.orchestrator.filter_pr_reviews", return_value=[review]), \ + patch("clayde.orchestrator.get_settings", return_value=self._mock_settings()), \ + patch("clayde.orchestrator.update_issue_state", + side_effect=lambda url, upd: calls.append(upd)), \ + patch("clayde.orchestrator.pr_work") as mock_pr_work: + mock_pr_work.run.side_effect = RuntimeError("boom") + _handle_standalone_pr(MagicMock(), self.PR_URL) + assert any(c.get("in_progress") is False for c in calls) + + def test_leaves_in_progress_on_usage_limit(self): + from clayde.claude import UsageLimitError + ts = "2024-01-01T12:00:00+00:00" + review = MagicMock() + review.submitted_at = datetime(2024, 2, 1, tzinfo=timezone.utc) + review.body = "Fix it" + review.id = 1 + calls = [] + with patch("clayde.orchestrator.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.orchestrator.get_issue_state", return_value={"last_seen_at": ts}), \ + patch("clayde.orchestrator.get_pr_reviews", return_value=[review]), \ + patch("clayde.orchestrator.get_pr_review_comments", return_value=[]), \ + patch("clayde.orchestrator.filter_pr_reviews", return_value=[review]), \ + patch("clayde.orchestrator.get_settings", return_value=self._mock_settings()), \ + patch("clayde.orchestrator.update_issue_state", + side_effect=lambda url, upd: calls.append(upd)), \ + patch("clayde.orchestrator.pr_work") as mock_pr_work: + mock_pr_work.run.side_effect = UsageLimitError("limit", cost_eur=0.0) + _handle_standalone_pr(MagicMock(), self.PR_URL) + # Should NOT clear in_progress + assert not any(c.get("in_progress") is False for c in calls) + + def test_run_loop_without_pebble_uses_legacy_path(monkeypatch): """When pebble_enabled is False, run_loop must use the existing sync path.""" from clayde import orchestrator diff --git a/tests/test_safety.py b/tests/test_safety.py index 8c474df..7affa9d 100644 --- a/tests/test_safety.py +++ b/tests/test_safety.py @@ -6,6 +6,7 @@ from clayde.safety import ( _has_whitelisted_reaction, filter_comments, + filter_pr_reviews, get_new_visible_comments, has_visible_content, is_comment_visible, @@ -189,6 +190,42 @@ def test_excludes_clayde_own_comments_with_last_seen(self): assert clayde_c not in result +class TestFilterPrReviews: + def _make_review(self, login): + r = MagicMock() + r.user.login = login + return r + + def test_includes_whitelisted_reviewer(self): + review = self._make_review("alice") + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + result = filter_pr_reviews([review], "ClaydeCode") + assert result == [review] + + def test_excludes_non_whitelisted_reviewer(self): + review = self._make_review("mallory") + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + result = filter_pr_reviews([review], "ClaydeCode") + assert result == [] + + def test_excludes_bot_own_review(self): + review = self._make_review("ClaydeCode") + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice", "ClaydeCode"])): + result = filter_pr_reviews([review], "ClaydeCode") + assert result == [] + + def test_keeps_whitelisted_non_bot(self): + alice = self._make_review("alice") + bot = self._make_review("ClaydeCode") + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice", "ClaydeCode"])): + result = filter_pr_reviews([alice, bot], "ClaydeCode") + assert result == [alice] + + def test_empty_input(self): + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + assert filter_pr_reviews([], "ClaydeCode") == [] + + class TestHasWhitelistedReaction: def test_matching_reaction(self): reactions = [_make_reaction("+1", "alice")] diff --git a/tests/test_tasks_pr_work.py b/tests/test_tasks_pr_work.py new file mode 100644 index 0000000..25eec5b --- /dev/null +++ b/tests/test_tasks_pr_work.py @@ -0,0 +1,200 @@ +"""Tests for clayde.tasks.pr_work — standalone PR review task.""" + +from unittest.mock import MagicMock, patch + +from clayde.claude import InvocationResult, UsageLimitError +from clayde.tasks.pr_work import _format_reviews, run + + +def _make_result(output: str, cost_eur: float = 0.50) -> InvocationResult: + return InvocationResult(output=output, cost_eur=cost_eur, input_tokens=100, output_tokens=50) + + +def _mock_settings(github_username="ClaydeCode"): + s = MagicMock() + s.github_username = github_username + return s + + +def _make_pr(title="Fix review", body="Some description", branch="clayde/feat"): + pr = MagicMock() + pr.title = title + pr.body = body + pr.head.ref = branch + return pr + + +class TestRun: + PR_URL = "https://github.com/o/r/pull/80" + + def _base_patches(self, pr=None): + if pr is None: + pr = _make_pr() + repo_obj = MagicMock() + repo_obj.get_pull.return_value = pr + g = MagicMock() + g.get_repo.return_value = repo_obj + return g, pr + + def test_posts_summary_on_success(self): + g, pr = self._base_patches() + with patch("clayde.tasks.pr_work.get_github_client", return_value=g), \ + patch("clayde.tasks.pr_work.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.tasks.pr_work.get_default_branch", return_value="main"), \ + patch("clayde.tasks.pr_work.get_pr_reviews", return_value=[]), \ + patch("clayde.tasks.pr_work.get_pr_review_comments", return_value=[]), \ + patch("clayde.tasks.pr_work.filter_pr_reviews", return_value=[]), \ + patch("clayde.tasks.pr_work.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.pr_work.update_issue_state"), \ + patch("clayde.tasks.pr_work.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.tasks.pr_work.render_template", return_value="prompt"), \ + patch("clayde.tasks.pr_work.invoke_claude", + return_value=_make_result('{"summary": "Addressed review"}')), \ + patch("clayde.tasks.pr_work.post_comment") as mock_post: + run(self.PR_URL) + + mock_post.assert_called_once() + body = mock_post.call_args[0][4] + assert "Addressed review" in body + + def test_propagates_usage_limit_error(self): + import pytest + g, pr = self._base_patches() + with patch("clayde.tasks.pr_work.get_github_client", return_value=g), \ + patch("clayde.tasks.pr_work.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.tasks.pr_work.get_default_branch", return_value="main"), \ + patch("clayde.tasks.pr_work.get_pr_reviews", return_value=[]), \ + patch("clayde.tasks.pr_work.get_pr_review_comments", return_value=[]), \ + patch("clayde.tasks.pr_work.filter_pr_reviews", return_value=[]), \ + patch("clayde.tasks.pr_work.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.pr_work.update_issue_state"), \ + patch("clayde.tasks.pr_work.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.tasks.pr_work.render_template", return_value="prompt"), \ + patch("clayde.tasks.pr_work.invoke_claude", + side_effect=UsageLimitError("limit", cost_eur=0.0)): + with pytest.raises(UsageLimitError): + run(self.PR_URL) + + def test_persists_metadata_before_invoke(self): + g, pr = self._base_patches() + state_calls = [] + with patch("clayde.tasks.pr_work.get_github_client", return_value=g), \ + patch("clayde.tasks.pr_work.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.tasks.pr_work.get_default_branch", return_value="main"), \ + patch("clayde.tasks.pr_work.get_pr_reviews", return_value=[]), \ + patch("clayde.tasks.pr_work.get_pr_review_comments", return_value=[]), \ + patch("clayde.tasks.pr_work.filter_pr_reviews", return_value=[]), \ + patch("clayde.tasks.pr_work.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.pr_work.update_issue_state", + side_effect=lambda url, upd: state_calls.append((url, upd))), \ + patch("clayde.tasks.pr_work.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.tasks.pr_work.render_template", return_value="prompt"), \ + patch("clayde.tasks.pr_work.invoke_claude", + return_value=_make_result('{"summary": "done"}')), \ + patch("clayde.tasks.pr_work.post_comment"): + run(self.PR_URL) + + # metadata update should have happened + assert any("is_standalone_pr" in upd for _, upd in state_calls) + first_update = state_calls[0][1] + assert first_update.get("is_standalone_pr") is True + assert first_update.get("number") == 80 + + def test_falls_back_to_raw_output_on_json_parse_failure(self): + g, pr = self._base_patches() + with patch("clayde.tasks.pr_work.get_github_client", return_value=g), \ + patch("clayde.tasks.pr_work.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.tasks.pr_work.get_default_branch", return_value="main"), \ + patch("clayde.tasks.pr_work.get_pr_reviews", return_value=[]), \ + patch("clayde.tasks.pr_work.get_pr_review_comments", return_value=[]), \ + patch("clayde.tasks.pr_work.filter_pr_reviews", return_value=[]), \ + patch("clayde.tasks.pr_work.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.pr_work.update_issue_state"), \ + patch("clayde.tasks.pr_work.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.tasks.pr_work.render_template", return_value="prompt"), \ + patch("clayde.tasks.pr_work.invoke_claude", + return_value=_make_result("I pushed a fix to the branch.")), \ + patch("clayde.tasks.pr_work.post_comment") as mock_post: + run(self.PR_URL) + + mock_post.assert_called_once() + body = mock_post.call_args[0][4] + assert "I pushed a fix" in body + + def test_filters_reviews_by_whitelist(self): + g, pr = self._base_patches() + review = MagicMock() + review.id = 1 + review.user.login = "max-tet" + review.body = "Please fix" + visible = [review] + + captured = {} + + def capture_render(**kwargs): + captured.update(kwargs) + return "prompt" + + with patch("clayde.tasks.pr_work.get_github_client", return_value=g), \ + patch("clayde.tasks.pr_work.parse_pr_url", return_value=("o", "r", 80)), \ + patch("clayde.tasks.pr_work.get_default_branch", return_value="main"), \ + patch("clayde.tasks.pr_work.get_pr_reviews", return_value=[review]), \ + patch("clayde.tasks.pr_work.get_pr_review_comments", return_value=[]), \ + patch("clayde.tasks.pr_work.filter_pr_reviews", return_value=visible), \ + patch("clayde.tasks.pr_work.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.pr_work.update_issue_state"), \ + patch("clayde.tasks.pr_work.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.tasks.pr_work.render_template", + side_effect=lambda name, **kw: (captured.update(kw), "prompt")[1]), \ + patch("clayde.tasks.pr_work.invoke_claude", + return_value=_make_result('{"summary": "done"}')), \ + patch("clayde.tasks.pr_work.post_comment"): + run(self.PR_URL) + + # The review text should include the visible review + assert "@max-tet" in captured.get("review_text", "") + + +class TestFormatReviews: + def test_formats_review_with_body(self): + review = MagicMock() + review.id = 1 + review.user.login = "alice" + review.state = "CHANGES_REQUESTED" + review.body = "Please add tests" + + result = _format_reviews([review], []) + assert "@alice" in result + assert "CHANGES_REQUESTED" in result + assert "Please add tests" in result + + def test_formats_inline_comment(self): + review = MagicMock() + review.id = 1 + review.user.login = "alice" + review.state = "COMMENTED" + review.body = "" + + rc = MagicMock() + rc.pull_request_review_id = 1 + rc.path = "src/foo.py" + rc.line = 10 + rc.body = "Rename this" + + result = _format_reviews([review], [rc]) + assert "src/foo.py" in result + assert "10" in result + assert "Rename this" in result + + def test_returns_placeholder_for_empty(self): + assert _format_reviews([], []) == "(none)" + + def test_skips_review_with_no_body_or_comments(self): + review = MagicMock() + review.id = 1 + review.user.login = "alice" + review.state = "APPROVED" + review.body = "" + + result = _format_reviews([review], []) + assert result == "(none)"