Move the Null GraphQL Node Check Into the Shared Transport

scripts/gh_graphql.py

@@ -32,6 +32,18 @@ def __init__(self, message: str, detail: str = "") -> None:
         self.detail = detail
 
 
+class NullNodeError(GraphQLError):
+    """A response holding a null list item, which GitHub could not resolve at read time.
+
+    GraphQL can return one with no entry in `errors`, so nothing read off the response is
+    trustworthy. `where` is the dotted path of the first connection holding one.
+    """
+
+    def __init__(self, where: str) -> None:
+        super().__init__(f"gh graphql returned a null node GitHub could not resolve under {where}")
+        self.where = where
+
+
 class Response(NamedTuple):
     """The `data` of one response, and the errors it carried that the caller chose to tolerate."""
 
@@ -90,6 +102,9 @@ def request(
     `gh api graphql` exits non-zero with the JSON still on stdout when the response carries
     errors, so that stdout is read first. An error is tolerated only where `tolerate` accepts it,
     and the tolerated ones come back beside the data for the caller to report.
+
+    A null item in any `nodes` list raises `NullNodeError`, since every caller would otherwise
+    index into it. A caller reading a query may read again before giving up.
     """
     code, stdout, stderr = run(build_argv(query, variables))
     try:
@@ -105,9 +120,24 @@ def request(
         raise GraphQLError("gh graphql returned no data", json.dumps(errors)[:DETAIL_CAP])
     if not isinstance(payload["data"], dict):
         raise GraphQLError("gh graphql returned data that is not an object")
+    if (where := null_node(payload["data"])) is not None:
+        raise NullNodeError(where)
     return Response(payload["data"], list(errors or []))
 
 
+def null_node(data: object, path: str = "") -> str | None:
+    """The path of the first connection under `data` whose `nodes` holds a null, else None."""
+    if isinstance(data, list):
+        items = [(path, value) for value in data]
+    elif isinstance(data, dict):
+        if isinstance(nodes := data.get("nodes"), list) and None in nodes:
+            return path or "nodes"
+        items = [(f"{path}.{key}".lstrip("."), value) for key, value in data.items()]
+    else:
+        return None
+    return next((found for key, value in items if (found := null_node(value, key))), None)
+
+
 def _descend(node: object, path: Sequence[str]) -> object:
     for key in path:
         if not isinstance(node, dict) or key not in node:

scripts/pr_cleanup.py

@@ -206,7 +206,7 @@ def branch_users(owner: str, repo: str, branch: str) -> list[str]:
     total = found.get("totalCount") if isinstance(found, dict) else None
     if not isinstance(nodes, list) or not isinstance(total, int):
         raise CleanupUnread(f"the open pull request read returned no bases list: {found!r}"[:400])
-    users += [f"#{n.get('number')} (base)" for n in nodes if isinstance(n, dict)]
+    users += [f"#{n.get('number')} (base)" for n in nodes]
     if total > len(nodes):
         users.append(f"{total - len(nodes)} more with the branch as base")
     return users
@@ -868,8 +868,7 @@ def finished(self, branch: str, tip: str) -> tuple[str, int | None] | None:
         matches = [
             n
             for n in nodes
-            if isinstance(n, dict)
-            and in_repository(n, self.owner, self.repo)
+            if in_repository(n, self.owner, self.repo)
             and (
                 n.get("headRefOid") == tip
                 or cleanup_behind(self.primary, tip, n.get("headRefOid") or "")

scripts/pr_review.py

@@ -280,7 +280,6 @@
     newest_first,
     newest_of,
     newest_request_event,
-    null_node,
     open_threads,
     polled_past,
     possible_quota,
@@ -495,25 +494,25 @@ def gh_graphql(query: str, **variables) -> dict:
 
     The transport raises `GraphQLError` and never exits, so a failed read prints its detail and
     ends the run at exit 1. Only an error set made of `workflowRun` paths is tolerated. A response
-    holding a null node ends the run too, and only that case is retried first. A query is read
-    again on `READ_RETRY_DELAYS`, and a mutation is never sent twice.
+    holding a null node ends the run too, after a query is read again on `READ_RETRY_DELAYS`. A
+    mutation is never sent twice.
     """
 
-    def send() -> shared.Response:
+    def send() -> shared.Response | shared.NullNodeError:
         try:
             return shared.request(query, variables, tolerate=_is_workflow_run_error)
+        except shared.NullNodeError as exc:
+            return exc
         except shared.GraphQLError as exc:
             sys.stderr.write(exc.detail)
             raise SystemExit(str(exc)) from exc
 
     reads = query.lstrip().startswith("query")
     delays = READ_RETRY_DELAYS if reads else ()
-    response = backoff(send(), send, lambda r: null_node(r.data) is not None, delays=delays)
-    if (where := null_node(response.data)) is not None:
-        raise SystemExit(
-            f"gh graphql returned a null node GitHub could not resolve under {where}, so the "
-            "response is unread" + ("" if reads else ", though the write it answers was sent")
-        )
+    response = backoff(send(), send, lambda r: isinstance(r, shared.NullNodeError), delays=delays)
+    if isinstance(response, shared.NullNodeError):
+        sent = "" if reads else ", though the write it answers was sent"
+        raise SystemExit(f"{response}, so the response is unread{sent}")
     data = response.data
     if response.tolerated:
         sys.stderr.write("gh graphql: workflowRun unreadable, checks are not deduped by workflow\n")

tests/test_gh_graphql.py

@@ -38,6 +38,20 @@ def test_the_failures_raise_and_never_exit(self) -> None:
             with self.subTest(label=label), self.assertRaises(gh_graphql.GraphQLError):
                 gh_graphql.request("q", {}, run=run)
 
+    def test_a_null_node_raises_naming_where_it_sat(self) -> None:
+        data = {"repo": {"issues": {"nodes": [{"labels": {"nodes": [None, {"name": "x"}]}}]}}}
+        with self.assertRaises(gh_graphql.NullNodeError) as caught:
+            gh_graphql.request("q", {}, run=answering(0, {"data": data}))
+        self.assertIsInstance(caught.exception, gh_graphql.GraphQLError)
+        self.assertEqual("repo.issues.nodes.labels", caught.exception.where)
+
+    def test_a_response_without_a_null_node_has_none_to_name(self) -> None:
+        data = {"a": {"nodes": [{"b": {"nodes": []}}]}, "c": [1, None]}
+        self.assertIsNone(gh_graphql.null_node(data))
+
+    def test_a_bare_nodes_list_holding_a_null_is_named_nodes(self) -> None:
+        self.assertEqual("nodes", gh_graphql.null_node({"nodes": [None]}))
+
     def test_a_failed_launch_raises_graphql_error(self) -> None:
         with (
             unittest.mock.patch("subprocess.run", side_effect=OSError("no gh")),

tests/test_handoff.py

@@ -795,6 +795,29 @@ def test_a_sibling_track_does_not_answer_for_this_one(self) -> None:
         self.assertEqual(run(fake, "current", "--repo", "o/r", "--track", "lane")[0], 1)
 
 
+class NullLabelNodeCase(unittest.TestCase):
+    """A null label node reports as a read that did not run, where it once raised TypeError."""
+
+    def test_a_null_label_node_is_an_execution_failure(self) -> None:
+        body = {
+            "repository": {
+                "hasIssuesEnabled": True,
+                "issues": {
+                    "pageInfo": {"hasNextPage": False, "endCursor": None},
+                    "nodes": [{"number": 1, "labels": {"nodes": [None, {"name": "handoff"}]}}],
+                },
+            }
+        }
+        with (
+            unittest.mock.patch.object(
+                handoff, "gh_runner", lambda argv: (0, json.dumps({"data": body}), "")
+            ),
+            self.assertRaises(handoff.Execution) as caught,
+        ):
+            handoff.labeled("o/r", "OPEN", 10, "open handoffs")
+        self.assertIn("null node", str(caught.exception))
+
+
 class ExitCodeCase(unittest.TestCase):
     """A finding and the check not having run never share a code."""
 

tests/test_pr_cleanup.py

@@ -309,6 +309,7 @@ def test_an_open_pull_request_read_that_does_not_answer_stops_at_2(self) -> None
             {"head_answer": {"heads": {"pageInfo": page, "nodes": ["#9"]}}},
             {"head_answer": {"heads": {"pageInfo": {"hasNextPage": True}, "nodes": []}}},
             {"base_answer": bad},
+            {"base_answer": pr_cleanup.shared.NullNodeError("repository.bases.nodes")},
             {"base_answer": {"bases": {"totalCount": "0", "nodes": []}}},
         ]
         for answer in malformed:
@@ -1017,6 +1018,7 @@ def setUp(self) -> None:
         super().setUp()
         self.merged_prs: dict[str, list[dict]] = {}
         self.failing: set[str] = set()
+        self.failure = pr_cleanup.shared.GraphQLError("gh graphql reported errors")
 
     def age(self, path: str, hours: float) -> None:
         """Set every file the sweep reads the age from to `hours` ago."""
@@ -1055,7 +1057,7 @@ def fake(query: str, variables: dict, **kw: Any) -> pr_cleanup.shared.Response:
                 return pr_cleanup.shared.Response({"repository": head}, [])
             if query == pr_cleanup.Q_SWEEP_MERGED:
                 if variables["b"] in self.failing:
-                    raise pr_cleanup.shared.GraphQLError("gh graphql reported errors")
+                    raise self.failure
                 nodes = self.merged_prs.get(variables["b"], [])
                 page = {"merged": {"totalCount": len(nodes), "nodes": nodes}}
                 return pr_cleanup.shared.Response({"repository": page}, [])
@@ -1168,6 +1170,19 @@ def test_a_read_failure_continues_and_exits_2(self) -> None:
         self.assertNotIn(made["merged"], self.listed())
         self.assertIn(early, self.listed())
 
+    def test_a_null_merged_node_reads_as_unread_and_exits_2(self) -> None:
+        made = self.scenario()
+        early, _ = self.worktree("aa-failing")
+        self.age(early, 48)
+        self.failing.add("aa-failing")
+        self.failure = pr_cleanup.shared.NullNodeError("repository.merged.nodes")
+        tip = run_git(self.primary, "rev-parse", CLEANUP_BRANCH)
+        self.wire_sweep(remote_tip=tip)
+        self.assertEqual(2, self.sweep("--apply"))
+        self.assertIn("unread worktree", self.out.getvalue())
+        self.assertNotIn(made["merged"], self.listed())
+        self.assertIn(early, self.listed())
+
     def test_the_remote_check_in_a_report_names_a_refusal(self) -> None:
         self.scenario()
         self.wire_sweep(remote_tip=run_git(self.primary, "rev-parse", CLEANUP_BRANCH), merged=False)

tests/test_pr_review.py

@@ -2847,12 +2847,6 @@ def holding_null(self) -> dict:
         pr["comments"]["nodes"].insert(0, None)
         return pr
 
-    def test_null_node_names_the_connection_holding_one(self) -> None:
-        pr = payload([review()], [thread("T1")])
-        self.assertIsNone(pr_review_rounds.null_node(pr))
-        pr["reviewThreads"]["nodes"][0]["comments"]["nodes"].append(None)
-        self.assertEqual("reviewThreads.nodes.comments", pr_review_rounds.null_node(pr))
-
     def test_a_query_is_read_again_past_a_null_node(self) -> None:
         clean = payload([review()], comments=[comment(at=EARLY)])
         reads = [self.body(self.holding_null()), self.body(clean)]