From 0af3ee94d039d3eeddf623697c11514af5557266 Mon Sep 17 00:00:00 2001 From: Emilio Jesus Gallego Arias Date: Sat, 15 Aug 2026 11:24:42 -0400 Subject: [PATCH] fix: prevent inherited PR titles --- CONTRIBUTING.md | 5 ++++ docs/TESTING.md | 3 +- scripts/pr-message.sh | 29 +++++++++++++++++-- tests/test-maintainer.sh | 2 ++ tests/test-pr-message.sh | 62 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 98 insertions(+), 3 deletions(-) create mode 100644 tests/test-pr-message.sh diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cfe60e1a..c0f6a50e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -102,6 +102,11 @@ Before opening or editing a PR, run: scripts/pr-message.sh ``` +Run it after committing the branch so its default title comes from the proposed change. The script +refuses to derive a title from a dirty checkout, a branch with no commits beyond the base, or a +commit subject that already ends in a pull-request number. When intentionally preparing metadata +before the commit exists, pass explicit `--title` and `--summary` values. + Use the emitted title/body scaffold as the public PR metadata. Do not hand-roll the PR body from local status notes or validation transcripts. diff --git a/docs/TESTING.md b/docs/TESTING.md index 7032b2a1..8a81d364 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -279,11 +279,12 @@ The maintainer surface covers local workflow helpers: - [tests/test-maintainer.sh](../tests/test-maintainer.sh) - [tests/test-codex-harness.sh](../tests/test-codex-harness.sh) +- [tests/test-pr-message.sh](../tests/test-pr-message.sh) - [tests/test-validate-defensive.sh](../tests/test-validate-defensive.sh) The aggregate maintainer runner skips [tests/test-codex-harness.sh](../tests/test-codex-harness.sh) when the current checkout has tracked edits, because that harness regression intentionally verifies that new task worktrees start from a clean primary checkout. -Run these when the change touches [scripts/codex-harness.sh](../scripts/codex-harness.sh), [scripts/codex-session-start.sh](../scripts/codex-session-start.sh), or [scripts/validate-defensive.sh](../scripts/validate-defensive.sh). +Run these when the change touches [scripts/codex-harness.sh](../scripts/codex-harness.sh), [scripts/codex-session-start.sh](../scripts/codex-session-start.sh), [scripts/pr-message.sh](../scripts/pr-message.sh), or [scripts/validate-defensive.sh](../scripts/validate-defensive.sh). ## CI Map diff --git a/scripts/pr-message.sh b/scripts/pr-message.sh index 5dbd3ebc..c47b7cdf 100755 --- a/scripts/pr-message.sh +++ b/scripts/pr-message.sh @@ -16,7 +16,9 @@ Usage: Print the public PR title/body scaffold for this branch. Options: - --title TITLE Override the PR title. Defaults to the current commit subject. + --title TITLE Override the PR title. Otherwise use the current commit subject only when the + checkout is clean, the branch is ahead of the base, and the subject has no PR + number suffix. --summary TEXT Override the opening body paragraph. It should start with "This PR". --change TEXT Add one optional behavior/review bullet. Repeat as needed. --base REF Override the base branch shown in the scaffold. Defaults to main. @@ -38,9 +40,15 @@ current_commit_subject() { git log -1 --pretty=%s } +die() { + echo "$*" >&2 + exit 2 +} + repo="ejgallego/lean-beam" base="main" -title="$(current_commit_subject)" +title="" +title_explicit=0 summary="This PR ." changes=() @@ -52,6 +60,7 @@ while [ "$#" -gt 0 ]; do exit 2 fi title="$2" + title_explicit=1 shift 2 ;; --summary) @@ -100,6 +109,22 @@ done branch="$(current_branch)" +if [ "$title_explicit" -eq 0 ]; then + if [ -n "$(git status --short --untracked-files=no)" ]; then + die "cannot derive a PR title while tracked changes are present; commit them or pass --title" + fi + if ! git rev-parse --verify --quiet "${base}^{commit}" >/dev/null; then + die "cannot derive a PR title because base ref $base is unavailable; fetch it or pass --title" + fi + if [ "$(git rev-list --count "${base}..HEAD")" -eq 0 ]; then + die "cannot derive a PR title because HEAD has no commits beyond $base; commit the change or pass --title" + fi + title="$(current_commit_subject)" + if printf '%s\n' "$title" | grep -Eq '\(#[0-9]+\)$'; then + die "cannot reuse commit subject ending in a PR number as a new PR title; pass --title" + fi +fi + printf 'repository=%s\n' "$repo" printf 'base=%s\n' "$base" printf 'head=%s\n' "$branch" diff --git a/tests/test-maintainer.sh b/tests/test-maintainer.sh index 6f6c969b..4790f7f5 100644 --- a/tests/test-maintainer.sh +++ b/tests/test-maintainer.sh @@ -8,6 +8,8 @@ set -euo pipefail cd "$(dirname "$0")/.." +bash tests/test-pr-message.sh + if [ -n "$(git status --short --untracked-files=no)" ]; then echo "[maintainer] skipping tests/test-codex-harness.sh because the current checkout has tracked edits" >&2 else diff --git a/tests/test-pr-message.sh b/tests/test-pr-message.sh new file mode 100644 index 00000000..573ceff5 --- /dev/null +++ b/tests/test-pr-message.sh @@ -0,0 +1,62 @@ +#!/usr/bin/env bash + +# Copyright (c) 2026 Lean FRO LLC. All rights reserved. +# Released under Apache 2.0 license as described in the file LICENSE. +# Author: Emilio J. Gallego Arias + +set -euo pipefail + +cd "$(dirname "$0")/.." + +# shellcheck source=tests/lib/tmp-guards.sh +. tests/lib/tmp-guards.sh + +tmp_root="$(mktemp -d /tmp/beam-pr-message-XXXXXX)" +beam_test_expect_owned_tmp_dir "$tmp_root" beam-pr-message +cleanup() { + beam_test_remove_owned_tmp_tree "$tmp_root" beam-pr-message +} +trap cleanup EXIT + +fixture_repo="$tmp_root/repo" +mkdir -p "$fixture_repo/scripts" +cp scripts/pr-message.sh "$fixture_repo/scripts/pr-message.sh" +git init -q -b main "$fixture_repo" +git -C "$fixture_repo" config user.name "Beam Test" +git -C "$fixture_repo" config user.email "beam-test@example.com" +printf 'base\n' >"$fixture_repo/README.md" +git -C "$fixture_repo" add README.md +git -C "$fixture_repo" commit -q -m "chore: establish fixture base" +git -C "$fixture_repo" switch -q -c topic + +if "$fixture_repo/scripts/pr-message.sh" >"$tmp_root/no-commit.out" 2>"$tmp_root/no-commit.err"; then + echo "expected default PR title to reject a branch without commits beyond main" >&2 + exit 1 +fi +grep -q 'HEAD has no commits beyond main' "$tmp_root/no-commit.err" + +printf 'dirty\n' >>"$fixture_repo/README.md" +if "$fixture_repo/scripts/pr-message.sh" >"$tmp_root/dirty.out" 2>"$tmp_root/dirty.err"; then + echo "expected default PR title to reject tracked changes" >&2 + exit 1 +fi +grep -q 'tracked changes are present' "$tmp_root/dirty.err" + +git -C "$fixture_repo" add README.md +git -C "$fixture_repo" commit -q -m "fix: stale inherited title (#237)" +if "$fixture_repo/scripts/pr-message.sh" >"$tmp_root/suffix.out" 2>"$tmp_root/suffix.err"; then + echo "expected default PR title to reject an existing PR-number suffix" >&2 + exit 1 +fi +grep -q 'commit subject ending in a PR number' "$tmp_root/suffix.err" + +git -C "$fixture_repo" commit -q --amend -m "fix: guard PR title defaults" +default_out="$("$fixture_repo/scripts/pr-message.sh")" +printf '%s\n' "$default_out" | grep -q '^pr_title=fix: guard PR title defaults$' + +printf 'more work\n' >>"$fixture_repo/README.md" +explicit_out="$("$fixture_repo/scripts/pr-message.sh" \ + --title "fix: describe staged work" \ + --summary "This PR describes the staged work explicitly.")" +printf '%s\n' "$explicit_out" | grep -q '^pr_title=fix: describe staged work$' +printf '%s\n' "$explicit_out" | grep -q '^This PR describes the staged work explicitly\.$'