🐛 [fix] github-read, approval: diff line splitting and non-default-base links

scripts/super-board-approval.py

@@ -25,6 +25,13 @@
 SCOPE = re.compile(r"[0-9a-f]{64}\Z")
 BLOCK = re.compile(r"Reason tag:[^\n]*🙋|^approval-request:", re.M)
 DONE = re.compile(r"\s*done[.!]?\s*\Z", re.I)
+# GitHub's closing keywords: KEYWORD[:] #N or KEYWORD[:] owner/repo#N.
+CLOSES = re.compile(r"\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?):?\s+([\w.-]+/[\w.-]+)?#(\d+)\b", re.I)
+
+
+def body_links(body, repo):
+    """Same-repo issues a PR body closes; other-repo references are ignored."""
+    return {int(n) for other, n in CLOSES.findall(body) if not other or other.lower() == repo}
 
 
 def scope_for(plan):
@@ -159,33 +166,41 @@ def comments(self, number):
     def pr(self, number):
         # Paginate linked issues too: an omitted newer request must never make
         # an older approval authoritative. Remote-repository links are ignored.
+        # GitHub fills closingIssuesReferences only for PRs into the default
+        # branch, so a PR into another base takes its body's closing keywords,
+        # read in this same head-pinned snapshot.
         query = '''query($owner:String!,$repo:String!,$pr:Int!,$endCursor:String){
-          repository(owner:$owner,name:$repo){pullRequest(number:$pr){headRefOid state
+          repository(owner:$owner,name:$repo){defaultBranchRef{name}
+            pullRequest(number:$pr){headRefOid state baseRefName body
             closingIssuesReferences(first:100,after:$endCursor){
               pageInfo{hasNextPage endCursor} nodes{number repository{nameWithOwner}}
             }}}}'''
         owner, name = self.repo.split("/")
         pages = self.read("api", "graphql", "--paginate", "--slurp", "-f", f"query={query}",
                           "-F", f"owner={owner}", "-F", f"repo={name}", "-F", f"pr={number}")
-        heads, issues = set(), set()
+        snapshots, issues = set(), set()
         if not isinstance(pages, list) or not pages:
             raise ValueError("unreadable PR")
         for page in pages:
             if page.get("errors"):
                 raise ValueError("incomplete PR response")
-            pr = page["data"]["repository"]["pullRequest"]
+            repository = page["data"]["repository"]
+            pr = repository["pullRequest"]
             if pr["state"] != "OPEN":
                 raise ValueError("PR is not open")
-            heads.add(pr["headRefOid"])
+            snapshots.add((pr["headRefOid"], pr["baseRefName"], pr["body"],
+                           (repository["defaultBranchRef"] or {}).get("name")))
             links = pr["closingIssuesReferences"]
             for issue in links["nodes"]:
                 if issue["repository"]["nameWithOwner"].lower() == self.repo:
                     issues.add(issue["number"])
-        if links["pageInfo"]["hasNextPage"] or len(heads) != 1:
+        if links["pageInfo"]["hasNextPage"] or len(snapshots) != 1:
             raise ValueError("PR changed or issue links are incomplete")
-        head = heads.pop()
-        if not SHA.fullmatch(head):
-            raise ValueError("unreadable PR head")
+        head, base, body, default = snapshots.pop()
+        if not all(isinstance(v, str) for v in (head, base, body, default)) or not SHA.fullmatch(head):
+            raise ValueError("unreadable PR head, base or body")
+        if base != default:
+            issues |= body_links(body, self.repo)
         return head, issues
 
     def check(self, number, head=None, scope=None, issue=None):

scripts/super-board-github-read.py

@@ -141,7 +141,9 @@ def validate(text, kind, meta=None):
         if not files:
             require(not text.strip() and meta['additions'] == meta['deletions'] == 0, 'diff and file list disagree')
             return text
-        lines = text.splitlines()
+        # Split on \n only: splitlines() also breaks on VT, FF, U+2028 and friends,
+        # which can sit inside a changed line and look like missing hunk data.
+        lines = text.split('\n')
         require(sum(line.startswith('diff --git ') for line in lines) == len(files), 'diff file list incomplete')
         added = removed = 0
         old_left = new_left = 0
@@ -303,7 +305,10 @@ def read(args, kind='json', meta=None, recovering=False):
         if not recovering:
             check_halt()
         try:
-            result = subprocess.run(['gh', *args], capture_output=True, text=True, timeout=30)
+            result = subprocess.run(['gh', *args], capture_output=True, timeout=30)
+            # Decode by hand: text=True turns a lone \r inside a diff line into \n.
+            result.stdout = result.stdout.decode('utf-8', 'replace')
+            result.stderr = result.stderr.decode('utf-8', 'replace')
             diagnostic = result.stderr
             if result.returncode:
                 diagnostic += result.stdout

tests/test-merge-gate.sh

@@ -152,7 +152,7 @@ for i, f in enumerate(m.get('files', [])):
 DIFF
     ;;
   "api graphql")
-    jq -n --arg head "$STUB_OID" '[{data:{repository:{pullRequest:{headRefOid:$head,state:"OPEN",closingIssuesReferences:{pageInfo:{hasNextPage:false,endCursor:null},nodes:[{number:42,repository:{nameWithOwner:"x/y"}}]}}}}}]' ;;
+    jq -n --arg head "$STUB_OID" '[{data:{repository:{defaultBranchRef:{name:"main"},pullRequest:{headRefOid:$head,state:"OPEN",baseRefName:"main",body:"",closingIssuesReferences:{pageInfo:{hasNextPage:false,endCursor:null},nodes:[{number:42,repository:{nameWithOwner:"x/y"}}]}}}}}]' ;;
   "api --paginate")
     [ "${STUB_APPROVAL:-0}" = 1 ] || { echo '[[]]'; exit 0; }
     case "$*" in

tests/test_approval.py

@@ -121,7 +121,8 @@ def test_missing_time_and_malformed_marker_hold(self):
 class GitHubTests(unittest.TestCase):
     def setUp(self):
         self.github = approval.GitHub("x/y")
-        self.pr_page = {"data": {"repository": {"pullRequest": {"headRefOid": HEAD, "state": "OPEN",
+        self.pr_page = {"data": {"repository": {"defaultBranchRef": {"name": "main"},
+            "pullRequest": {"headRefOid": HEAD, "state": "OPEN", "baseRefName": "main", "body": "",
             "closingIssuesReferences": {"pageInfo": {"hasNextPage": False, "endCursor": None},
                 "nodes": [{"number": 3, "repository": {"nameWithOwner": "x/y"}}]}}}}}
         self.calls = []
@@ -160,6 +161,33 @@ def test_incomplete_link_pagination_is_not_accepted(self):
     def test_unlinked_issue_cannot_supply_approval(self):
         self.assertFalse(self.github.check(9, HEAD, REQUEST["scope"], issue=77)["approved"])
 
+    def non_default_base(self, body):
+        # GitHub leaves closingIssuesReferences empty for PRs into a non-default base (#63).
+        pr = self.pr_page["data"]["repository"]["pullRequest"]
+        pr["baseRefName"], pr["body"] = "staging", body
+        pr["closingIssuesReferences"]["nodes"] = []
+
+    def test_non_default_base_takes_closing_keywords_from_body(self):
+        self.non_default_base("Ships login.\n\nCloses #3")
+        self.assertTrue(self.github.check(9, HEAD, REQUEST["scope"], issue=3)["approved"])
+
+    def test_non_default_base_without_closing_keyword_is_unlinked(self):
+        self.non_default_base("Related to #3, see x/y#3 discussion")
+        self.assertFalse(self.github.check(9, HEAD, REQUEST["scope"], issue=3)["approved"])
+
+    def test_non_default_base_ignores_other_repository_references(self):
+        self.non_default_base("Fixes other/repo#3")
+        self.assertFalse(self.github.check(9, HEAD, REQUEST["scope"], issue=3)["approved"])
+
+    def test_non_default_base_accepts_same_repository_form(self):
+        self.non_default_base("resolved: X/Y#3")
+        self.assertTrue(self.github.check(9, HEAD, REQUEST["scope"], issue=3)["approved"])
+
+    def test_default_base_ignores_body_keywords(self):
+        self.pr_page["data"]["repository"]["pullRequest"]["closingIssuesReferences"]["nodes"] = []
+        self.pr_page["data"]["repository"]["pullRequest"]["body"] = "Closes #3"
+        self.assertFalse(self.github.check(9, HEAD, REQUEST["scope"], issue=3)["approved"])
+
     def test_unreadable_permission_never_approves(self):
         self.permission_error = True
         self.assertFalse(self.github.check(9, HEAD, REQUEST["scope"])["approved"])
@@ -184,7 +212,7 @@ def test_live_planner_resume_requires_current_trusted_request(self):
 if "issues(states:OPEN" in args:
     out=[{"data":{"repository":{"issues":{"nodes":[f["issue"]],"pageInfo":{"hasNextPage":False,"endCursor":None}}}}}]
 elif "graphql" in args:
-    out=[{"data":{"repository":{"pullRequest":{"headRefOid":f["head"],"state":"OPEN","closingIssuesReferences":{"pageInfo":{"hasNextPage":False},"nodes":[{"number":3,"repository":{"nameWithOwner":"x/y"}}]}}}}}]
+    out=[{"data":{"repository":{"defaultBranchRef":{"name":"main"},"pullRequest":{"headRefOid":f["head"],"state":"OPEN","baseRefName":"main","body":"","closingIssuesReferences":{"pageInfo":{"hasNextPage":False},"nodes":[{"number":3,"repository":{"nameWithOwner":"x/y"}}]}}}}}]
 elif "/issues/3/comments" in args:
     out=[f["records"]]
 elif "/issues/9/comments" in args:

tests/test_github_reads.py

@@ -166,6 +166,21 @@ def test_truncated_diff_is_not_complete_evidence(self):
         self.assertEqual(result.stdout, b'')
         self.assertEqual(self.log.read_text(), '3')
 
+    def test_diff_lines_with_line_like_characters_are_complete_evidence(self):
+        # VT, FF, FS, NEL, U+2028/9 and a lone CR are content inside a changed line,
+        # not line breaks; only \n ends a diff line (#62).
+        metadata = self.root / 'meta.json'
+        metadata.write_text(json.dumps({'files':[{'path':'dump.sql'}], 'additions':4,'deletions':1}))
+        diff = ('diff --git a/dump.sql b/dump.sql\nindex 1..2 100644\n--- a/dump.sql\n+++ b/dump.sql\n'
+                '@@ -1,3 +1,6 @@\n keep\n-old\x0bline\n+new\x0bvt\x0cff\n+sep\u2028\u2029para\x1c\x85\n'
+                '+lone\rcr\n+crlf\r\n keep2\n')
+        self.env['READ_RESPONSES'] = json.dumps([{'out': diff}])
+        result = subprocess.run(['python3', str(HELPER), '--kind', 'diff', '--meta', str(metadata),
+                                 '--', 'pr', 'diff', '1'], env=self.env, cwd=self.root, capture_output=True)
+        self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertEqual(self.log.read_text(), '1')
+        self.assertEqual(result.stdout.decode(), diff)
+
     def test_corrected_query_can_resume_without_erasing_original_failure(self):
         bad = ['api', 'graphql', '-f', 'query=query { wrong }']
         self.assertEqual(self.read([{'rc':1,'err':'Cannot query field wrong'}], command=bad).returncode,79)