Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
bump: patch
---

### Fixed

- `/source-control:pull-request ready` runs its verify gate against the pull request's worktree and records that worktree's HEAD, whichever checkout the shell sits in. A session in the main checkout running `ready <pr>` for a PR built in another worktree used to run the main checkout's `scripts/check-affected.sh` over the main checkout. The new `scripts/pr-worktree.sh --pr <n>` prints the worktree that has the PR's head branch checked out with the PR's head commit in its history, and exits 2 when none has (a fork's `main` beside this repository's main checkout included), so nothing is recorded and the flip refuses. The flip's `HEAD_SHA` comes from the same worktree.
Original file line number Diff line number Diff line change
Expand Up @@ -121,13 +121,16 @@ when the count is above zero and unchanged across two polls, which keeps a late
flip's snapshot. Skip the wait when the head is unchanged, but still run the `HEAD_SHA`
assignment, which the flip block reads.

`HEAD_SHA` is the local `git rev-parse HEAD`, the commit the verify gate recorded. GitHub's
`HEAD_SHA` is the PR worktree's HEAD, the commit the verify gate recorded: `pr-worktree.sh`
finds the worktree that has the PR's head branch checked out with the PR's head commit in its
history, whichever checkout the shell sits in, and exits non-zero when none has. GitHub's
`headRefOid` can lag a push by a read, so the block polls the PR's head until it equals `HEAD_SHA`, up to 60 seconds, instead of trusting the
first read. A head that never matches (a peer push, a push that failed, a session whose HEAD is not
the PR branch) leaves `HEAD_MATCH=0`, and the flip below refuses.
first read. A head that never matches (a peer push, a push that failed, local commits never
pushed) leaves `HEAD_MATCH=0`, and the flip below refuses.

```bash
HEAD_SHA=$(git rev-parse HEAD) || exit 1
PR_TREE=$(bash "<skill-dir>/scripts/pr-worktree.sh" --pr "$PR_NUMBER") || exit 1
HEAD_SHA=$(git -C "$PR_TREE" rev-parse HEAD) || exit 1
HEAD_MATCH=0
for _ in $(seq 12); do # 12 polls x 5 s = 60 s
[ "$(gh pr view "$PR_NUMBER" --json headRefOid -q '.headRefOid')" = "$HEAD_SHA" ] && { HEAD_MATCH=1; break; }
Expand Down
Comment thread
melodic-automation-lanes[bot] marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ mention anyone, hide text or read as a hold.

Step `base-merge`. Disabled by `pre_ready` or skipped by `--skip`: skip the merge, but still push any local commits
first, through `push-branch.sh` as below, so the reviews (which read the PR's diff on the remote)
and the verify gate (which runs on local HEAD) in 2.5.3 cover the same commit.
and the verify gate (which runs on the PR worktree's HEAD) in 2.5.3 cover the same commit.

Record the PR's head commit as `HEAD_BEFORE` first
([GitHub](../../../reference/providers/github/pull-request/ready-for-review.md#record-the-head-before-the-refresh)),
Expand Down Expand Up @@ -193,27 +193,44 @@ The bullets below are the steps.

**Commit, then record the verify result.** Both run in every repository and every run, whatever
`pre_ready` or `--skip` says; neither is a step either can name. First commit whatever the reviews
changed, so the gate and the record cover the head 2.5.4 flips. Then record one result for
`git rev-parse HEAD`. 2.5.4 reads it: prose cannot stop the flip, a script in the flip's own shell
changed, so the gate and the record cover the head 2.5.4 flips. Then record one result for the PR
worktree's HEAD. 2.5.4 reads it: prose cannot stop the flip, a script in the flip's own shell
command can.

**The PR worktree** is the checkout that has the PR's head branch checked out, which need not be
the one the shell sits in: a session in the main checkout can run `ready <pr>` for a PR built in
another worktree. `scripts/pr-worktree.sh --pr "$PR_NUMBER"` prints its path: the worktree
`git worktree list` shows on the PR's head branch, whose HEAD contains the PR's head commit. A
shell already in the PR's worktree gets its own top level back, so the gate runs as it would from
there. When no worktree has the head branch checked out, or the one that has it lacks the head
commit (another branch of that name, such as a fork's `main` beside this repository's), the script
exits 2 and each record command below records nothing, so 2.5.4 refuses the flip: check the PR out
with the forge's PR checkout and run the gate again.

- `verify` ran: after the rest of §1.5, run its deterministic last part and the record as one
shell command, so `pass` is never typed by hand:
shell command, so `pass` is never typed by hand. The gate runs the PR worktree's own
`scripts/check-affected.sh` with `CHECK_DATA_ROOT` naming that tree, so the tree checked never
depends on the shell's directory, and the record names that tree's HEAD:

```bash
if bash scripts/check-affected.sh --tests; then R=pass; else R=fail; fi; bash "<skill-dir>/scripts/verify-gate.sh" record --head "$(git rev-parse HEAD)" --result "$R"
PR_TREE=$(bash "<skill-dir>/scripts/pr-worktree.sh" --pr "$PR_NUMBER") || exit 1; if CHECK_DATA_ROOT="$PR_TREE" bash "$PR_TREE/scripts/check-affected.sh" --tests; then R=pass; else R=fail; fi; bash "<skill-dir>/scripts/verify-gate.sh" record --head "$(git -C "$PR_TREE" rev-parse HEAD)" --result "$R"
```

Without `scripts/check-affected.sh`, the `if` holds §1.5's ecosystem-native commands joined
with `&&`. When an earlier part of §1.5 already failed, record `--result fail` directly. A
recorded `fail` stops the flip.
- `verify` did not run: record `--result skipped` when the `parse-ready-skip.sh` output has a
`skip verify` line or the `read-pre-ready.sh` output has a `verify disabled` line. The report
still names the skip or the disabling (2.5.6).
Without `scripts/check-affected.sh` in the PR worktree, the `if` holds §1.5's ecosystem-native
commands joined with `&&`, run from `$PR_TREE` (`(cd "$PR_TREE" && <commands>)`). When an earlier
part of §1.5 already failed, record `fail` directly, with the next bullet's command and
`--result fail`. A recorded `fail` stops the flip.
- `verify` did not run: record `skipped` with the command below when the `parse-ready-skip.sh`
output has a `skip verify` line or the `read-pre-ready.sh` output has a `verify disabled` line.
The report still names the skip or the disabling (2.5.6).

```bash
PR_TREE=$(bash "<skill-dir>/scripts/pr-worktree.sh" --pr "$PR_NUMBER") || exit 1; bash "<skill-dir>/scripts/verify-gate.sh" record --head "$(git -C "$PR_TREE" rev-parse HEAD)" --result skipped # or --result fail
```

Completion criterion, for each step that ran: any review evidence the plan asks for is linked,
the findings of each review that ran are fixed or recorded, and the verify gate is clean on
`git rev-parse HEAD`. In every run, a result is recorded for `git rev-parse HEAD`: `pass` or
the findings of each review that ran are fixed or recorded, and the verify gate is clean on the
PR worktree's HEAD. In every run, a result is recorded for the PR worktree's HEAD: `pass` or
`fail` from the gate's exit status, or `skipped`.

## 2.5.4 Flip to ready
Expand All @@ -238,7 +255,7 @@ bash "<skill-dir>/scripts/push-branch.sh" --pr "$PR_NUMBER" || exit 1 # only w
```

**Wait for the push's CI run.** When the PR was already out of draft at 2.5.1, skip the wait. Take
the head commit from local `git rev-parse HEAD`, the commit the verify gate recorded, and poll the
the head commit from the PR worktree's HEAD (2.5.3), the commit the verify gate recorded, and poll the
forge until the PR's head equals it, up to 60 seconds. When it is not the `HEAD_BEFORE` recorded in 2.5.2 (the base merge, the forge's update or a
push of local commits moved it), wait up to 90 seconds, polling every
5, for CI runs to exist for the new head commit and their count to hold steady across two polls.
Expand Down
Comment thread
kyle-sexton marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#!/usr/bin/env bash
# pr-worktree.sh: print the worktree that has a pull request's head branch checked out.
#
# The `ready` action's verify gate checks the PR's tree and records that tree's HEAD, whatever
# checkout the session's shell sits in: a session in the main checkout running `ready <pr>` for a
# PR built in a worktree would otherwise check the main checkout. The head branch and commit come
# from `gh pr view`, and the worktree is the one `git worktree list` shows on that branch, matched
# by local branch name as push-branch.sh --pr matches it, whose HEAD contains the PR's head
# commit. A name alone can match the wrong tree, such as a fork PR opened from the contributor's
# main beside this repository's main checkout. A shell already in the PR's worktree gets its own
# top level back.
#
# Usage:
# pr-worktree.sh --pr <number>
#
# Exit codes:
# 0 the worktree's absolute path is on stdout
# 1 invalid argument
# 2 the head branch or commit could not be read, or no worktree has the branch checked out
# with the head commit in its history

# portability-scope: the head branch is read through gh; the worktree list is local git.
set -uo pipefail

usage() {
sed -n '2,/^$/p' "${BASH_SOURCE[0]}" | sed 's/^# \{0,1\}//'
exit 0
}

die() {
printf 'pr-worktree: %s\n' "$2" >&2
exit "$1"
}

PR=""
while (($# > 0)); do
case "$1" in
-h | --help) usage ;;
--pr)
(($# >= 2)) && [[ -z "$PR" ]] || die 1 "--pr needs one value and is given once"
PR="$2"
shift 2
;;
*) die 1 "unknown argument $(printf '%q' "$1") (use --help)" ;;
esac
done
[[ "$PR" =~ ^[0-9]+$ ]] || die 1 "--pr needs a pull request number"

HEAD_READ=$(gh pr view "$PR" --json headRefName,headRefOid -q '.headRefName + " " + .headRefOid' 2>/dev/null | tr -d '\r') ||
die 2 "the head branch of PR $PR could not be read"
read -r BRANCH HEAD_OID <<<"$HEAD_READ"
[[ -n "${BRANCH:-}" && "${HEAD_OID:-}" =~ ^[0-9a-f]{40}$ ]] || die 2 "the head branch and commit of PR $PR could not be read"

WT=""
while IFS= read -r LINE; do
case "$LINE" in
"worktree "*) WT="${LINE#worktree }" ;;
"branch refs/heads/$BRANCH")
if [[ -d "$WT" ]]; then
git -C "$WT" merge-base --is-ancestor "$HEAD_OID" HEAD 2>/dev/null ||
die 2 "the worktree $WT on '$BRANCH' does not contain PR $PR's head commit $HEAD_OID; it is another branch of that name, or behind the PR: check the PR out with the forge's PR checkout, or bring that worktree up to date, then run the gate again"
printf '%s\n' "$WT"
exit 0
fi
;;
*) ;;
esac
done < <(git worktree list --porcelain 2>/dev/null)

die 2 "PR $PR's head branch '$BRANCH' is checked out in no worktree of this repository; check it out with the forge's PR checkout, then run the gate again"
Comment thread
melodic-automation-lanes[bot] marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
#!/usr/bin/env bash
# Regression tests for pr-worktree.sh and the ready action's verify gate command that uses it: a
# session in the main checkout running `ready <pr>` for a PR built in a worktree checks the PR's
# tree and records that tree's HEAD. The gate command is extracted from the ready
# reference and run in a fixture repository against a stubbed `gh`.

set -uo pipefail

SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
SKILL_DIR="$(cd "$SCRIPT_DIR/.." && pwd)"
SCRIPT="$SCRIPT_DIR/pr-worktree.sh"
REF="$SKILL_DIR/reference/ready-for-review.md"
TEST_TMPDIR="$(mktemp -d)"
trap 'rm -rf "$TEST_TMPDIR"' EXIT

# shellcheck source=../../../scripts/test-helpers.sh
source "$SCRIPT_DIR/../../../scripts/test-helpers.sh"

# A gh stand-in: the head read answers STUB_BRANCH and STUB_OID, and fails when STUB_FAIL is set.
STUB_DIR="$TEST_TMPDIR/stubs"
mkdir -p "$STUB_DIR"
cat >"$STUB_DIR/gh" <<'STUB_EOF'
#!/usr/bin/env bash
[[ -z "${STUB_FAIL:-}" ]] || exit 1
[[ "$1 $2" == "pr view" && " $* " == *" headRefName,headRefOid "* ]] || exit 99
echo "$STUB_BRANCH $STUB_OID"
STUB_EOF
chmod +x "$STUB_DIR/gh"
export PATH="$STUB_DIR:$PATH"

# Fixture: the main checkout on main, and the PR's branch in a linked worktree with one more
# commit. Each tree's scripts/check-affected.sh prints the files the checked tree changed since
# main and which tree it checked, so the gate output shows whose tree was checked. The stub takes
# that tree from CHECK_DATA_ROOT, else from the working directory, never from its own location, so
# the gate assertions fail unless the gate itself names the PR's tree.
MAIN="$TEST_TMPDIR/main"
git init -q -b main "$MAIN"
mkdir -p "$MAIN/scripts"
cat >"$MAIN/scripts/check-affected.sh" <<'EOF'
#!/usr/bin/env bash
top=$(git -C "${CHECK_DATA_ROOT:-$PWD}" rev-parse --show-toplevel) || exit 2
top=$(cd "$top" && pwd -P)
echo "checked $top"
git -C "$top" diff --name-only main HEAD | sed 's/^/changed: /'
EOF
git -C "$MAIN" add scripts/check-affected.sh
git -C "$MAIN" -c user.name=t -c user.email=t@t commit -q -m base
MAIN_HEAD=$(git -C "$MAIN" rev-parse HEAD)
PR_WT="$TEST_TMPDIR/pr-tree"
git -C "$MAIN" worktree add -q -b feat/pr "$PR_WT"
echo change >"$PR_WT/feature.md"
git -C "$PR_WT" add feature.md
git -C "$PR_WT" -c user.name=t -c user.email=t@t commit -q -m feature
PR_HEAD=$(git -C "$PR_WT" rev-parse HEAD)
PR_WT_REAL=$(physical_dir "$PR_WT")
MAIN_REAL=$(physical_dir "$MAIN")
export STUB_BRANCH=feat/pr STUB_OID="$PR_HEAD"

out=$(cd "$MAIN" && bash "$SCRIPT" --pr 7)
assert_exit "from the main checkout: the PR's worktree is found" 0 "$?"
assert_eq "from the main checkout: the path is the PR's worktree" "$PR_WT_REAL" "$(physical_dir "$out")"

out=$(cd "$PR_WT" && bash "$SCRIPT" --pr 7)
assert_exit "from the PR's worktree: found" 0 "$?"
assert_eq "from the PR's worktree: its own top level comes back" "$PR_WT_REAL" "$(physical_dir "$out")"

out=$(cd "$MAIN" && STUB_BRANCH=main STUB_OID="$MAIN_HEAD" bash "$SCRIPT" --pr 7)
assert_eq "a branch the main checkout holds resolves to the main checkout" "$MAIN_REAL" "$(physical_dir "$out")"

out=$(cd "$PR_WT" && STUB_OID="$MAIN_HEAD" bash "$SCRIPT" --pr 7)
assert_eq "a worktree ahead of the PR's head commit (local commits not pushed yet) is still the PR's" "$PR_WT_REAL" "$(physical_dir "$out")"

out=$(cd "$MAIN" && STUB_BRANCH=main bash "$SCRIPT" --pr 7 2>&1)
assert_exit "a same-named branch without the PR's head commit (a fork's main) exits 2" 2 "$?"
assert_contains "the refusal names the missing head commit" "$out" "$PR_HEAD"

(cd "$MAIN" && STUB_OID=ffffffffffffffffffffffffffffffffffffffff bash "$SCRIPT" --pr 7 >/dev/null 2>&1)
assert_exit "a head commit this repository lacks exits 2" 2 "$?"
(cd "$MAIN" && STUB_OID=abc bash "$SCRIPT" --pr 7 >/dev/null 2>&1)
assert_exit "a malformed head commit exits 2" 2 "$?"

out=$(cd "$MAIN" && STUB_BRANCH=feat/elsewhere bash "$SCRIPT" --pr 7 2>&1)
assert_exit "a head branch checked out nowhere exits 2" 2 "$?"
assert_contains "the refusal names the branch" "$out" "'feat/elsewhere'"

out=$(cd "$MAIN" && STUB_BRANCH=feat bash "$SCRIPT" --pr 7 2>&1)
assert_exit "a branch name that is only a prefix of the checked-out one is not a match" 2 "$?"

(cd "$MAIN" && STUB_FAIL=1 bash "$SCRIPT" --pr 7 >/dev/null 2>&1)
assert_exit "a failed head-branch read exits 2" 2 "$?"
(cd "$MAIN" && STUB_BRANCH="" bash "$SCRIPT" --pr 7 >/dev/null 2>&1)
assert_exit "an empty head-branch read exits 2" 2 "$?"

bash "$SCRIPT" --pr abc >/dev/null 2>&1
assert_exit "a non-numeric --pr is a usage error" 1 "$?"
bash "$SCRIPT" >/dev/null 2>&1
assert_exit "a missing --pr is a usage error" 1 "$?"
bash "$SCRIPT" --pr 7 --pr 8 >/dev/null 2>&1
assert_exit "--pr given twice is a usage error" 1 "$?"
bash "$SCRIPT" --bogus >/dev/null 2>&1
assert_exit "an unknown argument is a usage error" 1 "$?"

# The verify gate command of the ready reference (2.5.3), run from the main checkout.
GATE=$(grep -F 'pr-worktree.sh" --pr' "$REF" | grep -F 'check-affected.sh" --tests' | sed -e 's/^ *//' -e "s#<skill-dir>#$SKILL_DIR#g")
if [[ -z "$GATE" ]]; then
echo "FAIL: the verify gate command was not found in $REF" >&2
exit 1
fi
export READY_VERIFY_DIR="$TEST_TMPDIR/verify"
out=$(cd "$MAIN" && PR_NUMBER=7 bash -c "$GATE" 2>&1)
assert_exit "the gate command from the main checkout succeeds" 0 "$?"
assert_contains "the gate checks the PR's worktree" "$out" "checked $PR_WT_REAL"
assert_contains "the gate output lists the PR's changed files" "$out" "changed: feature.md"
assert_eq "the gate records a pass for the PR worktree's HEAD" "pass" "$(cat "$READY_VERIFY_DIR/$PR_HEAD" 2>/dev/null)"
assert_file_absent "nothing is recorded for the main checkout's HEAD" "$READY_VERIFY_DIR/$MAIN_HEAD"

rm -rf "$READY_VERIFY_DIR"
out=$(cd "$MAIN" && STUB_BRANCH=feat/elsewhere PR_NUMBER=7 bash -c "$GATE" 2>&1)
assert_exit "the gate command for a head checked out nowhere fails" 1 "$?"
[[ -e "$READY_VERIFY_DIR" ]] && recorded=yes || recorded=no
assert_eq "the gate command for a head checked out nowhere records nothing" "no" "$recorded"

[[ $FAILED -eq 0 ]] || exit 1
exit 0
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,15 @@ STUB_DIR="$TEST_TMPDIR/stubs"
mkdir -p "$STUB_DIR"
printf '#!/usr/bin/env bash\nexit 0\n' >"$STUB_DIR/sleep"
# The forge's first head read returns STUB_STALE, later reads STUB_FORGE (default: the pushed
# commit). Runs exist only for the pushed commit, and a new one only once the PR is flipped. Each
# commit); pr-worktree.sh's head read returns STUB_BRANCH and STUB_OID. Runs exist only for the pushed commit, and a new one only once the PR is flipped. Each
# run list logs the commit it asked about; `pr ready` logs the flip.
cat >"$STUB_DIR/gh" <<'STUB_EOF'
#!/usr/bin/env bash
case "$1 $2" in
"pr view")
if [[ ! -e "$STUB_STATE/read" ]]; then
if [[ "$*" == *headRefName* ]]; then
echo "$STUB_BRANCH $STUB_OID"
elif [[ ! -e "$STUB_STATE/read" ]]; then
: >"$STUB_STATE/read"
echo "$STUB_STALE"
else
Expand Down Expand Up @@ -95,10 +97,13 @@ new_state() {
[[ -z "${1:-}" ]] || READY_VERIFY_DIR="$STATE/verify" bash "$SKILL_DIR/scripts/verify-gate.sh" record --head "$1" --result pass
}

# run_flow <stale-head> <head-before> [<later-forge-head>], in the state new_state made.
# run_flow <stale-head> <head-before> [<later-forge-head>], in the state new_state made, from
# $FLOW_CWD (default: the PR's checkout), with pr-worktree.sh reading STUB_OID (default: the
# pre-push head, as a lagging read returns it).
STUB_BRANCH=$(git -C "$REPO" branch --show-current)
run_flow() {
(cd "$REPO" && PATH="$STUB_DIR:$PATH" STUB_STATE="$STATE" STUB_STALE="$1" STUB_PUSHED="$PUSHED" \
STUB_FORGE="${3:-$PUSHED}" \
(cd "${FLOW_CWD:-$REPO}" && PATH="$STUB_DIR:$PATH" STUB_STATE="$STATE" STUB_STALE="$1" STUB_PUSHED="$PUSHED" \
STUB_FORGE="${3:-$PUSHED}" STUB_BRANCH="$STUB_BRANCH" STUB_OID="${STUB_OID:-$BEFORE}" \
READY_VERIFY_DIR="$STATE/verify" PR_NUMBER=5 HEAD_BEFORE="$2" timeout 30 bash "$FLOW" 2>&1)
}

Expand Down Expand Up @@ -132,5 +137,25 @@ assert_contains "the forge's head is a peer commit: the guard refuses" "$out" "F
[[ -e "$STATE/flipped" ]] && flipped=yes || flipped=no
assert_eq "the forge's head is a peer commit: the PR is not flipped" "no" "$flipped"

# The session's shell sits in another checkout of the repository, on another branch at the
# pre-push commit: HEAD_SHA is still the PR worktree's HEAD.
git -C "$REPO" worktree add -q -b other "$TEST_TMPDIR/other" "$BEFORE"
new_state "$PUSHED"
out=$(FLOW_CWD="$TEST_TMPDIR/other" run_flow "$BEFORE" "$BEFORE")
assert_contains "from another checkout: HEAD_SHA is the PR worktree's HEAD" "$out" "HEAD_SHA=$PUSHED"
assert_contains "from another checkout: the guarded flip passes on the PR's head" "$out" "FLIP_EXIT=0"

new_state "$PUSHED"
out=$(FLOW_CWD="$TEST_TMPDIR/other" STUB_BRANCH=gone run_flow "$BEFORE" "$BEFORE")
[[ -e "$STATE/flipped" ]] && flipped=yes || flipped=no
assert_eq "the PR's head branch is checked out nowhere: the PR is not flipped" "no" "$flipped"
assert_contains "the PR's head branch is checked out nowhere: the refusal names the branch" "$out" "'gone'"

new_state "$PUSHED"
out=$(FLOW_CWD="$TEST_TMPDIR/other" STUB_OID="$PEER" run_flow "$BEFORE" "$BEFORE")
[[ -e "$STATE/flipped" ]] && flipped=yes || flipped=no
assert_eq "the worktree on the head branch lacks the PR's head commit: the PR is not flipped" "no" "$flipped"
assert_contains "the worktree on the head branch lacks the PR's head commit: the refusal names it" "$out" "$PEER"

[[ $FAILED -eq 0 ]] || exit 1
exit 0
Loading