diff --git a/scripts/super-board-approval.py b/scripts/super-board-approval.py index 2f161e96..67fe1897 100644 --- a/scripts/super-board-approval.py +++ b/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): diff --git a/scripts/super-board-github-read.py b/scripts/super-board-github-read.py index 9315a187..3a8b845f 100644 --- a/scripts/super-board-github-read.py +++ b/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 diff --git a/tests/test-merge-gate.sh b/tests/test-merge-gate.sh index 03b74768..aa1a60f4 100755 --- a/tests/test-merge-gate.sh +++ b/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 diff --git a/tests/test_approval.py b/tests/test_approval.py index 26148543..940f9bd8 100644 --- a/tests/test_approval.py +++ b/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: diff --git a/tests/test_github_reads.py b/tests/test_github_reads.py index c447c87f..3940bea8 100644 --- a/tests/test_github_reads.py +++ b/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)