From 6aa386ba1ad6bdc44263b88a3f454a7dad621436 Mon Sep 17 00:00:00 2001 From: Jeff Dickey <216188+jdx@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:35:14 -0500 Subject: [PATCH 1/9] feat: add release fix notifications workflow --- .github/workflows/comment-release-fixes.yml | 42 +++++++ scripts/comment-release-fixes.py | 132 ++++++++++++++++++++ scripts/test_comment_release_fixes.py | 30 +++++ 3 files changed, 204 insertions(+) create mode 100644 .github/workflows/comment-release-fixes.yml create mode 100644 scripts/comment-release-fixes.py create mode 100644 scripts/test_comment_release_fixes.py diff --git a/.github/workflows/comment-release-fixes.yml b/.github/workflows/comment-release-fixes.yml new file mode 100644 index 0000000..b293b78 --- /dev/null +++ b/.github/workflows/comment-release-fixes.yml @@ -0,0 +1,42 @@ +# Call after a GitHub Release is published, or directly from a publishing job +# whose GITHUB_TOKEN would suppress a release event. Pin callers to a release SHA. +name: comment-release-fixes +on: + workflow_dispatch: + inputs: + tag: { required: true, type: string } + tag_pattern: { required: false, type: string, default: '^v[0-9][0-9A-Za-z._+-]*$' } + marker_prefix: { required: true, type: string } + upgrade_text: { required: false, type: string, default: '' } + dry_run: { required: false, type: boolean, default: true } + workflow_call: + inputs: + tag: { required: true, type: string } + tag_pattern: { required: false, type: string, default: '^v[0-9][0-9A-Za-z._+-]*$' } + marker_prefix: { required: true, type: string } + upgrade_text: { required: false, type: string, default: '' } + dry_run: { required: false, type: boolean, default: false } +jobs: + comment: + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + issues: write + discussions: write + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + repository: jdx/workflows + ref: ${{ github.workflow_sha }} + path: workflows + persist-credentials: false + - env: + GH_TOKEN: ${{ github.token }} + GITHUB_REPOSITORY: ${{ github.repository }} + TAG: ${{ inputs.tag }} + TAG_PATTERN: ${{ inputs.tag_pattern }} + MARKER_PREFIX: ${{ inputs.marker_prefix }} + UPGRADE_TEXT: ${{ inputs.upgrade_text }} + DRY_RUN: ${{ inputs.dry_run && '1' || '0' }} + run: python3 workflows/scripts/comment-release-fixes.py diff --git a/scripts/comment-release-fixes.py b/scripts/comment-release-fixes.py new file mode 100644 index 0000000..8fdec3a --- /dev/null +++ b/scripts/comment-release-fixes.py @@ -0,0 +1,132 @@ +#!/usr/bin/env python3 +"""Notify closed, explicit same-repository fixes after a release ships.""" +import json, os, re, subprocess, sys + +KEYWORD = r"(?:close(?:s|d)?|fix(?:es|ed)?|resolve(?:s|d)?)" +REF = re.compile(rf"\b{KEYWORD}\s*:?\s+(?:discussion\s+)?(?:(?Phttps://github\.com/(?P[\w.-]+)/(?P[\w.-]+)/(?:issues|discussions)/(?P[1-9]\d*))|#(?P[1-9]\d*))\b", re.I) +SUBJECT = re.compile(r"(?:\(#(?P[1-9]\d*)\)|Merge pull request #(?P[1-9]\d*))", re.I) + +class Failure(Exception): pass + +def clean(body): + body = re.sub(r"", "", body or "") + body = re.sub(r"(?ms)^\s*(```|~~~).*?^\s*\1\s*$", "", body) + return "\n".join(re.sub(r"`[^`]*`", "", l) for l in body.splitlines() + if not re.match(r"^(?:\s{4}|\t|\s*>).*", l)) + +def refs(body, owner, repo): + result=[] + for m in REF.finditer(clean(body)): + if m["num"]: result.append(int(m["num"])) + elif m["owner"].lower()==owner.lower() and m["repo"].lower()==repo.lower(): result.append(int(m["urlnum"])) + return list(dict.fromkeys(result)) + +class GH: + def api(self, path, method="GET", data=None): + cmd=["gh","api",path,"-X",method] + if data is not None: cmd += ["--input","-"] + r=subprocess.run(cmd,input=json.dumps(data) if data is not None else None,text=True,capture_output=True) + if r.returncode: raise Failure(f"{path}: {r.stderr.strip() or 'GitHub API request failed'}") + try: return json.loads(r.stdout) + except json.JSONDecodeError as e: raise Failure(f"{path}: invalid JSON response") from e + def gql(self, query, **values): + cmd=["gh","api","graphql","-f",f"query={query}"] + for k,v in values.items(): + if v is not None: cmd += ["-F" if isinstance(v,int) else "-f",f"{k}={v}"] + r=subprocess.run(cmd,text=True,capture_output=True) + if r.returncode: raise Failure(f"GraphQL: {r.stderr.strip() or 'request failed'}") + d=json.loads(r.stdout) + if d.get("errors"): raise Failure("GraphQL: "+"; ".join(x["message"] for x in d["errors"])) + return d["data"] + +def paged(gh, path): + for page in range(1, 10000): + items=gh.api(f"{path}{'&' if '?' in path else '?'}per_page=100&page={page}") + yield from items + if len(items)<100: return + +def base_release(gh, repo, tag, pattern): + rs=[r for r in paged(gh,f"repos/{repo}/releases") if not r["draft"] and not r["prerelease"] and pattern.fullmatch(r["tag_name"])] + if tag not in {r["tag_name"] for r in rs}: raise Failure(f"{tag} is not a stable published release in this tag family") + choices=[]; errors=[] + for i,r in enumerate(rs): + old=r["tag_name"] + if old==tag: continue + try: cmp=gh.api(f"repos/{repo}/compare/{old}...{tag}?per_page=1") + except Failure as e: errors.append(str(e)); continue + # Identical tag aliases do not represent a fresh shipment. + if cmp.get("status")=="ahead" and cmp.get("ahead_by",0)>0: choices.append((cmp["ahead_by"],i,old)) + if errors: raise Failure("cannot safely establish release ancestry:\n"+"\n".join(errors)) + if not choices: raise Failure(f"no earlier contained release found for {tag}; refusing to widen the range") + return min(choices)[2] + +def commits(gh, repo, base, tag): + out=[] + for page in range(1,10000): + part=gh.api(f"repos/{repo}/compare/{base}...{tag}?per_page=100&page={page}").get("commits",[]); out+=part + if len(part)<100:return out + +def shipped_prs(gh, repo, changes): + shas={c["sha"] for c in changes}; nums=set() + for c in changes: + m=SUBJECT.search(c.get("commit",{}).get("message","").split("\n",1)[0]) + if m: nums.add(int(m["squash"] or m["merge"])) + out=[] + for n in sorted(nums): + pr=gh.api(f"repos/{repo}/pulls/{n}") + # Subject syntax is only a lead: require a merged PR and its merge commit + # in the compared range, preventing unmerged/spoofed PR notifications. + if pr.get("merged_at") and pr.get("merge_commit_sha") in shas: out.append(pr) + return out + +DISC="""query($owner:String!,$repo:String!,$number:Int!,$cursor:String){repository(owner:$owner,name:$repo){discussion(number:$number){id closed comments(first:100,after:$cursor){pageInfo{hasNextPage endCursor}nodes{body author{login}}}}}}""" +ADD="mutation($id:ID!,$body:String!){addDiscussionComment(input:{discussionId:$id,body:$body}){comment{id}}}" + +def get_discussion(gh, owner, repo, number): + cursor=None; comments=[] + while True: + d=gh.gql(DISC,owner=owner,repo=repo,number=number,cursor=cursor).get("repository",{}).get("discussion") + if not d:return None + comments += d["comments"]["nodes"]; pi=d["comments"]["pageInfo"] + if not pi["hasNextPage"]: return {**d,"all_comments":comments} + cursor=pi["endCursor"] + +def mark(prefix,tag): return f"" +def text(repo,tag,pr,prefix,upgrade): return f"{mark(prefix,tag)}\nFixed in [{tag}](https://github.com/{repo}/releases/tag/{tag}) by #{pr}."+(f" {upgrade}" if upgrade else "") +def has_issue_comment(gh,repo,num,marker,actor): + return any(c.get("user",{}).get("login")==actor and marker in (c.get("body") or "") for c in paged(gh,f"repos/{repo}/issues/{num}/comments")) + +def notify(gh,repo,tag,prefix,upgrade,dry,out=print): + owner,_=repo.split("/",1); pattern=re.compile(os.environ.get("TAG_PATTERN",r"^v[0-9][0-9A-Za-z._+-]*$")) + if not pattern.fullmatch(tag): raise Failure(f"{tag} does not match TAG_PATTERN") + actor=gh.api("user").get("login") + if not actor: raise Failure("cannot determine token identity for safe deduplication") + base=base_release(gh,repo,tag,pattern); changes=commits(gh,repo,base,tag); pulls=shipped_prs(gh,repo,changes) + out(json.dumps({"tag":tag,"base":base,"commits":len(changes),"pulls":[p["number"] for p in pulls],"dry_run":dry})) + failures=[]; seen=set() + for pr in pulls: + try: + for n in refs(pr.get("body") or "",owner,repo): + if n in seen: continue + body=text(repo,tag,pr["number"],prefix,upgrade); marker=mark(prefix,tag) + try: + issue=gh.api(f"repos/{repo}/issues/{n}") + if issue.get("state")!="closed" or issue.get("pull_request"): out(f"skip issue #{n}: not a closed issue") + elif has_issue_comment(gh,repo,n,marker,actor): out(f"skip issue #{n}: already commented for {tag}") + elif dry: out(f"[dry run] would comment on issue #{n} (from #{pr['number']})") + else: gh.api(f"repos/{repo}/issues/{n}/comments","POST",{"body":body}); out(f"commented on issue #{n} (from #{pr['number']})") + except Failure as e: + if "404" not in str(e): raise + d=get_discussion(gh,owner,repo,n) + if not d: raise Failure(f"target #{n} is neither an accessible issue nor discussion") + if not d["closed"]: out(f"skip discussion #{n}: still open") + elif any(c.get("author",{}).get("login")==actor and marker in (c.get("body") or "") for c in d["all_comments"]): out(f"skip discussion #{n}: already commented for {tag}") + elif dry: out(f"[dry run] would comment on discussion #{n} (from #{pr['number']})") + else: gh.gql(ADD,id=d["id"],body=body); out(f"commented on discussion #{n} (from #{pr['number']})") + seen.add(n) + except Exception as e: failures.append(f"PR #{pr['number']}: {e}") + if failures: raise Failure("release notification failures:\n"+"\n".join(failures)) + +if __name__=="__main__": + try: notify(GH(),os.environ["GITHUB_REPOSITORY"],os.environ["TAG"],os.environ["MARKER_PREFIX"],os.environ.get("UPGRADE_TEXT",""),os.environ.get("DRY_RUN")=="1") + except Failure as e: print(f"error: {e}",file=sys.stderr); sys.exit(1) diff --git a/scripts/test_comment_release_fixes.py b/scripts/test_comment_release_fixes.py new file mode 100644 index 0000000..c9aa1e5 --- /dev/null +++ b/scripts/test_comment_release_fixes.py @@ -0,0 +1,30 @@ +import importlib.util, pathlib, re, unittest +spec=importlib.util.spec_from_file_location("r",pathlib.Path(__file__).with_name("comment-release-fixes.py")); r=importlib.util.module_from_spec(spec); spec.loader.exec_module(r) +class API: + def __init__(self,x):self.x=x;self.calls=[] + def api(self,p,method="GET",data=None): + self.calls.append((p,method,data)); v=self.x.get(p,self.x.get(p.split("?")[0],[])) + if isinstance(v,Exception):raise v + return v + def gql(self,*args,**kw):return {"repository":{"discussion":None}} +class T(unittest.TestCase): + def test_refs_exclude_examples_and_support_url(self): + self.assertEqual(r.refs("Fixes #1\nResolves https://github.com/a/b/discussions/2\n`Fixes #3`\n Fixes #4\n> Fixes #5\n~~~\nFixes #6\n~~~", "a","b"),[1,2]) + def test_tags_use_ancestry_and_exclude_identical(self): + a=API({"repos/a/b/releases?per_page=100&page=1":[{"tag_name":"v3","draft":False,"prerelease":False},{"tag_name":"v1","draft":False,"prerelease":False},{"tag_name":"v2","draft":False,"prerelease":False},{"tag_name":"v2x","draft":False,"prerelease":False}],"repos/a/b/compare/v1...v3?per_page=1":{"status":"ahead","ahead_by":9},"repos/a/b/compare/v2...v3?per_page=1":{"status":"ahead","ahead_by":2},"repos/a/b/compare/v2x...v3?per_page=1":{"status":"identical","ahead_by":0}}) + self.assertEqual(r.base_release(a,"a/b","v3",re.compile("^v.*")),"v2") + def test_unmerged_or_missing_merge_commit_not_shipped(self): + a=API({"repos/a/b/pulls/1":{"number":1,"merged_at":"now","merge_commit_sha":"m"},"repos/a/b/pulls/2":{"number":2,"merged_at":None,"merge_commit_sha":"x"}}) + changes=[{"sha":"m","commit":{"message":"x (#1)"}},{"sha":"x","commit":{"message":"x (#2)"}}] + self.assertEqual([p["number"] for p in r.shipped_prs(a,"a/b",changes)],[1]) + def test_missing_release_tag_fails_without_widening_range(self): + a=API({"repos/a/b/releases?per_page=100&page=1":[{"tag_name":"v1","draft":False,"prerelease":False}]}) + with self.assertRaises(r.Failure): r.base_release(a,"a/b","v2",re.compile("^v.*")) + def test_issue_dedup_paginates_and_uses_current_identity(self): + a=API({"repos/a/b/issues/1/comments?per_page=100&page=1":[{"user":{"login":"old"},"body":""}]*100,"repos/a/b/issues/1/comments?per_page=100&page=2":[{"user":{"login":"new"},"body":""}]}) + self.assertTrue(r.has_issue_comment(a,"a/b",1,"","new")) + def test_dry_run_reports_historical_metadata_without_writes(self): + a=API({"user":{"login":"robot"},"repos/a/b/releases?per_page=100&page=1":[{"tag_name":"v2","draft":False,"prerelease":False},{"tag_name":"v1","draft":False,"prerelease":False}],"repos/a/b/compare/v1...v2?per_page=1":{"status":"ahead","ahead_by":1},"repos/a/b/compare/v1...v2?per_page=100&page=1":{"commits":[{"sha":"m","commit":{"message":"x (#1)"}}]},"repos/a/b/pulls/1":{"number":1,"merged_at":"now","merge_commit_sha":"m","body":"Fixes #2"},"repos/a/b/issues/2":{"state":"closed"},"repos/a/b/issues/2/comments?per_page=100&page=1":[]}) + output=[]; r.notify(a,"a/b","v2","fixed","",True,output.append) + self.assertIn('"base": "v1"',output[0]); self.assertFalse(any(m=="POST" for _,m,_ in a.calls)) +if __name__=="__main__":unittest.main() From 3f6e2ca3eb927166a8912c1259af215175f6802a Mon Sep 17 00:00:00 2001 From: Jeff Dickey <216188+jdx@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:41:15 -0500 Subject: [PATCH 2/9] fix: harden release notification contracts --- .github/workflows/comment-release-fixes.yml | 3 + scripts/comment-release-fixes.py | 33 ++- scripts/test_comment_release_fixes.py | 214 +++++++++++++++++--- 3 files changed, 211 insertions(+), 39 deletions(-) diff --git a/.github/workflows/comment-release-fixes.yml b/.github/workflows/comment-release-fixes.yml index b293b78..25807b0 100644 --- a/.github/workflows/comment-release-fixes.yml +++ b/.github/workflows/comment-release-fixes.yml @@ -9,6 +9,7 @@ on: marker_prefix: { required: true, type: string } upgrade_text: { required: false, type: string, default: '' } dry_run: { required: false, type: boolean, default: true } + comment_author: { required: false, type: string, default: 'github-actions[bot]' } workflow_call: inputs: tag: { required: true, type: string } @@ -16,6 +17,7 @@ on: marker_prefix: { required: true, type: string } upgrade_text: { required: false, type: string, default: '' } dry_run: { required: false, type: boolean, default: false } + comment_author: { required: false, type: string, default: 'github-actions[bot]' } jobs: comment: runs-on: ubuntu-latest @@ -39,4 +41,5 @@ jobs: MARKER_PREFIX: ${{ inputs.marker_prefix }} UPGRADE_TEXT: ${{ inputs.upgrade_text }} DRY_RUN: ${{ inputs.dry_run && '1' || '0' }} + COMMENT_AUTHOR: ${{ inputs.comment_author }} run: python3 workflows/scripts/comment-release-fixes.py diff --git a/scripts/comment-release-fixes.py b/scripts/comment-release-fixes.py index 8fdec3a..cc5a4f3 100644 --- a/scripts/comment-release-fixes.py +++ b/scripts/comment-release-fixes.py @@ -4,14 +4,19 @@ KEYWORD = r"(?:close(?:s|d)?|fix(?:es|ed)?|resolve(?:s|d)?)" REF = re.compile(rf"\b{KEYWORD}\s*:?\s+(?:discussion\s+)?(?:(?Phttps://github\.com/(?P[\w.-]+)/(?P[\w.-]+)/(?:issues|discussions)/(?P[1-9]\d*))|#(?P[1-9]\d*))\b", re.I) -SUBJECT = re.compile(r"(?:\(#(?P[1-9]\d*)\)|Merge pull request #(?P[1-9]\d*))", re.I) +SQUASH_SUBJECT = re.compile(r"\s*\(#(?P[1-9]\d*)\)\s*$") +MERGE_SUBJECT = re.compile(r"^Merge pull request #(?P[1-9]\d*)\b", re.I) class Failure(Exception): pass def clean(body): body = re.sub(r"", "", body or "") - body = re.sub(r"(?ms)^\s*(```|~~~).*?^\s*\1\s*$", "", body) - return "\n".join(re.sub(r"`[^`]*`", "", l) for l in body.splitlines() + # Match the whole delimiter, so four-backtick fences and longer delimiters + # cannot be escaped by a three-backtick line inside an example. + body = re.sub(r"(?ms)^\s*(?P`{3,}|~{3,})[^\n]*\n.*?^\s*(?P=fence)\s*$", "", body) + # GitHub permits arbitrary-length inline delimiters and multiline spans. + body = re.sub(r"(?s)(`+).*?\1", "", body) + return "\n".join(l for l in body.splitlines() if not re.match(r"^(?:\s{4}|\t|\s*>).*", l)) def refs(body, owner, repo): @@ -69,11 +74,16 @@ def commits(gh, repo, base, tag): def shipped_prs(gh, repo, changes): shas={c["sha"] for c in changes}; nums=set() for c in changes: - m=SUBJECT.search(c.get("commit",{}).get("message","").split("\n",1)[0]) - if m: nums.add(int(m["squash"] or m["merge"])) + subject=c.get("commit",{}).get("message","").split("\n",1)[0] + m=SQUASH_SUBJECT.search(subject) or MERGE_SUBJECT.search(subject) + if m: nums.add(int(m["number"])) out=[] for n in sorted(nums): - pr=gh.api(f"repos/{repo}/pulls/{n}") + try: pr=gh.api(f"repos/{repo}/pulls/{n}") + except Failure: + # A malformed historical subject is not grounds to abandon other + # verifiably shipped PRs in this release; it never creates a write. + continue # Subject syntax is only a lead: require a merged PR and its merge commit # in the compared range, preventing unmerged/spoofed PR notifications. if pr.get("merged_at") and pr.get("merge_commit_sha") in shas: out.append(pr) @@ -97,16 +107,19 @@ def has_issue_comment(gh,repo,num,marker,actor): return any(c.get("user",{}).get("login")==actor and marker in (c.get("body") or "") for c in paged(gh,f"repos/{repo}/issues/{num}/comments")) def notify(gh,repo,tag,prefix,upgrade,dry,out=print): - owner,_=repo.split("/",1); pattern=re.compile(os.environ.get("TAG_PATTERN",r"^v[0-9][0-9A-Za-z._+-]*$")) + owner,name=repo.split("/",1); pattern=re.compile(os.environ.get("TAG_PATTERN",r"^v[0-9][0-9A-Za-z._+-]*$")) if not pattern.fullmatch(tag): raise Failure(f"{tag} does not match TAG_PATTERN") - actor=gh.api("user").get("login") + # GITHUB_TOKEN comments are authored by github-actions[bot]. A different + # credential must explicitly provide its known actor, rather than guessing + # from GET /user (which is not a supported installation-token identity API). + actor=os.environ.get("COMMENT_AUTHOR", "github-actions[bot]") if not actor: raise Failure("cannot determine token identity for safe deduplication") base=base_release(gh,repo,tag,pattern); changes=commits(gh,repo,base,tag); pulls=shipped_prs(gh,repo,changes) out(json.dumps({"tag":tag,"base":base,"commits":len(changes),"pulls":[p["number"] for p in pulls],"dry_run":dry})) failures=[]; seen=set() for pr in pulls: try: - for n in refs(pr.get("body") or "",owner,repo): + for n in refs(pr.get("body") or "",owner,name): if n in seen: continue body=text(repo,tag,pr["number"],prefix,upgrade); marker=mark(prefix,tag) try: @@ -117,7 +130,7 @@ def notify(gh,repo,tag,prefix,upgrade,dry,out=print): else: gh.api(f"repos/{repo}/issues/{n}/comments","POST",{"body":body}); out(f"commented on issue #{n} (from #{pr['number']})") except Failure as e: if "404" not in str(e): raise - d=get_discussion(gh,owner,repo,n) + d=get_discussion(gh,owner,name,n) if not d: raise Failure(f"target #{n} is neither an accessible issue nor discussion") if not d["closed"]: out(f"skip discussion #{n}: still open") elif any(c.get("author",{}).get("login")==actor and marker in (c.get("body") or "") for c in d["all_comments"]): out(f"skip discussion #{n}: already commented for {tag}") diff --git a/scripts/test_comment_release_fixes.py b/scripts/test_comment_release_fixes.py index c9aa1e5..9ec89de 100644 --- a/scripts/test_comment_release_fixes.py +++ b/scripts/test_comment_release_fixes.py @@ -1,30 +1,186 @@ -import importlib.util, pathlib, re, unittest -spec=importlib.util.spec_from_file_location("r",pathlib.Path(__file__).with_name("comment-release-fixes.py")); r=importlib.util.module_from_spec(spec); spec.loader.exec_module(r) +import importlib.util +import pathlib +import re +import unittest + +spec = importlib.util.spec_from_file_location("release", pathlib.Path(__file__).with_name("comment-release-fixes.py")) +release = importlib.util.module_from_spec(spec) +spec.loader.exec_module(release) + + class API: - def __init__(self,x):self.x=x;self.calls=[] - def api(self,p,method="GET",data=None): - self.calls.append((p,method,data)); v=self.x.get(p,self.x.get(p.split("?")[0],[])) - if isinstance(v,Exception):raise v - return v - def gql(self,*args,**kw):return {"repository":{"discussion":None}} -class T(unittest.TestCase): - def test_refs_exclude_examples_and_support_url(self): - self.assertEqual(r.refs("Fixes #1\nResolves https://github.com/a/b/discussions/2\n`Fixes #3`\n Fixes #4\n> Fixes #5\n~~~\nFixes #6\n~~~", "a","b"),[1,2]) - def test_tags_use_ancestry_and_exclude_identical(self): - a=API({"repos/a/b/releases?per_page=100&page=1":[{"tag_name":"v3","draft":False,"prerelease":False},{"tag_name":"v1","draft":False,"prerelease":False},{"tag_name":"v2","draft":False,"prerelease":False},{"tag_name":"v2x","draft":False,"prerelease":False}],"repos/a/b/compare/v1...v3?per_page=1":{"status":"ahead","ahead_by":9},"repos/a/b/compare/v2...v3?per_page=1":{"status":"ahead","ahead_by":2},"repos/a/b/compare/v2x...v3?per_page=1":{"status":"identical","ahead_by":0}}) - self.assertEqual(r.base_release(a,"a/b","v3",re.compile("^v.*")),"v2") - def test_unmerged_or_missing_merge_commit_not_shipped(self): - a=API({"repos/a/b/pulls/1":{"number":1,"merged_at":"now","merge_commit_sha":"m"},"repos/a/b/pulls/2":{"number":2,"merged_at":None,"merge_commit_sha":"x"}}) - changes=[{"sha":"m","commit":{"message":"x (#1)"}},{"sha":"x","commit":{"message":"x (#2)"}}] - self.assertEqual([p["number"] for p in r.shipped_prs(a,"a/b",changes)],[1]) - def test_missing_release_tag_fails_without_widening_range(self): - a=API({"repos/a/b/releases?per_page=100&page=1":[{"tag_name":"v1","draft":False,"prerelease":False}]}) - with self.assertRaises(r.Failure): r.base_release(a,"a/b","v2",re.compile("^v.*")) - def test_issue_dedup_paginates_and_uses_current_identity(self): - a=API({"repos/a/b/issues/1/comments?per_page=100&page=1":[{"user":{"login":"old"},"body":""}]*100,"repos/a/b/issues/1/comments?per_page=100&page=2":[{"user":{"login":"new"},"body":""}]}) - self.assertTrue(r.has_issue_comment(a,"a/b",1,"","new")) - def test_dry_run_reports_historical_metadata_without_writes(self): - a=API({"user":{"login":"robot"},"repos/a/b/releases?per_page=100&page=1":[{"tag_name":"v2","draft":False,"prerelease":False},{"tag_name":"v1","draft":False,"prerelease":False}],"repos/a/b/compare/v1...v2?per_page=1":{"status":"ahead","ahead_by":1},"repos/a/b/compare/v1...v2?per_page=100&page=1":{"commits":[{"sha":"m","commit":{"message":"x (#1)"}}]},"repos/a/b/pulls/1":{"number":1,"merged_at":"now","merge_commit_sha":"m","body":"Fixes #2"},"repos/a/b/issues/2":{"state":"closed"},"repos/a/b/issues/2/comments?per_page=100&page=1":[]}) - output=[]; r.notify(a,"a/b","v2","fixed","",True,output.append) - self.assertIn('"base": "v1"',output[0]); self.assertFalse(any(m=="POST" for _,m,_ in a.calls)) -if __name__=="__main__":unittest.main() + def __init__(self, responses=None, discussions=None): + self.responses = responses or {} + self.discussions = discussions or {} + self.calls = [] + + def api(self, path, method="GET", data=None): + self.calls.append((path, method, data)) + response = self.responses.get(path, self.responses.get(path.split("?")[0], [])) + if callable(response): + return response(path, method, data) + if isinstance(response, Exception): + raise response + return response + + def gql(self, query, **values): + self.calls.append(("graphql", "POST", values)) + if "addDiscussionComment" in query: + return {} + item = self.discussions.get(values["number"]) + return {"repository": {"discussion": item}} + + +def release_fixture(pulls, issues=None, discussions=None): + responses = { + "user": {"login": "automation-user"}, + "repos/acme/tool/releases?per_page=100&page=1": [ + {"tag_name": "v2", "draft": False, "prerelease": False}, + {"tag_name": "v1", "draft": False, "prerelease": False}, + ], + "repos/acme/tool/compare/v1...v2?per_page=1": {"status": "ahead", "ahead_by": len(pulls)}, + "repos/acme/tool/compare/v1...v2?per_page=100&page=1": { + "commits": [{"sha": p["merge_commit_sha"], "commit": {"message": f"x (#{p['number']})"}} for p in pulls] + }, + } + for pull in pulls: + responses[f"repos/acme/tool/pulls/{pull['number']}"] = pull + responses.update(issues or {}) + return API(responses, discussions) + + +class ReleaseTests(unittest.TestCase): + def test_template_and_full_url_refs_exclude_all_code_examples(self): + body = """Fixes #12 +Resolves https://github.com/acme/tool/discussions/13 +`Fixes #14` + Fixes #15 +> Fixes #16 +~~~ +Fixes #17 +~~~ +Fixes https://github.com/other/tool/issues/18""" + self.assertEqual(release.refs(body, "acme", "tool"), [12, 13]) + + def test_four_backtick_double_and_multiline_inline_examples_are_ignored(self): + body = """Fixes #1 +````markdown +Fixes #2 +``` +still an example +```` +``Fixes #3`` +`Fixes +#4`""" + self.assertEqual(release.refs(body, "acme", "tool"), [1]) + + def test_interleaved_tags_choose_closest_ancestor_not_list_order(self): + api = API({ + "repos/acme/tool/releases?per_page=100&page=1": [ + {"tag_name": "v3", "draft": False, "prerelease": False}, + {"tag_name": "v1", "draft": False, "prerelease": False}, + {"tag_name": "v2", "draft": False, "prerelease": False}, + {"tag_name": "v2-alias", "draft": False, "prerelease": False}, + ], + "repos/acme/tool/compare/v1...v3?per_page=1": {"status": "ahead", "ahead_by": 9}, + "repos/acme/tool/compare/v2...v3?per_page=1": {"status": "ahead", "ahead_by": 2}, + "repos/acme/tool/compare/v2-alias...v3?per_page=1": {"status": "identical", "ahead_by": 0}, + }) + self.assertEqual(release.base_release(api, "acme/tool", "v3", re.compile(r"^v.*")), "v2") + + def test_missing_and_uncomparable_tags_fail_closed(self): + missing = API({"repos/acme/tool/releases?per_page=100&page=1": [{"tag_name": "v1", "draft": False, "prerelease": False}]}) + with self.assertRaisesRegex(release.Failure, "not a stable"): + release.base_release(missing, "acme/tool", "v2", re.compile(r"^v.*")) + blocked = API({ + "repos/acme/tool/releases?per_page=100&page=1": [{"tag_name": "v2", "draft": False, "prerelease": False}, {"tag_name": "v1", "draft": False, "prerelease": False}], + "repos/acme/tool/compare/v1...v2?per_page=1": release.Failure("404 tag missing"), + }) + with self.assertRaisesRegex(release.Failure, "cannot safely"): + release.base_release(blocked, "acme/tool", "v2", re.compile(r"^v.*")) + + def test_paginated_comparison_and_only_verified_merged_prs_ship(self): + api = API({ + "repos/acme/tool/compare/v1...v2?per_page=100&page=1": {"commits": [{"sha": "m", "commit": {"message": "x (#1)"}}] * 100}, + "repos/acme/tool/compare/v1...v2?per_page=100&page=2": {"commits": [{"sha": "last", "commit": {"message": "x (#2)"}}]}, + "repos/acme/tool/pulls/1": {"number": 1, "merged_at": "yes", "merge_commit_sha": "m"}, + "repos/acme/tool/pulls/2": {"number": 2, "merged_at": None, "merge_commit_sha": "last"}, + }) + changes = release.commits(api, "acme/tool", "v1", "v2") + self.assertEqual(len(changes), 101) + self.assertEqual([p["number"] for p in release.shipped_prs(api, "acme/tool", changes)], [1]) + + def test_squash_subject_uses_only_terminal_pr_number_and_skips_bad_leads(self): + api = API({ + "repos/acme/tool/pulls/345": {"number": 345, "merged_at": "yes", "merge_commit_sha": "m"}, + }) + changes = [{"sha": "m", "commit": {"message": "Fix regression (#12) (#345)"}}] + self.assertEqual([p["number"] for p in release.shipped_prs(api, "acme/tool", changes)], [345]) + + def test_issue_dedup_paginates_and_requires_current_token_identity(self): + api = API({ + "repos/acme/tool/issues/4/comments?per_page=100&page=1": [{"user": {"login": "old-bot"}, "body": ""}] * 100, + "repos/acme/tool/issues/4/comments?per_page=100&page=2": [{"user": {"login": "token-user"}, "body": ""}], + }) + self.assertTrue(release.has_issue_comment(api, "acme/tool", 4, "", "token-user")) + + def test_reopened_discussion_is_skipped(self): + pull = {"number": 1, "merged_at": "yes", "merge_commit_sha": "m", "body": "Fixes discussion #9"} + api = release_fixture([pull], {"repos/acme/tool/issues/9": release.Failure("404 Not Found")}, { + 9: {"id": "D_9", "closed": False, "comments": {"nodes": [], "pageInfo": {"hasNextPage": False, "endCursor": None}}} + }) + output = [] + release.notify(api, "acme/tool", "v2", "fixed", "", False, output.append) + self.assertIn("skip discussion #9: still open", output) + self.assertFalse(any(path == "graphql" and values.get("id") == "D_9" for path, _, values in api.calls)) + + def test_actual_owner_name_is_used_for_discussion_full_url_contract(self): + pull = {"number": 1, "merged_at": "yes", "merge_commit_sha": "m", "body": "Fixes https://github.com/acme/tool/discussions/9"} + api = release_fixture([pull], {"repos/acme/tool/issues/9": release.Failure("404 Not Found")}, { + 9: {"id": "D_9", "closed": True, "comments": {"nodes": [], "pageInfo": {"hasNextPage": False, "endCursor": None}}} + }) + release.notify(api, "acme/tool", "v2", "fixed", "", False, lambda _: None) + lookups = [values for path, _, values in api.calls if path == "graphql" and "number" in values] + self.assertEqual(lookups[0]["owner"], "acme") + self.assertEqual(lookups[0]["repo"], "tool") + self.assertTrue(any(path == "graphql" and values.get("id") == "D_9" for path, _, values in api.calls)) + + def test_partial_failure_then_retry_deduplicates_first_target(self): + pulls = [ + {"number": 1, "merged_at": "yes", "merge_commit_sha": "m1", "body": "Fixes #2"}, + {"number": 2, "merged_at": "yes", "merge_commit_sha": "m2", "body": "Fixes #3"}, + ] + state = {"first": False, "second_fails": True, "comments": []} + def issues(path, method, data): + if method == "POST": state["comments"].append(data["body"]); return {"id": 1} + if path.endswith("/issues/2"): return {"state": "closed"} + if path.endswith("/issues/3"): + if state["second_fails"]: raise release.Failure("403 private-repo permission denied") + return {"state": "closed"} + if "/comments?" in path: return [{"user": {"login": "github-actions[bot]"}, "body": body} for body in state["comments"]] if path.endswith("2/comments?per_page=100&page=1") else [] + api = release_fixture(pulls) + api.responses.update({ + "repos/acme/tool/issues/2": issues, "repos/acme/tool/issues/3": issues, + "repos/acme/tool/issues/2/comments": issues, "repos/acme/tool/issues/3/comments": issues, + }) + with self.assertRaisesRegex(release.Failure, "PR #2"): + release.notify(api, "acme/tool", "v2", "fixed", "", False, lambda _: None) + state["second_fails"] = False + release.notify(api, "acme/tool", "v2", "fixed", "", False, lambda _: None) + self.assertEqual(len(state["comments"]), 2) + + def test_normal_and_manual_recovery_use_same_dry_run_and_write_no_comments(self): + pull = {"number": 1, "merged_at": "yes", "merge_commit_sha": "m", "body": "Fixes #2"} + api = release_fixture([pull], { + "repos/acme/tool/issues/2": {"state": "closed"}, + "repos/acme/tool/issues/2/comments?per_page=100&page=1": [], + }) + output = [] + release.notify(api, "acme/tool", "v2", "fixed", "", True, output.append) + self.assertIn('"dry_run": true', output[0]) + self.assertTrue(any("[dry run] would comment on issue #2" in line for line in output)) + self.assertFalse(any(method == "POST" for _, method, _ in api.calls)) + + +if __name__ == "__main__": + unittest.main() From 652dd92c36927734924fcfe25d7a6bae33c06efc Mon Sep 17 00:00:00 2001 From: Jeff Dickey <216188+jdx@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:41:40 -0500 Subject: [PATCH 3/9] fix: pin reusable workflow implementation --- .github/workflows/comment-release-fixes.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/comment-release-fixes.yml b/.github/workflows/comment-release-fixes.yml index 25807b0..1e81959 100644 --- a/.github/workflows/comment-release-fixes.yml +++ b/.github/workflows/comment-release-fixes.yml @@ -30,7 +30,8 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: repository: jdx/workflows - ref: ${{ github.workflow_sha }} + # Immutable shared-code pin: never execute a caller checkout. + ref: 3f6e2ca3eb927166a8912c1259af215175f6802a path: workflows persist-credentials: false - env: From 6c25d057248f43c2a2a0eaf71b8fabbd308bca1f Mon Sep 17 00:00:00 2001 From: Jeff Dickey <216188+jdx@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:50:58 -0500 Subject: [PATCH 4/9] fix: handle identical release tags safely --- scripts/comment-release-fixes.py | 18 ++++++++++++------ scripts/test_comment_release_fixes.py | 12 +++++++++++- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/scripts/comment-release-fixes.py b/scripts/comment-release-fixes.py index cc5a4f3..21b121c 100644 --- a/scripts/comment-release-fixes.py +++ b/scripts/comment-release-fixes.py @@ -53,7 +53,8 @@ def paged(gh, path): def base_release(gh, repo, tag, pattern): rs=[r for r in paged(gh,f"repos/{repo}/releases") if not r["draft"] and not r["prerelease"] and pattern.fullmatch(r["tag_name"])] if tag not in {r["tag_name"] for r in rs}: raise Failure(f"{tag} is not a stable published release in this tag family") - choices=[]; errors=[] + choices=[]; identical=[]; errors=[] + target_index=next(i for i,r in enumerate(rs) if r["tag_name"] == tag) for i,r in enumerate(rs): old=r["tag_name"] if old==tag: continue @@ -61,9 +62,13 @@ def base_release(gh, repo, tag, pattern): except Failure as e: errors.append(str(e)); continue # Identical tag aliases do not represent a fresh shipment. if cmp.get("status")=="ahead" and cmp.get("ahead_by",0)>0: choices.append((cmp["ahead_by"],i,old)) + elif cmp.get("status")=="identical" and i > target_index: identical.append((i,old)) if errors: raise Failure("cannot safely establish release ancestry:\n"+"\n".join(errors)) - if not choices: raise Failure(f"no earlier contained release found for {tag}; refusing to widen the range") - return min(choices)[2] + if choices: return min(choices)[2],False + # A later-published alias of the same commit shipped no new work. Treat it + # as an empty range, rather than looking past it and re-announcing fixes. + if identical: return min(identical)[1],True + raise Failure(f"no earlier contained release found for {tag}; refusing to widen the range") def commits(gh, repo, base, tag): out=[] @@ -80,10 +85,11 @@ def shipped_prs(gh, repo, changes): out=[] for n in sorted(nums): try: pr=gh.api(f"repos/{repo}/pulls/{n}") - except Failure: + except Failure as e: # A malformed historical subject is not grounds to abandon other # verifiably shipped PRs in this release; it never creates a write. - continue + if "404" in str(e): continue + raise # Subject syntax is only a lead: require a merged PR and its merge commit # in the compared range, preventing unmerged/spoofed PR notifications. if pr.get("merged_at") and pr.get("merge_commit_sha") in shas: out.append(pr) @@ -114,7 +120,7 @@ def notify(gh,repo,tag,prefix,upgrade,dry,out=print): # from GET /user (which is not a supported installation-token identity API). actor=os.environ.get("COMMENT_AUTHOR", "github-actions[bot]") if not actor: raise Failure("cannot determine token identity for safe deduplication") - base=base_release(gh,repo,tag,pattern); changes=commits(gh,repo,base,tag); pulls=shipped_prs(gh,repo,changes) + base,empty=base_release(gh,repo,tag,pattern); changes=[] if empty else commits(gh,repo,base,tag); pulls=shipped_prs(gh,repo,changes) out(json.dumps({"tag":tag,"base":base,"commits":len(changes),"pulls":[p["number"] for p in pulls],"dry_run":dry})) failures=[]; seen=set() for pr in pulls: diff --git a/scripts/test_comment_release_fixes.py b/scripts/test_comment_release_fixes.py index 9ec89de..461ae9b 100644 --- a/scripts/test_comment_release_fixes.py +++ b/scripts/test_comment_release_fixes.py @@ -86,7 +86,17 @@ def test_interleaved_tags_choose_closest_ancestor_not_list_order(self): "repos/acme/tool/compare/v2...v3?per_page=1": {"status": "ahead", "ahead_by": 2}, "repos/acme/tool/compare/v2-alias...v3?per_page=1": {"status": "identical", "ahead_by": 0}, }) - self.assertEqual(release.base_release(api, "acme/tool", "v3", re.compile(r"^v.*")), "v2") + self.assertEqual(release.base_release(api, "acme/tool", "v3", re.compile(r"^v.*")), ("v2", False)) + + def test_prior_same_commit_tag_is_an_empty_range_by_publication_order(self): + api = API({ + "repos/acme/tool/releases?per_page=100&page=1": [ + {"tag_name": "v3-alias", "draft": False, "prerelease": False}, + {"tag_name": "v3", "draft": False, "prerelease": False}, + ], + "repos/acme/tool/compare/v3...v3-alias?per_page=1": {"status": "identical", "ahead_by": 0}, + }) + self.assertEqual(release.base_release(api, "acme/tool", "v3-alias", re.compile(r"^v.*")), ("v3", True)) def test_missing_and_uncomparable_tags_fail_closed(self): missing = API({"repos/acme/tool/releases?per_page=100&page=1": [{"tag_name": "v1", "draft": False, "prerelease": False}]}) From 949fefe3f7d4b43b7957e0ffcdc958c663aad31f Mon Sep 17 00:00:00 2001 From: Jeff Dickey <216188+jdx@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:51:19 -0500 Subject: [PATCH 5/9] fix: pin alias-safe implementation --- .github/workflows/comment-release-fixes.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/comment-release-fixes.yml b/.github/workflows/comment-release-fixes.yml index 1e81959..42f2147 100644 --- a/.github/workflows/comment-release-fixes.yml +++ b/.github/workflows/comment-release-fixes.yml @@ -31,7 +31,7 @@ jobs: with: repository: jdx/workflows # Immutable shared-code pin: never execute a caller checkout. - ref: 3f6e2ca3eb927166a8912c1259af215175f6802a + ref: 6c25d057248f43c2a2a0eaf71b8fabbd308bca1f path: workflows persist-credentials: false - env: From bb86e8027aab65c4c559df37657bef150bb70f5d Mon Sep 17 00:00:00 2001 From: Jeff Dickey <216188+jdx@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:52:45 -0500 Subject: [PATCH 6/9] fix: deduplicate GitHub Actions comments safely --- scripts/comment-release-fixes.py | 8 +++++--- scripts/test_comment_release_fixes.py | 8 +++++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/scripts/comment-release-fixes.py b/scripts/comment-release-fixes.py index 21b121c..4452621 100644 --- a/scripts/comment-release-fixes.py +++ b/scripts/comment-release-fixes.py @@ -13,7 +13,8 @@ def clean(body): body = re.sub(r"", "", body or "") # Match the whole delimiter, so four-backtick fences and longer delimiters # cannot be escaped by a three-backtick line inside an example. - body = re.sub(r"(?ms)^\s*(?P`{3,}|~{3,})[^\n]*\n.*?^\s*(?P=fence)\s*$", "", body) + body = re.sub(r"(?ms)^\s*(?P`{3,})[^\n]*\n.*?^\s*(?P=backtick)`*\s*$", "", body) + body = re.sub(r"(?ms)^\s*(?P~{3,})[^\n]*\n.*?^\s*(?P=tilde)~*\s*$", "", body) # GitHub permits arbitrary-length inline delimiters and multiline spans. body = re.sub(r"(?s)(`+).*?\1", "", body) return "\n".join(l for l in body.splitlines() @@ -109,8 +110,9 @@ def get_discussion(gh, owner, repo, number): def mark(prefix,tag): return f"" def text(repo,tag,pr,prefix,upgrade): return f"{mark(prefix,tag)}\nFixed in [{tag}](https://github.com/{repo}/releases/tag/{tag}) by #{pr}."+(f" {upgrade}" if upgrade else "") +def owned(login,actor): return login==actor or (actor=="github-actions[bot]" and login in {"github-actions", "github-actions[bot]"}) def has_issue_comment(gh,repo,num,marker,actor): - return any(c.get("user",{}).get("login")==actor and marker in (c.get("body") or "") for c in paged(gh,f"repos/{repo}/issues/{num}/comments")) + return any(owned(c.get("user",{}).get("login"),actor) and marker in (c.get("body") or "") for c in paged(gh,f"repos/{repo}/issues/{num}/comments")) def notify(gh,repo,tag,prefix,upgrade,dry,out=print): owner,name=repo.split("/",1); pattern=re.compile(os.environ.get("TAG_PATTERN",r"^v[0-9][0-9A-Za-z._+-]*$")) @@ -139,7 +141,7 @@ def notify(gh,repo,tag,prefix,upgrade,dry,out=print): d=get_discussion(gh,owner,name,n) if not d: raise Failure(f"target #{n} is neither an accessible issue nor discussion") if not d["closed"]: out(f"skip discussion #{n}: still open") - elif any(c.get("author",{}).get("login")==actor and marker in (c.get("body") or "") for c in d["all_comments"]): out(f"skip discussion #{n}: already commented for {tag}") + elif any(owned(c.get("author",{}).get("login"),actor) and marker in (c.get("body") or "") for c in d["all_comments"]): out(f"skip discussion #{n}: already commented for {tag}") elif dry: out(f"[dry run] would comment on discussion #{n} (from #{pr['number']})") else: gh.gql(ADD,id=d["id"],body=body); out(f"commented on discussion #{n} (from #{pr['number']})") seen.add(n) diff --git a/scripts/test_comment_release_fixes.py b/scripts/test_comment_release_fixes.py index 461ae9b..f98fada 100644 --- a/scripts/test_comment_release_fixes.py +++ b/scripts/test_comment_release_fixes.py @@ -68,7 +68,7 @@ def test_four_backtick_double_and_multiline_inline_examples_are_ignored(self): Fixes #2 ``` still an example -```` +````` ``Fixes #3`` `Fixes #4`""" @@ -134,6 +134,12 @@ def test_issue_dedup_paginates_and_requires_current_token_identity(self): }) self.assertTrue(release.has_issue_comment(api, "acme/tool", 4, "", "token-user")) + def test_github_actions_graphql_and_rest_author_aliases_deduplicate_safely(self): + api = API({"repos/acme/tool/issues/4/comments?per_page=100&page=1": [{"user": {"login": "github-actions[bot]"}, "body": ""}]}) + self.assertTrue(release.has_issue_comment(api, "acme/tool", 4, "", "github-actions[bot]")) + self.assertTrue(release.owned("github-actions", "github-actions[bot]")) + self.assertFalse(release.owned(None, "github-actions[bot]")) + def test_reopened_discussion_is_skipped(self): pull = {"number": 1, "merged_at": "yes", "merge_commit_sha": "m", "body": "Fixes discussion #9"} api = release_fixture([pull], {"repos/acme/tool/issues/9": release.Failure("404 Not Found")}, { From 1409441597c7bc44a2f84e4801eac13a6611b85b Mon Sep 17 00:00:00 2001 From: Jeff Dickey <216188+jdx@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:53:02 -0500 Subject: [PATCH 7/9] fix: pin comment deduplication implementation --- .github/workflows/comment-release-fixes.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/comment-release-fixes.yml b/.github/workflows/comment-release-fixes.yml index 42f2147..2137935 100644 --- a/.github/workflows/comment-release-fixes.yml +++ b/.github/workflows/comment-release-fixes.yml @@ -31,7 +31,7 @@ jobs: with: repository: jdx/workflows # Immutable shared-code pin: never execute a caller checkout. - ref: 6c25d057248f43c2a2a0eaf71b8fabbd308bca1f + ref: bb86e8027aab65c4c559df37657bef150bb70f5d path: workflows persist-credentials: false - env: From d232756f3e5aecaa16851ec1b8c7910290a3ff2d Mon Sep 17 00:00:00 2001 From: Jeff Dickey <216188+jdx@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:58:31 -0500 Subject: [PATCH 8/9] fix: fail closed for release aliases --- scripts/comment-release-fixes.py | 15 +++++++----- scripts/test_comment_release_fixes.py | 33 ++++++++++++++++++++++++--- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/scripts/comment-release-fixes.py b/scripts/comment-release-fixes.py index 4452621..a5a772a 100644 --- a/scripts/comment-release-fixes.py +++ b/scripts/comment-release-fixes.py @@ -7,7 +7,8 @@ SQUASH_SUBJECT = re.compile(r"\s*\(#(?P[1-9]\d*)\)\s*$") MERGE_SUBJECT = re.compile(r"^Merge pull request #(?P[1-9]\d*)\b", re.I) -class Failure(Exception): pass +class Failure(Exception): + def __init__(self, message, status=None): super().__init__(message); self.status=status def clean(body): body = re.sub(r"", "", body or "") @@ -32,7 +33,9 @@ def api(self, path, method="GET", data=None): cmd=["gh","api",path,"-X",method] if data is not None: cmd += ["--input","-"] r=subprocess.run(cmd,input=json.dumps(data) if data is not None else None,text=True,capture_output=True) - if r.returncode: raise Failure(f"{path}: {r.stderr.strip() or 'GitHub API request failed'}") + if r.returncode: + match=re.search(r"\bHTTP\s+(\d{3})\b",r.stderr) + raise Failure(f"{path}: {r.stderr.strip() or 'GitHub API request failed'}",int(match.group(1)) if match else None) try: return json.loads(r.stdout) except json.JSONDecodeError as e: raise Failure(f"{path}: invalid JSON response") from e def gql(self, query, **values): @@ -65,10 +68,10 @@ def base_release(gh, repo, tag, pattern): if cmp.get("status")=="ahead" and cmp.get("ahead_by",0)>0: choices.append((cmp["ahead_by"],i,old)) elif cmp.get("status")=="identical" and i > target_index: identical.append((i,old)) if errors: raise Failure("cannot safely establish release ancestry:\n"+"\n".join(errors)) - if choices: return min(choices)[2],False # A later-published alias of the same commit shipped no new work. Treat it # as an empty range, rather than looking past it and re-announcing fixes. if identical: return min(identical)[1],True + if choices: return min(choices)[2],False raise Failure(f"no earlier contained release found for {tag}; refusing to widen the range") def commits(gh, repo, base, tag): @@ -89,7 +92,7 @@ def shipped_prs(gh, repo, changes): except Failure as e: # A malformed historical subject is not grounds to abandon other # verifiably shipped PRs in this release; it never creates a write. - if "404" in str(e): continue + if e.status==404: continue raise # Subject syntax is only a lead: require a merged PR and its merge commit # in the compared range, preventing unmerged/spoofed PR notifications. @@ -137,11 +140,11 @@ def notify(gh,repo,tag,prefix,upgrade,dry,out=print): elif dry: out(f"[dry run] would comment on issue #{n} (from #{pr['number']})") else: gh.api(f"repos/{repo}/issues/{n}/comments","POST",{"body":body}); out(f"commented on issue #{n} (from #{pr['number']})") except Failure as e: - if "404" not in str(e): raise + if e.status!=404: raise d=get_discussion(gh,owner,name,n) if not d: raise Failure(f"target #{n} is neither an accessible issue nor discussion") if not d["closed"]: out(f"skip discussion #{n}: still open") - elif any(owned(c.get("author",{}).get("login"),actor) and marker in (c.get("body") or "") for c in d["all_comments"]): out(f"skip discussion #{n}: already commented for {tag}") + elif any(owned((c.get("author") or {}).get("login"),actor) and marker in (c.get("body") or "") for c in d["all_comments"]): out(f"skip discussion #{n}: already commented for {tag}") elif dry: out(f"[dry run] would comment on discussion #{n} (from #{pr['number']})") else: gh.gql(ADD,id=d["id"],body=body); out(f"commented on discussion #{n} (from #{pr['number']})") seen.add(n) diff --git a/scripts/test_comment_release_fixes.py b/scripts/test_comment_release_fixes.py index f98fada..400c572 100644 --- a/scripts/test_comment_release_fixes.py +++ b/scripts/test_comment_release_fixes.py @@ -86,7 +86,7 @@ def test_interleaved_tags_choose_closest_ancestor_not_list_order(self): "repos/acme/tool/compare/v2...v3?per_page=1": {"status": "ahead", "ahead_by": 2}, "repos/acme/tool/compare/v2-alias...v3?per_page=1": {"status": "identical", "ahead_by": 0}, }) - self.assertEqual(release.base_release(api, "acme/tool", "v3", re.compile(r"^v.*")), ("v2", False)) + self.assertEqual(release.base_release(api, "acme/tool", "v3", re.compile(r"^v.*")), ("v2-alias", True)) def test_prior_same_commit_tag_is_an_empty_range_by_publication_order(self): api = API({ @@ -98,6 +98,18 @@ def test_prior_same_commit_tag_is_an_empty_range_by_publication_order(self): }) self.assertEqual(release.base_release(api, "acme/tool", "v3-alias", re.compile(r"^v.*")), ("v3", True)) + def test_alias_wins_over_an_older_ancestor(self): + api = API({ + "repos/acme/tool/releases?per_page=100&page=1": [ + {"tag_name": "v3", "draft": False, "prerelease": False}, + {"tag_name": "v2", "draft": False, "prerelease": False}, + {"tag_name": "v1", "draft": False, "prerelease": False}, + ], + "repos/acme/tool/compare/v2...v3?per_page=1": {"status": "identical", "ahead_by": 0}, + "repos/acme/tool/compare/v1...v3?per_page=1": {"status": "ahead", "ahead_by": 4}, + }) + self.assertEqual(release.base_release(api, "acme/tool", "v3", re.compile(r"^v.*")), ("v2", True)) + def test_missing_and_uncomparable_tags_fail_closed(self): missing = API({"repos/acme/tool/releases?per_page=100&page=1": [{"tag_name": "v1", "draft": False, "prerelease": False}]}) with self.assertRaisesRegex(release.Failure, "not a stable"): @@ -127,6 +139,14 @@ def test_squash_subject_uses_only_terminal_pr_number_and_skips_bad_leads(self): changes = [{"sha": "m", "commit": {"message": "Fix regression (#12) (#345)"}}] self.assertEqual([p["number"] for p in release.shipped_prs(api, "acme/tool", changes)], [345]) + def test_pr_404_is_ignored_but_a_500_for_pr_404_propagates(self): + changes = [{"sha": "m", "commit": {"message": "x (#404)"}}] + missing = API({"repos/acme/tool/pulls/404": release.Failure("not found", 404)}) + self.assertEqual(release.shipped_prs(missing, "acme/tool", changes), []) + broken = API({"repos/acme/tool/pulls/404": release.Failure("server error", 500)}) + with self.assertRaisesRegex(release.Failure, "server error"): + release.shipped_prs(broken, "acme/tool", changes) + def test_issue_dedup_paginates_and_requires_current_token_identity(self): api = API({ "repos/acme/tool/issues/4/comments?per_page=100&page=1": [{"user": {"login": "old-bot"}, "body": ""}] * 100, @@ -142,7 +162,7 @@ def test_github_actions_graphql_and_rest_author_aliases_deduplicate_safely(self) def test_reopened_discussion_is_skipped(self): pull = {"number": 1, "merged_at": "yes", "merge_commit_sha": "m", "body": "Fixes discussion #9"} - api = release_fixture([pull], {"repos/acme/tool/issues/9": release.Failure("404 Not Found")}, { + api = release_fixture([pull], {"repos/acme/tool/issues/9": release.Failure("404 Not Found", 404)}, { 9: {"id": "D_9", "closed": False, "comments": {"nodes": [], "pageInfo": {"hasNextPage": False, "endCursor": None}}} }) output = [] @@ -152,7 +172,7 @@ def test_reopened_discussion_is_skipped(self): def test_actual_owner_name_is_used_for_discussion_full_url_contract(self): pull = {"number": 1, "merged_at": "yes", "merge_commit_sha": "m", "body": "Fixes https://github.com/acme/tool/discussions/9"} - api = release_fixture([pull], {"repos/acme/tool/issues/9": release.Failure("404 Not Found")}, { + api = release_fixture([pull], {"repos/acme/tool/issues/9": release.Failure("404 Not Found", 404)}, { 9: {"id": "D_9", "closed": True, "comments": {"nodes": [], "pageInfo": {"hasNextPage": False, "endCursor": None}}} }) release.notify(api, "acme/tool", "v2", "fixed", "", False, lambda _: None) @@ -161,6 +181,13 @@ def test_actual_owner_name_is_used_for_discussion_full_url_contract(self): self.assertEqual(lookups[0]["repo"], "tool") self.assertTrue(any(path == "graphql" and values.get("id") == "D_9" for path, _, values in api.calls)) + def test_null_discussion_author_is_safe_in_notification_flow(self): + pull = {"number": 1, "merged_at": "yes", "merge_commit_sha": "m", "body": "Fixes discussion #9"} + api = release_fixture([pull], {"repos/acme/tool/issues/9": release.Failure("404 Not Found", 404)}, { + 9: {"id": "D_9", "closed": True, "comments": {"nodes": [{"author": None, "body": ""}], "pageInfo": {"hasNextPage": False, "endCursor": None}}} + }) + release.notify(api, "acme/tool", "v2", "fixed", "", True, lambda _: None) + def test_partial_failure_then_retry_deduplicates_first_target(self): pulls = [ {"number": 1, "merged_at": "yes", "merge_commit_sha": "m1", "body": "Fixes #2"}, From d2274b273271f69e2f34909339a67cf4a47eca91 Mon Sep 17 00:00:00 2001 From: Jeff Dickey <216188+jdx@users.noreply.github.com> Date: Thu, 8 Oct 2026 16:58:48 -0500 Subject: [PATCH 9/9] fix: pin fail-closed implementation --- .github/workflows/comment-release-fixes.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/comment-release-fixes.yml b/.github/workflows/comment-release-fixes.yml index 2137935..4aa1b99 100644 --- a/.github/workflows/comment-release-fixes.yml +++ b/.github/workflows/comment-release-fixes.yml @@ -31,7 +31,7 @@ jobs: with: repository: jdx/workflows # Immutable shared-code pin: never execute a caller checkout. - ref: bb86e8027aab65c4c559df37657bef150bb70f5d + ref: d232756f3e5aecaa16851ec1b8c7910290a3ff2d path: workflows persist-credentials: false - env: