diff --git a/CLAUDE.md b/CLAUDE.md index 93ad63b..84506b1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,23 +48,34 @@ src/clayde/ state.py # load_state(), save_state(), get_issue_state(), # update_issue_state() github.py # PyGitHub wrappers: parse_issue_url(), fetch_issue(), - # fetch_issue_comments(), post_comment(), fetch_comment(), - # get_default_branch(), get_assigned_issues(), - # extract_branch_name(), find_open_pr(), create_pull_request() + # fetch_issue_comments(), post_comment(), edit_comment(), + # fetch_comment(), get_default_branch(), + # get_assigned_issues(), extract_branch_name(), + # find_open_pr(), create_pull_request(), is_blocked(), + # add_pr_reviewer(), get_pr_reviews(), + # get_pr_review_comments(), parse_pr_url(), + # get_issue_author() git.py # ensure_repo() — clone or update repos under REPOS_DIR - safety.py # is_issue_authorized(), is_plan_approved() — safety gates + safety.py # Content filtering & plan approval: is_comment_visible(), + # filter_comments(), is_issue_visible(), + # has_visible_content(), is_plan_approved() claude.py # invoke_claude(prompt, repo_path) — Anthropic SDK with # tool-use loop (bash + text_editor) telemetry.py # OpenTelemetry tracing: init_tracer(), get_tracer(), # FileSpanExporter (JSONL) orchestrator.py # main() — single cycle, run_loop() — container entry point prompts/ - plan.j2 # Jinja2 template for plan prompt + preliminary_plan.j2 # Jinja2 template for short preliminary plan + thorough_plan.j2 # Jinja2 template for detailed thorough plan + update_plan.j2 # Jinja2 template for updating a plan on new comments implement.j2 # Jinja2 template for implement prompt + address_review.j2 # Jinja2 template for addressing PR review comments + plan.j2 # Legacy template (kept for reference) tasks/ __init__.py - plan.py # run(issue_url) — research + post plan comment - implement.py # run(issue_url) — implement + open PR + post result + plan.py # run_preliminary(url), run_thorough(url), run_update(url, phase) + implement.py # run(issue_url) — implement + open PR + assign reviewer + review.py # run(issue_url) — address PR review comments # Container paths /opt/clayde/ # application code (WORKDIR) @@ -102,32 +113,58 @@ Config is loaded via `get_settings()` (singleton). `GH_TOKEN` is exported at sta Issue lifecycle stored in `state.json` under `{"issues": {"": {...}}}`. ``` -(none) → planning → awaiting_approval → implementing → done - ↘ failed +(none) → preliminary_planning → awaiting_preliminary_approval + → planning → awaiting_plan_approval → implementing → pr_open → done + ↘ failed ``` +New comments in `awaiting_preliminary_approval` or `awaiting_plan_approval` +trigger plan updates (edit existing plan comment + post change summary). + +PR reviews in `pr_open` trigger `addressing_review` → back to `pr_open`. + | Status | Meaning | |--------|---------| -| `planning` | Claude is being invoked for the plan phase (skip in next cron tick) | -| `awaiting_approval` | Plan posted as comment; waiting for 👍 | -| `implementing` | Claude is being invoked for implementation (skip in next cron tick) | -| `done` | PR opened; issue complete | -| `failed` | Error during plan or implement; cleared manually to retry | -| `interrupted` | Claude usage/rate limit hit mid-task; retried automatically each cron tick | - -State entries also store: `owner`, `repo`, `number`, `plan_comment_id`, `pr_url`, `branch_name`. -Interrupted entries also store: `interrupted_phase` (`"planning"` or `"implementing"`). +| `preliminary_planning` | Claude is producing a short preliminary plan | +| `awaiting_preliminary_approval` | Preliminary plan posted; waiting for 👍 | +| `planning` | Claude is producing a thorough implementation plan | +| `awaiting_plan_approval` | Thorough plan posted; waiting for 👍 | +| `implementing` | Claude is implementing the approved plan | +| `pr_open` | PR exists; monitoring for review comments | +| `addressing_review` | Claude is addressing review comments | +| `done` | PR approved or complete; issue finished | +| `failed` | Error during any phase; cleared manually to retry | +| `interrupted` | Claude usage/rate limit hit mid-task; retried automatically | + +State entries store: `owner`, `repo`, `number`, `preliminary_comment_id`, +`plan_comment_id`, `pr_url`, `branch_name`, `last_seen_comment_id`, +`last_seen_review_id`. + +Interrupted entries also store: `interrupted_phase` (`"preliminary_planning"`, +`"planning"`, `"implementing"`, or `"addressing_review"`). + +Backward compatibility: old `awaiting_approval` status is mapped to +`awaiting_plan_approval`. --- -## Safety Gates +## Safety & Content Filtering -Two independent checks must pass before any work begins: +Instead of gatekeeping which issues to work on, content is **filtered** so +the LLM only sees comments and issue bodies that are created by or approved +(👍) by a whitelisted user. Every assigned issue is a candidate for work, +but: -1. **Issue-level gate** (before planning): issue must be created by a whitelisted user OR have a 👍 reaction from a whitelisted user on the issue itself. -2. **Plan approval gate** (before implementation): the plan comment must have a 👍 reaction from any whitelisted user. +1. **Blocked issues** are skipped — detected via "blocked by #N" / "depends + on #N" text patterns in the issue body, and via GitHub sub-issue + relationships (timeline API). +2. **No visible content** → issue is skipped. If the issue body and all + comments are from non-whitelisted users without any whitelisted 👍, there + is nothing for the LLM to work with. +3. **Plan approval gates** remain: preliminary plan needs 👍 to proceed to + thorough plan; thorough plan needs 👍 to proceed to implementation. -Whitelisted users: configured via `CLAYDE_WHITELISTED_USERS` in `data/config.env` (comma-separated). +Whitelisted users: configured via `CLAYDE_WHITELISTED_USERS` in `data/config.env`. --- @@ -155,34 +192,76 @@ Uses PyGitHub. All functions accept a `Github` client instance as first argument Repo cloning convention: `repos/{owner}__{repo}/` (double underscore separator). `git.ensure_repo()` clones on first use, then `git checkout && git pull` on subsequent calls. +Key functions: +- `is_blocked(g, owner, repo, number)` — checks body text patterns and timeline API for blocking relationships +- `add_pr_reviewer(g, owner, repo, pr_number, login)` — requests a review on a PR +- `get_pr_reviews()` / `get_pr_review_comments()` — fetch PR review data +- `edit_comment()` — edit an existing issue comment +- `parse_pr_url()` — parse PR URL into (owner, repo, pr_number) + --- ## Safety Gates (`safety.py`) -- `is_issue_authorized(issue)` — True if issue author is whitelisted OR a whitelisted user reacted +1. +- `is_comment_visible(comment)` — True if comment author is whitelisted OR has 👍 from whitelisted user. +- `filter_comments(comments)` — returns only visible comments. +- `is_issue_visible(issue)` — True if issue author is whitelisted OR has 👍 from whitelisted user. +- `has_visible_content(issue, comments)` — True if there is any visible content at all. - `is_plan_approved(g, owner, repo, number, comment_id)` — True if a whitelisted user reacted +1 to the plan comment. --- ## Plan Task (`tasks/plan.py`) -1. Fetch issue metadata and comments via PyGitHub +Two-phase planning with update support: + +### Phase 1: Preliminary Plan (`run_preliminary`) +1. Fetch issue metadata and filtered comments 2. `ensure_repo()` to have the code on disk -3. Build prompt with issue body, labels, comments, repo path -4. `invoke_claude()` — Claude explores the repo and returns a markdown plan -5. Post plan as issue comment with instructions to react 👍 to approve -6. Save `plan_comment_id` and set status → `awaiting_approval` +3. Build prompt with filtered issue body, labels, visible comments, repo path +4. `invoke_claude()` — Claude explores the repo and returns a short overview with questions +5. Post preliminary plan as issue comment +6. Set status → `awaiting_preliminary_approval` + +### Phase 2: Thorough Plan (`run_thorough`) +1. Fetch preliminary plan comment and discussion after it +2. Build prompt including preliminary plan + discussion +3. `invoke_claude()` — Claude produces the full detailed plan +4. Post thorough plan as issue comment +5. Set status → `awaiting_plan_approval` + +### Plan Updates (`run_update`) +Triggered when new visible comments are detected in `awaiting_preliminary_approval` +or `awaiting_plan_approval` states: +1. Fetch new visible comments since `last_seen_comment_id` +2. Build update prompt with current plan + new comments +3. `invoke_claude()` — Claude produces summary + updated plan +4. **Edit** the existing plan comment AND **post** a new comment with change summary --- ## Implementation Task (`tasks/implement.py`) -1. Fetch plan comment text and any discussion comments posted after the plan +1. Fetch plan comment text and filtered discussion comments after the plan 2. `ensure_repo()` to reset to latest default branch 3. Build prompt with issue body, plan, discussion, repo path -4. `invoke_claude()` — Claude creates a branch (`clayde/issue-{number}-{slug}`, where the slug is extracted from the plan), implements, commits, and pushes -5. Python code creates PR via PyGitHub (`create_pull_request()`) or finds an existing one -6. Post result comment on issue; set status → `done` +4. `invoke_claude()` — Claude creates a branch, implements, commits, and pushes +5. Python code creates PR via PyGitHub or finds an existing one +6. **Assign the issue author as PR reviewer** via `add_pr_reviewer()` +7. Post result comment on issue; set status → `pr_open` + +--- + +## Review Task (`tasks/review.py`) + +Handles PR review comments after implementation: + +1. Fetch PR reviews and review comments via PyGitHub +2. Filter to new reviews since `last_seen_review_id`, ignoring own reviews +3. If reviews have comments/body: invoke Claude with `address_review.j2` prompt +4. Claude makes changes and pushes to the existing branch +5. Post summary comment on issue; update `last_seen_review_id`; status stays `pr_open` +6. If a review is "APPROVED" with no comments: set status → `done` --- @@ -190,7 +269,7 @@ Repo cloning convention: `repos/{owner}__{repo}/` (double underscore separator). Format: `[YYYY-MM-DD HH:MM:SS] [clayde.] ` File: `/data/logs/agent.log` (appended) -Logger names: `clayde.orchestrator`, `clayde.tasks.plan`, `clayde.tasks.implement`, `clayde.github`, `clayde.claude` +Logger names: `clayde.orchestrator`, `clayde.tasks.plan`, `clayde.tasks.implement`, `clayde.tasks.review`, `clayde.github`, `clayde.claude` --- diff --git a/pyproject.toml b/pyproject.toml index a06d75a..7b852c5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,6 +11,7 @@ dependencies = [ "opentelemetry-exporter-otlp-proto-grpc>=1.20", "pydantic-settings>=2.0", "PyGitHub>=2.5.0", + "requests>=2.31", ] [project.optional-dependencies] diff --git a/src/clayde/github.py b/src/clayde/github.py index 4ca2e6d..e073603 100644 --- a/src/clayde/github.py +++ b/src/clayde/github.py @@ -3,6 +3,7 @@ import logging import re +import requests from github import Github, GithubException log = logging.getLogger("clayde.github") @@ -29,6 +30,11 @@ def post_comment(g: Github, owner: str, repo: str, number: int, body: str) -> in return comment.id +def edit_comment(g: Github, owner: str, repo: str, number: int, comment_id: int, body: str) -> None: + """Edit an existing issue comment.""" + g.get_repo(f"{owner}/{repo}").get_issue(number).get_comment(comment_id).edit(body) + + def fetch_comment(g: Github, owner: str, repo: str, number: int, comment_id: int): return g.get_repo(f"{owner}/{repo}").get_issue(number).get_comment(comment_id) @@ -71,3 +77,164 @@ def create_pull_request( title=title, body=body, head=head, base=base, ) return pr.html_url + + +# --------------------------------------------------------------------------- +# Blocked-issue detection via GitHub sub-issue relationships +# --------------------------------------------------------------------------- + +def is_blocked(g: Github, owner: str, repo: str, number: int) -> bool: + """Return True if an issue is blocked by another open issue. + + Uses the GitHub timeline API to find cross-reference events that + represent explicit sub-issue / tracked-by relationships. An issue + is blocked if any of its parent/blocking issues are still open. + + Also parses the issue body for "blocked by #N" / "depends on #N" text + patterns as a fallback. + """ + issue = g.get_repo(f"{owner}/{repo}").get_issue(number) + + # Check body text for blocking patterns + if issue.body: + if _has_blocking_references(g, owner, repo, issue.body): + return True + + # Check explicit sub-issue relationships via timeline events + try: + token = g.auth.token if hasattr(g.auth, "token") else None + if token: + if _has_blocking_sub_issue_parents(token, owner, repo, number): + return True + except Exception as e: + log.warning("Failed to check sub-issue relationships for %s/%s#%d: %s", + owner, repo, number, e) + + return False + + +def _has_blocking_references(g: Github, owner: str, repo: str, body: str) -> bool: + """Check issue body for 'blocked by #N' / 'depends on #N' patterns. + + Supports both same-repo (#N) and cross-repo (owner/repo#N) references. + """ + # Patterns: "blocked by #123", "depends on #45", "blocked by owner/repo#67" + patterns = [ + r"(?:blocked\s+by|depends\s+on)\s+#(\d+)", + r"(?:blocked\s+by|depends\s+on)\s+([A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+)#(\d+)", + ] + + # Same-repo references + for m in re.finditer(patterns[0], body, re.IGNORECASE): + ref_number = int(m.group(1)) + try: + ref_issue = g.get_repo(f"{owner}/{repo}").get_issue(ref_number) + if ref_issue.state == "open": + log.info("Issue %s/%s#%d is blocked by #%d (open)", owner, repo, + ref_number, ref_number) + return True + except GithubException: + pass + + # Cross-repo references + for m in re.finditer(patterns[1], body, re.IGNORECASE): + ref_repo_full = m.group(1) + ref_number = int(m.group(2)) + try: + ref_issue = g.get_repo(ref_repo_full).get_issue(ref_number) + if ref_issue.state == "open": + log.info("Issue %s/%s is blocked by %s#%d (open)", owner, repo, + ref_repo_full, ref_number) + return True + except GithubException: + pass + + return False + + +def _has_blocking_sub_issue_parents(token: str, owner: str, repo: str, number: int) -> bool: + """Check for explicit GitHub sub-issue parent relationships via timeline API. + + GitHub's sub-issues feature creates timeline events of type + 'sub_issue_added' / 'sub_issue_removed' on the parent issue, and + 'cross-referenced' events with specific source types. We use the + REST timeline API to detect these. + """ + url = f"https://api.github.com/repos/{owner}/{repo}/issues/{number}/timeline" + headers = { + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + } + + try: + response = requests.get(url, headers=headers, timeout=30) + response.raise_for_status() + events = response.json() + except Exception as e: + log.warning("Timeline API request failed for %s/%s#%d: %s", + owner, repo, number, e) + return False + + for event in events: + event_type = event.get("event") + + # Check for "connected" / "cross-referenced" events that indicate + # this issue is a sub-issue (child) of another issue + if event_type == "cross-referenced": + source = event.get("source", {}) + source_issue = source.get("issue", {}) + # If the source issue references this one as a sub-issue and + # the source issue is still open, we're blocked + if source_issue.get("state") == "open": + # Check if the source issue's body contains a task-list + # reference or sub-issue tracking for our issue + source_body = source_issue.get("body") or "" + # GitHub tracks sub-issues with task list items like + # "- [ ] #N" or "- [ ] owner/repo#N" + task_pattern = rf"- \[ \]\s+(?:https://github\.com/{owner}/{repo}/issues/{number}|{owner}/{repo}#{number}|#{number})" + if re.search(task_pattern, source_body): + source_url = source_issue.get("html_url", "unknown") + log.info("Issue %s/%s#%d is blocked by parent issue %s (open)", + owner, repo, number, source_url) + return True + + return False + + +# --------------------------------------------------------------------------- +# PR review helpers +# --------------------------------------------------------------------------- + +def add_pr_reviewer(g: Github, owner: str, repo: str, pr_number: int, reviewer_login: str) -> None: + """Request a review from the specified user on a PR.""" + try: + pr = g.get_repo(f"{owner}/{repo}").get_pull(pr_number) + pr.create_review_request(reviewers=[reviewer_login]) + log.info("Requested review from %s on PR #%d", reviewer_login, pr_number) + except GithubException as e: + log.warning("Failed to add reviewer %s to PR #%d: %s", reviewer_login, pr_number, e) + + +def get_pr_reviews(g: Github, owner: str, repo: str, pr_number: int) -> list: + """Return all reviews on a PR.""" + return list(g.get_repo(f"{owner}/{repo}").get_pull(pr_number).get_reviews()) + + +def get_pr_review_comments(g: Github, owner: str, repo: str, pr_number: int) -> list: + """Return all review comments (inline) on a PR.""" + return list(g.get_repo(f"{owner}/{repo}").get_pull(pr_number).get_review_comments()) + + +def parse_pr_url(url: str) -> tuple[str, str, int]: + """Parse a PR URL into (owner, repo, pr_number).""" + m = re.match(r"https://github\.com/([^/]+)/([^/]+)/pull/(\d+)", url) + if not m: + raise ValueError(f"Cannot parse PR URL: {url}") + return m.group(1), m.group(2), int(m.group(3)) + + +def get_issue_author(g: Github, owner: str, repo: str, number: int) -> str: + """Return the login of the issue author.""" + issue = g.get_repo(f"{owner}/{repo}").get_issue(number) + return issue.user.login diff --git a/src/clayde/orchestrator.py b/src/clayde/orchestrator.py index 75045b1..bd87b61 100644 --- a/src/clayde/orchestrator.py +++ b/src/clayde/orchestrator.py @@ -1,7 +1,11 @@ """Clayde orchestrator — manages the issue lifecycle state machine. - new → planning → awaiting_approval → implementing → done - ↘ interrupted (usage limit) → (retry) + new → preliminary_planning → awaiting_preliminary_approval + → planning → awaiting_plan_approval → implementing → pr_open → done + ↘ failed + +New comment detection triggers plan updates in awaiting_* states. +PR reviews are handled in pr_open state. Entry points: main() — single cycle (one-shot mode, used for testing/debugging) @@ -19,58 +23,165 @@ from clayde.claude import is_claude_available from clayde.config import get_github_client, get_settings, setup_logging -from clayde.github import get_assigned_issues -from clayde.safety import is_issue_authorized, is_plan_approved -from clayde.state import load_state, update_issue_state -from clayde.tasks import implement, plan +from clayde.github import ( + fetch_issue, + fetch_issue_comments, + get_assigned_issues, + is_blocked, + parse_issue_url, +) +from clayde.safety import filter_comments, has_visible_content, is_plan_approved +from clayde.state import get_issue_state, load_state, update_issue_state +from clayde.tasks import implement, plan, review from clayde.telemetry import get_tracer, init_tracer log = logging.getLogger("clayde.orchestrator") _shutdown = False +# Backward compatibility: treat old status names as their new equivalents +_STATUS_COMPAT = { + "awaiting_approval": "awaiting_plan_approval", +} + def _handle_new_issue(g, issue, url: str) -> None: + """Handle a newly assigned issue — check for visible content and blocked state.""" tracer = get_tracer() with tracer.start_as_current_span("clayde.handle_issue", attributes={"issue.url": url, "issue.handler": "new"}) as span: - if not is_issue_authorized(issue): - log.info("Skipping issue %s — not created by or approved by a whitelisted user", url) + owner, repo, number = parse_issue_url(url) + + # Check if issue is blocked by another open issue + try: + if is_blocked(g, owner, repo, number): + log.info("Skipping issue %s — blocked by another open issue", url) + span.set_attribute("issue.skipped", True) + span.set_attribute("issue.skip_reason", "blocked") + return + except Exception as e: + log.warning("Failed to check blocked status for %s: %s — proceeding", url, e) + + # Check if there is any visible content to work with + comments = fetch_issue_comments(g, owner, repo, number) + if not has_visible_content(issue, comments): + log.info("Skipping issue %s — no visible content (all filtered out)", url) span.set_attribute("issue.skipped", True) - span.set_attribute("issue.skip_reason", "unauthorized") + span.set_attribute("issue.skip_reason", "no_visible_content") return - log.info("New issue: %s — starting plan phase", url) + + log.info("New issue: %s — starting preliminary plan phase", url) try: - plan.run(url) + plan.run_preliminary(url) span.set_attribute("issue.failed", False) except Exception as e: - log.error("ERROR in plan for %s: %s", url, e) + log.error("ERROR in preliminary plan for %s: %s", url, e) span.set_status(StatusCode.ERROR, str(e)) span.record_exception(e) span.set_attribute("issue.failed", True) update_issue_state(url, {"status": "failed"}) -def _handle_awaiting_approval(g, url: str, entry: dict) -> None: +def _handle_awaiting_preliminary(g, url: str, entry: dict) -> None: + """Handle awaiting_preliminary_approval — check for 👍 or new comments.""" tracer = get_tracer() - with tracer.start_as_current_span("clayde.handle_issue", attributes={"issue.url": url, "issue.handler": "awaiting_approval"}) as span: + with tracer.start_as_current_span("clayde.handle_issue", attributes={"issue.url": url, "issue.handler": "awaiting_preliminary_approval"}) as span: owner = entry["owner"] repo = entry["repo"] number = entry["number"] - comment_id = entry["plan_comment_id"] + comment_id = entry.get("preliminary_comment_id") span.set_attribute("issue.number", number) - span.set_attribute("issue.owner", owner) - span.set_attribute("issue.repo", repo) - if not is_plan_approved(g, owner, repo, number, comment_id): - log.info("Issue %s awaiting approval", url) - span.set_attribute("issue.skipped", True) - span.set_attribute("issue.skip_reason", "not_approved") + + if not comment_id: + log.warning("No preliminary_comment_id for %s — marking as failed", url) + update_issue_state(url, {"status": "failed"}) + return + + # Check for new visible comments first (plan update) + if _has_new_comments(g, owner, repo, number, entry): + log.info("New comments on %s — updating preliminary plan", url) + try: + plan.run_update(url, "preliminary") + span.set_attribute("issue.action", "plan_update") + except Exception as e: + log.error("ERROR updating preliminary plan for %s: %s", url, e) + span.set_status(StatusCode.ERROR, str(e)) + span.record_exception(e) + return + + # Check for approval (👍 on preliminary plan comment) + if is_plan_approved(g, owner, repo, number, comment_id): + log.info("Preliminary plan approved: %s — starting thorough plan", url) + try: + plan.run_thorough(url) + span.set_attribute("issue.action", "thorough_plan") + except Exception as e: + log.error("ERROR in thorough plan for %s: %s", url, e) + span.set_status(StatusCode.ERROR, str(e)) + span.record_exception(e) + update_issue_state(url, {"status": "failed"}) + return + + log.info("Issue %s awaiting preliminary approval", url) + span.set_attribute("issue.skipped", True) + span.set_attribute("issue.skip_reason", "not_approved") + + +def _handle_awaiting_plan(g, url: str, entry: dict) -> None: + """Handle awaiting_plan_approval — check for 👍 or new comments.""" + tracer = get_tracer() + with tracer.start_as_current_span("clayde.handle_issue", attributes={"issue.url": url, "issue.handler": "awaiting_plan_approval"}) as span: + owner = entry["owner"] + repo = entry["repo"] + number = entry["number"] + comment_id = entry.get("plan_comment_id") + span.set_attribute("issue.number", number) + + if not comment_id: + log.warning("No plan_comment_id for %s — marking as failed", url) + update_issue_state(url, {"status": "failed"}) + return + + # Check for new visible comments first (plan update) + if _has_new_comments(g, owner, repo, number, entry): + log.info("New comments on %s — updating thorough plan", url) + try: + plan.run_update(url, "thorough") + span.set_attribute("issue.action", "plan_update") + except Exception as e: + log.error("ERROR updating thorough plan for %s: %s", url, e) + span.set_status(StatusCode.ERROR, str(e)) + span.record_exception(e) + return + + # Check for approval (👍 on thorough plan comment) + if is_plan_approved(g, owner, repo, number, comment_id): + log.info("Plan approved: %s — starting implementation", url) + try: + implement.run(url) + span.set_attribute("issue.action", "implement") + except Exception as e: + log.error("ERROR in implement for %s: %s", url, e) + span.set_status(StatusCode.ERROR, str(e)) + span.record_exception(e) + update_issue_state(url, {"status": "failed"}) return - log.info("Plan approved: %s — starting implementation", url) + + log.info("Issue %s awaiting plan approval", url) + span.set_attribute("issue.skipped", True) + span.set_attribute("issue.skip_reason", "not_approved") + + +def _handle_pr_open(g, url: str, entry: dict) -> None: + """Handle pr_open — check for new reviews on the PR.""" + tracer = get_tracer() + with tracer.start_as_current_span("clayde.handle_issue", attributes={"issue.url": url, "issue.handler": "pr_open"}) as span: + span.set_attribute("issue.number", entry.get("number", 0)) + log.info("Checking for PR reviews on %s", url) try: - implement.run(url) + review.run(url) span.set_attribute("issue.failed", False) except Exception as e: - log.error("ERROR in implement for %s: %s", url, e) + log.error("ERROR in review handling for %s: %s", url, e) span.set_status(StatusCode.ERROR, str(e)) span.record_exception(e) span.set_attribute("issue.failed", True) @@ -78,11 +189,19 @@ def _handle_awaiting_approval(g, url: str, entry: dict) -> None: def _handle_interrupted(url: str, entry: dict) -> None: + """Handle interrupted issues — retry the interrupted phase.""" tracer = get_tracer() phase = entry.get("interrupted_phase") with tracer.start_as_current_span("clayde.handle_issue", attributes={"issue.url": url, "issue.handler": "interrupted", "issue.interrupted_phase": phase or "unknown"}) as span: log.info("Retrying interrupted issue %s (phase: %s)", url, phase) - task = {"planning": plan.run, "implementing": implement.run}.get(phase) + + task_map = { + "preliminary_planning": plan.run_preliminary, + "planning": plan.run_thorough, + "implementing": implement.run, + "addressing_review": review.run, + } + task = task_map.get(phase) if task is None: log.warning("Unknown interrupted_phase '%s' for %s — skipping", phase, url) span.set_attribute("issue.skipped", True) @@ -100,6 +219,20 @@ def _handle_interrupted(url: str, entry: dict) -> None: update_issue_state(url, {"status": "interrupted"}) +def _has_new_comments(g, owner: str, repo: str, number: int, entry: dict) -> bool: + """Check if there are new visible comments from non-Clayde users.""" + last_seen = entry.get("last_seen_comment_id", 0) + github_username = get_settings().github_username + + comments = fetch_issue_comments(g, owner, repo, number) + visible = filter_comments(comments) + new_visible = [ + c for c in visible + if c.id > last_seen and c.user.login != github_username + ] + return len(new_visible) > 0 + + def main(): settings = get_settings() @@ -138,14 +271,23 @@ def main(): entry = issues_state.get(url, {}) status = entry.get("status") - if status in ("done", "planning", "implementing"): + # Backward compatibility + status = _STATUS_COMPAT.get(status, status) + + # Skip in-progress or completed states + if status in ("done", "preliminary_planning", "planning", + "implementing", "addressing_review"): continue processed += 1 if status is None: _handle_new_issue(g, issue, url) - elif status == "awaiting_approval": - _handle_awaiting_approval(g, url, entry) + elif status == "awaiting_preliminary_approval": + _handle_awaiting_preliminary(g, url, entry) + elif status == "awaiting_plan_approval": + _handle_awaiting_plan(g, url, entry) + elif status == "pr_open": + _handle_pr_open(g, url, entry) elif status == "interrupted": _handle_interrupted(url, entry) elif status == "failed": diff --git a/src/clayde/prompts/address_review.j2 b/src/clayde/prompts/address_review.j2 new file mode 100644 index 0000000..1eb3154 --- /dev/null +++ b/src/clayde/prompts/address_review.j2 @@ -0,0 +1,28 @@ +A pull request you created has received review comments. Address the feedback +and push updates to the branch. + +ISSUE #{{ number }}: {{ title }} +REPO: {{ owner }}/{{ repo }} + +ISSUE BODY: +{{ body }} + +PR BRANCH: {{ branch_name }} + +REVIEW COMMENTS: +{{ review_text }} + +REPOSITORY ON DISK: {{ repo_path }} + +Steps: +1. Check out the branch: git checkout {{ branch_name }} && git pull origin {{ branch_name }} +2. Read and understand each review comment +3. Make the requested changes +4. Run tests if applicable +5. Commit with a clear message referencing the review, e.g.: "Address review: " +6. Push: git push origin {{ branch_name }} + +After making all changes, provide a short summary of what you changed in response +to each review comment. This will be posted as a comment on the issue. + +Output ONLY the summary in markdown format. No preamble, no wrapping. diff --git a/src/clayde/prompts/preliminary_plan.j2 b/src/clayde/prompts/preliminary_plan.j2 new file mode 100644 index 0000000..ce596b2 --- /dev/null +++ b/src/clayde/prompts/preliminary_plan.j2 @@ -0,0 +1,25 @@ +Review the following GitHub issue and produce a short preliminary plan. + +ISSUE #{{ number }}: {{ title }} +REPO: {{ owner }}/{{ repo }} +LABELS: {{ labels }} + +ISSUE BODY: +{{ body }} + +EXISTING COMMENTS: +{{ comments_text }} + +REPOSITORY ON DISK: {{ repo_path }} + +Explore the repository briefly to understand the codebase relevant to this issue. +Pay special attention to agents.md files throughout. +Then produce a SHORT preliminary plan that includes: +- A brief overview of your approach (a few paragraphs, NOT an exhaustive plan) +- Any clarifying questions you have +- A rough scope estimate (small / medium / large) + +Keep this concise — this is just a preliminary overview for discussion, not the +full implementation plan. That comes later after approval. + +Output ONLY the preliminary plan in markdown format. No preamble, no wrapping. diff --git a/src/clayde/prompts/thorough_plan.j2 b/src/clayde/prompts/thorough_plan.j2 new file mode 100644 index 0000000..a39f65f --- /dev/null +++ b/src/clayde/prompts/thorough_plan.j2 @@ -0,0 +1,38 @@ +Research the following GitHub issue and produce a detailed implementation plan. + +ISSUE #{{ number }}: {{ title }} +REPO: {{ owner }}/{{ repo }} +LABELS: {{ labels }} + +ISSUE BODY: +{{ body }} + +PRELIMINARY PLAN (already approved): +{{ preliminary_plan_text }} + +DISCUSSION AFTER PRELIMINARY PLAN: +{{ discussion_text }} + +REPOSITORY ON DISK: {{ repo_path }} + +Explore the repository to understand the codebase relevant to this issue. +Pay special attention to agents.md files throughout. +Then produce a thorough plan that includes: +- Summary of what needs to be done and why +- Files to create or modify (with specific locations) +- Implementation approach and key decisions +- Edge cases and risks +- How to verify the implementation is correct + +At the end of your plan, include a branch name suggestion on its own line in +exactly this format: + +**Branch:** `clayde/issue-{{ number }}-` + +where `` is a 1-to-3 word lowercase hyphenated description of the issue +(e.g., `clayde/issue-13-better-branch-naming`). This MUST be present. + +If anything is ambiguous or you need clarification, include your questions +at the end under a "Questions" section (before the branch name line). + +Output ONLY the plan in markdown format. No preamble, no wrapping. diff --git a/src/clayde/prompts/update_plan.j2 b/src/clayde/prompts/update_plan.j2 new file mode 100644 index 0000000..b508e00 --- /dev/null +++ b/src/clayde/prompts/update_plan.j2 @@ -0,0 +1,33 @@ +New comments have been posted on a GitHub issue you are working on. Review the +new comments and update your plan accordingly. + +ISSUE #{{ number }}: {{ title }} +REPO: {{ owner }}/{{ repo }} + +ISSUE BODY: +{{ body }} + +CURRENT PLAN: +{{ current_plan_text }} + +NEW COMMENTS SINCE LAST UPDATE: +{{ new_comments_text }} + +REPOSITORY ON DISK: {{ repo_path }} + +Based on the new comments: +1. Consider whether the current plan needs to be updated +2. Answer any questions directed at you +3. Incorporate any requested changes or clarifications + +Produce two outputs separated by the marker `---UPDATED_PLAN---`: + +FIRST: A short summary of what changed and your responses to any questions. +This will be posted as a new comment. + +---UPDATED_PLAN--- + +SECOND: The complete updated plan (the full plan text, not just the changes). +This will replace the existing plan comment. + +Output ONLY these two sections. No preamble, no wrapping. diff --git a/src/clayde/safety.py b/src/clayde/safety.py index 4f0f029..6ec33a7 100644 --- a/src/clayde/safety.py +++ b/src/clayde/safety.py @@ -1,23 +1,71 @@ -"""Safety gates — authorization checks before planning or implementing.""" +"""Safety gates — content filtering and plan approval checks. + +Instead of gatekeeping which issues to work on, we filter *content* so the +LLM only sees comments/issue bodies that are created by or approved (👍) +by a whitelisted user. Every assigned issue is a candidate, but if all +visible content is filtered out the issue is skipped. +""" from github import Github from clayde.config import get_settings -def is_issue_authorized(issue) -> bool: - """Return True if the issue author is whitelisted OR a whitelisted user reacted +1.""" - if issue.user.login in get_settings().whitelisted_users_list: +# --------------------------------------------------------------------------- +# Content filtering +# --------------------------------------------------------------------------- + +def is_comment_visible(comment) -> bool: + """Return True if a comment was created by or 👍'd by a whitelisted user.""" + whitelist = get_settings().whitelisted_users_list + if comment.user.login in whitelist: + return True + return _has_whitelisted_reaction(comment.get_reactions()) + + +def filter_comments(comments: list) -> list: + """Return only comments that are visible (created/approved by a whitelisted user).""" + return [c for c in comments if is_comment_visible(c)] + + +def is_issue_visible(issue) -> bool: + """Return True if the issue was created by or 👍'd by a whitelisted user. + + This checks the issue *body* visibility — whether the LLM should see the + issue body text. + """ + whitelist = get_settings().whitelisted_users_list + if issue.user.login in whitelist: return True return _has_whitelisted_reaction(issue.get_reactions()) +def has_visible_content(issue, comments: list) -> bool: + """Return True if there is any visible content (issue body or comments). + + An issue with no visible content at all should not be worked on. + """ + if is_issue_visible(issue): + return True + if filter_comments(comments): + return True + return False + + +# --------------------------------------------------------------------------- +# Plan approval +# --------------------------------------------------------------------------- + def is_plan_approved(g: Github, owner: str, repo: str, number: int, comment_id: int) -> bool: """Return True if a whitelisted user reacted +1 to the plan comment.""" comment = g.get_repo(f"{owner}/{repo}").get_issue(number).get_comment(comment_id) return _has_whitelisted_reaction(comment.get_reactions()) +# --------------------------------------------------------------------------- +# Internal helpers +# --------------------------------------------------------------------------- + def _has_whitelisted_reaction(reactions) -> bool: return any( r.content == "+1" and r.user.login in get_settings().whitelisted_users_list diff --git a/src/clayde/tasks/implement.py b/src/clayde/tasks/implement.py index e3ab9cd..de79172 100644 --- a/src/clayde/tasks/implement.py +++ b/src/clayde/tasks/implement.py @@ -1,4 +1,8 @@ -"""Implement task — implement the approved plan, open PR, post result.""" +"""Implement task — implement the approved plan, open PR, assign reviewer. + +After implementation, assigns the issue author as PR reviewer and sets +status to ``pr_open`` for review monitoring. +""" import logging import subprocess @@ -10,6 +14,7 @@ from clayde.config import DATA_DIR, get_github_client from clayde.git import ensure_repo from clayde.github import ( + add_pr_reviewer, create_pull_request, extract_branch_name, fetch_comment, @@ -17,9 +22,12 @@ fetch_issue_comments, find_open_pr, get_default_branch, + get_issue_author, parse_issue_url, + parse_pr_url, post_comment, ) +from clayde.safety import filter_comments from clayde.state import get_issue_state, update_issue_state from clayde.telemetry import get_tracer @@ -48,10 +56,7 @@ def run(issue_url: str) -> None: existing_pr = find_open_pr(g, owner, repo, branch_name) if existing_pr: log.info("Resuming interrupted #%d — found existing PR %s", number, existing_pr) - post_comment(g, owner, repo, number, f"Implementation complete — PR opened: {existing_pr}") - update_issue_state(issue_url, {"status": "done", "pr_url": existing_pr}) - span.set_attribute("implement.status", "done") - span.set_attribute("implement.pr_url", existing_pr) + _assign_reviewer_and_finish(g, owner, repo, number, issue_url, existing_pr, span) return update_issue_state(issue_url, {"status": "implementing"}) @@ -71,7 +76,8 @@ def run(issue_url: str) -> None: _checkout_wip_branch(repo_path, branch_name) all_comments = fetch_issue_comments(g, owner, repo, number) - discussion_text = _collect_discussion(all_comments, plan_comment_id) + visible_comments = filter_comments(all_comments) + discussion_text = _collect_discussion(visible_comments, plan_comment_id) prompt = _build_prompt(issue, plan_text, discussion_text, owner, repo, number, repo_path, branch_name) @@ -111,12 +117,8 @@ def run(issue_url: str) -> None: log.error("Failed to create PR for #%d: %s", number, e) if pr_url: - _post_result(g, owner, repo, number, pr_url) - update_issue_state(issue_url, {"status": "done", "pr_url": pr_url}) + _assign_reviewer_and_finish(g, owner, repo, number, issue_url, pr_url, span) log.info("Conversation saved to %s", conv_path) - span.set_attribute("implement.status", "done") - span.set_attribute("implement.pr_url", pr_url) - log.info("Issue #%d done — PR: %s", number, pr_url) else: log.error("Implementation of #%d produced no PR", number) log.error("Claude output (last 2000 chars): %s", (output or "")[-2000:]) @@ -138,6 +140,27 @@ def run(issue_url: str) -> None: "retry_count": retry_count}) +def _assign_reviewer_and_finish(g, owner, repo, number, issue_url, pr_url, span): + """Post result, assign reviewer, set status to pr_open.""" + _post_result(g, owner, repo, number, pr_url) + + # Assign the issue author as PR reviewer + try: + issue_author = get_issue_author(g, owner, repo, number) + _, _, pr_number = parse_pr_url(pr_url) + add_pr_reviewer(g, owner, repo, pr_number, issue_author) + except Exception as e: + log.warning("Failed to assign reviewer for PR %s: %s", pr_url, e) + + update_issue_state(issue_url, { + "status": "pr_open", + "pr_url": pr_url, + "last_seen_review_id": 0, + }) + span.set_attribute("implement.status", "pr_open") + span.set_attribute("implement.pr_url", pr_url) + log.info("Issue #%d PR open — monitoring for reviews: %s", number, pr_url) + def _checkout_wip_branch(repo_path, branch_name: str) -> None: """Checkout an existing WIP branch if it exists (locally or on remote).""" diff --git a/src/clayde/tasks/plan.py b/src/clayde/tasks/plan.py index b76cfd5..95ed47c 100644 --- a/src/clayde/tasks/plan.py +++ b/src/clayde/tasks/plan.py @@ -1,4 +1,10 @@ -"""Plan task — research repo, produce plan, post as issue comment.""" +"""Plan task — two-phase planning with preliminary and thorough plans. + +Phase 1 (preliminary): Post a short overview with questions. +Phase 2 (thorough): After preliminary approval, post the full detailed plan. +Updates: When new visible comments arrive, update the current plan and post a + summary of changes. +""" import logging from pathlib import Path @@ -6,15 +12,18 @@ from jinja2 import Environment, StrictUndefined from clayde.claude import UsageLimitError, invoke_claude -from clayde.config import get_github_client +from clayde.config import get_github_client, get_settings from clayde.git import ensure_repo from clayde.github import ( + edit_comment, + fetch_comment, fetch_issue, fetch_issue_comments, get_default_branch, parse_issue_url, post_comment, ) +from clayde.safety import filter_comments, is_issue_visible from clayde.state import update_issue_state from clayde.telemetry import get_tracer @@ -22,80 +31,350 @@ _PROMPTS_DIR = Path(__file__).parent.parent / "prompts" +_UPDATE_PLAN_SEPARATOR = "---UPDATED_PLAN---" -def run(issue_url: str) -> None: + +# --------------------------------------------------------------------------- +# Phase 1: Preliminary plan +# --------------------------------------------------------------------------- + +def run_preliminary(issue_url: str) -> None: + """Research the issue and post a short preliminary plan with questions.""" tracer = get_tracer() - with tracer.start_as_current_span("clayde.task.plan") as span: + with tracer.start_as_current_span("clayde.task.preliminary_plan") as span: g = get_github_client() owner, repo, number = parse_issue_url(issue_url) span.set_attribute("issue.number", number) span.set_attribute("issue.owner", owner) span.set_attribute("issue.repo", repo) update_issue_state(issue_url, { - "status": "planning", "owner": owner, "repo": repo, "number": number, + "status": "preliminary_planning", + "owner": owner, "repo": repo, "number": number, + }) + + issue = fetch_issue(g, owner, repo, number) + default_branch = get_default_branch(g, owner, repo) + repo_path = ensure_repo(owner, repo, default_branch) + + prompt = _build_preliminary_prompt(g, issue, owner, repo, number, repo_path) + + log.info("Invoking Claude for preliminary plan on issue #%d", number) + try: + plan_text = invoke_claude(prompt, repo_path) + except UsageLimitError: + log.warning("Usage limit hit during preliminary planning #%d", number) + span.set_attribute("plan.status", "limit") + update_issue_state(issue_url, { + "status": "interrupted", + "interrupted_phase": "preliminary_planning", + }) + return + + span.set_attribute("plan.output_length", len(plan_text)) + + if not plan_text.strip(): + log.error("Claude returned empty preliminary plan for issue #%d", number) + span.set_attribute("plan.status", "empty") + update_issue_state(issue_url, {"status": "failed"}) + return + + if len(plan_text.strip()) < 100: + log.warning("Claude returned very short preliminary plan for issue #%d (%d chars)", + number, len(plan_text.strip())) + span.set_attribute("plan.status", "short") + update_issue_state(issue_url, { + "status": "interrupted", + "interrupted_phase": "preliminary_planning", + }) + return + + comment_id = _post_preliminary_comment(g, owner, repo, number, plan_text) + + # Track the last seen comment so we can detect new ones later + all_comments = fetch_issue_comments(g, owner, repo, number) + last_comment_id = all_comments[-1].id if all_comments else 0 + + update_issue_state(issue_url, { + "status": "awaiting_preliminary_approval", + "preliminary_comment_id": comment_id, + "last_seen_comment_id": last_comment_id, }) + span.set_attribute("plan.status", "posted") + log.info("Preliminary plan posted for issue #%d (comment %s)", number, comment_id) + + +# --------------------------------------------------------------------------- +# Phase 2: Thorough plan +# --------------------------------------------------------------------------- + +def run_thorough(issue_url: str) -> None: + """Post a thorough implementation plan (after preliminary approval).""" + tracer = get_tracer() + with tracer.start_as_current_span("clayde.task.thorough_plan") as span: + g = get_github_client() + owner, repo, number = parse_issue_url(issue_url) + span.set_attribute("issue.number", number) + span.set_attribute("issue.owner", owner) + span.set_attribute("issue.repo", repo) + update_issue_state(issue_url, {"status": "planning"}) issue = fetch_issue(g, owner, repo, number) default_branch = get_default_branch(g, owner, repo) repo_path = ensure_repo(owner, repo, default_branch) - prompt = _build_prompt(g, issue, owner, repo, number, repo_path) + from clayde.state import get_issue_state + issue_state = get_issue_state(issue_url) + preliminary_comment_id = issue_state.get("preliminary_comment_id") + preliminary_comment = fetch_comment(g, owner, repo, number, preliminary_comment_id) + preliminary_text = preliminary_comment.body + + # Collect discussion after the preliminary plan + all_comments = fetch_issue_comments(g, owner, repo, number) + visible_comments = filter_comments(all_comments) + discussion_text = _collect_discussion_after(visible_comments, preliminary_comment_id) + + prompt = _build_thorough_prompt( + g, issue, owner, repo, number, repo_path, + preliminary_text, discussion_text, + ) - log.info("Invoking Claude for planning issue #%d", number) + log.info("Invoking Claude for thorough plan on issue #%d", number) try: plan_text = invoke_claude(prompt, repo_path) except UsageLimitError: - log.warning("Usage limit hit during planning #%d — will retry next cycle", number) + log.warning("Usage limit hit during thorough planning #%d", number) span.set_attribute("plan.status", "limit") - update_issue_state(issue_url, {"status": "interrupted", "interrupted_phase": "planning"}) + update_issue_state(issue_url, { + "status": "interrupted", + "interrupted_phase": "planning", + }) return span.set_attribute("plan.output_length", len(plan_text)) if not plan_text.strip(): - log.error("Claude returned empty plan for issue #%d", number) + log.error("Claude returned empty thorough plan for issue #%d", number) span.set_attribute("plan.status", "empty") update_issue_state(issue_url, {"status": "failed"}) return - # Reject suspiciously short output that may be a rate limit message if len(plan_text.strip()) < 200: - log.warning("Claude returned very short plan for issue #%d (%d chars) — treating as failed", - number, len(plan_text.strip())) + log.warning("Claude returned very short thorough plan for issue #%d (%d chars)", + number, len(plan_text.strip())) span.set_attribute("plan.status", "short") - update_issue_state(issue_url, {"status": "interrupted", "interrupted_phase": "planning"}) + update_issue_state(issue_url, { + "status": "interrupted", + "interrupted_phase": "planning", + }) return - comment_id = _post_plan_comment(g, owner, repo, number, plan_text) + comment_id = _post_thorough_plan_comment(g, owner, repo, number, plan_text) + + # Update last seen comment + all_comments = fetch_issue_comments(g, owner, repo, number) + last_comment_id = all_comments[-1].id if all_comments else 0 + update_issue_state(issue_url, { - "status": "awaiting_approval", + "status": "awaiting_plan_approval", "plan_comment_id": comment_id, + "last_seen_comment_id": last_comment_id, }) span.set_attribute("plan.status", "posted") - log.info("Plan posted for issue #%d (comment %s)", number, comment_id) + log.info("Thorough plan posted for issue #%d (comment %s)", number, comment_id) + + +# --------------------------------------------------------------------------- +# Plan update (new comments detected) +# --------------------------------------------------------------------------- + +def run_update(issue_url: str, phase: str) -> None: + """Process new visible comments and update the current plan. + + Args: + issue_url: The issue URL. + phase: Either "preliminary" or "thorough" — which plan to update. + """ + tracer = get_tracer() + with tracer.start_as_current_span("clayde.task.update_plan") as span: + g = get_github_client() + owner, repo, number = parse_issue_url(issue_url) + span.set_attribute("issue.number", number) + span.set_attribute("plan.update_phase", phase) + + from clayde.state import get_issue_state + issue_state = get_issue_state(issue_url) + + if phase == "preliminary": + plan_comment_id = issue_state.get("preliminary_comment_id") + return_status = "awaiting_preliminary_approval" + else: + plan_comment_id = issue_state.get("plan_comment_id") + return_status = "awaiting_plan_approval" + + last_seen = issue_state.get("last_seen_comment_id", 0) + + issue = fetch_issue(g, owner, repo, number) + default_branch = get_default_branch(g, owner, repo) + repo_path = ensure_repo(owner, repo, default_branch) + + plan_comment = fetch_comment(g, owner, repo, number, plan_comment_id) + current_plan_text = plan_comment.body + all_comments = fetch_issue_comments(g, owner, repo, number) + visible_comments = filter_comments(all_comments) -def _build_prompt(g, issue, owner: str, repo: str, number: int, repo_path: str) -> str: + # Get new visible comments since last seen + github_username = get_settings().github_username + new_comments = [ + c for c in visible_comments + if c.id > last_seen and c.user.login != github_username + ] + + if not new_comments: + log.info("No new visible comments for issue #%d — skipping update", number) + return + + new_comments_text = "\n---\n".join( + f"@{c.user.login}:\n{c.body}" for c in new_comments + ) + + body_text = issue.body or "(empty)" + if not is_issue_visible(issue): + body_text = "(filtered)" + + prompt = _build_update_prompt( + number, issue.title, owner, repo, + body_text, current_plan_text, new_comments_text, repo_path, + ) + + log.info("Invoking Claude for plan update on issue #%d (%s phase)", number, phase) + try: + output = invoke_claude(prompt, repo_path) + except UsageLimitError: + log.warning("Usage limit hit during plan update #%d", number) + span.set_attribute("plan.update_status", "limit") + update_issue_state(issue_url, { + "status": "interrupted", + "interrupted_phase": f"{phase}_planning" if phase == "preliminary" else "planning", + }) + return + + # Parse output into summary + updated plan + summary, updated_plan = _parse_update_output(output) + + if updated_plan: + # Edit the existing plan comment + edit_comment(g, owner, repo, number, plan_comment_id, updated_plan) + log.info("Updated %s plan comment %d for issue #%d", phase, plan_comment_id, number) + + if summary: + # Post a new comment with the change summary + post_comment(g, owner, repo, number, + f"**Plan updated.** {summary}") + + # Update last seen comment + all_comments = fetch_issue_comments(g, owner, repo, number) + last_comment_id = all_comments[-1].id if all_comments else 0 + + update_issue_state(issue_url, { + "status": return_status, + "last_seen_comment_id": last_comment_id, + }) + span.set_attribute("plan.update_status", "updated") + log.info("Plan update complete for issue #%d", number) + + +# --------------------------------------------------------------------------- +# Backward compatibility: old `run()` for the `interrupted` retry path +# --------------------------------------------------------------------------- + +def run(issue_url: str) -> None: + """Run the preliminary plan phase (backward-compatible entry point).""" + run_preliminary(issue_url) + + +# --------------------------------------------------------------------------- +# Prompt builders +# --------------------------------------------------------------------------- + +def _build_preliminary_prompt(g, issue, owner: str, repo: str, number: int, repo_path: str) -> str: labels = ", ".join(l.name for l in issue.labels) or "none" comments = fetch_issue_comments(g, owner, repo, number) + visible = filter_comments(comments) comments_text = "\n".join( - f"@{c.user.login}:\n{c.body}\n---" for c in comments + f"@{c.user.login}:\n{c.body}\n---" for c in visible ) or "(none)" - template_src = (_PROMPTS_DIR / "plan.j2").read_text() + body_text = issue.body or "(empty)" + if not is_issue_visible(issue): + body_text = "(filtered)" + + template_src = (_PROMPTS_DIR / "preliminary_plan.j2").read_text() return Environment(undefined=StrictUndefined).from_string(template_src).render( number=number, title=issue.title, owner=owner, repo=repo, labels=labels, - body=issue.body or "(empty)", + body=body_text, comments_text=comments_text, repo_path=repo_path, ) -def _post_plan_comment(g, owner: str, repo: str, number: int, plan_text: str) -> int: +def _build_thorough_prompt(g, issue, owner, repo, number, repo_path, + preliminary_text, discussion_text) -> str: + labels = ", ".join(l.name for l in issue.labels) or "none" + + body_text = issue.body or "(empty)" + if not is_issue_visible(issue): + body_text = "(filtered)" + + template_src = (_PROMPTS_DIR / "thorough_plan.j2").read_text() + return Environment(undefined=StrictUndefined).from_string(template_src).render( + number=number, + title=issue.title, + owner=owner, + repo=repo, + labels=labels, + body=body_text, + preliminary_plan_text=preliminary_text, + discussion_text=discussion_text, + repo_path=repo_path, + ) + + +def _build_update_prompt(number, title, owner, repo, body, current_plan_text, + new_comments_text, repo_path) -> str: + template_src = (_PROMPTS_DIR / "update_plan.j2").read_text() + return Environment(undefined=StrictUndefined).from_string(template_src).render( + number=number, + title=title, + owner=owner, + repo=repo, + body=body, + current_plan_text=current_plan_text, + new_comments_text=new_comments_text, + repo_path=repo_path, + ) + + +# --------------------------------------------------------------------------- +# Comment formatting +# --------------------------------------------------------------------------- + +def _post_preliminary_comment(g, owner: str, repo: str, number: int, plan_text: str) -> int: + comment_body = ( + f"## Preliminary Plan\n\n" + f"{plan_text}\n\n" + f"---\n" + f"*React with \U0001f44d to approve this preliminary plan and proceed " + f"to a thorough implementation plan.*" + ) + return post_comment(g, owner, repo, number, comment_body) + + +def _post_thorough_plan_comment(g, owner: str, repo: str, number: int, plan_text: str) -> int: comment_body = ( f"## Implementation Plan\n\n" f"{plan_text}\n\n" @@ -103,3 +382,37 @@ def _post_plan_comment(g, owner: str, repo: str, number: int, plan_text: str) -> f"*React with \U0001f44d to approve this plan and start implementation.*" ) return post_comment(g, owner, repo, number, comment_body) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _collect_discussion_after(comments: list, after_comment_id: int) -> str: + """Collect visible comment text posted after a specific comment ID.""" + found = False + discussion = [] + for c in comments: + if c.id == after_comment_id: + found = True + continue + if found: + discussion.append(f"@{c.user.login}:\n{c.body}") + return "\n---\n".join(discussion) or "(none)" + + +def _parse_update_output(output: str) -> tuple[str, str]: + """Parse Claude output into (summary, updated_plan). + + Expected format: + + ---UPDATED_PLAN--- + + + Returns (summary, updated_plan). If separator not found, treats entire + output as summary with empty updated_plan. + """ + if _UPDATE_PLAN_SEPARATOR in output: + parts = output.split(_UPDATE_PLAN_SEPARATOR, 1) + return parts[0].strip(), parts[1].strip() + return output.strip(), "" diff --git a/src/clayde/tasks/review.py b/src/clayde/tasks/review.py new file mode 100644 index 0000000..8c32a9a --- /dev/null +++ b/src/clayde/tasks/review.py @@ -0,0 +1,169 @@ +"""Review task — address PR review comments and push updates.""" + +import logging +from pathlib import Path + +from jinja2 import Environment, StrictUndefined + +from clayde.claude import UsageLimitError, invoke_claude +from clayde.config import DATA_DIR, get_github_client, get_settings +from clayde.git import ensure_repo +from clayde.github import ( + fetch_issue, + get_default_branch, + get_pr_review_comments, + get_pr_reviews, + parse_issue_url, + parse_pr_url, + post_comment, +) +from clayde.state import get_issue_state, update_issue_state +from clayde.telemetry import get_tracer + +log = logging.getLogger("clayde.tasks.review") + +_PROMPTS_DIR = Path(__file__).parent.parent / "prompts" + + +def run(issue_url: str) -> None: + """Check for new PR reviews, address comments, push updates.""" + tracer = get_tracer() + with tracer.start_as_current_span("clayde.task.review") as span: + g = get_github_client() + owner, repo, number = parse_issue_url(issue_url) + issue_state = get_issue_state(issue_url) + pr_url = issue_state.get("pr_url") + span.set_attribute("issue.number", number) + span.set_attribute("issue.owner", owner) + span.set_attribute("issue.repo", repo) + + if not pr_url: + log.warning("No PR URL in state for issue #%d — skipping review", number) + return + + _, _, pr_number = parse_pr_url(pr_url) + last_seen_review_id = issue_state.get("last_seen_review_id", 0) + github_username = get_settings().github_username + + # Get all reviews and filter to new ones + reviews = get_pr_reviews(g, owner, repo, pr_number) + new_reviews = [ + r for r in reviews + if r.id > last_seen_review_id + and r.user.login != github_username + ] + + if not new_reviews: + log.info("No new reviews on PR #%d for issue #%d", pr_number, number) + return + + # Check if any new review has actual content (comments or body) + review_comments = get_pr_review_comments(g, owner, repo, pr_number) + new_review_ids = {r.id for r in new_reviews} + relevant_comments = [ + rc for rc in review_comments + if rc.pull_request_review_id in new_review_ids + ] + + # Also include reviews with body text + review_bodies = [ + r for r in new_reviews + if r.body and r.body.strip() + ] + + if not relevant_comments and not review_bodies: + # Reviews with no comments (e.g. just "approved") — update seen ID + max_review_id = max(r.id for r in new_reviews) + update_issue_state(issue_url, {"last_seen_review_id": max_review_id}) + + # Check if any review is an approval + approved = any(r.state == "APPROVED" for r in new_reviews) + if approved: + log.info("PR #%d approved for issue #%d — marking as done", pr_number, number) + update_issue_state(issue_url, {"status": "done"}) + span.set_attribute("review.status", "approved") + return + + # Build review text for Claude + review_text = _format_reviews(new_reviews, relevant_comments) + + log.info("Addressing %d new review(s) on PR #%d for issue #%d", + len(new_reviews), pr_number, number) + + update_issue_state(issue_url, {"status": "addressing_review"}) + + issue = fetch_issue(g, owner, repo, number) + default_branch = get_default_branch(g, owner, repo) + repo_path = ensure_repo(owner, repo, default_branch) + branch_name = issue_state.get("branch_name", f"clayde/issue-{number}") + + prompt = _build_prompt( + issue, owner, repo, number, repo_path, branch_name, review_text, + ) + + conv_path = DATA_DIR / "conversations" / f"{owner}__{repo}__issue-{number}-review.json" + conv_path.parent.mkdir(parents=True, exist_ok=True) + + try: + output = invoke_claude( + prompt, + repo_path, + branch_name=branch_name, + conversation_path=conv_path, + ) + except UsageLimitError: + log.warning("Usage limit hit during review handling #%d", number) + span.set_attribute("review.status", "limit") + update_issue_state(issue_url, { + "status": "interrupted", + "interrupted_phase": "addressing_review", + }) + return + + # Post summary comment on the issue + if output and output.strip(): + post_comment(g, owner, repo, number, + f"**Review addressed.** {output.strip()}") + + # Update last seen review ID and return to pr_open + max_review_id = max(r.id for r in new_reviews) + update_issue_state(issue_url, { + "status": "pr_open", + "last_seen_review_id": max_review_id, + }) + span.set_attribute("review.status", "addressed") + log.info("Review comments addressed for issue #%d", number) + + +def _format_reviews(reviews: list, review_comments: list) -> str: + """Format reviews and review comments into text for the prompt.""" + parts = [] + + for review in reviews: + header = f"Review by @{review.user.login} (state: {review.state}):" + if review.body and review.body.strip(): + parts.append(f"{header}\n{review.body}") + + # Add inline comments from this review + for rc in review_comments: + if rc.pull_request_review_id == review.id: + 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 "(no review content)" + + +def _build_prompt(issue, owner, repo, number, repo_path, branch_name, review_text) -> str: + template_src = (_PROMPTS_DIR / "address_review.j2").read_text() + return Environment(undefined=StrictUndefined).from_string(template_src).render( + number=number, + title=issue.title, + owner=owner, + repo=repo, + body=issue.body or "(empty)", + branch_name=branch_name, + review_text=review_text, + repo_path=repo_path, + ) diff --git a/tests/test_github.py b/tests/test_github.py index fec9be1..7da058d 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -6,7 +6,10 @@ from github import GithubException from clayde.github import ( + _has_blocking_references, + add_pr_reviewer, create_pull_request, + edit_comment, extract_branch_name, fetch_comment, fetch_issue, @@ -14,7 +17,12 @@ find_open_pr, get_assigned_issues, get_default_branch, + get_issue_author, + get_pr_review_comments, + get_pr_reviews, + is_blocked, parse_issue_url, + parse_pr_url, post_comment, ) @@ -35,6 +43,18 @@ def test_pr_url_raises(self): parse_issue_url("https://github.com/alice/repo/pull/1") +class TestParsePrUrl: + def test_valid_url(self): + owner, repo, number = parse_pr_url("https://github.com/alice/myrepo/pull/5") + assert owner == "alice" + assert repo == "myrepo" + assert number == 5 + + def test_invalid_url_raises(self): + with pytest.raises(ValueError, match="Cannot parse PR URL"): + parse_pr_url("https://github.com/alice/repo/issues/1") + + class TestFetchIssue: def test_calls_correct_api(self): g = MagicMock() @@ -66,6 +86,14 @@ def test_returns_comment_id(self): assert result == 12345 +class TestEditComment: + def test_calls_edit(self): + g = MagicMock() + edit_comment(g, "alice", "repo", 5, 999, "new body") + g.get_repo.return_value.get_issue.return_value.get_comment.assert_called_once_with(999) + g.get_repo.return_value.get_issue.return_value.get_comment.return_value.edit.assert_called_once_with("new body") + + class TestFetchComment: def test_calls_correct_api(self): g = MagicMock() @@ -146,3 +174,171 @@ def test_creates_pr_and_returns_url(self): head="clayde/issue-5", base="main", ) assert result == "https://github.com/alice/repo/pull/11" + + +class TestIsBlocked: + def test_not_blocked_with_no_references(self): + g = MagicMock() + issue = MagicMock() + issue.body = "Just a normal issue" + g.get_repo.return_value.get_issue.return_value = issue + g.auth = MagicMock() + g.auth.token = "tok" + with patch("clayde.github._has_blocking_sub_issue_parents", return_value=False): + assert is_blocked(g, "o", "r", 1) is False + + def test_blocked_by_open_issue_in_body(self): + g = MagicMock() + issue = MagicMock() + issue.body = "This is blocked by #5" + ref_issue = MagicMock() + ref_issue.state = "open" + + # get_repo called twice: once for the issue, once for the ref + repo_mock = MagicMock() + repo_mock.get_issue.side_effect = lambda n: issue if n == 1 else ref_issue + g.get_repo.return_value = repo_mock + g.auth = MagicMock() + g.auth.token = "tok" + + assert is_blocked(g, "o", "r", 1) is True + + def test_not_blocked_when_ref_is_closed(self): + g = MagicMock() + issue = MagicMock() + issue.body = "This is blocked by #5" + ref_issue = MagicMock() + ref_issue.state = "closed" + + repo_mock = MagicMock() + repo_mock.get_issue.side_effect = lambda n: issue if n == 1 else ref_issue + g.get_repo.return_value = repo_mock + g.auth = MagicMock() + g.auth.token = "tok" + with patch("clayde.github._has_blocking_sub_issue_parents", return_value=False): + assert is_blocked(g, "o", "r", 1) is False + + def test_not_blocked_with_no_body(self): + g = MagicMock() + issue = MagicMock() + issue.body = None + g.get_repo.return_value.get_issue.return_value = issue + g.auth = MagicMock() + g.auth.token = "tok" + with patch("clayde.github._has_blocking_sub_issue_parents", return_value=False): + assert is_blocked(g, "o", "r", 1) is False + + def test_blocked_by_depends_on_pattern(self): + g = MagicMock() + issue = MagicMock() + issue.body = "This depends on #10" + ref_issue = MagicMock() + ref_issue.state = "open" + + repo_mock = MagicMock() + repo_mock.get_issue.side_effect = lambda n: issue if n == 1 else ref_issue + g.get_repo.return_value = repo_mock + g.auth = MagicMock() + g.auth.token = "tok" + + assert is_blocked(g, "o", "r", 1) is True + + def test_blocked_by_sub_issue_parent(self): + g = MagicMock() + issue = MagicMock() + issue.body = "Normal issue" + g.get_repo.return_value.get_issue.return_value = issue + g.auth = MagicMock() + g.auth.token = "tok" + with patch("clayde.github._has_blocking_sub_issue_parents", return_value=True): + assert is_blocked(g, "o", "r", 1) is True + + def test_timeline_failure_does_not_block(self): + g = MagicMock() + issue = MagicMock() + issue.body = "Normal issue" + g.get_repo.return_value.get_issue.return_value = issue + g.auth = MagicMock() + g.auth.token = "tok" + with patch("clayde.github._has_blocking_sub_issue_parents", side_effect=Exception("fail")): + # Should not raise, and should not block + assert is_blocked(g, "o", "r", 1) is False + + +class TestHasBlockingReferences: + def test_same_repo_blocked_by(self): + g = MagicMock() + ref_issue = MagicMock() + ref_issue.state = "open" + g.get_repo.return_value.get_issue.return_value = ref_issue + assert _has_blocking_references(g, "o", "r", "blocked by #5") is True + + def test_same_repo_depends_on(self): + g = MagicMock() + ref_issue = MagicMock() + ref_issue.state = "open" + g.get_repo.return_value.get_issue.return_value = ref_issue + assert _has_blocking_references(g, "o", "r", "depends on #5") is True + + def test_cross_repo_blocked_by(self): + g = MagicMock() + ref_issue = MagicMock() + ref_issue.state = "open" + g.get_repo.return_value.get_issue.return_value = ref_issue + assert _has_blocking_references(g, "o", "r", "blocked by other/repo#3") is True + g.get_repo.assert_called_with("other/repo") + + def test_closed_reference_not_blocking(self): + g = MagicMock() + ref_issue = MagicMock() + ref_issue.state = "closed" + g.get_repo.return_value.get_issue.return_value = ref_issue + assert _has_blocking_references(g, "o", "r", "blocked by #5") is False + + def test_no_patterns(self): + g = MagicMock() + assert _has_blocking_references(g, "o", "r", "no blocking text here") is False + + def test_github_exception_ignored(self): + g = MagicMock() + g.get_repo.return_value.get_issue.side_effect = GithubException(404, "not found", None) + assert _has_blocking_references(g, "o", "r", "blocked by #99") is False + + +class TestAddPrReviewer: + def test_requests_review(self): + g = MagicMock() + add_pr_reviewer(g, "alice", "repo", 5, "bob") + g.get_repo.return_value.get_pull.assert_called_once_with(5) + g.get_repo.return_value.get_pull.return_value.create_review_request.assert_called_once_with(reviewers=["bob"]) + + def test_handles_failure_gracefully(self): + g = MagicMock() + g.get_repo.return_value.get_pull.side_effect = GithubException(422, "error", None) + # Should not raise + add_pr_reviewer(g, "alice", "repo", 5, "bob") + + +class TestGetPrReviews: + def test_returns_reviews(self): + g = MagicMock() + reviews = [MagicMock(), MagicMock()] + g.get_repo.return_value.get_pull.return_value.get_reviews.return_value = reviews + result = get_pr_reviews(g, "alice", "repo", 5) + assert result == reviews + + +class TestGetPrReviewComments: + def test_returns_review_comments(self): + g = MagicMock() + comments = [MagicMock()] + g.get_repo.return_value.get_pull.return_value.get_review_comments.return_value = comments + result = get_pr_review_comments(g, "alice", "repo", 5) + assert result == comments + + +class TestGetIssueAuthor: + def test_returns_author_login(self): + g = MagicMock() + g.get_repo.return_value.get_issue.return_value.user.login = "alice" + assert get_issue_author(g, "o", "r", 1) == "alice" diff --git a/tests/test_orchestrator.py b/tests/test_orchestrator.py index bcb4a30..47143cd 100644 --- a/tests/test_orchestrator.py +++ b/tests/test_orchestrator.py @@ -5,17 +5,21 @@ import pytest from clayde.orchestrator import ( - _handle_awaiting_approval, + _handle_awaiting_plan, + _handle_awaiting_preliminary, _handle_interrupted, _handle_new_issue, + _handle_pr_open, + _has_new_comments, main, ) -def _mock_settings(enabled=False, github_token="tok"): +def _mock_settings(enabled=False, github_token="tok", github_username="ClaydeCode"): s = MagicMock() s.enabled = enabled s.github_token = github_token + s.github_username = github_username return s @@ -72,71 +76,272 @@ def test_skips_done_issues(self): patch("clayde.orchestrator.load_state", return_value=state), \ patch("clayde.orchestrator.plan") as mock_plan: main() - mock_plan.run.assert_not_called() + mock_plan.run_preliminary.assert_not_called() + + def test_dispatches_awaiting_preliminary(self): + issue = MagicMock() + issue.html_url = "url1" + state = {"issues": {"url1": { + "status": "awaiting_preliminary_approval", + "owner": "o", "repo": "r", "number": 1, + "preliminary_comment_id": 100, + }}} + 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=[issue]), \ + patch("clayde.orchestrator.load_state", return_value=state), \ + patch("clayde.orchestrator._handle_awaiting_preliminary") as mock_handle: + main() + mock_handle.assert_called_once() + + def test_dispatches_awaiting_plan(self): + issue = MagicMock() + issue.html_url = "url1" + state = {"issues": {"url1": { + "status": "awaiting_plan_approval", + "owner": "o", "repo": "r", "number": 1, + "plan_comment_id": 200, + }}} + 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=[issue]), \ + patch("clayde.orchestrator.load_state", return_value=state), \ + patch("clayde.orchestrator._handle_awaiting_plan") as mock_handle: + main() + mock_handle.assert_called_once() + + def test_dispatches_pr_open(self): + issue = MagicMock() + issue.html_url = "url1" + state = {"issues": {"url1": { + "status": "pr_open", + "owner": "o", "repo": "r", "number": 1, + "pr_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=[issue]), \ + patch("clayde.orchestrator.load_state", return_value=state), \ + patch("clayde.orchestrator._handle_pr_open") as mock_handle: + main() + mock_handle.assert_called_once() + + def test_backward_compat_awaiting_approval(self): + """Old 'awaiting_approval' status maps to 'awaiting_plan_approval'.""" + issue = MagicMock() + issue.html_url = "url1" + state = {"issues": {"url1": { + "status": "awaiting_approval", + "owner": "o", "repo": "r", "number": 1, + "plan_comment_id": 200, + }}} + 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=[issue]), \ + patch("clayde.orchestrator.load_state", return_value=state), \ + patch("clayde.orchestrator._handle_awaiting_plan") as mock_handle: + main() + mock_handle.assert_called_once() class TestHandleNewIssue: - def test_skips_unauthorized(self): + def test_skips_blocked_issue(self): g = MagicMock() issue = MagicMock() - with patch("clayde.orchestrator.is_issue_authorized", return_value=False), \ + issue.html_url = "https://github.com/o/r/issues/1" + with patch("clayde.orchestrator.is_blocked", return_value=True), \ + patch("clayde.orchestrator.parse_issue_url", return_value=("o", "r", 1)), \ patch("clayde.orchestrator.plan") as mock_plan: - _handle_new_issue(g, issue, "url") - mock_plan.run.assert_not_called() + _handle_new_issue(g, issue, issue.html_url) + mock_plan.run_preliminary.assert_not_called() - def test_runs_plan_when_authorized(self): + def test_skips_no_visible_content(self): g = MagicMock() issue = MagicMock() - with patch("clayde.orchestrator.is_issue_authorized", return_value=True), \ + issue.html_url = "https://github.com/o/r/issues/1" + with patch("clayde.orchestrator.is_blocked", return_value=False), \ + patch("clayde.orchestrator.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.orchestrator.fetch_issue_comments", return_value=[]), \ + patch("clayde.orchestrator.has_visible_content", return_value=False), \ patch("clayde.orchestrator.plan") as mock_plan: - _handle_new_issue(g, issue, "url") - mock_plan.run.assert_called_once_with("url") + _handle_new_issue(g, issue, issue.html_url) + mock_plan.run_preliminary.assert_not_called() + + def test_runs_preliminary_plan_when_visible(self): + g = MagicMock() + issue = MagicMock() + issue.html_url = "https://github.com/o/r/issues/1" + with patch("clayde.orchestrator.is_blocked", return_value=False), \ + patch("clayde.orchestrator.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.orchestrator.fetch_issue_comments", return_value=[]), \ + patch("clayde.orchestrator.has_visible_content", return_value=True), \ + patch("clayde.orchestrator.plan") as mock_plan: + _handle_new_issue(g, issue, issue.html_url) + mock_plan.run_preliminary.assert_called_once_with(issue.html_url) def test_sets_failed_on_exception(self): g = MagicMock() issue = MagicMock() - with patch("clayde.orchestrator.is_issue_authorized", return_value=True), \ + issue.html_url = "https://github.com/o/r/issues/1" + with patch("clayde.orchestrator.is_blocked", return_value=False), \ + patch("clayde.orchestrator.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.orchestrator.fetch_issue_comments", return_value=[]), \ + patch("clayde.orchestrator.has_visible_content", return_value=True), \ patch("clayde.orchestrator.plan") as mock_plan, \ patch("clayde.orchestrator.update_issue_state") as mock_update: - mock_plan.run.side_effect = RuntimeError("boom") - _handle_new_issue(g, issue, "url") + mock_plan.run_preliminary.side_effect = RuntimeError("boom") + _handle_new_issue(g, issue, issue.html_url) + mock_update.assert_called_once_with(issue.html_url, {"status": "failed"}) + + def test_proceeds_if_blocked_check_fails(self): + """If blocked-check raises, proceed anyway (fail open).""" + g = MagicMock() + issue = MagicMock() + issue.html_url = "https://github.com/o/r/issues/1" + with patch("clayde.orchestrator.is_blocked", side_effect=Exception("API error")), \ + patch("clayde.orchestrator.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.orchestrator.fetch_issue_comments", return_value=[]), \ + patch("clayde.orchestrator.has_visible_content", return_value=True), \ + patch("clayde.orchestrator.plan") as mock_plan: + _handle_new_issue(g, issue, issue.html_url) + mock_plan.run_preliminary.assert_called_once() + + +class TestHandleAwaitingPreliminary: + def test_does_nothing_when_not_approved_and_no_new_comments(self): + g = MagicMock() + entry = {"owner": "o", "repo": "r", "number": 1, "preliminary_comment_id": 100} + with patch("clayde.orchestrator._has_new_comments", return_value=False), \ + patch("clayde.orchestrator.is_plan_approved", return_value=False), \ + patch("clayde.orchestrator.plan") as mock_plan: + _handle_awaiting_preliminary(g, "url", entry) + mock_plan.run_thorough.assert_not_called() + mock_plan.run_update.assert_not_called() + + def test_runs_thorough_when_approved(self): + g = MagicMock() + entry = {"owner": "o", "repo": "r", "number": 1, "preliminary_comment_id": 100} + with patch("clayde.orchestrator._has_new_comments", return_value=False), \ + patch("clayde.orchestrator.is_plan_approved", return_value=True), \ + patch("clayde.orchestrator.plan") as mock_plan: + _handle_awaiting_preliminary(g, "url", entry) + mock_plan.run_thorough.assert_called_once_with("url") + + def test_runs_update_when_new_comments(self): + g = MagicMock() + entry = {"owner": "o", "repo": "r", "number": 1, "preliminary_comment_id": 100} + with patch("clayde.orchestrator._has_new_comments", return_value=True), \ + patch("clayde.orchestrator.plan") as mock_plan: + _handle_awaiting_preliminary(g, "url", entry) + mock_plan.run_update.assert_called_once_with("url", "preliminary") + + def test_sets_failed_on_thorough_exception(self): + g = MagicMock() + entry = {"owner": "o", "repo": "r", "number": 1, "preliminary_comment_id": 100} + with patch("clayde.orchestrator._has_new_comments", return_value=False), \ + patch("clayde.orchestrator.is_plan_approved", return_value=True), \ + patch("clayde.orchestrator.plan") as mock_plan, \ + patch("clayde.orchestrator.update_issue_state") as mock_update: + mock_plan.run_thorough.side_effect = RuntimeError("boom") + _handle_awaiting_preliminary(g, "url", entry) + mock_update.assert_called_once_with("url", {"status": "failed"}) + + def test_marks_failed_if_no_preliminary_comment_id(self): + g = MagicMock() + entry = {"owner": "o", "repo": "r", "number": 1} + with patch("clayde.orchestrator.update_issue_state") as mock_update: + _handle_awaiting_preliminary(g, "url", entry) mock_update.assert_called_once_with("url", {"status": "failed"}) -class TestHandleAwaitingApproval: - def test_does_nothing_when_not_approved(self): +class TestHandleAwaitingPlan: + def test_does_nothing_when_not_approved_and_no_new_comments(self): g = MagicMock() - entry = {"owner": "o", "repo": "r", "number": 1, "plan_comment_id": 100} - with patch("clayde.orchestrator.is_plan_approved", return_value=False), \ + entry = {"owner": "o", "repo": "r", "number": 1, "plan_comment_id": 200} + with patch("clayde.orchestrator._has_new_comments", return_value=False), \ + patch("clayde.orchestrator.is_plan_approved", return_value=False), \ patch("clayde.orchestrator.implement") as mock_impl: - _handle_awaiting_approval(g, "url", entry) + _handle_awaiting_plan(g, "url", entry) mock_impl.run.assert_not_called() def test_runs_implement_when_approved(self): g = MagicMock() - entry = {"owner": "o", "repo": "r", "number": 1, "plan_comment_id": 100} - with patch("clayde.orchestrator.is_plan_approved", return_value=True), \ + entry = {"owner": "o", "repo": "r", "number": 1, "plan_comment_id": 200} + with patch("clayde.orchestrator._has_new_comments", return_value=False), \ + patch("clayde.orchestrator.is_plan_approved", return_value=True), \ patch("clayde.orchestrator.implement") as mock_impl: - _handle_awaiting_approval(g, "url", entry) + _handle_awaiting_plan(g, "url", entry) mock_impl.run.assert_called_once_with("url") + def test_runs_update_when_new_comments(self): + g = MagicMock() + entry = {"owner": "o", "repo": "r", "number": 1, "plan_comment_id": 200} + with patch("clayde.orchestrator._has_new_comments", return_value=True), \ + patch("clayde.orchestrator.plan") as mock_plan: + _handle_awaiting_plan(g, "url", entry) + mock_plan.run_update.assert_called_once_with("url", "thorough") + def test_sets_failed_on_exception(self): g = MagicMock() - entry = {"owner": "o", "repo": "r", "number": 1, "plan_comment_id": 100} - with patch("clayde.orchestrator.is_plan_approved", return_value=True), \ + entry = {"owner": "o", "repo": "r", "number": 1, "plan_comment_id": 200} + with patch("clayde.orchestrator._has_new_comments", return_value=False), \ + patch("clayde.orchestrator.is_plan_approved", return_value=True), \ patch("clayde.orchestrator.implement") as mock_impl, \ patch("clayde.orchestrator.update_issue_state") as mock_update: mock_impl.run.side_effect = RuntimeError("boom") - _handle_awaiting_approval(g, "url", entry) + _handle_awaiting_plan(g, "url", entry) + mock_update.assert_called_once_with("url", {"status": "failed"}) + + def test_marks_failed_if_no_plan_comment_id(self): + g = MagicMock() + entry = {"owner": "o", "repo": "r", "number": 1} + with patch("clayde.orchestrator.update_issue_state") as mock_update: + _handle_awaiting_plan(g, "url", entry) + mock_update.assert_called_once_with("url", {"status": "failed"}) + + +class TestHandlePrOpen: + def test_runs_review(self): + g = MagicMock() + entry = {"number": 1, "pr_url": "https://github.com/o/r/pull/5"} + with patch("clayde.orchestrator.review") as mock_review: + _handle_pr_open(g, "url", entry) + mock_review.run.assert_called_once_with("url") + + def test_sets_failed_on_exception(self): + g = MagicMock() + entry = {"number": 1} + with patch("clayde.orchestrator.review") as mock_review, \ + patch("clayde.orchestrator.update_issue_state") as mock_update: + mock_review.run.side_effect = RuntimeError("boom") + _handle_pr_open(g, "url", entry) mock_update.assert_called_once_with("url", {"status": "failed"}) class TestHandleInterrupted: + def test_retries_preliminary_planning(self): + entry = {"interrupted_phase": "preliminary_planning"} + with patch("clayde.orchestrator.plan") as mock_plan: + _handle_interrupted("url", entry) + mock_plan.run_preliminary.assert_called_once_with("url") + def test_retries_planning(self): entry = {"interrupted_phase": "planning"} with patch("clayde.orchestrator.plan") as mock_plan: _handle_interrupted("url", entry) - mock_plan.run.assert_called_once_with("url") + mock_plan.run_thorough.assert_called_once_with("url") def test_retries_implementing(self): entry = {"interrupted_phase": "implementing"} @@ -144,18 +349,71 @@ def test_retries_implementing(self): _handle_interrupted("url", entry) mock_impl.run.assert_called_once_with("url") + def test_retries_addressing_review(self): + entry = {"interrupted_phase": "addressing_review"} + with patch("clayde.orchestrator.review") as mock_review: + _handle_interrupted("url", entry) + mock_review.run.assert_called_once_with("url") + def test_skips_unknown_phase(self): entry = {"interrupted_phase": "unknown"} with patch("clayde.orchestrator.plan") as mock_plan, \ patch("clayde.orchestrator.implement") as mock_impl: _handle_interrupted("url", entry) - mock_plan.run.assert_not_called() + mock_plan.run_preliminary.assert_not_called() + mock_plan.run_thorough.assert_not_called() mock_impl.run.assert_not_called() def test_stays_interrupted_on_error(self): - entry = {"interrupted_phase": "planning"} + entry = {"interrupted_phase": "preliminary_planning"} with patch("clayde.orchestrator.plan") as mock_plan, \ patch("clayde.orchestrator.update_issue_state") as mock_update: - mock_plan.run.side_effect = RuntimeError("boom") + mock_plan.run_preliminary.side_effect = RuntimeError("boom") _handle_interrupted("url", entry) mock_update.assert_called_once_with("url", {"status": "interrupted"}) + + +class TestHasNewComments: + def test_detects_new_visible_comments(self): + g = MagicMock() + comment = MagicMock() + comment.id = 200 + comment.user.login = "alice" + with patch("clayde.orchestrator.fetch_issue_comments", return_value=[comment]), \ + patch("clayde.orchestrator.filter_comments", return_value=[comment]), \ + patch("clayde.orchestrator.get_settings", return_value=_mock_settings()): + entry = {"last_seen_comment_id": 100} + assert _has_new_comments(g, "o", "r", 1, entry) is True + + def test_ignores_clayde_comments(self): + g = MagicMock() + comment = MagicMock() + comment.id = 200 + comment.user.login = "ClaydeCode" + with patch("clayde.orchestrator.fetch_issue_comments", return_value=[comment]), \ + patch("clayde.orchestrator.filter_comments", return_value=[comment]), \ + patch("clayde.orchestrator.get_settings", return_value=_mock_settings()): + entry = {"last_seen_comment_id": 100} + assert _has_new_comments(g, "o", "r", 1, entry) is False + + def test_no_new_comments(self): + g = MagicMock() + comment = MagicMock() + comment.id = 50 + comment.user.login = "alice" + with patch("clayde.orchestrator.fetch_issue_comments", return_value=[comment]), \ + patch("clayde.orchestrator.filter_comments", return_value=[comment]), \ + patch("clayde.orchestrator.get_settings", return_value=_mock_settings()): + entry = {"last_seen_comment_id": 100} + assert _has_new_comments(g, "o", "r", 1, entry) is False + + def test_invisible_comments_ignored(self): + g = MagicMock() + comment = MagicMock() + comment.id = 200 + comment.user.login = "bob" + with patch("clayde.orchestrator.fetch_issue_comments", return_value=[comment]), \ + patch("clayde.orchestrator.filter_comments", return_value=[]), \ + patch("clayde.orchestrator.get_settings", return_value=_mock_settings()): + entry = {"last_seen_comment_id": 100} + assert _has_new_comments(g, "o", "r", 1, entry) is False diff --git a/tests/test_safety.py b/tests/test_safety.py index 3df0bea..aa4a5a8 100644 --- a/tests/test_safety.py +++ b/tests/test_safety.py @@ -2,7 +2,14 @@ from unittest.mock import MagicMock, patch -from clayde.safety import _has_whitelisted_reaction, is_issue_authorized, is_plan_approved +from clayde.safety import ( + _has_whitelisted_reaction, + filter_comments, + has_visible_content, + is_comment_visible, + is_issue_visible, + is_plan_approved, +) def _make_reaction(content, login): @@ -18,33 +25,113 @@ def _mock_settings(users): return s -class TestIsIssueAuthorized: - def test_whitelisted_author(self): +def _make_comment(login, reactions=None): + c = MagicMock() + c.user.login = login + c.get_reactions.return_value = reactions or [] + return c + + +class TestIsCommentVisible: + def test_visible_if_whitelisted_author(self): + c = _make_comment("alice") + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + assert is_comment_visible(c) is True + + def test_visible_if_whitelisted_thumbsup(self): + c = _make_comment("bob", [_make_reaction("+1", "alice")]) + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + assert is_comment_visible(c) is True + + def test_not_visible_without_whitelisted_approval(self): + c = _make_comment("bob", [_make_reaction("+1", "charlie")]) + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + assert is_comment_visible(c) is False + + def test_not_visible_with_no_reactions(self): + c = _make_comment("bob") + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + assert is_comment_visible(c) is False + + +class TestFilterComments: + def test_filters_out_invisible_comments(self): + visible = _make_comment("alice") + invisible = _make_comment("bob") + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + result = filter_comments([visible, invisible]) + assert result == [visible] + + def test_empty_input(self): + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + assert filter_comments([]) == [] + + def test_all_visible(self): + c1 = _make_comment("alice") + c2 = _make_comment("bob", [_make_reaction("+1", "alice")]) + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + result = filter_comments([c1, c2]) + assert result == [c1, c2] + + def test_all_filtered_out(self): + c1 = _make_comment("bob") + c2 = _make_comment("charlie") + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + result = filter_comments([c1, c2]) + assert result == [] + + +class TestIsIssueVisible: + def test_visible_if_whitelisted_author(self): issue = MagicMock() issue.user.login = "alice" with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): - assert is_issue_authorized(issue) is True + assert is_issue_visible(issue) is True - def test_non_whitelisted_author_with_thumbsup(self): + def test_visible_if_whitelisted_reaction(self): issue = MagicMock() issue.user.login = "bob" issue.get_reactions.return_value = [_make_reaction("+1", "alice")] with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): - assert is_issue_authorized(issue) is True + assert is_issue_visible(issue) is True + + def test_not_visible_without_approval(self): + issue = MagicMock() + issue.user.login = "bob" + issue.get_reactions.return_value = [] + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + assert is_issue_visible(issue) is False + + +class TestHasVisibleContent: + def test_true_when_issue_is_visible(self): + issue = MagicMock() + issue.user.login = "alice" + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + assert has_visible_content(issue, []) is True - def test_non_whitelisted_author_without_approval(self): + def test_true_when_visible_comments_exist(self): issue = MagicMock() issue.user.login = "bob" - issue.get_reactions.return_value = [_make_reaction("+1", "charlie")] + issue.get_reactions.return_value = [] + visible_comment = _make_comment("alice") + with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): + assert has_visible_content(issue, [visible_comment]) is True + + def test_false_when_nothing_visible(self): + issue = MagicMock() + issue.user.login = "bob" + issue.get_reactions.return_value = [] + invisible_comment = _make_comment("charlie") with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): - assert is_issue_authorized(issue) is False + assert has_visible_content(issue, [invisible_comment]) is False - def test_no_reactions(self): + def test_false_when_no_comments_and_invisible_issue(self): issue = MagicMock() issue.user.login = "bob" issue.get_reactions.return_value = [] with patch("clayde.safety.get_settings", return_value=_mock_settings(["alice"])): - assert is_issue_authorized(issue) is False + assert has_visible_content(issue, []) is False class TestIsPlanApproved: diff --git a/tests/test_tasks_implement.py b/tests/test_tasks_implement.py index 05ad9c1..2370cfb 100644 --- a/tests/test_tasks_implement.py +++ b/tests/test_tasks_implement.py @@ -5,6 +5,7 @@ from clayde.claude import UsageLimitError from clayde.tasks.implement import ( + _assign_reviewer_and_finish, _checkout_wip_branch, _collect_discussion, _post_result, @@ -60,8 +61,40 @@ def test_posts_pr_url(self): assert "complete" in body.lower() +class TestAssignReviewerAndFinish: + def test_assigns_reviewer_and_sets_pr_open(self): + g = MagicMock() + span = MagicMock() + with patch("clayde.tasks.implement.get_issue_author", return_value="alice"), \ + patch("clayde.tasks.implement.parse_pr_url", return_value=("o", "r", 5)), \ + patch("clayde.tasks.implement.add_pr_reviewer") as mock_reviewer, \ + patch("clayde.tasks.implement.post_comment"), \ + patch("clayde.tasks.implement.update_issue_state") as mock_update: + _assign_reviewer_and_finish(g, "o", "r", 1, "url", "https://github.com/o/r/pull/5", span) + + mock_reviewer.assert_called_once_with(g, "o", "r", 5, "alice") + mock_update.assert_called_once() + update_data = mock_update.call_args[0][1] + assert update_data["status"] == "pr_open" + assert update_data["pr_url"] == "https://github.com/o/r/pull/5" + assert update_data["last_seen_review_id"] == 0 + + def test_handles_reviewer_failure_gracefully(self): + g = MagicMock() + span = MagicMock() + with patch("clayde.tasks.implement.get_issue_author", side_effect=Exception("fail")), \ + patch("clayde.tasks.implement.post_comment"), \ + patch("clayde.tasks.implement.update_issue_state") as mock_update: + # Should not raise + _assign_reviewer_and_finish(g, "o", "r", 1, "url", "https://github.com/o/r/pull/5", span) + + # Status should still be set to pr_open + mock_update.assert_called_once() + assert mock_update.call_args[0][1]["status"] == "pr_open" + + class TestRun: - def test_full_success_creates_pr(self, tmp_path): + def test_full_success_creates_pr_and_assigns_reviewer(self, tmp_path): with patch("clayde.tasks.implement.get_github_client") as mock_gc, \ patch("clayde.tasks.implement.parse_issue_url", return_value=("o", "r", 1)), \ patch("clayde.tasks.implement.get_issue_state", return_value={"plan_comment_id": 100}), \ @@ -71,20 +104,19 @@ def test_full_success_creates_pr(self, tmp_path): patch("clayde.tasks.implement.ensure_repo", return_value="/tmp/repo"), \ patch("clayde.tasks.implement.fetch_comment") as mock_fc, \ patch("clayde.tasks.implement.fetch_issue_comments", return_value=[]), \ + patch("clayde.tasks.implement.filter_comments", return_value=[]), \ patch("clayde.tasks.implement._build_prompt", return_value="prompt"), \ patch("clayde.tasks.implement.invoke_claude", return_value="IMPLEMENTATION_COMPLETE"), \ patch("clayde.tasks.implement.find_open_pr", return_value=None), \ patch("clayde.tasks.implement.create_pull_request", return_value="https://github.com/o/r/pull/5") as mock_cpr, \ - patch("clayde.tasks.implement._post_result"), \ + patch("clayde.tasks.implement._assign_reviewer_and_finish") as mock_finish, \ patch("clayde.tasks.implement.DATA_DIR", tmp_path): mock_fc.return_value.body = "plan text" mock_fi.return_value.title = "Test issue" run("https://github.com/o/r/issues/1") mock_cpr.assert_called_once() - last_call = mock_update.call_args_list[-1] - assert last_call[0][1]["status"] == "done" - assert last_call[0][1]["pr_url"] == "https://github.com/o/r/pull/5" + mock_finish.assert_called_once() def test_existing_pr_reused(self, tmp_path): with patch("clayde.tasks.implement.get_github_client") as mock_gc, \ @@ -96,19 +128,18 @@ def test_existing_pr_reused(self, tmp_path): patch("clayde.tasks.implement.ensure_repo", return_value="/tmp/repo"), \ patch("clayde.tasks.implement.fetch_comment") as mock_fc, \ patch("clayde.tasks.implement.fetch_issue_comments", return_value=[]), \ + patch("clayde.tasks.implement.filter_comments", return_value=[]), \ patch("clayde.tasks.implement._build_prompt", return_value="prompt"), \ patch("clayde.tasks.implement.invoke_claude", return_value="IMPLEMENTATION_COMPLETE"), \ patch("clayde.tasks.implement.find_open_pr", return_value="https://github.com/o/r/pull/5"), \ patch("clayde.tasks.implement.create_pull_request") as mock_cpr, \ - patch("clayde.tasks.implement._post_result"), \ + patch("clayde.tasks.implement._assign_reviewer_and_finish") as mock_finish, \ patch("clayde.tasks.implement.DATA_DIR", tmp_path): mock_fc.return_value.body = "plan text" run("https://github.com/o/r/issues/1") mock_cpr.assert_not_called() - last_call = mock_update.call_args_list[-1] - assert last_call[0][1]["status"] == "done" - assert last_call[0][1]["pr_url"] == "https://github.com/o/r/pull/5" + mock_finish.assert_called_once() def test_usage_limit_sets_interrupted(self, tmp_path): with patch("clayde.tasks.implement.get_github_client"), \ @@ -120,6 +151,7 @@ def test_usage_limit_sets_interrupted(self, tmp_path): patch("clayde.tasks.implement.ensure_repo", return_value="/tmp/repo"), \ patch("clayde.tasks.implement.fetch_comment") as mock_fc, \ patch("clayde.tasks.implement.fetch_issue_comments", return_value=[]), \ + patch("clayde.tasks.implement.filter_comments", return_value=[]), \ patch("clayde.tasks.implement._build_prompt", return_value="prompt"), \ patch("clayde.tasks.implement.invoke_claude", side_effect=UsageLimitError("limit")), \ patch("clayde.tasks.implement.DATA_DIR", tmp_path): @@ -136,13 +168,12 @@ def test_resumes_interrupted_with_existing_pr(self): patch("clayde.tasks.implement.parse_issue_url", return_value=("o", "r", 1)), \ patch("clayde.tasks.implement.get_issue_state", return_value=state), \ patch("clayde.tasks.implement.find_open_pr", return_value="https://github.com/o/r/pull/5"), \ - patch("clayde.tasks.implement.post_comment"), \ - patch("clayde.tasks.implement.update_issue_state") as mock_update, \ + patch("clayde.tasks.implement._assign_reviewer_and_finish") as mock_finish, \ patch("clayde.tasks.implement.invoke_claude") as mock_claude: run("url") mock_claude.assert_not_called() - mock_update.assert_called_once_with("url", {"status": "done", "pr_url": "https://github.com/o/r/pull/5"}) + mock_finish.assert_called_once() def test_pr_creation_failure_sets_interrupted(self, tmp_path): with patch("clayde.tasks.implement.get_github_client") as mock_gc, \ @@ -154,6 +185,7 @@ def test_pr_creation_failure_sets_interrupted(self, tmp_path): patch("clayde.tasks.implement.ensure_repo", return_value="/tmp/repo"), \ patch("clayde.tasks.implement.fetch_comment") as mock_fc, \ patch("clayde.tasks.implement.fetch_issue_comments", return_value=[]), \ + patch("clayde.tasks.implement.filter_comments", return_value=[]), \ patch("clayde.tasks.implement._build_prompt", return_value="prompt"), \ patch("clayde.tasks.implement.invoke_claude", return_value="IMPLEMENTATION_COMPLETE"), \ patch("clayde.tasks.implement.find_open_pr", return_value=None), \ @@ -179,6 +211,7 @@ def test_no_pr_fails_after_max_retries(self, tmp_path): patch("clayde.tasks.implement.ensure_repo", return_value="/tmp/repo"), \ patch("clayde.tasks.implement.fetch_comment") as mock_fc, \ patch("clayde.tasks.implement.fetch_issue_comments", return_value=[]), \ + patch("clayde.tasks.implement.filter_comments", return_value=[]), \ patch("clayde.tasks.implement._build_prompt", return_value="prompt"), \ patch("clayde.tasks.implement.invoke_claude", return_value="IMPLEMENTATION_COMPLETE"), \ patch("clayde.tasks.implement.find_open_pr", return_value=None), \ @@ -220,10 +253,11 @@ def test_conversation_path_passed_to_invoke_claude(self): patch("clayde.tasks.implement.ensure_repo", return_value="/tmp/repo"), \ patch("clayde.tasks.implement.fetch_comment") as mock_fc, \ patch("clayde.tasks.implement.fetch_issue_comments", return_value=[]), \ + patch("clayde.tasks.implement.filter_comments", return_value=[]), \ patch("clayde.tasks.implement._build_prompt", return_value="prompt"), \ patch("clayde.tasks.implement.invoke_claude", return_value="done") as mock_claude, \ patch("clayde.tasks.implement.find_open_pr", return_value="https://github.com/o/r/pull/5"), \ - patch("clayde.tasks.implement._post_result"), \ + patch("clayde.tasks.implement._assign_reviewer_and_finish"), \ patch("clayde.tasks.implement.DATA_DIR", Path("/tmp/test-data")): mock_fc.return_value.body = "plan text" mock_fi.return_value.title = "Test" @@ -234,32 +268,6 @@ def test_conversation_path_passed_to_invoke_claude(self): assert call_kwargs.kwargs["conversation_path"] is not None assert "o__r__issue-1" in str(call_kwargs.kwargs["conversation_path"]) - def test_conversation_preserved_on_success(self, tmp_path): - conv_dir = tmp_path / "conversations" - conv_dir.mkdir() - conv_file = conv_dir / "o__r__issue-1.json" - conv_file.write_text("[]") - - with patch("clayde.tasks.implement.get_github_client") as mock_gc, \ - patch("clayde.tasks.implement.parse_issue_url", return_value=("o", "r", 1)), \ - patch("clayde.tasks.implement.get_issue_state", return_value={"plan_comment_id": 100}), \ - patch("clayde.tasks.implement.update_issue_state"), \ - patch("clayde.tasks.implement.fetch_issue") as mock_fi, \ - patch("clayde.tasks.implement.get_default_branch", return_value="main"), \ - patch("clayde.tasks.implement.ensure_repo", return_value="/tmp/repo"), \ - patch("clayde.tasks.implement.fetch_comment") as mock_fc, \ - patch("clayde.tasks.implement.fetch_issue_comments", return_value=[]), \ - patch("clayde.tasks.implement._build_prompt", return_value="prompt"), \ - patch("clayde.tasks.implement.invoke_claude", return_value="done"), \ - patch("clayde.tasks.implement.find_open_pr", return_value="https://github.com/o/r/pull/5"), \ - patch("clayde.tasks.implement._post_result"), \ - patch("clayde.tasks.implement.DATA_DIR", tmp_path): - mock_fc.return_value.body = "plan text" - mock_fi.return_value.title = "Test" - run("https://github.com/o/r/issues/1") - - assert conv_file.exists() - def test_resumed_issue_checks_out_wip_branch(self): state = { "plan_comment_id": 100, @@ -276,10 +284,11 @@ def test_resumed_issue_checks_out_wip_branch(self): patch("clayde.tasks.implement.ensure_repo", return_value="/tmp/repo"), \ patch("clayde.tasks.implement.fetch_comment") as mock_fc, \ patch("clayde.tasks.implement.fetch_issue_comments", return_value=[]), \ + patch("clayde.tasks.implement.filter_comments", return_value=[]), \ patch("clayde.tasks.implement._build_prompt", return_value="prompt"), \ patch("clayde.tasks.implement.invoke_claude", return_value="done"), \ patch("clayde.tasks.implement.create_pull_request", return_value="https://github.com/o/r/pull/5"), \ - patch("clayde.tasks.implement._post_result"), \ + patch("clayde.tasks.implement._assign_reviewer_and_finish"), \ patch("clayde.tasks.implement._checkout_wip_branch") as mock_checkout, \ patch("clayde.tasks.implement.DATA_DIR", Path("/tmp/test-data")): mock_fc.return_value.body = "plan text" diff --git a/tests/test_tasks_plan.py b/tests/test_tasks_plan.py index b289727..3086cec 100644 --- a/tests/test_tasks_plan.py +++ b/tests/test_tasks_plan.py @@ -1,88 +1,230 @@ -"""Tests for clayde.tasks.plan.""" +"""Tests for clayde.tasks.plan — two-phase planning.""" from unittest.mock import MagicMock, patch from clayde.claude import UsageLimitError -from clayde.tasks.plan import _build_prompt, _post_plan_comment, run +from clayde.tasks.plan import ( + _build_preliminary_prompt, + _build_thorough_prompt, + _build_update_prompt, + _collect_discussion_after, + _parse_update_output, + _post_preliminary_comment, + _post_thorough_plan_comment, + run_preliminary, + run_thorough, + run_update, +) -class TestBuildPrompt: - def test_renders_template_with_issue_data(self): +def _mock_settings(users=None): + s = MagicMock() + s.whitelisted_users_list = users or ["alice"] + s.github_username = "ClaydeCode" + return s + + +class TestBuildPreliminaryPrompt: + def test_renders_with_issue_data(self): g = MagicMock() issue = MagicMock() issue.title = "Fix bug" issue.body = "There is a bug" issue.labels = [] + issue.user.login = "alice" comment = MagicMock() comment.user.login = "alice" comment.body = "I can confirm this" + comment.get_reactions.return_value = [] g.get_repo.return_value.get_issue.return_value.get_comments.return_value = [comment] - prompt = _build_prompt(g, issue, "owner", "repo", 42, "/tmp/repo") + settings = _mock_settings() + with patch("clayde.tasks.plan.get_settings", return_value=settings), \ + patch("clayde.safety.get_settings", return_value=settings): + prompt = _build_preliminary_prompt(g, issue, "owner", "repo", 42, "/tmp/repo") assert "Fix bug" in prompt assert "There is a bug" in prompt - assert "#42" in prompt or "42" in prompt - assert "@alice" in prompt - assert "I can confirm this" in prompt + assert "42" in prompt assert "/tmp/repo" in prompt + # Should be a preliminary plan prompt, not thorough + assert "preliminary" in prompt.lower() or "short" in prompt.lower() - def test_handles_empty_body(self): + def test_filters_invisible_comments(self): g = MagicMock() issue = MagicMock() issue.title = "Title" - issue.body = None + issue.body = "body" issue.labels = [] - g.get_repo.return_value.get_issue.return_value.get_comments.return_value = [] + issue.user.login = "alice" + + visible = MagicMock() + visible.user.login = "alice" + visible.body = "visible comment" + visible.get_reactions.return_value = [] + + invisible = MagicMock() + invisible.user.login = "bob" + invisible.body = "invisible comment" + invisible.get_reactions.return_value = [] - prompt = _build_prompt(g, issue, "owner", "repo", 1, "/path") - assert "(empty)" in prompt + g.get_repo.return_value.get_issue.return_value.get_comments.return_value = [visible, invisible] - def test_includes_labels(self): + settings = _mock_settings() + with patch("clayde.tasks.plan.get_settings", return_value=settings), \ + patch("clayde.safety.get_settings", return_value=settings): + prompt = _build_preliminary_prompt(g, issue, "owner", "repo", 1, "/path") + assert "visible comment" in prompt + assert "invisible comment" not in prompt + + def test_filters_issue_body_when_not_visible(self): g = MagicMock() issue = MagicMock() issue.title = "Title" - issue.body = "body" - label = MagicMock() - label.name = "bug" - issue.labels = [label] + issue.body = "secret body" + issue.labels = [] + issue.user.login = "bob" # not whitelisted + issue.get_reactions.return_value = [] + g.get_repo.return_value.get_issue.return_value.get_comments.return_value = [] - prompt = _build_prompt(g, issue, "owner", "repo", 1, "/path") - assert "bug" in prompt + settings = _mock_settings() + with patch("clayde.tasks.plan.get_settings", return_value=settings), \ + patch("clayde.safety.get_settings", return_value=settings): + prompt = _build_preliminary_prompt(g, issue, "owner", "repo", 1, "/path") + assert "secret body" not in prompt + assert "(filtered)" in prompt + + +class TestBuildThoroughPrompt: + def test_renders_with_preliminary_plan(self): + g = MagicMock() + issue = MagicMock() + issue.title = "Fix bug" + issue.body = "body" + issue.labels = [] + issue.user.login = "alice" + + settings = _mock_settings() + with patch("clayde.tasks.plan.get_settings", return_value=settings), \ + patch("clayde.safety.get_settings", return_value=settings): + prompt = _build_thorough_prompt( + g, issue, "owner", "repo", 1, "/path", + "my preliminary plan", "discussion text", + ) + assert "my preliminary plan" in prompt + assert "discussion text" in prompt + assert "thorough" in prompt.lower() or "detailed" in prompt.lower() -class TestPostPlanComment: +class TestBuildUpdatePrompt: + def test_renders_with_new_comments(self): + prompt = _build_update_prompt( + 1, "Title", "o", "r", "body", + "current plan text", "new comment text", "/path", + ) + assert "current plan text" in prompt + assert "new comment text" in prompt + assert "UPDATED_PLAN" in prompt + + +class TestPostPreliminaryComment: def test_posts_formatted_comment(self): g = MagicMock() mock_comment = MagicMock() mock_comment.id = 555 g.get_repo.return_value.get_issue.return_value.create_comment.return_value = mock_comment - result = _post_plan_comment(g, "owner", "repo", 1, "My plan text") + result = _post_preliminary_comment(g, "owner", "repo", 1, "My preliminary plan") assert result == 555 posted_body = g.get_repo.return_value.get_issue.return_value.create_comment.call_args[0][0] + assert "## Preliminary Plan" in posted_body + assert "My preliminary plan" in posted_body + assert "\U0001f44d" in posted_body + assert "preliminary" in posted_body.lower() + + +class TestPostThoroughPlanComment: + def test_posts_formatted_comment(self): + g = MagicMock() + mock_comment = MagicMock() + mock_comment.id = 666 + g.get_repo.return_value.get_issue.return_value.create_comment.return_value = mock_comment + + result = _post_thorough_plan_comment(g, "owner", "repo", 1, "My thorough plan") + assert result == 666 + posted_body = g.get_repo.return_value.get_issue.return_value.create_comment.call_args[0][0] assert "## Implementation Plan" in posted_body - assert "My plan text" in posted_body + assert "My thorough plan" in posted_body assert "\U0001f44d" in posted_body -class TestRun: +class TestCollectDiscussionAfter: + def test_collects_comments_after_id(self): + c1 = MagicMock() + c1.id = 100 + c1.user.login = "plan" + c1.body = "plan text" + + c2 = MagicMock() + c2.id = 101 + c2.user.login = "alice" + c2.body = "discussion" + + result = _collect_discussion_after([c1, c2], 100) + assert "discussion" in result + assert "plan text" not in result + + def test_none_when_no_comments_after(self): + c1 = MagicMock() + c1.id = 100 + result = _collect_discussion_after([c1], 100) + assert result == "(none)" + + +class TestParseUpdateOutput: + def test_parses_with_separator(self): + output = "Here is the summary\n---UPDATED_PLAN---\nHere is the updated plan" + summary, plan = _parse_update_output(output) + assert summary == "Here is the summary" + assert plan == "Here is the updated plan" + + def test_no_separator_returns_all_as_summary(self): + output = "Just a summary with no updated plan" + summary, plan = _parse_update_output(output) + assert summary == output + assert plan == "" + + def test_empty_output(self): + summary, plan = _parse_update_output("") + assert summary == "" + assert plan == "" + + +class TestRunPreliminary: def test_full_success(self): + mock_comment = MagicMock() + mock_comment.id = 500 + with patch("clayde.tasks.plan.get_github_client") as mock_gc, \ patch("clayde.tasks.plan.parse_issue_url", return_value=("o", "r", 1)), \ patch("clayde.tasks.plan.update_issue_state") as mock_update, \ patch("clayde.tasks.plan.fetch_issue") as mock_fi, \ patch("clayde.tasks.plan.get_default_branch", return_value="main"), \ patch("clayde.tasks.plan.ensure_repo", return_value="/tmp/repo"), \ - patch("clayde.tasks.plan._build_prompt", return_value="prompt"), \ - patch("clayde.tasks.plan.invoke_claude", return_value="x" * 250), \ - patch("clayde.tasks.plan._post_plan_comment", return_value=999): - run("https://github.com/o/r/issues/1") + patch("clayde.tasks.plan._build_preliminary_prompt", return_value="prompt"), \ + patch("clayde.tasks.plan.invoke_claude", return_value="x" * 150), \ + patch("clayde.tasks.plan._post_preliminary_comment", return_value=999), \ + patch("clayde.tasks.plan.fetch_issue_comments", return_value=[mock_comment]): + run_preliminary("https://github.com/o/r/issues/1") calls = mock_update.call_args_list - assert calls[0][0] == ("https://github.com/o/r/issues/1", {"status": "planning", "owner": "o", "repo": "r", "number": 1}) - assert calls[1][0] == ("https://github.com/o/r/issues/1", {"status": "awaiting_approval", "plan_comment_id": 999}) + assert calls[0][0] == ("https://github.com/o/r/issues/1", + {"status": "preliminary_planning", "owner": "o", "repo": "r", "number": 1}) + last = calls[-1][0][1] + assert last["status"] == "awaiting_preliminary_approval" + assert last["preliminary_comment_id"] == 999 + assert last["last_seen_comment_id"] == 500 def test_empty_plan_sets_failed(self): with patch("clayde.tasks.plan.get_github_client"), \ @@ -91,9 +233,9 @@ def test_empty_plan_sets_failed(self): patch("clayde.tasks.plan.fetch_issue"), \ patch("clayde.tasks.plan.get_default_branch", return_value="main"), \ patch("clayde.tasks.plan.ensure_repo", return_value="/tmp/repo"), \ - patch("clayde.tasks.plan._build_prompt", return_value="prompt"), \ + patch("clayde.tasks.plan._build_preliminary_prompt", return_value="prompt"), \ patch("clayde.tasks.plan.invoke_claude", return_value=" "): - run("url") + run_preliminary("url") last_call = mock_update.call_args_list[-1] assert last_call[0][1]["status"] == "failed" @@ -105,10 +247,155 @@ def test_usage_limit_sets_interrupted(self): patch("clayde.tasks.plan.fetch_issue"), \ patch("clayde.tasks.plan.get_default_branch", return_value="main"), \ patch("clayde.tasks.plan.ensure_repo", return_value="/tmp/repo"), \ - patch("clayde.tasks.plan._build_prompt", return_value="prompt"), \ + patch("clayde.tasks.plan._build_preliminary_prompt", return_value="prompt"), \ patch("clayde.tasks.plan.invoke_claude", side_effect=UsageLimitError("limit")): - run("url") + run_preliminary("url") + + last_call = mock_update.call_args_list[-1] + assert last_call[0][1]["status"] == "interrupted" + assert last_call[0][1]["interrupted_phase"] == "preliminary_planning" + + +class TestRunThorough: + def test_full_success(self): + mock_comment = MagicMock() + mock_comment.id = 600 + + with patch("clayde.tasks.plan.get_github_client") as mock_gc, \ + patch("clayde.tasks.plan.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.tasks.plan.update_issue_state") as mock_update, \ + patch("clayde.tasks.plan.fetch_issue") as mock_fi, \ + patch("clayde.tasks.plan.get_default_branch", return_value="main"), \ + patch("clayde.tasks.plan.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.state.get_issue_state", return_value={"preliminary_comment_id": 100}), \ + patch("clayde.tasks.plan.fetch_comment") as mock_fc, \ + patch("clayde.tasks.plan.fetch_issue_comments", return_value=[mock_comment]), \ + patch("clayde.tasks.plan.filter_comments", return_value=[]), \ + patch("clayde.tasks.plan._build_thorough_prompt", return_value="prompt"), \ + patch("clayde.tasks.plan.invoke_claude", return_value="x" * 250), \ + patch("clayde.tasks.plan._post_thorough_plan_comment", return_value=888): + mock_fc.return_value.body = "preliminary plan" + mock_fi.return_value.labels = [] + run_thorough("https://github.com/o/r/issues/1") + + calls = mock_update.call_args_list + assert calls[0][0] == ("https://github.com/o/r/issues/1", {"status": "planning"}) + last = calls[-1][0][1] + assert last["status"] == "awaiting_plan_approval" + assert last["plan_comment_id"] == 888 + + def test_usage_limit_sets_interrupted(self): + with patch("clayde.tasks.plan.get_github_client"), \ + patch("clayde.tasks.plan.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.tasks.plan.update_issue_state") as mock_update, \ + patch("clayde.tasks.plan.fetch_issue"), \ + patch("clayde.tasks.plan.get_default_branch", return_value="main"), \ + patch("clayde.tasks.plan.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.state.get_issue_state", return_value={"preliminary_comment_id": 100}), \ + patch("clayde.tasks.plan.fetch_comment") as mock_fc, \ + patch("clayde.tasks.plan.fetch_issue_comments", return_value=[]), \ + patch("clayde.tasks.plan.filter_comments", return_value=[]), \ + patch("clayde.tasks.plan._build_thorough_prompt", return_value="prompt"), \ + patch("clayde.tasks.plan.invoke_claude", side_effect=UsageLimitError("limit")): + mock_fc.return_value.body = "plan" + run_thorough("url") last_call = mock_update.call_args_list[-1] assert last_call[0][1]["status"] == "interrupted" assert last_call[0][1]["interrupted_phase"] == "planning" + + +class TestRunUpdate: + def test_updates_plan_and_posts_summary(self): + new_comment = MagicMock() + new_comment.id = 300 + new_comment.user.login = "alice" + new_comment.body = "please change X" + + last_comment = MagicMock() + last_comment.id = 400 + + with patch("clayde.tasks.plan.get_github_client") as mock_gc, \ + patch("clayde.tasks.plan.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.state.get_issue_state", return_value={ + "preliminary_comment_id": 100, + "last_seen_comment_id": 200, + }), \ + patch("clayde.tasks.plan.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.plan.fetch_issue") as mock_fi, \ + patch("clayde.tasks.plan.get_default_branch", return_value="main"), \ + patch("clayde.tasks.plan.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.tasks.plan.fetch_comment") as mock_fc, \ + patch("clayde.tasks.plan.fetch_issue_comments", side_effect=[ + [new_comment], + [last_comment], + ]), \ + patch("clayde.tasks.plan.filter_comments", return_value=[new_comment]), \ + patch("clayde.tasks.plan.is_issue_visible", return_value=True), \ + patch("clayde.tasks.plan.invoke_claude", return_value="Summary\n---UPDATED_PLAN---\nUpdated plan text"), \ + patch("clayde.tasks.plan.edit_comment") as mock_edit, \ + patch("clayde.tasks.plan.post_comment") as mock_post, \ + patch("clayde.tasks.plan.update_issue_state") as mock_update: + mock_fc.return_value.body = "current plan" + mock_fi.return_value.body = "issue body" + mock_fi.return_value.title = "Title" + run_update("url", "preliminary") + + mock_edit.assert_called_once() + mock_post.assert_called_once() + posted_body = mock_post.call_args[0][4] + assert "Plan updated" in posted_body + + def test_skips_when_no_new_comments(self): + old_comment = MagicMock() + old_comment.id = 50 + old_comment.user.login = "alice" + + with patch("clayde.tasks.plan.get_github_client"), \ + patch("clayde.tasks.plan.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.state.get_issue_state", return_value={ + "preliminary_comment_id": 100, + "last_seen_comment_id": 200, + }), \ + patch("clayde.tasks.plan.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.plan.fetch_issue") as mock_fi, \ + patch("clayde.tasks.plan.get_default_branch", return_value="main"), \ + patch("clayde.tasks.plan.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.tasks.plan.fetch_comment") as mock_fc, \ + patch("clayde.tasks.plan.fetch_issue_comments", return_value=[old_comment]), \ + patch("clayde.tasks.plan.filter_comments", return_value=[old_comment]), \ + patch("clayde.tasks.plan.is_issue_visible", return_value=True), \ + patch("clayde.tasks.plan.invoke_claude") as mock_claude: + mock_fc.return_value.body = "plan" + mock_fi.return_value.body = "body" + mock_fi.return_value.title = "Title" + run_update("url", "preliminary") + + mock_claude.assert_not_called() + + def test_ignores_clayde_comments(self): + clayde_comment = MagicMock() + clayde_comment.id = 300 + clayde_comment.user.login = "ClaydeCode" + + with patch("clayde.tasks.plan.get_github_client"), \ + patch("clayde.tasks.plan.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.state.get_issue_state", return_value={ + "preliminary_comment_id": 100, + "last_seen_comment_id": 200, + }), \ + patch("clayde.tasks.plan.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.plan.fetch_issue") as mock_fi, \ + patch("clayde.tasks.plan.get_default_branch", return_value="main"), \ + patch("clayde.tasks.plan.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.tasks.plan.fetch_comment") as mock_fc, \ + patch("clayde.tasks.plan.fetch_issue_comments", return_value=[clayde_comment]), \ + patch("clayde.tasks.plan.filter_comments", return_value=[clayde_comment]), \ + patch("clayde.tasks.plan.is_issue_visible", return_value=True), \ + patch("clayde.tasks.plan.invoke_claude") as mock_claude: + mock_fc.return_value.body = "plan" + mock_fi.return_value.body = "body" + mock_fi.return_value.title = "Title" + run_update("url", "preliminary") + + mock_claude.assert_not_called() diff --git a/tests/test_tasks_review.py b/tests/test_tasks_review.py new file mode 100644 index 0000000..38691a8 --- /dev/null +++ b/tests/test_tasks_review.py @@ -0,0 +1,204 @@ +"""Tests for clayde.tasks.review.""" + +from pathlib import Path +from unittest.mock import MagicMock, patch + +from clayde.claude import UsageLimitError +from clayde.tasks.review import _build_prompt, _format_reviews, run + + +def _mock_settings(): + s = MagicMock() + s.github_username = "ClaydeCode" + return s + + +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 fix the typo" + + result = _format_reviews([review], []) + assert "@alice" in result + assert "CHANGES_REQUESTED" in result + assert "Please fix the typo" in result + + def test_formats_review_with_inline_comments(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/main.py" + rc.line = 42 + rc.body = "This line looks wrong" + + result = _format_reviews([review], [rc]) + assert "src/main.py" in result + assert "42" in result + assert "This line looks wrong" in result + + def test_empty_reviews(self): + result = _format_reviews([], []) + assert result == "(no review content)" + + +class TestBuildPrompt: + def test_renders_template(self): + issue = MagicMock() + issue.title = "Fix bug" + issue.body = "body" + prompt = _build_prompt(issue, "o", "r", 1, "/path", "clayde/issue-1", "review text") + assert "Fix bug" in prompt + assert "review text" in prompt + assert "clayde/issue-1" in prompt + + +class TestRun: + def test_no_reviews_does_nothing(self): + with patch("clayde.tasks.review.get_github_client") as mock_gc, \ + patch("clayde.tasks.review.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.tasks.review.get_issue_state", return_value={ + "pr_url": "https://github.com/o/r/pull/5", + "last_seen_review_id": 0, + }), \ + patch("clayde.tasks.review.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.review.parse_pr_url", return_value=("o", "r", 5)), \ + patch("clayde.tasks.review.get_pr_reviews", return_value=[]), \ + patch("clayde.tasks.review.invoke_claude") as mock_claude: + run("url") + mock_claude.assert_not_called() + + def test_addresses_new_review(self, tmp_path): + review = MagicMock() + review.id = 100 + review.user.login = "alice" + review.state = "CHANGES_REQUESTED" + review.body = "Please change X" + + review_comment = MagicMock() + review_comment.pull_request_review_id = 100 + review_comment.path = "src/file.py" + review_comment.line = 10 + review_comment.body = "Fix this line" + + with patch("clayde.tasks.review.get_github_client") as mock_gc, \ + patch("clayde.tasks.review.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.tasks.review.get_issue_state", return_value={ + "pr_url": "https://github.com/o/r/pull/5", + "last_seen_review_id": 0, + "branch_name": "clayde/issue-1", + }), \ + patch("clayde.tasks.review.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.review.parse_pr_url", return_value=("o", "r", 5)), \ + patch("clayde.tasks.review.get_pr_reviews", return_value=[review]), \ + patch("clayde.tasks.review.get_pr_review_comments", return_value=[review_comment]), \ + patch("clayde.tasks.review.update_issue_state") as mock_update, \ + patch("clayde.tasks.review.fetch_issue") as mock_fi, \ + patch("clayde.tasks.review.get_default_branch", return_value="main"), \ + patch("clayde.tasks.review.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.tasks.review.invoke_claude", return_value="Changes made") as mock_claude, \ + patch("clayde.tasks.review.post_comment") as mock_post, \ + patch("clayde.tasks.review.DATA_DIR", tmp_path): + mock_fi.return_value.title = "Fix bug" + mock_fi.return_value.body = "body" + run("url") + + mock_claude.assert_called_once() + mock_post.assert_called_once() + posted_body = mock_post.call_args[0][4] + assert "Review addressed" in posted_body + # Should return to pr_open + last_update = mock_update.call_args_list[-1][0][1] + assert last_update["status"] == "pr_open" + assert last_update["last_seen_review_id"] == 100 + + def test_approval_marks_done(self): + review = MagicMock() + review.id = 100 + review.user.login = "alice" + review.state = "APPROVED" + review.body = "" + + with patch("clayde.tasks.review.get_github_client"), \ + patch("clayde.tasks.review.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.tasks.review.get_issue_state", return_value={ + "pr_url": "https://github.com/o/r/pull/5", + "last_seen_review_id": 0, + }), \ + patch("clayde.tasks.review.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.review.parse_pr_url", return_value=("o", "r", 5)), \ + patch("clayde.tasks.review.get_pr_reviews", return_value=[review]), \ + patch("clayde.tasks.review.get_pr_review_comments", return_value=[]), \ + patch("clayde.tasks.review.update_issue_state") as mock_update: + run("url") + + # Should update review id first, then set done + calls = mock_update.call_args_list + assert any(c[0][1].get("status") == "done" for c in calls) + + def test_usage_limit_sets_interrupted(self, tmp_path): + review = MagicMock() + review.id = 100 + review.user.login = "alice" + review.state = "CHANGES_REQUESTED" + review.body = "Fix it" + + with patch("clayde.tasks.review.get_github_client"), \ + patch("clayde.tasks.review.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.tasks.review.get_issue_state", return_value={ + "pr_url": "https://github.com/o/r/pull/5", + "last_seen_review_id": 0, + "branch_name": "clayde/issue-1", + }), \ + patch("clayde.tasks.review.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.review.parse_pr_url", return_value=("o", "r", 5)), \ + patch("clayde.tasks.review.get_pr_reviews", return_value=[review]), \ + patch("clayde.tasks.review.get_pr_review_comments", return_value=[]), \ + patch("clayde.tasks.review.update_issue_state") as mock_update, \ + patch("clayde.tasks.review.fetch_issue") as mock_fi, \ + patch("clayde.tasks.review.get_default_branch", return_value="main"), \ + patch("clayde.tasks.review.ensure_repo", return_value="/tmp/repo"), \ + patch("clayde.tasks.review.invoke_claude", side_effect=UsageLimitError("limit")), \ + patch("clayde.tasks.review.DATA_DIR", tmp_path): + mock_fi.return_value.title = "Fix bug" + mock_fi.return_value.body = "body" + run("url") + + last_call = mock_update.call_args_list[-1] + assert last_call[0][1]["status"] == "interrupted" + assert last_call[0][1]["interrupted_phase"] == "addressing_review" + + def test_no_pr_url_skips(self): + with patch("clayde.tasks.review.get_github_client"), \ + patch("clayde.tasks.review.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.tasks.review.get_issue_state", return_value={}), \ + patch("clayde.tasks.review.invoke_claude") as mock_claude: + run("url") + mock_claude.assert_not_called() + + def test_ignores_own_reviews(self): + review = MagicMock() + review.id = 100 + review.user.login = "ClaydeCode" + review.state = "COMMENTED" + review.body = "My own review" + + with patch("clayde.tasks.review.get_github_client"), \ + patch("clayde.tasks.review.parse_issue_url", return_value=("o", "r", 1)), \ + patch("clayde.tasks.review.get_issue_state", return_value={ + "pr_url": "https://github.com/o/r/pull/5", + "last_seen_review_id": 0, + }), \ + patch("clayde.tasks.review.get_settings", return_value=_mock_settings()), \ + patch("clayde.tasks.review.parse_pr_url", return_value=("o", "r", 5)), \ + patch("clayde.tasks.review.get_pr_reviews", return_value=[review]), \ + patch("clayde.tasks.review.invoke_claude") as mock_claude: + run("url") + mock_claude.assert_not_called() diff --git a/uv.lock b/uv.lock index 9354235..8984db9 100644 --- a/uv.lock +++ b/uv.lock @@ -183,6 +183,7 @@ dependencies = [ { name = "opentelemetry-sdk" }, { name = "pydantic-settings" }, { name = "pygithub" }, + { name = "requests" }, ] [package.optional-dependencies] @@ -200,6 +201,7 @@ requires-dist = [ { name = "pydantic-settings", specifier = ">=2.0" }, { name = "pygithub", specifier = ">=2.5.0" }, { name = "pytest", marker = "extra == 'dev'", specifier = ">=8.0" }, + { name = "requests", specifier = ">=2.31" }, ] provides-extras = ["dev"]