Skip to content

Commit a5c0299

Browse files
merge-pr: refuse closing keywords in the PR description and commit messages
The script checked only the PR title. A contributor's commit message with a closing line closed its issue under the operator as soon as the merge reached master, before the fix shipped. The description and every merged commit message are now checked too, with one shared pattern that also covers 'Fixes: #N' and cross-repository references. Co-authored-by: Alfonso <alfonso-magic-context@users.noreply.github.com>
1 parent 6e1f32d commit a5c0299

3 files changed

Lines changed: 58 additions & 4 deletions

File tree

‎scripts/merge-pr-test-fixtures/command-shim.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,12 @@ def real_git(*command):
6161
if scenario.get("graphql_errors"):
6262
response["errors"] = [{"message": "fixture GraphQL failure"}]
6363
print(json.dumps(response))
64+
elif args == ["api", "repos/cortexkit/magic-context/pulls/123"]:
65+
prior = sum(item["name"] == "gh" and item["args"] == args for item in previous)
66+
body = scenario.get("body", "A plain description.")
67+
if prior and "body" in scenario.get("recheck", {}):
68+
body = scenario["recheck"]["body"]
69+
print(json.dumps({"number": 123, "body": body}))
6470
elif args[:1] == ["api"] and any("/check-runs?per_page=100" in arg for arg in args):
6571
if "--paginate" not in args or "--slurp" not in args or not any(scenario["head"] in arg for arg in args):
6672
sys.exit("Check-runs query must paginate the exact head")

‎scripts/merge-pr.sh‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,12 +127,38 @@ check_runs() {
127127
$pending"
128128
}
129129

130+
# GitHub closes an issue when a closing keyword ("Fixes #N") appears in a PR
131+
# description at merge or in a commit message pushed to master. This script also
132+
# puts the PR title in its merge commit message. Issues must stay open until the
133+
# fix is released, so a keyword in the title, the description or any merged
134+
# commit message is refused.
135+
CLOSING_KEYWORD='(^|[^[:alnum:]_])(fix(es|ed)?|close[sd]?|resolve[sd]?):?[[:space:]]+([[:alnum:]_.-]+/[[:alnum:]_.-]+)?#[0-9]+'
136+
130137
check_title() {
131-
local closing
132-
closing=$(run jq -nr --arg title "$title" '$title | test("(^|[^[:alnum:]_])(fix(es|ed)?|close[sd]?|resolve[sd]?)[[:space:]]+#[0-9]+"; "i")')
138+
local closing pull body
139+
closing=$(run jq -nr --arg title "$title" --arg re "$CLOSING_KEYWORD" '$title | test($re; "i")')
133140
if [ "$closing" = true ]; then
134141
die 'PR title contains a GitHub closing keyword with a number. Edit the title/message before merging; no automatic issue closing is allowed.'
135142
fi
143+
pull=$(run gh api "repos/$REPOSITORY/pulls/$number")
144+
body=$(run jq -r '.body // ""' <<< "$pull")
145+
closing=$(run jq -nr --arg body "$body" --arg re "$CLOSING_KEYWORD" '$body | test($re; "i")')
146+
if [ "$closing" = true ]; then
147+
die 'PR description contains a GitHub closing keyword with a number. Remove it from the description before merging; no automatic issue closing is allowed.'
148+
fi
149+
}
150+
151+
# Run after the PR head is fetched: the merge keeps the contributor's commits,
152+
# so their messages reach master unchanged.
153+
check_commit_messages() {
154+
local messages closing
155+
messages=$(run git log --format='%h %s%n%b' "master..$head")
156+
closing=$(printf '%s\n' "$messages" | grep -iE "$CLOSING_KEYWORD" || true)
157+
if [ -n "$closing" ]; then
158+
die "A commit message in PR #$number contains a GitHub closing keyword, which would close the issue as soon as master is pushed:
159+
$closing
160+
Ask the contributor to reword the commit, or squash the PR into one commit with a clean message, before merging."
161+
fi
136162
}
137163

138164
head=''
@@ -157,6 +183,7 @@ master_sha=$(run git rev-parse master)
157183
run git fetch origin "pull/$number/head"
158184
fetched_head=$(run git rev-parse FETCH_HEAD)
159185
[ "$fetched_head" = "$head" ] || die 'PR head changed between the GitHub query and fetch; retry with the new head.'
186+
check_commit_messages
160187

161188
worktree="${TMPDIR:-/tmp}/magic-context/merge-pr-$number"
162189
[ ! -e "$worktree" ] && [ ! -L "$worktree" ] || die "Temporary worktree already exists: $worktree (not removed)."

‎scripts/merge-pr.test.py‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,14 +82,14 @@ def git(self, *args):
8282
self.assertEqual(result.returncode, 0, result.stderr)
8383
return result.stdout
8484

85-
def prepare_pr(self, paths):
85+
def prepare_pr(self, paths, message="Fixture PR"):
8686
self.git("checkout", "-B", "fixture-pr", self.master)
8787
for name in paths:
8888
path = self.repo / name
8989
path.parent.mkdir(parents=True, exist_ok=True)
9090
path.write_text("export const changed = true;\n")
9191
self.git("add", ".")
92-
self.git("commit", "-m", "Fixture PR")
92+
self.git("commit", "-m", message)
9393
self.scenario["head"] = self.git("rev-parse", "HEAD").strip()
9494
self.git("checkout", "master")
9595

@@ -214,6 +214,27 @@ def test_closing_keyword_in_title_refuses(self):
214214
self.scenario["title"] = title
215215
self.assert_refused(self.run_script(), "closing keyword")
216216

217+
def test_closing_keyword_in_description_refuses(self):
218+
for body in ("Summary.\n\nFixes #777", "fixes: #7", "Closes cortexkit/magic-context#9"):
219+
with self.subTest(body=body):
220+
self.scenario["body"] = body
221+
self.assert_refused(self.run_script(), "PR description contains a GitHub closing keyword")
222+
223+
def test_closing_keyword_in_commit_message_refuses(self):
224+
self.prepare_pr(["packages/plugin/demo.ts"], message="fix(agents): keep named allows\n\nFixes #777")
225+
run = self.run_script()
226+
self.assert_refused(run, "commit message in PR #123 contains a GitHub closing keyword")
227+
self.assertIn("Fixes #777", run.stderr)
228+
229+
def test_issue_mentions_without_closing_keyword_proceed(self):
230+
self.scenario["body"] = "Implements the direction agreed in issue #777 (see #777)."
231+
self.prepare_pr(["packages/plugin/demo.ts"], message="fix(agents): keep named allows (#777)")
232+
self.assertEqual(self.run_script().returncode, 0)
233+
234+
def test_rechecks_description_after_gates(self):
235+
self.scenario["recheck"] = {"body": "Fixes #123"}
236+
self.assert_refused(self.run_script(), "PR description contains a GitHub closing keyword")
237+
217238
def test_head_changed_between_query_and_fetch_refuses(self):
218239
self.scenario["fetched_head"] = self.master
219240
self.assert_refused(self.run_script(), "head changed between")

0 commit comments

Comments
 (0)