diff --git a/hermes_cli/kanban.py b/hermes_cli/kanban.py index 1e7169c26cf83..080734547a58d 100644 --- a/hermes_cli/kanban.py +++ b/hermes_cli/kanban.py @@ -24,6 +24,7 @@ from typing import Any, Optional from hermes_cli import kanban_db as kb +from hermes_cli import kanban_pr_review from hermes_cli import kanban_swarm as ks from hermes_cli.profiles import get_active_profile_name, get_profile_dir, seed_profile_skills @@ -514,6 +515,13 @@ def build_parser(parent_subparsers: argparse._SubParsersAction) -> argparse.Argu help='JSON dict of structured facts (e.g. \'{"changed_files": [...], ' '"tests_run": 12}\'). Stored on the closing run.') + p_pr_poll = sub.add_parser( + "pr-review-poll", + help="One-shot poll of a task's GitHub PR checks and review feedback", + ) + p_pr_poll.add_argument("task_id") + p_pr_poll.add_argument("--json", action="store_true") + p_edit = sub.add_parser( "edit", help="Edit recovery fields on an already-completed task", @@ -928,6 +936,7 @@ def _restore_board_env() -> None: "claim": _cmd_claim, "comment": _cmd_comment, "complete": _cmd_complete, + "pr-review-poll": _cmd_pr_review_poll, "edit": _cmd_edit, "block": _cmd_block, "schedule": _cmd_schedule, @@ -1901,6 +1910,23 @@ def _cmd_complete(args: argparse.Namespace) -> int: return 0 if not failed else 1 +def _cmd_pr_review_poll(args: argparse.Namespace) -> int: + with kb.connect() as conn: + result = kanban_pr_review.poll_task(conn, args.task_id) + if getattr(args, "json", False): + print(json.dumps(result.to_event_payload(), indent=2, ensure_ascii=False)) + return 0 + print(f"{args.task_id}: {result.summary}") + if result.action_items: + for item in result.action_items: + suffix = f" ({item.url})" if item.url else "" + print(f"- [{item.kind}] {item.title}: {item.state}{suffix}") + elif result.pending_items: + for item in result.pending_items: + print(f"- [pending] {item.title}: {item.state}") + return 0 + + def _cmd_edit(args: argparse.Namespace) -> int: raw_meta = getattr(args, "metadata", None) metadata = None diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index c89e697c98d29..d199d807c780d 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -2145,6 +2145,7 @@ def _synthesize_ended_run( task_id: str, *, outcome: str, + status: Optional[str] = None, summary: Optional[str] = None, error: Optional[str] = None, metadata: Optional[dict] = None, @@ -2182,7 +2183,7 @@ def _synthesize_ended_run( """, ( task_id, profile, step_key, - outcome, outcome, + status or outcome, outcome, summary, error, json.dumps(metadata, ensure_ascii=False) if metadata else None, now, now, @@ -2840,6 +2841,61 @@ def _scan_prose_for_phantom_ids( return [m for m in unique if m not in existing] +def extract_pr_metadata(metadata: Optional[dict]) -> Optional[dict]: + """Return normalized PR metadata from a worker handoff, if present.""" + if not isinstance(metadata, dict): + return None + sources: list[dict] = [metadata] + github = metadata.get("github") + if isinstance(github, dict): + sources.insert(0, github) + pr: dict[str, Any] = {} + for source in sources: + for key in ( + "pr_url", "pr_number", "repo", "repository", "branch", + "head_ref", "base_ref", "worktree", "worktree_path", "repo_path", + ): + value = source.get(key) + if value is not None and value != "": + pr[key] = value + if "pr_url" not in pr and "url" in (github or {}): + pr["pr_url"] = github["url"] + if "pr_number" not in pr and "number" in (github or {}): + pr["pr_number"] = github["number"] + if "repo" not in pr and "full_name" in (github or {}): + pr["repo"] = github["full_name"] + if "pr_url" not in pr and "pr_number" not in pr: + return None + return pr + + +def _run_claimed_from_review( + conn: sqlite3.Connection, + task_id: str, + run_id: Optional[int], +) -> bool: + """Return True if ``run_id`` was spawned from the review column.""" + if run_id is None: + return False + row = conn.execute( + """ + SELECT payload + FROM task_events + WHERE task_id = ? AND run_id = ? AND kind = 'claimed' + ORDER BY id DESC + LIMIT 1 + """, + (task_id, int(run_id)), + ).fetchone() + if not row or not row["payload"]: + return False + try: + payload = json.loads(row["payload"]) + except Exception: + return False + return isinstance(payload, dict) and payload.get("source_status") == "review" + + class HallucinatedCardsError(ValueError): """Raised by ``complete_task`` when ``created_cards`` contains ids that don't exist or weren't created by the completing worker. @@ -2897,6 +2953,7 @@ def complete_task( and never blocks. """ now = int(time.time()) + pr_metadata = extract_pr_metadata(metadata) # Gate: verify created_cards BEFORE the main write txn. A rejected # completion still needs an auditable event, so we emit it in a @@ -2926,28 +2983,37 @@ def complete_task( verified_cards = [] with write_txn(conn): + current = conn.execute( + "SELECT current_run_id FROM tasks WHERE id = ?", + (task_id,), + ).fetchone() + active_run_id = expected_run_id + if active_run_id is None and current and current["current_run_id"] is not None: + active_run_id = int(current["current_run_id"]) + from_review = _run_claimed_from_review(conn, task_id, active_run_id) + target_status = "review" if pr_metadata and not from_review else "done" if expected_run_id is None: cur = conn.execute( """ UPDATE tasks - SET status = 'done', + SET status = ?, result = ?, - completed_at = ?, + completed_at = CASE WHEN ? = 'done' THEN ? ELSE completed_at END, claim_lock = NULL, claim_expires= NULL, worker_pid = NULL WHERE id = ? AND status IN ('running', 'ready', 'blocked') """, - (result, now, task_id), + (target_status, result, target_status, now, task_id), ) else: cur = conn.execute( """ UPDATE tasks - SET status = 'done', + SET status = ?, result = ?, - completed_at = ?, + completed_at = CASE WHEN ? = 'done' THEN ? ELSE completed_at END, claim_lock = NULL, claim_expires= NULL, worker_pid = NULL @@ -2955,13 +3021,13 @@ def complete_task( AND status IN ('running', 'ready', 'blocked') AND current_run_id = ? """, - (result, now, task_id, int(expected_run_id)), + (target_status, result, target_status, now, task_id, int(expected_run_id)), ) if cur.rowcount != 1: return False run_id = _end_run( conn, task_id, - outcome="completed", status="done", + outcome="completed", status=target_status, summary=summary if summary is not None else result, metadata=metadata, ) @@ -2973,6 +3039,7 @@ def complete_task( run_id = _synthesize_ended_run( conn, task_id, outcome="completed", + status=target_status, summary=summary if summary is not None else result, metadata=metadata, ) @@ -3002,11 +3069,21 @@ def complete_task( ] if cleaned_artifacts: completed_payload["artifacts"] = cleaned_artifacts - _append_event( - conn, task_id, "completed", - completed_payload, - run_id=run_id, - ) + pr_review_handoff = bool(pr_metadata and target_status == "review") + if pr_review_handoff: + review_payload = dict(completed_payload) + review_payload["pr"] = pr_metadata + _append_event( + conn, task_id, "review_requested", + review_payload, + run_id=run_id, + ) + else: + _append_event( + conn, task_id, "completed", + completed_payload, + run_id=run_id, + ) # Prose-scan the summary + result for t_ references that do # not resolve. Advisory — does not block the completion. Runs in # its own txn so the completion itself is already durable by the @@ -3033,10 +3110,45 @@ def complete_task( # just tracks "is there a current pathology the breaker should # care about", and a success resets that question. _clear_failure_counter(conn, task_id) - # Recompute ready status for dependents (separate txn so children see done). - recompute_ready(conn) - # Clean up the scratch workspace and any stale tmux session for the worker. - _cleanup_workspace(conn, task_id) + if target_status == "done": + # Recompute ready status for dependents (separate txn so children see done). + recompute_ready(conn) + # Clean up the scratch workspace and any stale tmux session for the worker. + _cleanup_workspace(conn, task_id) + return True + + +def record_pr_review_poll( + conn: sqlite3.Connection, + task_id: str, + payload: dict, + *, + comment: Optional[str] = None, + author: str = "kanban-pr-review-poll", +) -> bool: + """Record a one-shot PR review poll result and keep the task in review.""" + with write_txn(conn): + row = conn.execute("SELECT status FROM tasks WHERE id = ?", (task_id,)).fetchone() + if not row: + return False + if row["status"] not in {"done", "running"}: + conn.execute( + "UPDATE tasks SET status = 'review', claim_lock = NULL, " + "claim_expires = NULL, worker_pid = NULL WHERE id = ?", + (task_id,), + ) + _append_event(conn, task_id, "pr_review_poll", payload) + if comment and comment.strip(): + now = int(time.time()) + conn.execute( + "INSERT INTO task_comments (task_id, author, body, created_at) " + "VALUES (?, ?, ?, ?)", + (task_id, author, comment.strip(), now), + ) + _append_event( + conn, task_id, "commented", + {"author": author, "len": len(comment.strip())}, + ) return True diff --git a/hermes_cli/kanban_pr_review.py b/hermes_cli/kanban_pr_review.py new file mode 100644 index 0000000000000..0c26c5c5cab0a --- /dev/null +++ b/hermes_cli/kanban_pr_review.py @@ -0,0 +1,459 @@ +"""One-shot GitHub PR review polling for Kanban tasks.""" + +from __future__ import annotations + +import json +import re +import subprocess +from dataclasses import dataclass, field +from pathlib import Path +from typing import Any, Callable, Optional + +from hermes_cli import kanban_db as kb + +_MISSING = object() + +BLOCKING_CHECK_CONCLUSIONS = { + "ACTION_REQUIRED", + "CANCELLED", + "FAILURE", + "STARTUP_FAILURE", + "TIMED_OUT", +} +PENDING_CHECK_STATES = { + "EXPECTED", + "PENDING", + "QUEUED", + "REQUESTED", + "WAITING", + "IN_PROGRESS", +} +_GITHUB_PR_URL_RE = re.compile(r"github\.com/([^/]+/[^/]+)/pull/(\d+)") + + +@dataclass +class PRReference: + selector: str + number: Optional[int] = None + repo: Optional[str] = None + url: Optional[str] = None + cwd: Optional[str] = None + + +@dataclass +class ActionItem: + id: str + kind: str + title: str + state: str + url: Optional[str] = None + body: Optional[str] = None + + +@dataclass +class PollResult: + state: str + summary: str + pr: dict[str, Any] + action_items: list[ActionItem] = field(default_factory=list) + pending_items: list[ActionItem] = field(default_factory=list) + seen_ids: set[str] = field(default_factory=set) + closed_unmerged: bool = False + merged: bool = False + + @property + def has_new_action(self) -> bool: + return bool(self.action_items) + + def to_event_payload(self) -> dict[str, Any]: + return { + "state": self.state, + "summary": self.summary, + "pr": self.pr, + "action_items": [item.__dict__ for item in self.action_items], + "pending_items": [item.__dict__ for item in self.pending_items], + "seen_ids": sorted(self.seen_ids), + "closed_unmerged": self.closed_unmerged, + "merged": self.merged, + } + + +def seen_review_ids(events: list[kb.Event]) -> set[str]: + seen: set[str] = set() + for event in events: + if event.kind != "pr_review_poll" or not isinstance(event.payload, dict): + continue + raw = event.payload.get("seen_ids") + if isinstance(raw, list): + seen.update(str(v) for v in raw if v) + for key in ("action_items", "pending_items"): + items = event.payload.get(key) + if isinstance(items, list): + for item in items: + if isinstance(item, dict) and item.get("id"): + seen.add(str(item["id"])) + return seen + + +def pr_reference_from_task(task: kb.Task, run: Optional[kb.Run]) -> Optional[PRReference]: + metadata = run.metadata if run else None + pr = kb.extract_pr_metadata(metadata) + if not pr: + return None + raw_number = pr.get("pr_number") + number = int(raw_number) if str(raw_number).isdigit() else None + repo = pr.get("repo") or pr.get("repository") + url = pr.get("pr_url") + if url and (not repo or not number): + match = _GITHUB_PR_URL_RE.search(str(url)) + if match: + repo = repo or match.group(1) + number = number or int(match.group(2)) + selector = str(url or number or "").strip() + if not selector: + return None + cwd = pr.get("worktree") or pr.get("worktree_path") or pr.get("repo_path") + if not cwd and task.workspace_path: + cwd = task.workspace_path + return PRReference(selector=selector, number=number, repo=repo, url=url, cwd=cwd) + + +def parse_checks( + raw: Any, + *, + seen_ids: set[str], +) -> tuple[list[ActionItem], list[ActionItem], set[str], bool]: + checks = _coerce_checks(raw) + failing: list[ActionItem] = [] + pending: list[ActionItem] = [] + all_ids: set[str] = set() + has_blocking = False + for check in checks: + name = str( + check.get("name") + or check.get("workflow") + or check.get("context") + or "check" + ) + state = str( + check.get("state") or check.get("status") or check.get("bucket") or "" + ).upper() + conclusion = str(check.get("conclusion") or "").upper() + bucket = str(check.get("bucket") or "").upper() + url = check.get("link") or check.get("detailsUrl") or check.get("url") + identifier = f"check:{check.get('id') or name}:{conclusion or state or bucket}" + all_ids.add(identifier) + if conclusion in BLOCKING_CHECK_CONCLUSIONS or bucket == "FAIL" or state == "FAIL": + has_blocking = True + if identifier not in seen_ids: + failing.append( + ActionItem( + identifier, + "check", + name, + conclusion or state or bucket, + url=url, + ) + ) + elif state in PENDING_CHECK_STATES or bucket == "PENDING": + pending.append(ActionItem(identifier, "check", name, state or bucket, url=url)) + return failing, pending, all_ids, has_blocking + + +def parse_pr_review_state( + pr_view: dict[str, Any], + checks: Any, + *, + review_threads: Optional[dict[str, Any]] = None, + seen_ids: Optional[set[str]] = None, +) -> PollResult: + seen_ids = set(seen_ids or set()) + pr = _normalize_pr(pr_view) + action_items: list[ActionItem] = [] + pending_items: list[ActionItem] = [] + all_seen: set[str] = set() + + has_blocking = False + check_actions, check_pending, check_ids, check_blocking = parse_checks( + checks, + seen_ids=seen_ids, + ) + has_blocking = has_blocking or check_blocking + action_items.extend(check_actions) + pending_items.extend(check_pending) + all_seen.update(check_ids) + + review_decision = str(pr_view.get("reviewDecision") or "").upper() + if review_decision == "CHANGES_REQUESTED": + has_blocking = True + identifier = "review-decision:changes-requested" + all_seen.add(identifier) + if identifier not in seen_ids: + action_items.append( + ActionItem(identifier, "review", "Changes requested", review_decision) + ) + + for review in _iter_nodes(pr_view.get("latestReviews")): + state = str(review.get("state") or "").upper() + if state != "CHANGES_REQUESTED": + continue + has_blocking = True + identifier = f"review:{review.get('id') or review.get('url') or review.get('submittedAt')}" + all_seen.add(identifier) + if identifier not in seen_ids: + author = _author_login(review.get("author")) + title = f"Changes requested by {author}" if author else "Changes requested" + action_items.append( + ActionItem( + identifier, + "review", + title, + state, + url=review.get("url"), + body=review.get("body"), + ) + ) + + thread_source = review_threads if review_threads is not None else pr_view.get("reviewThreads") + for thread in _iter_nodes(thread_source): + if bool(thread.get("isResolved")): + continue + has_blocking = True + identifier = f"thread:{thread.get('id') or thread.get('url')}" + all_seen.add(identifier) + if identifier in seen_ids: + continue + first_comment = next(iter(_iter_nodes(thread.get("comments"))), {}) + path = thread.get("path") or first_comment.get("path") + title = f"Unresolved review thread{f' in {path}' if path else ''}" + action_items.append( + ActionItem( + identifier, + "thread", + title, + "UNRESOLVED", + url=first_comment.get("url") or thread.get("url"), + body=first_comment.get("body"), + ) + ) + + merged = bool(pr_view.get("merged")) or str(pr_view.get("state") or "").upper() == "MERGED" + closed = str(pr_view.get("state") or "").upper() == "CLOSED" + closed_unmerged = closed and not merged + if closed_unmerged: + identifier = f"pr:{pr.get('number') or pr.get('url')}:closed-unmerged" + all_seen.add(identifier) + if identifier not in seen_ids: + action_items.append( + ActionItem( + identifier, + "pr", + "PR is closed without merge", + "CLOSED", + url=pr.get("url"), + ) + ) + + if closed_unmerged: + state = "closed_unmerged" + summary = "PR is closed without merge; task needs operator action." + elif action_items: + state = "action_required" + summary = f"{len(action_items)} new PR review item(s) need attention." + elif has_blocking: + state = "action_required" + summary = "PR still has blocking feedback already recorded on this task." + elif pending_items: + state = "pending" + summary = "PR checks are still pending; task remains in review." + elif merged: + state = "merged" + summary = "PR is merged; review agent may complete deployment/closure checks." + else: + state = "green" + summary = "PR checks are green and no unresolved blocking feedback was found." + return PollResult( + state=state, + summary=summary, + pr=pr, + action_items=action_items, + pending_items=pending_items, + seen_ids=seen_ids | all_seen, + closed_unmerged=closed_unmerged, + merged=merged, + ) + + +def poll_task( + conn, + task_id: str, + *, + runner: Callable[..., subprocess.CompletedProcess] = subprocess.run, +) -> PollResult: + task = kb.get_task(conn, task_id) + if not task: + raise ValueError(f"unknown task {task_id}") + run = kb.latest_run(conn, task_id) + ref = pr_reference_from_task(task, run) + if not ref: + raise ValueError(f"task {task_id} has no PR metadata on its latest run") + seen = seen_review_ids(kb.list_events(conn, task_id)) + pr_view = _gh_json(_gh_pr_view_cmd(ref), cwd=ref.cwd, runner=runner) + checks = _gh_json(_gh_pr_checks_cmd(ref), cwd=ref.cwd, runner=runner, default=[]) + threads = pr_view.get("reviewThreads") + if threads is None and ref.repo and ref.number: + threads = _fetch_review_threads_graphql(ref, cwd=ref.cwd, runner=runner) + result = parse_pr_review_state(pr_view, checks, review_threads=threads, seen_ids=seen) + kb.record_pr_review_poll( + conn, + task_id, + result.to_event_payload(), + comment=_comment_for_result(result), + ) + return result + + +def _gh_pr_view_cmd(ref: PRReference) -> list[str]: + fields = [ + "number", "url", "state", "merged", "isDraft", "mergeStateStatus", + "reviewDecision", "latestReviews", "headRefName", "baseRefName", + "headRepository", "headRepositoryOwner", + ] + cmd = ["gh", "pr", "view", ref.selector, "--json", ",".join(fields)] + if ref.repo: + cmd.extend(["--repo", ref.repo]) + return cmd + + +def _gh_pr_checks_cmd(ref: PRReference) -> list[str]: + cmd = [ + "gh", "pr", "checks", ref.selector, "--json", + "name,state,conclusion,link,bucket,workflow", + ] + if ref.repo: + cmd.extend(["--repo", ref.repo]) + return cmd + + +def _gh_json( + cmd: list[str], + *, + cwd: Optional[str], + runner, + default: Any = _MISSING, +) -> Any: + proc = runner(cmd, cwd=_safe_cwd(cwd), capture_output=True, text=True, timeout=30) + if proc.returncode != 0: + if default is not _MISSING: + return default + raise RuntimeError((proc.stderr or proc.stdout or "gh command failed").strip()) + text = (proc.stdout or "").strip() + if not text and default is not _MISSING: + return default + return json.loads(text) + + +def _fetch_review_threads_graphql( + ref: PRReference, + *, + cwd: Optional[str], + runner, +) -> Optional[dict[str, Any]]: + if not ref.repo or not ref.number or "/" not in ref.repo: + return None + owner, name = ref.repo.split("/", 1) + query = """ + query($owner:String!, $name:String!, $number:Int!) { + repository(owner:$owner, name:$name) { + pullRequest(number:$number) { + reviewThreads(first:100) { + nodes { + id + isResolved + path + comments(first:1) { nodes { id body url path author { login } } } + } + } + } + } + } + """ + cmd = [ + "gh", "api", "graphql", "-f", f"query={query}", + "-F", f"owner={owner}", "-F", f"name={name}", "-F", f"number={ref.number}", + ] + data = _gh_json(cmd, cwd=cwd, runner=runner, default=None) + if not isinstance(data, dict): + return None + return ( + ((data.get("data") or {}).get("repository") or {}).get("pullRequest") or {} + ).get("reviewThreads") + + +def _comment_for_result(result: PollResult) -> str: + lines = [f"PR review poll: {result.summary}"] + for item in result.action_items: + suffix = f" ({item.url})" if item.url else "" + lines.append(f"- [{item.kind}] {item.title}: {item.state}{suffix}") + if item.body: + lines.append(f" {item.body.strip()[:500]}") + return "\n".join(lines) + + +def _normalize_pr(pr_view: dict[str, Any]) -> dict[str, Any]: + return { + "number": pr_view.get("number"), + "url": pr_view.get("url"), + "state": pr_view.get("state"), + "merged": bool(pr_view.get("merged")), + "reviewDecision": pr_view.get("reviewDecision"), + "mergeStateStatus": pr_view.get("mergeStateStatus"), + "headRefName": pr_view.get("headRefName"), + "baseRefName": pr_view.get("baseRefName"), + } + + +def _coerce_checks(raw: Any) -> list[dict[str, Any]]: + if isinstance(raw, list): + return [c for c in raw if isinstance(c, dict)] + if isinstance(raw, dict): + for key in ("checks", "nodes"): + if isinstance(raw.get(key), list): + return [c for c in raw[key] if isinstance(c, dict)] + return [raw] + if isinstance(raw, str): + checks: list[dict[str, Any]] = [] + for line in raw.splitlines(): + parts = re.split(r"\s{2,}|\t", line.strip()) + if len(parts) >= 2: + checks.append({"name": parts[0], "state": parts[1]}) + return checks + return [] + + +def _iter_nodes(value: Any) -> list[dict[str, Any]]: + if isinstance(value, dict): + nodes = value.get("nodes") + if isinstance(nodes, list): + return [n for n in nodes if isinstance(n, dict)] + edges = value.get("edges") + if isinstance(edges, list): + return [ + e.get("node") + for e in edges + if isinstance(e, dict) and isinstance(e.get("node"), dict) + ] + if isinstance(value, list): + return [v for v in value if isinstance(v, dict)] + return [] + + +def _author_login(author: Any) -> Optional[str]: + return author.get("login") if isinstance(author, dict) else None + + +def _safe_cwd(cwd: Optional[str]) -> Optional[str]: + if not cwd: + return None + path = Path(str(cwd)).expanduser() + return str(path) if path.exists() else None diff --git a/skills/devops/kanban-orchestrator/SKILL.md b/skills/devops/kanban-orchestrator/SKILL.md index 25f634205c847..8bb13fd0278f5 100644 --- a/skills/devops/kanban-orchestrator/SKILL.md +++ b/skills/devops/kanban-orchestrator/SKILL.md @@ -154,7 +154,7 @@ Tell them what you created in plain prose, naming the actual profiles you used: **Parallel implementation + validation:** one implementer card makes the change while one explorer/researcher card verifies config, docs, or source mapping. A reviewer card can depend on both. Do not make the implementer own unrelated verification just because the user mentioned both in one sentence. -**Pipeline with gates:** `planner → implementer → reviewer`. Each stage's `parents=[previous_task]`. Reviewer blocks or completes; if reviewer blocks, the operator unblocks with feedback and respawns. +**Pipeline with PR review gate:** `planner → implementer → reviewer`. Each stage's `parents=[previous_task]`. When an implementer opens a PR and completes with `metadata.github.pr_url` or `metadata.github.pr_number`, Hermes moves that same task to `review` instead of `done`; dependent tasks stay gated. The dispatcher then starts the existing `sdlc-review` flow. `hermes kanban pr-review-poll ` is a one-shot operator command for recording CI failures, requested changes, unresolved review threads, pending checks, or a closed-unmerged PR on the same card. Review agents are expected to merge/deploy or explicitly complete only after the PR is actually terminal. **Same-profile queue:** N tasks, all assigned to the same profile, no dependencies between them. Dispatcher serializes — that profile processes them in priority order, accumulating experience in its own memory. @@ -170,7 +170,7 @@ Tell them what you created in plain prose, naming the actual profiles you used: **Forgetting dependency links.** If the task graph says `research -> implement -> review`, do not create all tasks as independent ready cards. Use parent links so implement/review cannot run before their inputs exist. -**Reassignment vs. new task.** If a reviewer blocks with "needs changes," create a NEW task linked from the reviewer's task — don't re-run the same task with a stern look. The new task is assigned to the original implementer profile. +**Reassignment vs. PR review feedback.** For PR-bearing coding tasks, keep fixes on the same `review` card so the review agent sees the PR metadata, poll events, and latest run handoff. Create a new task only when the feedback is genuinely separate work outside the PR loop. **Argument order for links.** `kanban_link(parent_id=..., child_id=...)` — parent first. Mixing them up demotes the wrong task to `todo`. diff --git a/skills/devops/kanban-worker/SKILL.md b/skills/devops/kanban-worker/SKILL.md index 4954e6dc9dd47..43206a505d370 100644 --- a/skills/devops/kanban-worker/SKILL.md +++ b/skills/devops/kanban-worker/SKILL.md @@ -47,28 +47,30 @@ kanban_complete( ) ``` -**Coding task that needs human review (review-required):** +**Coding task with a PR:** -For most code-changing tasks, the work isn't truly *done* until a human reviewer has eyes on it. Block instead of complete, with `reason` prefixed `review-required: ` so the dashboard surfaces the row as needing review. Drop the structured metadata (changed files, test counts, diff/PR url) into a comment first, since `kanban_block` only carries the human-readable reason — comments are the durable annotation channel. Reviewer either approves and runs `hermes kanban unblock ` (which re-spawns you with the comment thread for any follow-ups) or asks for changes via another comment. +For code-changing tasks that open a PR, call `kanban_complete` with PR metadata. Hermes treats that as a coding handoff, not final completion: the task moves to the single `review` state, keeps the closing run metadata, does not set `completed_at`, and does not unblock dependent tasks. The dispatcher will spawn a review agent with `sdlc-review`; operators can also run `hermes kanban pr-review-poll ` to record CI/reviewer feedback on the same card. ```python -import json - -kanban_comment( - body="review-required handoff:\n" + json.dumps({ +kanban_complete( + summary="rate limiter shipped in PR #123 — token bucket, keys on user_id with IP fallback, 14/14 tests pass", + metadata={ "changed_files": ["rate_limiter.py", "tests/test_rate_limiter.py"], "tests_run": 14, "tests_passed": 14, - "diff_path": "/path/to/worktree", # or PR url if pushed + "github": { + "repo": "org/repo", + "pr_number": 123, + "pr_url": "https://github.com/org/repo/pull/123", + }, + "branch": "feat/rate-limiter", + "worktree_path": "/path/to/worktree", "decisions": ["user_id primary, IP fallback for unauthenticated requests"], - }, indent=2), -) -kanban_block( - reason="review-required: rate limiter shipped, 14/14 tests pass — needs eyes on the user_id/IP fallback choice before merging", + }, ) ``` -Use `kanban_complete` only when the task is genuinely terminal — e.g. a one-line typo fix, a docs change with no functional consequences, or a research task where the artifact IS the writeup itself. +Use `kanban_block` only when you need human input before a PR exists. Use plain terminal `kanban_complete` only when the task is genuinely terminal — e.g. a one-line typo fix, a docs change with no functional consequences, or a research task where the artifact IS the writeup itself. **Research task:** ```python @@ -184,6 +186,7 @@ You can configure the gateway to receive cross-profile Kanban task notifications Every tool has a CLI equivalent for human operators and scripts: - `kanban_show` ↔ `hermes kanban show --json` - `kanban_complete` ↔ `hermes kanban complete --summary "..." --metadata '{...}'` +- PR review poll ↔ `hermes kanban pr-review-poll ` - `kanban_block` ↔ `hermes kanban block "reason"` - `kanban_create` ↔ `hermes kanban create "title" --assignee [--parent ]` - etc. diff --git a/skills/github/github-pr-workflow/SKILL.md b/skills/github/github-pr-workflow/SKILL.md index 0b02eca3d1eb9..5e09c2a5c5b9e 100644 --- a/skills/github/github-pr-workflow/SKILL.md +++ b/skills/github/github-pr-workflow/SKILL.md @@ -149,6 +149,27 @@ The response JSON includes the PR `number` — save it for later commands. To create as a draft, add `"draft": true` to the JSON body. +### Kanban handoff metadata + +When this PR belongs to a Hermes Kanban coding task, finish the worker run with PR metadata instead of marking the work truly done. `kanban_complete` with `pr_url` or `pr_number` moves the task to the existing `review` column, preserves the run handoff, and keeps dependents blocked until review/merge is complete. + +```python +kanban_complete( + summary="opened PR #123 for the auth flow; unit tests pass locally", + metadata={ + "changed_files": ["src/auth.py", "tests/test_auth.py"], + "tests_run": ["python -m pytest tests/test_auth.py -q"], + "github": { + "repo": "OWNER/REPO", + "pr_number": 123, + "pr_url": "https://github.com/OWNER/REPO/pull/123", + }, + "branch": "feat/add-user-authentication", + "worktree_path": "/absolute/path/to/worktree", + }, +) +``` + ## 4. Monitoring CI Status ### Check CI Status @@ -163,6 +184,14 @@ gh pr checks gh pr checks --watch ``` +For a Kanban task, operators can do a one-shot poll without running a long watch: + +```bash +hermes kanban pr-review-poll +``` + +The poll records pending checks, failed checks, requested changes, unresolved review threads, and closed-unmerged PRs as Kanban events/comments. It deliberately keeps the task in `review`; the existing `sdlc-review` flow or a human operator handles merge/deploy and final completion. + **With git + curl:** ```bash diff --git a/tests/hermes_cli/test_kanban_cli.py b/tests/hermes_cli/test_kanban_cli.py index fd9b157251351..8860f4ea2bd33 100644 --- a/tests/hermes_cli/test_kanban_cli.py +++ b/tests/hermes_cli/test_kanban_cli.py @@ -11,6 +11,7 @@ from hermes_cli import kanban as kc from hermes_cli import kanban_db as kb +from hermes_cli import kanban_pr_review as prp @pytest.fixture @@ -90,6 +91,24 @@ def test_run_slash_create_and_list(kanban_home): assert "alice" in out +def test_run_slash_pr_review_poll_wired(monkeypatch, kanban_home): + called = {} + + def fake_poll(conn, task_id): + called["task_id"] = task_id + return prp.PollResult( + state="pending", + summary="PR checks are still pending; task remains in review.", + pr={"number": 42}, + pending_items=[prp.ActionItem("check:ci:PENDING", "check", "ci", "PENDING")], + ) + + monkeypatch.setattr(kc.kanban_pr_review, "poll_task", fake_poll) + out = kc.run_slash("pr-review-poll t_deadbeef") + assert called["task_id"] == "t_deadbeef" + assert "PR checks are still pending" in out + + def test_run_slash_create_worktree_path_and_branch(kanban_home, tmp_path): target = tmp_path / ".worktrees" / "t6-wire" target_arg = target.as_posix() diff --git a/tests/hermes_cli/test_kanban_db.py b/tests/hermes_cli/test_kanban_db.py index 883cf8f4d5db0..427d2743788c0 100644 --- a/tests/hermes_cli/test_kanban_db.py +++ b/tests/hermes_cli/test_kanban_db.py @@ -671,6 +671,79 @@ def test_complete_records_result(kanban_home): assert task.completed_at is not None +def test_complete_with_pr_metadata_enters_review_without_unblocking_child(kanban_home): + with kb.connect() as conn: + parent = kb.create_task(conn, title="ship pr", assignee="a") + child = kb.create_task(conn, title="dependent", assignee="a", parents=[parent]) + kb.claim_task(conn, parent) + assert kb.complete_task( + conn, + parent, + result="PR opened", + summary="Implemented and opened PR.", + metadata={ + "github": { + "pr_url": "https://github.com/org/repo/pull/42", + "pr_number": 42, + "repo": "org/repo", + }, + "branch": "feat/x", + }, + ) + task = kb.get_task(conn, parent) + runs = kb.list_runs(conn, parent) + events = kb.list_events(conn, parent) + child_task = kb.get_task(conn, child) + + assert task.status == "review" + assert task.completed_at is None + assert task.result == "PR opened" + assert child_task.status == "todo" + assert len(runs) == 1 + assert runs[0].status == "review" + assert runs[0].outcome == "completed" + assert runs[0].metadata["github"]["pr_number"] == 42 + kinds = [event.kind for event in events] + assert "review_requested" in kinds + assert "completed" not in kinds + + +def test_review_agent_completion_with_pr_metadata_can_finish_task(kanban_home): + with kb.connect() as conn: + parent = kb.create_task(conn, title="review pr", assignee="a") + child = kb.create_task(conn, title="dependent", assignee="a", parents=[parent]) + _set_task_status(conn, parent, "review") + claimed = kb.claim_review_task(conn, parent) + assert claimed is not None + assert kb.complete_task( + conn, + parent, + result="PR merged and deployed", + summary="Review passed; PR merged and deployed.", + metadata={ + "github": { + "pr_url": "https://github.com/org/repo/pull/42", + "pr_number": 42, + "repo": "org/repo", + }, + }, + ) + task = kb.get_task(conn, parent) + runs = kb.list_runs(conn, parent) + events = kb.list_events(conn, parent) + child_task = kb.get_task(conn, child) + + assert task is not None + assert child_task is not None + assert task.status == "done" + assert task.completed_at is not None + assert child_task.status == "ready" + assert runs[-1].status == "done" + assert runs[-1].outcome == "completed" + kinds = [event.kind for event in events] + assert "completed" in kinds + + def test_block_then_unblock(kanban_home): with kb.connect() as conn: t = kb.create_task(conn, title="x", assignee="a") diff --git a/tests/hermes_cli/test_kanban_pr_review.py b/tests/hermes_cli/test_kanban_pr_review.py new file mode 100644 index 0000000000000..111e91c4630fc --- /dev/null +++ b/tests/hermes_cli/test_kanban_pr_review.py @@ -0,0 +1,124 @@ +"""Parser coverage for Kanban PR review polling.""" + +from __future__ import annotations + +from hermes_cli import kanban_pr_review as prp + + +BASE_PR = { + "number": 42, + "url": "https://github.com/org/repo/pull/42", + "state": "OPEN", + "merged": False, + "reviewDecision": "REVIEW_REQUIRED", + "mergeStateStatus": "CLEAN", +} + + +def test_parse_all_green(): + result = prp.parse_pr_review_state( + BASE_PR, + [{"name": "tests", "state": "COMPLETED", "conclusion": "SUCCESS"}], + review_threads={"nodes": []}, + ) + assert result.state == "green" + assert not result.action_items + + +def test_pr_reference_extracts_repo_and_number_from_url(): + task = type("Task", (), {"workspace_path": "/tmp/worktree"})() + run = type( + "Run", + (), + {"metadata": {"github": {"pr_url": "https://github.com/org/repo/pull/42"}}}, + )() + ref = prp.pr_reference_from_task(task, run) + assert ref.repo == "org/repo" + assert ref.number == 42 + assert ref.cwd == "/tmp/worktree" + + +def test_parse_pending_checks(): + result = prp.parse_pr_review_state( + BASE_PR, + [{"name": "tests", "state": "IN_PROGRESS", "conclusion": ""}], + review_threads={"nodes": []}, + ) + assert result.state == "pending" + assert result.pending_items[0].kind == "check" + + +def test_parse_failing_checks(): + result = prp.parse_pr_review_state( + BASE_PR, + [{"name": "tests", "state": "COMPLETED", "conclusion": "FAILURE"}], + review_threads={"nodes": []}, + ) + assert result.state == "action_required" + assert result.action_items[0].id == "check:tests:FAILURE" + + +def test_parse_failing_checks_from_stdout_fixture(): + result = prp.parse_pr_review_state( + BASE_PR, + "lint\tFAIL\nunit\tSUCCESS\n", + review_threads={"nodes": []}, + ) + assert result.state == "action_required" + assert result.action_items[0].id == "check:lint:FAIL" + + +def test_parse_requested_changes(): + result = prp.parse_pr_review_state( + {**BASE_PR, "reviewDecision": "CHANGES_REQUESTED"}, + [], + review_threads={"nodes": []}, + ) + assert result.state == "action_required" + assert any(item.kind == "review" for item in result.action_items) + + +def test_parse_unresolved_review_threads(): + result = prp.parse_pr_review_state( + BASE_PR, + [], + review_threads={ + "nodes": [ + { + "id": "thread-1", + "isResolved": False, + "path": "app.py", + "comments": {"nodes": [{"body": "Please fix this", "url": "https://example.test"}]}, + } + ] + }, + ) + assert result.state == "action_required" + assert result.action_items[0].id == "thread:thread-1" + + +def test_parse_already_seen_comments_and_checks_are_not_reprocessed(): + result = prp.parse_pr_review_state( + {**BASE_PR, "reviewDecision": "CHANGES_REQUESTED"}, + [{"name": "tests", "state": "COMPLETED", "conclusion": "FAILURE"}], + review_threads={"nodes": [{"id": "thread-1", "isResolved": False}]}, + seen_ids={ + "review-decision:changes-requested", + "check:tests:FAILURE", + "thread:thread-1", + }, + ) + assert result.state == "action_required" + assert result.action_items == [] + assert {"review-decision:changes-requested", "check:tests:FAILURE", "thread:thread-1"} <= result.seen_ids + + +def test_parse_closed_unmerged_pr(): + result = prp.parse_pr_review_state( + {**BASE_PR, "state": "CLOSED", "merged": False}, + [], + review_threads={"nodes": []}, + ) + assert result.state == "closed_unmerged" + assert result.closed_unmerged is True + assert result.action_items[0].kind == "pr" diff --git a/website/docs/user-guide/features/kanban.md b/website/docs/user-guide/features/kanban.md index 7a51957828dce..edb75a18caf18 100644 --- a/website/docs/user-guide/features/kanban.md +++ b/website/docs/user-guide/features/kanban.md @@ -314,7 +314,25 @@ For engineering and review tasks, prefer this optional metadata shape: } ``` -These keys are a convention, not a schema requirement. The useful property is +These keys are a convention, not a schema requirement. For coding tasks that open GitHub PRs, include PR metadata so Hermes can keep the card in the `review` column instead of treating PR creation as final completion: + +```json +{ + "changed_files": ["src/auth.py", "tests/test_auth.py"], + "verification": ["pytest tests/test_auth.py -q"], + "github": { + "repo": "OWNER/REPO", + "pr_number": 123, + "pr_url": "https://github.com/OWNER/REPO/pull/123" + }, + "branch": "feat/auth-flow", + "worktree_path": "/absolute/path/to/worktree" +} +``` + +A PR-bearing completion preserves the run handoff, does not set `completed_at`, and does not unblock dependents until the review agent later completes the task after merge/deploy or explicit handoff. Operators can run `hermes kanban pr-review-poll ` to record CI failures, pending checks, requested changes, unresolved review threads, or a closed-unmerged PR as durable events/comments on the same card. + +The useful property is that every worker leaves enough evidence for the next reader to answer four questions quickly: