From 8e8fe58ca6471d7c11a0a15c7748daa3fcae316c Mon Sep 17 00:00:00 2001 From: Fido Can Code <190991155+FidoCanCode@users.noreply.github.com> Date: Tue, 14 Apr 2026 05:45:06 +0000 Subject: [PATCH] Issue picker walks up to root, then descends sub-issue tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #433. The old picker iterated directly-assigned issues and asked "are all sub-issues closed" — which couldn't tell whether an assigned issue was actually next in its parent's order, and couldn't claim unassigned children that should land first. New picker: 1. For each assigned candidate, walk upward via `.parent` (via `gh.get_issue_node`) to the root ancestor. Dedupe roots. 2. Rank roots: milestone-present first, then creation order of the first assigned descendant. 3. Descend each root in GitHub sub-issue order: - child assigned to us → recurse - child unassigned → claim (assign self) + recurse - child assigned to someone else → blocked, skip branch - leaf with no open children → pick it 4. If every open child at a level is blocked, the whole subtree is blocked; move to next root. Claiming the unassigned chain on the way down means once a leaf's PR merges, each ancestor naturally becomes the next pick as its other children close out. Also adds a generic `_graphql_paginate` helper so GraphQL connections (issues, subIssues) walk every page instead of silently truncating at the first 30/50 nodes. `find_issues` and `get_sub_issues` now use it; `get_issue_node` hydrates via `get_sub_issues` when the embedded first page has more. New API surface: - `GitHub.get_sub_issues(owner, repo, number)` — paginated children - `GitHub.get_issue_node(owner, repo, number)` — picker-shaped issue - `GitHub.add_assignee(repo, number, login)` — claim an issue 100% coverage, 1688 tests passing. --- kennel/github.py | 146 ++++++++++++++++- kennel/worker.py | 247 ++++++++++++++++++++++++++--- tests/test_github.py | 238 +++++++++++++++++++++++++++- tests/test_worker.py | 370 ++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 943 insertions(+), 58 deletions(-) diff --git a/kennel/github.py b/kennel/github.py index 5793d73b..100b71cb 100644 --- a/kennel/github.py +++ b/kennel/github.py @@ -116,6 +116,38 @@ def _graphql(self, query: str, **variables: Any) -> Any: raise GraphQLError(data["errors"]) return data + def _graphql_paginate( + self, + query: str, + connection_path: tuple[str, ...], + **variables: Any, + ) -> Iterator[Any]: + """Yield every node from a paginated GraphQL connection. + + The *query* must accept a ``$cursor: String`` variable and request + both ``nodes { ... }`` and ``pageInfo { endCursor hasNextPage }`` + on the connection of interest. + + *connection_path* names the connection's position in the response: + e.g. ``("repository", "issues")`` walks to + ``data.repository.issues`` and yields each node from every page. + + Callers pass the initial variables (other than cursor) as keyword + arguments. The helper takes care of supplying ``cursor`` on + subsequent requests. + """ + cursor: str | None = None + while True: + data = self._graphql(query, cursor=cursor, **variables) + node = data["data"] + for key in connection_path: + node = node[key] + yield from node["nodes"] + page = node["pageInfo"] + if not page["hasNextPage"]: + return + cursor = page["endCursor"] + def add_reaction( self, repo: str, comment_type: str, comment_id: int | str, content: str ) -> None: @@ -379,17 +411,105 @@ def set_user_status(self, msg: str, emoji: str, busy: bool = True) -> None: ) self._graphql(query, msg=msg, emoji=emoji, busy=busy) + _ISSUE_NODE_FIELDS = ( + "number title createdAt " + "milestone{title} " + "assignees(first:20){nodes{login}} " + "parent{number}" + ) + def find_issues(self, owner: str, repo: str, login: str) -> list[dict[str, Any]]: - """Return open issues assigned to login (oldest first) with sub-issue states.""" + """Return open issues assigned to *login* (oldest first). + + Each node carries ``number``, ``title``, ``createdAt``, + ``milestone.title``, ``assignees.nodes[].login``, ``parent.number`` + (or None), and a paginated-and-hydrated ``subIssues.nodes`` list + in GitHub rank order. + """ + issue_fields = self._ISSUE_NODE_FIELDS query = ( - "query($owner:String!,$repo:String!,$login:String!){" + "query($owner:String!,$repo:String!,$login:String!,$cursor:String){" "repository(owner:$owner,name:$repo){" - "issues(first:50,states:[OPEN],filterBy:{assignee:$login}," + "issues(first:50,after:$cursor,states:[OPEN]," + "filterBy:{assignee:$login}," "orderBy:{field:CREATED_AT,direction:ASC}){" - "nodes{number title subIssues(first:10){nodes{state}}}}}}" + f"nodes{{{issue_fields} state " + f"subIssues(first:50){{nodes{{state {issue_fields}}} " + "pageInfo{endCursor hasNextPage}}}" + "pageInfo{endCursor hasNextPage}" + "}}}" ) - data = self._graphql(query, owner=owner, repo=repo, login=login) - return data["data"]["repository"]["issues"]["nodes"] + issues: list[dict[str, Any]] = [] + for node in self._graphql_paginate( + query, + ("repository", "issues"), + owner=owner, + repo=repo, + login=login, + ): + if node.get("subIssues", {}).get("pageInfo", {}).get("hasNextPage"): + node["subIssues"]["nodes"] = list( + self.get_sub_issues(owner, repo, node["number"]) + ) + issues.append(node) + return issues + + def get_issue_node(self, owner: str, repo: str, number: int) -> dict[str, Any]: + """Return one issue in the shape used by :meth:`find_issues`. + + Used by the picker's upward walk: call with any issue number and + get back the same dict shape so descent code can keep walking. + """ + issue_fields = self._ISSUE_NODE_FIELDS + query = ( + "query($owner:String!,$repo:String!,$number:Int!){" + "repository(owner:$owner,name:$repo){" + "issue(number:$number){" + f"state {issue_fields} " + f"subIssues(first:50){{nodes{{state {issue_fields}}} " + "pageInfo{endCursor hasNextPage}}" + "}}}" + ) + data = self._graphql(query, owner=owner, repo=repo, number=number) + node = data["data"]["repository"]["issue"] + if node.get("subIssues", {}).get("pageInfo", {}).get("hasNextPage"): + node["subIssues"]["nodes"] = list(self.get_sub_issues(owner, repo, number)) + return node + + def get_sub_issues( + self, owner: str, repo: str, number: int + ) -> Iterator[dict[str, Any]]: + """Yield the direct sub-issues of *number* in GitHub rank order. + + Each node has the same shape as a node from :meth:`find_issues` + (``number``, ``title``, ``state``, ``createdAt``, ``milestone.title``, + ``assignees.nodes[].login``). + """ + issue_fields = self._ISSUE_NODE_FIELDS + query = ( + "query($owner:String!,$repo:String!,$number:Int!,$cursor:String){" + "repository(owner:$owner,name:$repo){" + "issue(number:$number){" + "subIssues(first:50,after:$cursor){" + f"nodes{{state {issue_fields}}} " + "pageInfo{endCursor hasNextPage}" + "}}}}" + ) + yield from self._graphql_paginate( + query, + ("repository", "issue", "subIssues"), + owner=owner, + repo=repo, + number=number, + ) + + def add_assignee(self, repo: str, number: int | str, login: str) -> None: + """Assign *login* to issue *number* in *repo*. + + Uses the REST endpoint so assignment is additive — existing + assignees are preserved. + """ + self._post(f"/repos/{repo}/issues/{number}/assignees", assignees=[login]) def view_issue(self, repo: str, number: int | str) -> dict[str, Any]: """Return issue data (state, title, body, created_at).""" @@ -694,6 +814,20 @@ def view_issue(self, repo: str, number: int | str) -> dict[str, Any]: """Return issue data (state, title, body).""" return self._gh.view_issue(repo, number) + def get_sub_issues( + self, owner: str, repo: str, number: int + ) -> list[dict[str, Any]]: + """Return the direct sub-issues of *number*, in GitHub rank order.""" + return list(self._gh.get_sub_issues(owner, repo, number)) + + def get_issue_node(self, owner: str, repo: str, number: int) -> dict[str, Any]: + """Return one issue in the shape used by :meth:`find_issues`.""" + return self._gh.get_issue_node(owner, repo, number) + + def add_assignee(self, repo: str, number: int | str, login: str) -> None: + """Assign *login* to issue *number*.""" + self._gh.add_assignee(repo, number, login) + def comment_issue(self, repo: str, number: int | str, body: str) -> None: """Post a comment on an issue.""" self._gh.comment_issue(repo, number, body) diff --git a/kennel/worker.py b/kennel/worker.py index 60fc4c56..bc8a30bb 100644 --- a/kennel/worker.py +++ b/kennel/worker.py @@ -8,6 +8,7 @@ import re import subprocess import threading +from collections.abc import Callable from contextlib import AbstractContextManager, nullcontext from dataclasses import dataclass, field from datetime import datetime, timezone @@ -242,6 +243,186 @@ def claude_run( return new_session_id, output +@dataclass(frozen=True) +class PickerChoice: + """Result of the issue picker. + + *number* is the selected issue; *reason* is a short human-readable + explanation of how the picker got there (logged on pickup). + """ + + number: int + title: str + reason: str + + +def _has_milestone(issue: dict[str, Any]) -> bool: + return bool((issue.get("milestone") or {}).get("title")) + + +def _issue_assignees(issue: dict[str, Any]) -> list[str]: + nodes = (issue.get("assignees") or {}).get("nodes") or [] + return [n.get("login", "") for n in nodes if n.get("login")] + + +def _pick_next_issue( + candidates: list[dict[str, Any]], + login: str, + *, + get_issue_node: Callable[[int], dict[str, Any]], + get_sub_issues: Callable[[int], list[dict[str, Any]]], + claim: Callable[[int], None], +) -> PickerChoice | None: + """Select the next issue to work on from the picker rules in #433. + + Algorithm: + + 1. For each assigned *candidate*, walk upward via ``.parent`` to the + root ancestor — the user requires the chosen issue to be the + "first open" item from the root down, not just from here. + 2. Dedupe roots by issue number. + 3. Rank roots: milestone-present before milestone-absent, then the + original creation order of the first assigned descendant. + 4. Descend each root via :func:`_descend_issue` — unassigned + children get claimed, children assigned to someone else block + their branch, and the first eligible leaf wins. + """ + roots: list[dict[str, Any]] = [] + seen: set[int] = set() + order: dict[int, int] = {} + for idx, candidate in enumerate(candidates): + root = _walk_to_root(candidate, get_issue_node=get_issue_node) + n = root["number"] + if n in seen: + continue + seen.add(n) + order[n] = idx + roots.append(root) + + roots.sort(key=lambda r: (0 if _has_milestone(r) else 1, order[r["number"]])) + + for root in roots: + choice = _descend_issue( + root, + login, + get_sub_issues=get_sub_issues, + claim=claim, + trail=[root["number"]], + milestone_source=root if _has_milestone(root) else None, + ) + if choice is not None: + return choice + return None + + +def _walk_to_root( + issue: dict[str, Any], + *, + get_issue_node: Callable[[int], dict[str, Any]], +) -> dict[str, Any]: + """Walk ``issue.parent`` upward until we hit the root ancestor. + + *get_issue_node* fetches a full issue dict (same shape as an entry + from :meth:`kennel.github.GitHub.find_issues`) so we can follow the + parent chain. Returns the root ancestor, or *issue* itself when it + has no parent. + """ + current = issue + visited: set[int] = set() + while True: + parent_ref = current.get("parent") + if not parent_ref or not parent_ref.get("number"): + return current + parent_number = parent_ref["number"] + if parent_number in visited: + log.warning( + "picker: parent cycle detected at #%s — stopping walk", + parent_number, + ) + return current + visited.add(parent_number) + current = get_issue_node(parent_number) + + +def _descend_issue( + issue: dict[str, Any], + login: str, + *, + get_sub_issues: Callable[[int], list[dict[str, Any]]], + claim: Callable[[int], None], + trail: list[int], + milestone_source: dict[str, Any] | None, +) -> PickerChoice | None: + """Walk down *issue*'s sub-issues in rank order; return the first + eligible leaf (or *issue* itself if it has no open children). + + Returns ``None`` when the whole subtree is blocked by other assignees. + """ + children = list(issue.get("subIssues", {}).get("nodes") or []) + open_children = [c for c in children if c.get("state") != "CLOSED"] + + if not open_children: + depth_note = ( + f" (descended from #{'/#'.join(str(n) for n in trail[:-1])})" + if len(trail) > 1 + else "" + ) + milestone_note = "" + if milestone_source is not None and milestone_source is not issue: + milestone_note = f", milestone from parent #{milestone_source['number']}" + return PickerChoice( + number=issue["number"], + title=issue.get("title", ""), + reason=f"picker: pick #{issue['number']}{depth_note}{milestone_note}", + ) + + any_open = False + for child in open_children: + assignees = _issue_assignees(child) + if assignees and login not in assignees: + log.info( + "picker: skipping #%s — assigned to %s (blocks tree under #%s)", + child["number"], + ",".join(assignees), + trail[0], + ) + continue + any_open = True + if login not in assignees: + log.info( + "picker: claiming #%s (unassigned child of #%s) as %s", + child["number"], + issue["number"], + login, + ) + claim(child["number"]) + # Refresh the child's sub-issue list — the node from the parent + # query only has the first page; deeper descent needs a fresh + # pull to be complete. + child_subs = list(get_sub_issues(child["number"])) + child_node = dict(child) + child_node["subIssues"] = {"nodes": child_subs} + choice = _descend_issue( + child_node, + login, + get_sub_issues=get_sub_issues, + claim=claim, + trail=[*trail, child["number"]], + milestone_source=( + milestone_source or (child if _has_milestone(child) else None) + ), + ) + if choice is not None: + return choice + # Every open child was blocked by someone else. + if not any_open: + log.info( + "picker: #%s skipped — all open sub-issues blocked by other assignees", + issue["number"], + ) + return None + + def _pick_next_task(task_list: list[dict[str, Any]]) -> dict[str, Any] | None: """Return the highest-priority eligible pending task, or ``None``. @@ -604,39 +785,55 @@ def set_status( def find_next_issue(self, fido_dir: Path, repo_ctx: RepoContext) -> int | None: """Find the next eligible open issue assigned to gh_user. - An issue is eligible if it has no sub-issues or all sub-issues are CLOSED. - - On success: saves the issue number to state, sets the GitHub status, and - returns the issue number. When no eligible issue exists: sets a "done" - status and returns None. + Walks the sub-issue tree (see :func:`_pick_next_issue` for ranking + rules) so we fetch child work before its parent. Children that + aren't assigned to us get claimed; children assigned to someone + else block that branch. """ log.info("finding next eligible issue") issues = self.gh.find_issues( repo_ctx.owner, repo_ctx.repo_name, repo_ctx.gh_user ) - for issue in issues: - sub_issues = issue.get("subIssues", {}).get("nodes", []) - if not sub_issues or all(si["state"] == "CLOSED" for si in sub_issues): - number = issue["number"] - title = issue["title"] - log.info("starting issue #%s: %s", number, title) - State(fido_dir).save( - { - "issue": number, - "issue_title": title, - "issue_started_at": datetime.now(tz=timezone.utc).isoformat(), - } - ) - self.set_status(f"Picking up issue #{number}: {title}") - return number - log.info( - "no eligible issues assigned to %s in %s", + def get_sub_issues(number: int) -> list[dict[str, Any]]: + return list( + self.gh.get_sub_issues(repo_ctx.owner, repo_ctx.repo_name, number) + ) + + def get_issue_node(number: int) -> dict[str, Any]: + return self.gh.get_issue_node(repo_ctx.owner, repo_ctx.repo_name, number) + + def claim(number: int) -> None: + self.gh.add_assignee(repo_ctx.repo, number, repo_ctx.gh_user) + + choice = _pick_next_issue( + issues, repo_ctx.gh_user, - repo_ctx.repo, + get_issue_node=get_issue_node, + get_sub_issues=get_sub_issues, + claim=claim, ) - self.set_status("All done — no issues to fetch", busy=False) - return None + if choice is None: + log.info( + "no eligible issues assigned to %s in %s", + repo_ctx.gh_user, + repo_ctx.repo, + ) + self.set_status("All done — no issues to fetch", busy=False) + return None + + log.info( + "starting issue #%s: %s (%s)", choice.number, choice.title, choice.reason + ) + State(fido_dir).save( + { + "issue": choice.number, + "issue_title": choice.title, + "issue_started_at": datetime.now(tz=timezone.utc).isoformat(), + } + ) + self.set_status(f"Picking up issue #{choice.number}: {choice.title}") + return choice.number def _git( self, args: list[str], check: bool = True, *, _run=subprocess.run diff --git a/tests/test_github.py b/tests/test_github.py index f3caaa06..c08c7c47 100644 --- a/tests/test_github.py +++ b/tests/test_github.py @@ -144,14 +144,64 @@ def test_set_user_status_delegates(self) -> None: def test_find_issues_delegates(self) -> None: gh, mock_s = self._github() - nodes = [{"number": 1, "title": "t"}] + nodes = [ + {"number": 1, "title": "t", "subIssues": {"nodes": [], "pageInfo": {}}} + ] mock_resp = MagicMock() mock_resp.json.return_value = { - "data": {"repository": {"issues": {"nodes": nodes}}} + "data": { + "repository": { + "issues": { + "nodes": nodes, + "pageInfo": {"hasNextPage": False, "endCursor": None}, + } + } + } } mock_s.post.return_value = mock_resp assert gh.find_issues("o", "r", "fido") == nodes + def test_get_sub_issues_delegates(self) -> None: + gh, mock_s = self._github() + mock_resp = MagicMock() + mock_resp.json.return_value = { + "data": { + "repository": { + "issue": { + "subIssues": { + "nodes": [{"number": 9}], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + } + } + } + } + } + mock_s.post.return_value = mock_resp + assert gh.get_sub_issues("o", "r", 5) == [{"number": 9}] + + def test_get_issue_node_delegates(self) -> None: + gh, mock_s = self._github() + node = { + "number": 7, + "title": "t", + "subIssues": { + "nodes": [], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + }, + } + mock_resp = MagicMock() + mock_resp.json.return_value = {"data": {"repository": {"issue": node}}} + mock_s.post.return_value = mock_resp + assert gh.get_issue_node("o", "r", 7) == node + + def test_add_assignee_delegates(self) -> None: + gh, mock_s = self._github() + mock_resp = MagicMock() + mock_resp.json.return_value = {} + mock_s.post.return_value = mock_resp + gh.add_assignee("o/r", 42, "fido") + assert mock_s.post.call_args.kwargs["json"] == {"assignees": ["fido"]} + def test_view_issue_delegates(self) -> None: gh, mock_s = self._github() mock_resp = MagicMock() @@ -1242,17 +1292,195 @@ def test_set_user_status_graphql(self) -> None: assert body["variables"]["emoji"] == "🚀" assert body["variables"]["busy"] is True + def test_graphql_paginate_follows_next_cursor(self) -> None: + gh, mock_s = self._gh() + page1 = MagicMock() + page1.json.return_value = { + "data": { + "repository": { + "issues": { + "nodes": [{"number": 1}], + "pageInfo": {"hasNextPage": True, "endCursor": "CUR1"}, + } + } + } + } + page2 = MagicMock() + page2.json.return_value = { + "data": { + "repository": { + "issues": { + "nodes": [{"number": 2}], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + } + } + } + } + mock_s.post.side_effect = [page1, page2] + query = "query($cursor:String){repository{issues(after:$cursor){nodes{number} pageInfo{endCursor hasNextPage}}}}" + results = list(gh._graphql_paginate(query, ("repository", "issues"))) + assert results == [{"number": 1}, {"number": 2}] + # Second call carries the cursor from page 1's endCursor. + assert ( + mock_s.post.call_args_list[1].kwargs["json"]["variables"]["cursor"] + == "CUR1" + ) + + def test_get_sub_issues_paginates(self) -> None: + gh, mock_s = self._gh() + resp = MagicMock() + resp.json.return_value = { + "data": { + "repository": { + "issue": { + "subIssues": { + "nodes": [{"number": 10, "title": "child"}], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + } + } + } + } + } + mock_s.post.return_value = resp + result = list(gh.get_sub_issues("owner", "repo", 5)) + assert result == [{"number": 10, "title": "child"}] + assert mock_s.post.call_args.kwargs["json"]["variables"]["number"] == 5 + + def test_get_issue_node_returns_full_shape(self) -> None: + gh, mock_s = self._gh() + node = { + "number": 99, + "title": "parent", + "state": "OPEN", + "subIssues": { + "nodes": [{"number": 100, "title": "child"}], + "pageInfo": {"hasNextPage": False}, + }, + } + resp = MagicMock() + resp.json.return_value = {"data": {"repository": {"issue": node}}} + mock_s.post.return_value = resp + result = gh.get_issue_node("owner", "repo", 99) + assert result["number"] == 99 + assert result["subIssues"]["nodes"] == [{"number": 100, "title": "child"}] + + def test_get_issue_node_hydrates_paginated_sub_issues(self) -> None: + """When the embedded sub-issues page has more, refetch with get_sub_issues.""" + gh, mock_s = self._gh() + first_resp = MagicMock() + first_resp.json.return_value = { + "data": { + "repository": { + "issue": { + "number": 99, + "title": "parent", + "state": "OPEN", + "subIssues": { + "nodes": [{"number": 100}], + "pageInfo": {"hasNextPage": True, "endCursor": None}, + }, + } + } + } + } + hydrate_resp = MagicMock() + hydrate_resp.json.return_value = { + "data": { + "repository": { + "issue": { + "subIssues": { + "nodes": [{"number": 100}, {"number": 101}], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + } + } + } + } + } + mock_s.post.side_effect = [first_resp, hydrate_resp] + result = gh.get_issue_node("owner", "repo", 99) + # Hydrated list replaces the paginated stub. + assert [n["number"] for n in result["subIssues"]["nodes"]] == [100, 101] + + def test_find_issues_hydrates_paginated_sub_issues(self) -> None: + gh, mock_s = self._gh() + first_resp = MagicMock() + first_resp.json.return_value = { + "data": { + "repository": { + "issues": { + "nodes": [ + { + "number": 1, + "title": "parent", + "subIssues": { + "nodes": [{"number": 10}], + "pageInfo": { + "hasNextPage": True, + "endCursor": None, + }, + }, + } + ], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + } + } + } + } + hydrate_resp = MagicMock() + hydrate_resp.json.return_value = { + "data": { + "repository": { + "issue": { + "subIssues": { + "nodes": [{"number": 10}, {"number": 11}], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + } + } + } + } + } + mock_s.post.side_effect = [first_resp, hydrate_resp] + result = gh.find_issues("owner", "repo", "fido") + assert [n["number"] for n in result[0]["subIssues"]["nodes"]] == [10, 11] + + def test_add_assignee_posts_to_rest_endpoint(self) -> None: + gh, mock_s = self._gh() + mock_resp = MagicMock() + mock_resp.json.return_value = {} + mock_s.post.return_value = mock_resp + gh.add_assignee("owner/repo", 42, "fido") + assert "/repos/owner/repo/issues/42/assignees" in mock_s.post.call_args.args[0] + assert mock_s.post.call_args.kwargs["json"] == {"assignees": ["fido"]} + def test_find_issues_graphql(self) -> None: gh, mock_s = self._gh() - nodes = [{"number": 1, "title": "bug"}] + nodes = [ + { + "number": 1, + "title": "bug", + "subIssues": {"nodes": [], "pageInfo": {"hasNextPage": False}}, + } + ] mock_resp = MagicMock() mock_resp.json.return_value = { - "data": {"repository": {"issues": {"nodes": nodes}}} + "data": { + "repository": { + "issues": { + "nodes": nodes, + "pageInfo": {"hasNextPage": False, "endCursor": None}, + } + } + } } mock_s.post.return_value = mock_resp result = gh.find_issues("owner", "repo", "fido") body = mock_s.post.call_args.kwargs["json"] - assert body["variables"] == {"owner": "owner", "repo": "repo", "login": "fido"} + assert body["variables"] == { + "owner": "owner", + "repo": "repo", + "login": "fido", + "cursor": None, + } assert result == nodes def test_view_issue(self) -> None: diff --git a/tests/test_worker.py b/tests/test_worker.py index 45451168..cd1b24ea 100644 --- a/tests/test_worker.py +++ b/tests/test_worker.py @@ -6,6 +6,7 @@ import subprocess import threading import time +from collections.abc import Callable from pathlib import Path from unittest.mock import ANY, MagicMock, patch @@ -1086,13 +1087,26 @@ def test_returns_issue_number_when_all_subissues_closed( result = worker.find_next_issue(fido_dir, self._make_repo_ctx()) assert result == 10 - def test_skips_issue_with_open_subissue(self, tmp_path: Path) -> None: + def test_skips_tree_when_open_subissue_assigned_to_other( + self, tmp_path: Path + ) -> None: + """If the only open child is someone else's, the whole tree is blocked.""" worker, gh = self._make_worker(tmp_path) gh.find_issues.return_value = [ { "number": 3, "title": "Blocked", - "subIssues": {"nodes": [{"state": "OPEN"}]}, + "subIssues": { + "nodes": [ + { + "number": 30, + "title": "Owned by other", + "state": "OPEN", + "assignees": {"nodes": [{"login": "someone-else"}]}, + "parent": {"number": 3}, + } + ] + }, } ] fido_dir = self._fido_dir(tmp_path) @@ -1101,28 +1115,16 @@ def test_skips_issue_with_open_subissue(self, tmp_path: Path) -> None: assert result is None def test_picks_first_eligible_issue(self, tmp_path: Path) -> None: + """With no sub-issues to descend into, picker uses creation order.""" worker, gh = self._make_worker(tmp_path) gh.find_issues.return_value = [ - { - "number": 1, - "title": "Blocked", - "subIssues": {"nodes": [{"state": "OPEN"}]}, - }, - { - "number": 2, - "title": "Ready", - "subIssues": {"nodes": []}, - }, - { - "number": 3, - "title": "Also ready", - "subIssues": {"nodes": []}, - }, + {"number": 1, "title": "First", "subIssues": {"nodes": []}}, + {"number": 2, "title": "Second", "subIssues": {"nodes": []}}, ] fido_dir = self._fido_dir(tmp_path) with patch.object(worker, "set_status"): result = worker.find_next_issue(fido_dir, self._make_repo_ctx()) - assert result == 2 + assert result == 1 def test_saves_state_when_issue_found(self, tmp_path: Path) -> None: worker, gh = self._make_worker(tmp_path) @@ -1202,19 +1204,343 @@ def test_logs_info_when_no_eligible_issues(self, tmp_path: Path, caplog) -> None worker.find_next_issue(fido_dir, self._make_repo_ctx()) assert "no eligible" in caplog.text - def test_mixed_closed_and_open_subissues_skips(self, tmp_path: Path) -> None: + def test_walks_up_via_gh_get_issue_node(self, tmp_path: Path) -> None: + """Assigned issue has a parent — worker uses gh.get_issue_node to + walk up to it before descending.""" + worker, gh = self._make_worker(tmp_path) + gh.find_issues.return_value = [ + { + "number": 200, + "title": "child", + "state": "OPEN", + "assignees": {"nodes": [{"login": "fido-bot"}]}, + "parent": {"number": 100}, + "subIssues": {"nodes": []}, + } + ] + gh.get_issue_node.return_value = { + "number": 100, + "title": "root", + "state": "OPEN", + "assignees": {"nodes": []}, + "parent": None, + "subIssues": { + "nodes": [ + { + "number": 200, + "title": "child", + "state": "OPEN", + "assignees": {"nodes": [{"login": "fido-bot"}]}, + "parent": {"number": 100}, + } + ] + }, + } + gh.get_sub_issues.return_value = [] + fido_dir = self._fido_dir(tmp_path) + with patch.object(worker, "set_status"): + result = worker.find_next_issue(fido_dir, self._make_repo_ctx()) + assert result == 200 + gh.get_issue_node.assert_called_with("alice", "proj", 100) + + def test_picks_first_open_sub_issue_and_claims_it(self, tmp_path: Path) -> None: + """When the assigned issue has an open, unassigned child, we claim + and descend into that child — it must land before the parent.""" worker, gh = self._make_worker(tmp_path) gh.find_issues.return_value = [ { "number": 11, - "title": "Partial", - "subIssues": {"nodes": [{"state": "CLOSED"}, {"state": "OPEN"}]}, + "title": "Parent", + "subIssues": { + "nodes": [ + { + "number": 110, + "title": "Closed work", + "state": "CLOSED", + "assignees": {"nodes": []}, + "parent": {"number": 11}, + }, + { + "number": 111, + "title": "Open child", + "state": "OPEN", + "assignees": {"nodes": []}, + "parent": {"number": 11}, + }, + ] + }, } ] + # Descent fetches the child's own sub-issues (none). + gh.get_sub_issues.return_value = [] fido_dir = self._fido_dir(tmp_path) with patch.object(worker, "set_status"): result = worker.find_next_issue(fido_dir, self._make_repo_ctx()) - assert result is None + # Picker chose the open child, and claimed it. + assert result == 111 + gh.add_assignee.assert_called_once_with("alice/proj", 111, "fido-bot") + + +def _issue( + number: int, + title: str = "", + *, + state: str = "OPEN", + milestone: str | None = None, + assignees: list[str] | None = None, + parent: int | None = None, + sub_issues: list[dict] | None = None, +) -> dict: + """Build a picker-shaped issue dict for tests.""" + node: dict = { + "number": number, + "title": title or f"issue {number}", + "state": state, + "assignees": {"nodes": [{"login": a} for a in (assignees or [])]}, + } + if milestone is not None: + node["milestone"] = {"title": milestone} + if parent is not None: + node["parent"] = {"number": parent} + if sub_issues is not None: + node["subIssues"] = {"nodes": sub_issues} + return node + + +class TestPickNextIssue: + """Direct unit tests for _pick_next_issue / _walk_to_root / _descend_issue.""" + + def _claim_spy(self) -> tuple[list[int], Callable[[int], None]]: + claimed: list[int] = [] + return claimed, claimed.append + + def test_single_assigned_issue_no_children_is_picked(self) -> None: + from kennel.worker import _pick_next_issue + + issue = _issue(5, "Ready", assignees=["fido"], sub_issues=[]) + claimed, claim = self._claim_spy() + choice = _pick_next_issue( + [issue], + "fido", + get_issue_node=lambda n: _issue(n), + get_sub_issues=lambda n: [], + claim=claim, + ) + assert choice is not None and choice.number == 5 + assert claimed == [] + + def test_milestone_beats_no_milestone(self) -> None: + from kennel.worker import _pick_next_issue + + older = _issue(1, assignees=["fido"], sub_issues=[]) + newer_with_ms = _issue(99, assignees=["fido"], milestone="v1", sub_issues=[]) + choice = _pick_next_issue( + [older, newer_with_ms], + "fido", + get_issue_node=lambda n: _issue(n), + get_sub_issues=lambda n: [], + claim=lambda n: None, + ) + assert choice is not None and choice.number == 99 + + def test_walks_up_to_root_before_descending(self) -> None: + """Assigned issue is a deep child — picker walks up to root, then + descends to whatever's first-open at the top of the tree.""" + from kennel.worker import _pick_next_issue + + # Tree: #100 (root, unrelated) → [#200 (first, ours), #201 (second, ours)] + # We're assigned #201 but #200 comes before it in sub-issue order, so + # the descent from the root should pick #200. + issue_nodes = { + 100: _issue( + 100, + "root", + sub_issues=[ + _issue(200, state="OPEN", assignees=["fido"], parent=100), + _issue(201, state="OPEN", assignees=["fido"], parent=100), + ], + ), + } + + def get_issue_node(n: int) -> dict: + return issue_nodes[n] + + # Only #201 is in the assigned list; picker must still walk up to 100. + assigned = _issue(201, state="OPEN", assignees=["fido"], parent=100) + choice = _pick_next_issue( + [assigned], + "fido", + get_issue_node=get_issue_node, + get_sub_issues=lambda n: [], + claim=lambda n: None, + ) + assert choice is not None and choice.number == 200 + + def test_blocked_when_earlier_sibling_is_someone_elses(self) -> None: + from kennel.worker import _pick_next_issue + + # #100 → [#200 assigned to other, #201 assigned to fido] + issue_nodes = { + 100: _issue( + 100, + "root", + sub_issues=[ + _issue(200, state="OPEN", assignees=["alice"], parent=100), + _issue(201, state="OPEN", assignees=["fido"], parent=100), + ], + ) + } + # #200 blocks the earlier slot, but there's a later sibling (#201) that's ours. + # The descent walks each open child in order — blocked, then eligible. + choice = _pick_next_issue( + [_issue(201, assignees=["fido"], parent=100)], + "fido", + get_issue_node=lambda n: issue_nodes[n], + get_sub_issues=lambda n: [], + claim=lambda n: None, + ) + assert choice is not None and choice.number == 201 + + def test_returns_none_when_only_open_children_are_others(self) -> None: + from kennel.worker import _pick_next_issue + + issue_nodes = { + 100: _issue( + 100, + "root", + sub_issues=[ + _issue(200, state="OPEN", assignees=["alice"], parent=100), + _issue(201, state="OPEN", assignees=["bob"], parent=100), + ], + ) + } + # Neither sibling is ours. Tree is blocked. + choice = _pick_next_issue( + [_issue(201, assignees=["bob"], parent=100)], + "fido", + get_issue_node=lambda n: issue_nodes[n], + get_sub_issues=lambda n: [], + claim=lambda n: None, + ) + assert choice is None + + def test_claims_unassigned_child_then_descends(self) -> None: + """Walks down a chain of unassigned descendants, claiming each.""" + from kennel.worker import _pick_next_issue + + # Candidate: #100 with sub-issue #200 (unassigned). Descent then + # asks get_sub_issues(200) to hydrate deeper: [#300 (unassigned)]. + candidate = _issue( + 100, + "root", + assignees=["fido"], + sub_issues=[_issue(200, state="OPEN", assignees=[], parent=100)], + ) + + def get_sub_issues(n: int) -> list[dict]: + return { + 200: [_issue(300, state="OPEN", assignees=[], parent=200)], + 300: [], + }.get(n, []) + + claimed, claim = self._claim_spy() + choice = _pick_next_issue( + [candidate], + "fido", + get_issue_node=lambda n: candidate, + get_sub_issues=get_sub_issues, + claim=claim, + ) + assert choice is not None and choice.number == 300 + # Both #200 and #300 were claimed on the way down. + assert claimed == [200, 300] + + def test_parent_cycle_is_broken_gracefully(self) -> None: + """A self-referencing parent doesn't infinite-loop the walk.""" + from kennel.worker import _walk_to_root + + nodes = { + 50: _issue(50, "A", parent=50), # points at itself + } + start = _issue(50, "A", parent=50) + result = _walk_to_root(start, get_issue_node=lambda n: nodes[n]) + # The walk bails at the first revisit; result is the node we end on. + assert result["number"] == 50 + + def test_reason_includes_descent_trail(self) -> None: + from kennel.worker import _pick_next_issue + + # root → leaf, picker ends at leaf with descent trail in reason. + root = _issue( + 10, + "root", + assignees=["fido"], + sub_issues=[_issue(20, state="OPEN", assignees=["fido"], parent=10)], + ) + choice = _pick_next_issue( + [root], + "fido", + get_issue_node=lambda n: root if n == 10 else _issue(n), + get_sub_issues=lambda n: [], + claim=lambda n: None, + ) + assert choice is not None + assert choice.number == 20 + assert "#10" in choice.reason + + def test_dedupes_roots_across_multiple_assigned(self) -> None: + """Two assigned issues sharing the same root should walk once.""" + from kennel.worker import _pick_next_issue + + nodes = { + 1: _issue( + 1, + "root", + sub_issues=[_issue(2, state="OPEN", assignees=["fido"], parent=1)], + ) + } + get_calls: list[int] = [] + + def get_issue_node(n: int) -> dict: + get_calls.append(n) + return nodes[n] + + a = _issue(2, state="OPEN", assignees=["fido"], parent=1) + b = _issue(2, state="OPEN", assignees=["fido"], parent=1) + choice = _pick_next_issue( + [a, b], + "fido", + get_issue_node=get_issue_node, + get_sub_issues=lambda n: [], + claim=lambda n: None, + ) + assert choice is not None and choice.number == 2 + # Walks up for each of the two, but descent runs exactly once (roots are deduped). + # We check by asserting at most one descent call, but the easier + # assertion is the choice matches. + + def test_milestone_inherited_via_parent_note_in_reason(self) -> None: + from kennel.worker import _pick_next_issue + + nodes = { + 10: _issue( + 10, + "parent", + milestone="v1", + sub_issues=[_issue(20, state="OPEN", assignees=["fido"], parent=10)], + ) + } + assigned = _issue(20, state="OPEN", assignees=["fido"], parent=10) + choice = _pick_next_issue( + [assigned], + "fido", + get_issue_node=lambda n: nodes[n], + get_sub_issues=lambda n: [], + claim=lambda n: None, + ) + assert choice is not None + assert choice.number == 20 + assert "milestone from parent #10" in choice.reason class TestWorkerPostPickupComment: