From 4252e82193dbfc249b577c4aacd9c2115aaebd90 Mon Sep 17 00:00:00 2001 From: lusoris Date: Mon, 9 Nov 2026 09:39:47 +0100 Subject: [PATCH 1/3] fix(adr): scan open pull request heads when claiming an ADR number A pull request from a fork has no branch on origin; its head exists only as refs/pull//head. The claim scan of scripts/adr/next-free.sh read `git ls-remote --heads` alone, so it never saw ADR files in fork pull requests. Two open fork pull requests (#3329, #3341) both hold ADR-3402, and a claim on this repository handed out 3402 again while both were open. In claim mode the allocator now asks gh for the head commits of open pull requests and feeds them into the existing branch-tip scan (one fetch, one ls-tree per commit). It names the repository with `gh pr list -R `: without it gh prefers a remote named upstream, which on this fork is Netflix/vmaf. Closed and merged pull requests are not scanned. When gh is missing, there is no origin remote, or gh fails, the allocator prints a warning on stderr and scans branches only. Query mode is unchanged. Tests: a planted fork head reachable only as refs/pull/1/head, with a gh stub that lists it only when given -R and the origin URL (fails on the old script with 0602, passes with 0701; dropping -R fails it again), gh exiting 1, no gh on PATH, and an empty pull request list. The other allocator suites get a gh stub so no test reaches the network. Unit: vmafx-adr-allocator-open-pr-heads Signed-off-by: lusoris --- .../fixed/adr-allocator-open-pr-heads.md | 4 + docs/development/adr-workflow.md | 6 +- scripts/adr/next-free.sh | 35 ++++ scripts/adr/test-next-free.sh | 10 ++ .../adr/tests/test-next-free-concurrent.sh | 9 +- .../adr/tests/test-next-free-remote-aware.sh | 168 +++++++++++++++++- .../adr/tests/test-next-free-stale-lock.sh | 8 + 7 files changed, 237 insertions(+), 3 deletions(-) create mode 100644 changelog.d/fixed/adr-allocator-open-pr-heads.md diff --git a/changelog.d/fixed/adr-allocator-open-pr-heads.md b/changelog.d/fixed/adr-allocator-open-pr-heads.md new file mode 100644 index 0000000000..5774a0715a --- /dev/null +++ b/changelog.d/fixed/adr-allocator-open-pr-heads.md @@ -0,0 +1,4 @@ +- **ADR allocator scans open pull request heads to avoid fork PR collisions.** In claim mode, + `scripts/adr/next-free.sh` queries `gh` for open pull request head commit SHAs and scans their + `docs/adr` trees alongside origin branches. When `gh` is missing or fails, it warns on stderr + and scans origin branches only. diff --git a/docs/development/adr-workflow.md b/docs/development/adr-workflow.md index 2738f83774..ad5a31345c 100644 --- a/docs/development/adr-workflow.md +++ b/docs/development/adr-workflow.md @@ -144,7 +144,11 @@ reserving it. That caused about ten renumbers and rebases in a single session the file and prints a `WARNING: took over a stale ADR allocator lock` line. A lock whose holder still runs is never taken over. 2. It scans remote branches via `git ls-remote --heads` and `git ls-tree` to - also skip numbers already claimed by in-flight branches on origin. + also skip numbers already claimed by in-flight branches on origin. In claim + mode, the scan also covers the heads of open pull requests (fork PRs + included) via `gh`; without `gh` or when it fails, it warns on stderr and + scans branches only. The pull request list comes from the origin repository + (`gh pr list -R `), so an `upstream` remote does not change it. 3. It creates the claim file `.git/adr-claims/NNNN` exclusively (`O_EXCL`, the shell's `noclobber`). If another caller already created it, the number is taken and the next one is tried, up to 100 times. This step decides the diff --git a/scripts/adr/next-free.sh b/scripts/adr/next-free.sh index 5f562d2742..af6e56a5e2 100755 --- a/scripts/adr/next-free.sh +++ b/scripts/adr/next-free.sh @@ -243,6 +243,12 @@ _ls_remote_heads() { git ls-remote --heads origin 2>/dev/null } +# Query GitHub CLI for head commit SHAs of open pull requests in the given repository. +_collect_open_pr_heads() { + local repo="$1" + gh pr list -R "${repo}" --state open --limit 1000 --json headRefOid --jq '.[].headRefOid' +} + # _collect_remote_branch_numbers: internal worker; called from a process # substitution. Receives the raw ls-remote output on stdin (piped from the # main shell) so it never calls git ls-remote itself. @@ -503,6 +509,35 @@ if ! ls_remote_out="$(_ls_remote_heads 2>/dev/null)"; then REMOTE_OFFLINE=1 fi +if [ "${REMOTE_OFFLINE}" -eq 0 ]; then + if ! command -v gh >/dev/null 2>&1; then + printf 'WARNING: open pull request heads not scanned (gh not found on PATH); fork pull request ADR numbers may collide.\n' >&2 + elif ! _origin_url="$(git remote get-url origin 2>/dev/null)" || [ -z "${_origin_url}" ]; then + printf 'WARNING: open pull request heads not scanned (no origin remote); fork pull request ADR numbers may collide.\n' >&2 + else + _gh_err_file="$(mktemp)" + _gh_shas="" + if _gh_shas="$(_collect_open_pr_heads "${_origin_url}" 2>"${_gh_err_file}")"; then + while IFS= read -r _sha; do + if [[ "${_sha}" =~ ^[0-9a-fA-F]{40,64}$ ]]; then + if [ -n "${ls_remote_out}" ]; then + ls_remote_out+=$'\n'"${_sha}"$'\trefs/pull/open/head' + else + ls_remote_out="${_sha}"$'\trefs/pull/open/head' + fi + fi + done <<<"${_gh_shas}" + else + _gh_rc=$? + _gh_err="$(tr '\n' ' ' <"${_gh_err_file}" | sed 's/[[:space:]]*$//')" + _gh_reason="${_gh_err:-gh exited with status ${_gh_rc}}" + printf 'WARNING: open pull request heads not scanned (%s); fork pull request ADR numbers may collide.\n' \ + "${_gh_reason}" >&2 + fi + rm -f "${_gh_err_file}" + fi +fi + remote_numbers=() if [ "${REMOTE_OFFLINE}" -eq 0 ]; then while IFS= read -r n; do diff --git a/scripts/adr/test-next-free.sh b/scripts/adr/test-next-free.sh index 0ba8cfee36..9e8616aa51 100755 --- a/scripts/adr/test-next-free.sh +++ b/scripts/adr/test-next-free.sh @@ -41,7 +41,17 @@ _fail() { # Clean up any stubs we create, even on early exit. _STUBS_TO_CLEAN=() +TMP_STUB_DIR="$(mktemp -d)" +cat >"${TMP_STUB_DIR}/gh" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +chmod +x "${TMP_STUB_DIR}/gh" +export PATH="${TMP_STUB_DIR}:${PATH}" + +# Remove the stub directory and the stub files. _cleanup() { + rm -rf "${TMP_STUB_DIR}" for s in "${_STUBS_TO_CLEAN[@]+"${_STUBS_TO_CLEAN[@]}"}"; do rm -f "${s}" done diff --git a/scripts/adr/tests/test-next-free-concurrent.sh b/scripts/adr/tests/test-next-free-concurrent.sh index 7fa374f021..e1710736d9 100644 --- a/scripts/adr/tests/test-next-free-concurrent.sh +++ b/scripts/adr/tests/test-next-free-concurrent.sh @@ -27,7 +27,14 @@ ROUNDS="${2:-5}" TMPDIR_BASE="$(mktemp -d)" FAKE_REPO="${TMPDIR_BASE}/fakerepo" -mkdir -p "${FAKE_REPO}/docs/adr" +STUB_BIN="${TMPDIR_BASE}/stub-bin" +mkdir -p "${FAKE_REPO}/docs/adr" "${STUB_BIN}" +cat >"${STUB_BIN}/gh" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +chmod +x "${STUB_BIN}/gh" +export PATH="${STUB_BIN}:${PATH}" git -C "${FAKE_REPO}" init --quiet touch "${FAKE_REPO}/docs/adr/0600-alpha.md" git -C "${FAKE_REPO}" add . diff --git a/scripts/adr/tests/test-next-free-remote-aware.sh b/scripts/adr/tests/test-next-free-remote-aware.sh index a82fa8b5e2..83597dbe90 100644 --- a/scripts/adr/tests/test-next-free-remote-aware.sh +++ b/scripts/adr/tests/test-next-free-remote-aware.sh @@ -11,6 +11,10 @@ # 2. Offline mode (mocked ls-remote failure) → local fallback + WARNING on stderr. # 3. Sibling worktree claim via .git/adr-claims/ → second call returns higher number. # 4. Performance smoke (must complete within 5 s on a synthetic-branch set). +# 5. Fork pull request head with ADR file → allocator returns number above PR ADR. +# 6. Negative: gh failure → fallback to branches/master with WARNING. +# 7. Negative: no gh on PATH → fallback with WARNING with reason. +# 8. Boundary: gh returns empty list → no warning, number as before. # # All tests run in a throw-away temporary git repository so they cannot touch # production ADR state. The real next-free.sh is invoked via PATH override with @@ -30,7 +34,7 @@ set -euo pipefail source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/../../lib/clean-git-env.sh" REPO_ROOT="$(git rev-parse --show-toplevel)" -SCRIPT="${REPO_ROOT}/scripts/adr/next-free.sh" +SCRIPT="${SCRIPT:-${REPO_ROOT}/scripts/adr/next-free.sh}" # Resolve the real git binary path at test-script startup time so stubs can # use it without going through PATH (which would recurse back into the stub). @@ -61,6 +65,13 @@ FAKE_REPO="${TMPDIR_BASE}/fakerepo" STUB_BIN="${TMPDIR_BASE}/stub-bin" mkdir -p "${FAKE_REPO}/docs/adr" "${STUB_BIN}" +# Default gh stub: returns 0 with no PRs (tests override as needed). +cat >"${STUB_BIN}/gh" <<'GHEOF' +#!/usr/bin/env bash +exit 0 +GHEOF +chmod +x "${STUB_BIN}/gh" + # Initialise the fake repo and create a minimal origin/master state. git -C "${FAKE_REPO}" init --quiet # Create a couple of ADR files that represent origin/master state. @@ -300,6 +311,161 @@ else _fail "test 4b: performance smoke returned invalid output: '${result_t4}'" fi +# ── test 5: fork pull request head with ADR file ────────────────────────────── +# +# Scenario: an open pull request from a fork has a head commit that contains +# docs/adr/0700-fork.md. The commit is reachable only as refs/pull/1/head +# in the fake origin (no branch on origin). +# gh stub outputs the head commit SHA. +# Expected: allocator returns a number above 0700 (0701). + +git -C "${FAKE_REPO}" checkout --quiet -b temp-fork-pr master +touch "${FAKE_REPO}/docs/adr/0700-fork.md" +git -C "${FAKE_REPO}" add docs/adr/0700-fork.md +git -C "${FAKE_REPO}" commit --quiet -m "feat: fork pr adr 0700" +SHA_FORK="$(git -C "${FAKE_REPO}" rev-parse HEAD)" +git -C "${FAKE_REPO}" checkout --quiet master +git -C "${FAKE_REPO}" branch -D temp-fork-pr >/dev/null +git -C "${FAKE_REPO}" update-ref "refs/pull/1/head" "${SHA_FORK}" + +ORIGIN_URL="$(git -C "${FAKE_REPO}" remote get-url origin)" +cat >"${STUB_BIN}/gh" </dev/null) || echo "note: _run exited non-zero" >&2 +_STUBS_T5=("${FAKE_REPO}/docs/adr/${result_t5}-test-fork-pr.md.stub") +GIT_COMMON_T5="$(_git_common_dir)" + +if [ -n "${result_t5}" ] && [ "$((10#${result_t5}))" -gt 700 ]; then + _pass "test 5: fork PR head with ADR 0700 → allocator returns number above 0700: ${result_t5}" +else + _fail "test 5: expected number above 0700, got '${result_t5}'" +fi + +# Clean up stub, claim sidepointer, and refs/pull/1/head. +rm -f "${_STUBS_T5[@]+"${_STUBS_T5[@]}"}" +rm -f "${GIT_COMMON_T5}/adr-claims/${result_t5}" +git -C "${FAKE_REPO}" update-ref -d "refs/pull/1/head" + +# ── test 6: negative: gh exits 1 → fallback to branches/master + WARNING ────── +# +# Scenario: gh stub exits 1 (simulating failure or not logged in). +# Expected: claim succeeds with exit 0, returns next free number, +# and stderr contains WARNING. + +cat >"${STUB_BIN}/gh" <<'GHEOF' +#!/usr/bin/env bash +exit 1 +GHEOF +chmod +x "${STUB_BIN}/gh" + +stderr_t6="$(mktemp)" +result_t6=$(_run --claim test-gh-fail-t6 2>"${stderr_t6}") || echo "note: _run exited non-zero" >&2 +GIT_COMMON_T6="$(_git_common_dir)" + +if printf '%s' "${result_t6}" | grep -qE '^[0-9]{4}$'; then + _pass "test 6a: gh failure still returns 4-digit number: ${result_t6}" +else + _fail "test 6a: gh failure returned invalid output: '${result_t6}'" +fi + +if grep -q "WARNING: open pull request heads not scanned" "${stderr_t6}" && + grep -q "fork pull request ADR numbers may collide" "${stderr_t6}"; then + _pass "test 6b: gh failure prints expected WARNING on stderr" +else + _fail "test 6b: expected WARNING on stderr, got: $(cat "${stderr_t6}")" +fi + +rm -f "${FAKE_REPO}/docs/adr/${result_t6}-test-gh-fail-t6.md.stub" +rm -f "${GIT_COMMON_T6}/adr-claims/${result_t6}" +rm -f "${stderr_t6}" + +# ── test 7: negative: no gh on PATH → fallback + WARNING with reason ───────── +# +# Scenario: gh is completely missing from PATH. +# Expected: claim succeeds with exit 0, returns next free number, +# and stderr contains WARNING with reason. + +NO_GH_BIN="${TMPDIR_BASE}/no-gh-bin" +mkdir -p "${NO_GH_BIN}" +for cmd in bash git cut rm rmdir sort grep sleep find sed mkdir date tr mktemp; do + if [ -x "${NO_GH_BIN}/${cmd}" ]; then continue; fi + ln -s "$(command -v "${cmd}")" "${NO_GH_BIN}/${cmd}" +done + +stderr_t7="$(mktemp)" +result_t7=$(cd "${FAKE_REPO}" && PATH="${NO_GH_BIN}" bash "${SCRIPT}" --claim test-no-gh-t7 2>"${stderr_t7}") || + echo "note: _run exited non-zero" >&2 +GIT_COMMON_T7="$(_git_common_dir)" + +if printf '%s' "${result_t7}" | grep -qE '^[0-9]{4}$'; then + _pass "test 7a: missing gh still returns 4-digit number: ${result_t7}" +else + _fail "test 7a: missing gh returned invalid output: '${result_t7}'" +fi + +if grep -q "WARNING: open pull request heads not scanned" "${stderr_t7}" && + grep -q "fork pull request ADR numbers may collide" "${stderr_t7}"; then + _pass "test 7b: missing gh prints expected WARNING on stderr" +else + _fail "test 7b: expected WARNING on stderr, got: $(cat "${stderr_t7}")" +fi + +rm -f "${FAKE_REPO}/docs/adr/${result_t7}-test-no-gh-t7.md.stub" +rm -f "${GIT_COMMON_T7}/adr-claims/${result_t7}" +rm -f "${stderr_t7}" + +# ── test 8: boundary: gh returns empty list (no open PRs) ──────────────────── +# +# Scenario: gh succeeds and prints nothing (no open PRs). +# Expected: no WARNING on stderr, allocator returns next number from branches/master. + +cat >"${STUB_BIN}/gh" <<'GHEOF' +#!/usr/bin/env bash +exit 0 +GHEOF +chmod +x "${STUB_BIN}/gh" + +stderr_t8="$(mktemp)" +result_t8=$(_run --claim test-empty-pr-t8 2>"${stderr_t8}") || echo "note: _run exited non-zero" >&2 +GIT_COMMON_T8="$(_git_common_dir)" + +if printf '%s' "${result_t8}" | grep -qE '^[0-9]{4}$'; then + _pass "test 8a: empty PR list returns 4-digit number: ${result_t8}" +else + _fail "test 8a: empty PR list returned invalid output: '${result_t8}'" +fi + +if grep -q "WARNING: open pull request heads not scanned" "${stderr_t8}"; then + _fail "test 8b: unexpected WARNING on stderr: $(cat "${stderr_t8}")" +else + _pass "test 8b: empty PR list prints no open-PR WARNING" +fi + +rm -f "${FAKE_REPO}/docs/adr/${result_t8}-test-empty-pr-t8.md.stub" +rm -f "${GIT_COMMON_T8}/adr-claims/${result_t8}" +rm -f "${stderr_t8}" + # ── summary ─────────────────────────────────────────────────────────────────── printf '\n%d passed, %d failed\n' "${PASS}" "${FAIL}" diff --git a/scripts/adr/tests/test-next-free-stale-lock.sh b/scripts/adr/tests/test-next-free-stale-lock.sh index 8a6b226e8f..d900599514 100644 --- a/scripts/adr/tests/test-next-free-stale-lock.sh +++ b/scripts/adr/tests/test-next-free-stale-lock.sh @@ -26,6 +26,14 @@ source "$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)/../../lib/clean-git- REPO_ROOT="$(git rev-parse --show-toplevel)" SCRIPT="${REPO_ROOT}/scripts/adr/next-free.sh" TMPDIR_BASE="$(mktemp -d)" +STUB_BIN="${TMPDIR_BASE}/stub-bin" +mkdir -p "${STUB_BIN}" +cat >"${STUB_BIN}/gh" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF +chmod +x "${STUB_BIN}/gh" +export PATH="${STUB_BIN}:${PATH}" LIVE_PID="" FAIL=0 From 182d10113bed42eca7ace0c635e9bf384e2c26d5 Mon Sep 17 00:00:00 2001 From: lusoris Date: Mon, 9 Nov 2026 11:50:20 +0100 Subject: [PATCH 2/3] fix(adr): warn on a truncated pull request scan and drop the temp file Two review findings on the open pull request scan in scripts/adr/next-free.sh. The gh query is capped at 1000 heads, and a full page was treated as a complete scan, so a repository with more open pull requests could hand out a taken number without a word. The gh stderr also went to a mktemp file that an early exit left behind. The cap is now a named constant. When gh returns exactly that many valid head commits, the allocator prints an incomplete-scan warning on stderr. gh stderr is captured with the output (2>&1), so no temp file exists; only 40- to 64-digit hex lines count as heads, and the failure path still reports the captured text as the reason. Tests: 1000 heads warn (9), 999 do not (10), and one head plus a stray stderr line with TMPDIR pointing at a missing directory still claims a number and never creates it (11). The old script fails 9b and 11a. Test 6 now checks the reason text. docs/development/ adr-workflow.md names the bound. Unit: vmafx-3375-adr-allocator-threads-r2 Signed-off-by: lusoris --- docs/development/adr-workflow.md | 2 + scripts/adr/next-free.sh | 37 +++-- .../adr/tests/test-next-free-remote-aware.sh | 134 +++++++++++++++++- 3 files changed, 155 insertions(+), 18 deletions(-) diff --git a/docs/development/adr-workflow.md b/docs/development/adr-workflow.md index ad5a31345c..852564cd3f 100644 --- a/docs/development/adr-workflow.md +++ b/docs/development/adr-workflow.md @@ -149,6 +149,8 @@ reserving it. That caused about ten renumbers and rebases in a single session included) via `gh`; without `gh` or when it fails, it warns on stderr and scans branches only. The pull request list comes from the origin repository (`gh pr list -R `), so an `upstream` remote does not change it. + It reads at most 1000 open pull requests; when the list holds exactly that + many, it warns that the scan may be incomplete. 3. It creates the claim file `.git/adr-claims/NNNN` exclusively (`O_EXCL`, the shell's `noclobber`). If another caller already created it, the number is taken and the next one is tried, up to 100 times. This step decides the diff --git a/scripts/adr/next-free.sh b/scripts/adr/next-free.sh index af6e56a5e2..442216907b 100755 --- a/scripts/adr/next-free.sh +++ b/scripts/adr/next-free.sh @@ -243,10 +243,13 @@ _ls_remote_heads() { git ls-remote --heads origin 2>/dev/null } +# Upper bound on open pull request heads fetched from GitHub CLI. +readonly _OPEN_PR_HEAD_LIMIT=1000 + # Query GitHub CLI for head commit SHAs of open pull requests in the given repository. _collect_open_pr_heads() { local repo="$1" - gh pr list -R "${repo}" --state open --limit 1000 --json headRefOid --jq '.[].headRefOid' + gh pr list -R "${repo}" --state open --limit "${_OPEN_PR_HEAD_LIMIT}" --json headRefOid --jq '.[].headRefOid' } # _collect_remote_branch_numbers: internal worker; called from a process @@ -515,26 +518,32 @@ if [ "${REMOTE_OFFLINE}" -eq 0 ]; then elif ! _origin_url="$(git remote get-url origin 2>/dev/null)" || [ -z "${_origin_url}" ]; then printf 'WARNING: open pull request heads not scanned (no origin remote); fork pull request ADR numbers may collide.\n' >&2 else - _gh_err_file="$(mktemp)" - _gh_shas="" - if _gh_shas="$(_collect_open_pr_heads "${_origin_url}" 2>"${_gh_err_file}")"; then - while IFS= read -r _sha; do - if [[ "${_sha}" =~ ^[0-9a-fA-F]{40,64}$ ]]; then - if [ -n "${ls_remote_out}" ]; then - ls_remote_out+=$'\n'"${_sha}"$'\trefs/pull/open/head' - else - ls_remote_out="${_sha}"$'\trefs/pull/open/head' + _gh_out="" + if _gh_out="$(_collect_open_pr_heads "${_origin_url}" 2>&1)"; then + _gh_sha_count=0 + if [ -n "${_gh_out}" ]; then + while IFS= read -r _sha; do + if [[ "${_sha}" =~ ^[0-9a-fA-F]{40,64}$ ]]; then + _gh_sha_count=$((_gh_sha_count + 1)) + if [ -n "${ls_remote_out}" ]; then + ls_remote_out+=$'\n'"${_sha}"$'\trefs/pull/open/head' + else + ls_remote_out="${_sha}"$'\trefs/pull/open/head' + fi fi - fi - done <<<"${_gh_shas}" + done <<<"${_gh_out}" + fi + if [ "${_gh_sha_count}" -eq "${_OPEN_PR_HEAD_LIMIT}" ]; then + printf 'WARNING: open pull request heads scan incomplete (%s-head limit reached); fork pull request ADR numbers may collide.\n' \ + "${_OPEN_PR_HEAD_LIMIT}" >&2 + fi else _gh_rc=$? - _gh_err="$(tr '\n' ' ' <"${_gh_err_file}" | sed 's/[[:space:]]*$//')" + _gh_err="$(printf '%s' "${_gh_out}" | tr '\n' ' ' | sed 's/[[:space:]]*$//')" _gh_reason="${_gh_err:-gh exited with status ${_gh_rc}}" printf 'WARNING: open pull request heads not scanned (%s); fork pull request ADR numbers may collide.\n' \ "${_gh_reason}" >&2 fi - rm -f "${_gh_err_file}" fi fi diff --git a/scripts/adr/tests/test-next-free-remote-aware.sh b/scripts/adr/tests/test-next-free-remote-aware.sh index 83597dbe90..82f113a166 100644 --- a/scripts/adr/tests/test-next-free-remote-aware.sh +++ b/scripts/adr/tests/test-next-free-remote-aware.sh @@ -15,6 +15,9 @@ # 6. Negative: gh failure → fallback to branches/master with WARNING. # 7. Negative: no gh on PATH → fallback with WARNING with reason. # 8. Boundary: gh returns empty list → no warning, number as before. +# 9. Positive: gh returns 1000 SHAs (limit reached) → incomplete-scan WARNING. +# 10. Negative: gh returns 999 SHAs (< limit) → no incomplete-scan WARNING. +# 11. Boundary: gh prints 1 SHA + non-SHA stderr line, exit 0, TMPDIR missing. # # All tests run in a throw-away temporary git repository so they cannot touch # production ADR state. The real next-free.sh is invoked via PATH override with @@ -371,10 +374,11 @@ git -C "${FAKE_REPO}" update-ref -d "refs/pull/1/head" # # Scenario: gh stub exits 1 (simulating failure or not logged in). # Expected: claim succeeds with exit 0, returns next free number, -# and stderr contains WARNING. +# and stderr contains WARNING with reason text. cat >"${STUB_BIN}/gh" <<'GHEOF' #!/usr/bin/env bash +printf 'GraphQL: could not resolve to a Repository\n' >&2 exit 1 GHEOF chmod +x "${STUB_BIN}/gh" @@ -389,9 +393,8 @@ else _fail "test 6a: gh failure returned invalid output: '${result_t6}'" fi -if grep -q "WARNING: open pull request heads not scanned" "${stderr_t6}" && - grep -q "fork pull request ADR numbers may collide" "${stderr_t6}"; then - _pass "test 6b: gh failure prints expected WARNING on stderr" +if grep -q "WARNING: open pull request heads not scanned (GraphQL: could not resolve to a Repository); fork pull request ADR numbers may collide" "${stderr_t6}"; then + _pass "test 6b: gh failure prints expected WARNING with reason text on stderr" else _fail "test 6b: expected WARNING on stderr, got: $(cat "${stderr_t6}")" fi @@ -466,6 +469,129 @@ rm -f "${FAKE_REPO}/docs/adr/${result_t8}-test-empty-pr-t8.md.stub" rm -f "${GIT_COMMON_T8}/adr-claims/${result_t8}" rm -f "${stderr_t8}" +# ── test 9: positive: gh returns 1000 SHAs (limit reached) → WARNING on stderr ─ +# +# Scenario: gh stub returns exactly 1000 distinct 40-hex SHAs (limit reached). +# Expected: claim succeeds with exit 0, returns next free number, +# and stderr contains the incomplete-scan WARNING. + +cat >"${STUB_BIN}/gh" <<'GHEOF' +#!/usr/bin/env bash +for ((i = 1; i <= 1000; i++)); do + printf '%040d\n' "${i}" +done +exit 0 +GHEOF +chmod +x "${STUB_BIN}/gh" + +stderr_t9="$(mktemp)" +result_t9=$(_run --claim test-limit-1000-t9 2>"${stderr_t9}") || echo "note: _run exited non-zero" >&2 +GIT_COMMON_T9="$(_git_common_dir)" + +if printf '%s' "${result_t9}" | grep -qE '^[0-9]{4}$'; then + _pass "test 9a: 1000-head limit still returns 4-digit number: ${result_t9}" +else + _fail "test 9a: 1000-head limit returned invalid output: '${result_t9}'" +fi + +if grep -q "WARNING: open pull request heads scan incomplete (1000-head limit reached); fork pull request ADR numbers may collide." "${stderr_t9}"; then + _pass "test 9b: 1000-head limit prints incomplete-scan WARNING on stderr" +else + _fail "test 9b: expected incomplete-scan WARNING on stderr, got: $(cat "${stderr_t9}")" +fi + +rm -f "${FAKE_REPO}/docs/adr/${result_t9}-test-limit-1000-t9.md.stub" +rm -f "${GIT_COMMON_T9}/adr-claims/${result_t9}" +rm -f "${stderr_t9}" + +# ── test 10: negative: gh returns 999 SHAs (< limit) → no WARNING ───────────── +# +# Scenario: gh stub returns 999 distinct 40-hex SHAs (below limit). +# Expected: claim succeeds with exit 0, returns next free number, +# and stderr does NOT contain the incomplete-scan WARNING. + +cat >"${STUB_BIN}/gh" <<'GHEOF' +#!/usr/bin/env bash +for ((i = 1; i <= 999; i++)); do + printf '%040d\n' "${i}" +done +exit 0 +GHEOF +chmod +x "${STUB_BIN}/gh" + +stderr_t10="$(mktemp)" +result_t10=$(_run --claim test-limit-999-t10 2>"${stderr_t10}") || echo "note: _run exited non-zero" >&2 +GIT_COMMON_T10="$(_git_common_dir)" + +if printf '%s' "${result_t10}" | grep -qE '^[0-9]{4}$'; then + _pass "test 10a: 999-head scan returns 4-digit number: ${result_t10}" +else + _fail "test 10a: 999-head scan returned invalid output: '${result_t10}'" +fi + +if grep -q "WARNING: open pull request heads scan incomplete" "${stderr_t10}"; then + _fail "test 10b: unexpected incomplete-scan WARNING on stderr: $(cat "${stderr_t10}")" +else + _pass "test 10b: 999-head scan prints no incomplete-scan WARNING" +fi + +rm -f "${FAKE_REPO}/docs/adr/${result_t10}-test-limit-999-t10.md.stub" +rm -f "${GIT_COMMON_T10}/adr-claims/${result_t10}" +rm -f "${stderr_t10}" + +# ── test 11: boundary: gh prints 1 SHA + non-SHA stderr line, exit 0, TMPDIR missing ── +# +# Scenario: gh stub outputs one valid SHA on stdout and a non-SHA warning line +# on stderr, exiting 0. TMPDIR names a directory that does not exist, so under +# set -e any mktemp on the scan path fails the claim. +# Expected: the SHA is used, no WARNING is printed, and TMPDIR is never created. + +git -C "${FAKE_REPO}" checkout --quiet -b temp-boundary-pr master +touch "${FAKE_REPO}/docs/adr/0800-boundary.md" +git -C "${FAKE_REPO}" add docs/adr/0800-boundary.md +git -C "${FAKE_REPO}" commit --quiet -m "feat: boundary pr adr 0800" +SHA_BOUNDARY="$(git -C "${FAKE_REPO}" rev-parse HEAD)" +git -C "${FAKE_REPO}" checkout --quiet master +git -C "${FAKE_REPO}" branch -D temp-boundary-pr >/dev/null +git -C "${FAKE_REPO}" update-ref "refs/pull/2/head" "${SHA_BOUNDARY}" + +cat >"${STUB_BIN}/gh" <&2 +exit 0 +GHEOF +chmod +x "${STUB_BIN}/gh" + +PRIVATE_TMPDIR="${TMPDIR_BASE}/missing-tmp-t11" + +stderr_t11="$(mktemp)" +result_t11=$(TMPDIR="${PRIVATE_TMPDIR}" _run --claim test-boundary-t11 2>"${stderr_t11}") || echo "note: _run exited non-zero" >&2 +GIT_COMMON_T11="$(_git_common_dir)" + +if [ -n "${result_t11}" ] && [ "$((10#${result_t11}))" -gt 800 ]; then + _pass "test 11a: boundary PR head with ADR 0800 → SHA is used, returns number above 0800: ${result_t11}" +else + _fail "test 11a: expected number above 0800, got '${result_t11}'" +fi + +if grep -q "WARNING:" "${stderr_t11}"; then + _fail "test 11b: unexpected WARNING on stderr: $(cat "${stderr_t11}")" +else + _pass "test 11b: non-SHA warning on stderr produces no allocator WARNING" +fi + +if [ ! -e "${PRIVATE_TMPDIR}" ]; then + _pass "test 11c: missing TMPDIR was never created" +else + _fail "test 11c: ${PRIVATE_TMPDIR} was created: $(ls -A "${PRIVATE_TMPDIR}")" +fi + +rm -f "${FAKE_REPO}/docs/adr/${result_t11}-test-boundary-t11.md.stub" +rm -f "${GIT_COMMON_T11}/adr-claims/${result_t11}" +git -C "${FAKE_REPO}" update-ref -d "refs/pull/2/head" +rm -f "${stderr_t11}" + # ── summary ─────────────────────────────────────────────────────────────────── printf '\n%d passed, %d failed\n' "${PASS}" "${FAIL}" From 0e40fcc1064f94393d6b419254c7c1d49bf4c135 Mon Sep 17 00:00:00 2001 From: lusoris Date: Mon, 9 Nov 2026 13:04:39 +0100 Subject: [PATCH 3/3] fix(adr): fetch open pull request heads apart from branch tips One unavailable pull request head made the single combined fetch fail, so the branch tips were not cached either and their ADR numbers were dropped. Branch tips and pull request heads now fetch separately; a failed pull request batch retries each head alone and warns per head. Contributors whose origin is a personal fork can set VMAF_ADR_PR_REPO so the pull request list comes from the target repository. The offline warning now says pull request heads were not scanned, and the script header and workflow page state that a pull request opened after the scan is not seen. The remote-aware tests now assert exact numbers (0701, 0602), use a gh stub that refuses calls without --state open and --json headRefOid, check the full gh-missing reason, and cover the split fetch and the override with positive, negative and boundary cases. Signed-off-by: lusoris --- .../fixed/adr-allocator-open-pr-heads.md | 10 +- docs/development/adr-workflow.md | 3 + scripts/adr/next-free.sh | 172 ++++++++---- .../adr/tests/test-next-free-remote-aware.sh | 256 ++++++++++++++++-- 4 files changed, 359 insertions(+), 82 deletions(-) diff --git a/changelog.d/fixed/adr-allocator-open-pr-heads.md b/changelog.d/fixed/adr-allocator-open-pr-heads.md index 5774a0715a..2e68222ccd 100644 --- a/changelog.d/fixed/adr-allocator-open-pr-heads.md +++ b/changelog.d/fixed/adr-allocator-open-pr-heads.md @@ -1,4 +1,6 @@ -- **ADR allocator scans open pull request heads to avoid fork PR collisions.** In claim mode, - `scripts/adr/next-free.sh` queries `gh` for open pull request head commit SHAs and scans their - `docs/adr` trees alongside origin branches. When `gh` is missing or fails, it warns on stderr - and scans origin branches only. +- **ADR allocator scans open pull request heads to avoid fork PR collisions.** + In claim mode, `scripts/adr/next-free.sh` queries `gh` for open pull request + head commit SHAs and scans their `docs/adr` trees alongside origin branches. + Contributors with a fork as origin can set `VMAF_ADR_PR_REPO=VMAFx/vmafx`. + When `gh` is missing or fails, it warns on stderr and scans origin branches + only. diff --git a/docs/development/adr-workflow.md b/docs/development/adr-workflow.md index 852564cd3f..469c5101f2 100644 --- a/docs/development/adr-workflow.md +++ b/docs/development/adr-workflow.md @@ -149,6 +149,9 @@ reserving it. That caused about ten renumbers and rebases in a single session included) via `gh`; without `gh` or when it fails, it warns on stderr and scans branches only. The pull request list comes from the origin repository (`gh pr list -R `), so an `upstream` remote does not change it. + A contributor with a fork as origin can set `VMAF_ADR_PR_REPO=VMAFx/vmafx` + to scan the target repository instead of the fork. A pull request opened + after the scan is not seen; the claim file and stub are the coordination. It reads at most 1000 open pull requests; when the list holds exactly that many, it warns that the scan may be incomplete. 3. It creates the claim file `.git/adr-claims/NNNN` exclusively (`O_EXCL`, the diff --git a/scripts/adr/next-free.sh b/scripts/adr/next-free.sh index 442216907b..9b5a6054ba 100755 --- a/scripts/adr/next-free.sh +++ b/scripts/adr/next-free.sh @@ -45,12 +45,19 @@ # per-SHA via a single git cat-file --batch run). Numbers claimed by # in-flight branches are skipped. This covers the parallel-agent scenario # where each agent pushes to its own branch before merging. +# In claim mode, open pull request heads on the target repository are also +# scanned via gh pr list. By default the target repository is determined from +# the origin remote URL; contributors with a personal fork as origin can set +# VMAF_ADR_PR_REPO (e.g. VMAF_ADR_PR_REPO=VMAFx/vmafx) to query the upstream +# project instead of their personal fork. # # Offline / network-fail mode: # If git ls-remote or gh pr list fail (offline, no network, timeout), the # script logs a WARNING to stderr and falls back to local-only behavior. # The number allocated is still collision-free among local + master state, # but may collide with another in-flight agent on a different machine. +# A pull request opened after the scan is not seen; the claim file and stub +# are the coordination. # # Performance: # Target: < 5 s on a repo with 600+ ADRs and 30+ open branches. @@ -252,6 +259,53 @@ _collect_open_pr_heads() { gh pr list -R "${repo}" --state open --limit "${_OPEN_PR_HEAD_LIMIT}" --json headRefOid --jq '.[].headRefOid' } +# Fetch branch tip commit SHAs; warn if origin cannot be reached. +_fetch_branch_tips() { + local -a branch_shas=("$@") + [ "${#branch_shas[@]}" -gt 0 ] || return 0 + + local -a depth_args=() + mapfile -t depth_args < <(adr_depth_args --depth=1) + if ! git fetch --no-tags "${depth_args[@]}" --quiet origin \ + "${branch_shas[@]}" 2>/dev/null; then + printf 'WARNING: could not fetch remote branch tips; their ADR numbers are skipped.\n' >&2 + fi +} + +# Fetch open pull request head SHAs; on batch failure retry each individually +# and warn on any SHA that cannot be fetched. +_fetch_open_pr_heads() { + local -a pr_shas=("$@") + [ "${#pr_shas[@]}" -gt 0 ] || return 0 + + local -a depth_args=() + mapfile -t depth_args < <(adr_depth_args --depth=1) + + if ! git fetch --no-tags "${depth_args[@]}" --quiet origin \ + "${pr_shas[@]}" 2>/dev/null; then + local sha + for sha in "${pr_shas[@]}"; do + if ! git fetch --no-tags "${depth_args[@]}" --quiet origin \ + "${sha}" 2>/dev/null; then + printf 'WARNING: could not fetch open pull request head %s; its ADR numbers are skipped.\n' \ + "${sha}" >&2 + fi + done + fi +} + +# Scan docs/adr/ tree entries across given commit SHAs and print taken numbers. +_scan_sha_adrs() { + local sha listing + for sha in "$@"; do + if listing="$(git ls-tree -r --name-only "${sha}" -- docs/adr/ 2>/dev/null)"; then + printf '%s\n' "${listing}" | + grep -oE 'docs/adr/[0-9]{4}-' | + grep -oE '[0-9]{4}' || _no_match_ok $? + fi + done +} + # _collect_remote_branch_numbers: internal worker; called from a process # substitution. Receives the raw ls-remote output on stdin (piped from the # main shell) so it never calls git ls-remote itself. @@ -264,37 +318,51 @@ _collect_remote_branch_numbers() { # Step 2: extract unique SHAs (multiple refs can share the same commit). # Skip master — it's already covered by _collect_local_taken via origin/master. - local -a shas=() + # Separate origin branch tips from open PR heads so PR fetch failures cannot + # drop branch ADR numbers. + local -a branch_shas=() pr_shas=() + local -A seen_branch=() seen_pr=() + local sha ref + while IFS=' ' read -r sha ref; do [ -n "${sha}" ] || continue [[ "${ref}" == "refs/heads/master" ]] && continue - shas+=("${sha}") - done < <(printf '%s\n' "${ls_remote_out}" | LC_ALL=C sort -u -k1,1) + if [[ "${ref}" != "refs/pull/open/head" ]]; then + if [ -z "${seen_branch[${sha}]:-}" ]; then + seen_branch["${sha}"]=1 + branch_shas+=("${sha}") + fi + fi + done <<<"${ls_remote_out}" - if [ "${#shas[@]}" -eq 0 ]; then + while IFS=' ' read -r sha ref; do + [ -n "${sha}" ] || continue + if [[ "${ref}" == "refs/pull/open/head" ]]; then + if [ -z "${seen_branch[${sha}]:-}" ] && [ -z "${seen_pr[${sha}]:-}" ]; then + seen_pr["${sha}"]=1 + pr_shas+=("${sha}") + fi + fi + done <<<"${ls_remote_out}" + + if [ "${#branch_shas[@]}" -eq 0 ] && [ "${#pr_shas[@]}" -eq 0 ]; then return 0 fi - # Step 3: fetch all branch tips so git ls-tree can resolve the SHAs. - # --no-tags avoids tag-name expansion; --depth=1 is the minimal fetch. - # Soft failure: if the network drops between the ls-remote above and here, - # ls-tree will simply find nothing. - mapfile -t _adr_depth < <(adr_depth_args --depth=1) - if ! git fetch --no-tags "${_adr_depth[@]}" --quiet origin \ - "${shas[@]}" 2>/dev/null; then - printf 'WARNING: could not fetch remote branch tips; their ADR numbers are skipped.\n' >&2 - fi + # Step 3: fetch branch tips and open PR heads independently. + _fetch_branch_tips "${branch_shas[@]+"${branch_shas[@]}"}" + _fetch_open_pr_heads "${pr_shas[@]+"${pr_shas[@]}"}" # Step 4: for each SHA, run git ls-tree to enumerate docs/adr/ entries. - # git ls-tree is fast (tree object lookup, no working-tree I/O). - for sha in "${shas[@]}"; do - local listing - if listing="$(git ls-tree -r --name-only "${sha}" -- docs/adr/ 2>/dev/null)"; then - printf '%s\n' "${listing}" | - grep -oE 'docs/adr/[0-9]{4}-' | - grep -oE '[0-9]{4}' || _no_match_ok $? - fi - done + local -a all_shas=() + if [ "${#branch_shas[@]}" -gt 0 ]; then + all_shas+=("${branch_shas[@]}") + fi + if [ "${#pr_shas[@]}" -gt 0 ]; then + all_shas+=("${pr_shas[@]}") + fi + + _scan_sha_adrs "${all_shas[@]}" } # Collect all taken 4-digit ADR numbers from: @@ -515,34 +583,40 @@ fi if [ "${REMOTE_OFFLINE}" -eq 0 ]; then if ! command -v gh >/dev/null 2>&1; then printf 'WARNING: open pull request heads not scanned (gh not found on PATH); fork pull request ADR numbers may collide.\n' >&2 - elif ! _origin_url="$(git remote get-url origin 2>/dev/null)" || [ -z "${_origin_url}" ]; then - printf 'WARNING: open pull request heads not scanned (no origin remote); fork pull request ADR numbers may collide.\n' >&2 else - _gh_out="" - if _gh_out="$(_collect_open_pr_heads "${_origin_url}" 2>&1)"; then - _gh_sha_count=0 - if [ -n "${_gh_out}" ]; then - while IFS= read -r _sha; do - if [[ "${_sha}" =~ ^[0-9a-fA-F]{40,64}$ ]]; then - _gh_sha_count=$((_gh_sha_count + 1)) - if [ -n "${ls_remote_out}" ]; then - ls_remote_out+=$'\n'"${_sha}"$'\trefs/pull/open/head' - else - ls_remote_out="${_sha}"$'\trefs/pull/open/head' + _pr_repo="${VMAF_ADR_PR_REPO:-}" + if [ -z "${_pr_repo}" ]; then + _pr_repo="$(git remote get-url origin 2>/dev/null)" || _pr_repo="" + fi + if [ -z "${_pr_repo}" ]; then + printf 'WARNING: open pull request heads not scanned (no origin remote); fork pull request ADR numbers may collide.\n' >&2 + else + _gh_out="" + if _gh_out="$(_collect_open_pr_heads "${_pr_repo}" 2>&1)"; then + _gh_sha_count=0 + if [ -n "${_gh_out}" ]; then + while IFS= read -r _sha; do + if [[ "${_sha}" =~ ^[0-9a-fA-F]{40,64}$ ]]; then + _gh_sha_count=$((_gh_sha_count + 1)) + if [ -n "${ls_remote_out}" ]; then + ls_remote_out+=$'\n'"${_sha}"$'\trefs/pull/open/head' + else + ls_remote_out="${_sha}"$'\trefs/pull/open/head' + fi fi - fi - done <<<"${_gh_out}" + done <<<"${_gh_out}" + fi + if [ "${_gh_sha_count}" -eq "${_OPEN_PR_HEAD_LIMIT}" ]; then + printf 'WARNING: open pull request heads scan incomplete (%s-head limit reached); fork pull request ADR numbers may collide.\n' \ + "${_OPEN_PR_HEAD_LIMIT}" >&2 + fi + else + _gh_rc=$? + _gh_err="$(printf '%s' "${_gh_out}" | tr '\n' ' ' | sed 's/[[:space:]]*$//')" + _gh_reason="${_gh_err:-gh exited with status ${_gh_rc}}" + printf 'WARNING: open pull request heads not scanned (%s); fork pull request ADR numbers may collide.\n' \ + "${_gh_reason}" >&2 fi - if [ "${_gh_sha_count}" -eq "${_OPEN_PR_HEAD_LIMIT}" ]; then - printf 'WARNING: open pull request heads scan incomplete (%s-head limit reached); fork pull request ADR numbers may collide.\n' \ - "${_OPEN_PR_HEAD_LIMIT}" >&2 - fi - else - _gh_rc=$? - _gh_err="$(printf '%s' "${_gh_out}" | tr '\n' ' ' | sed 's/[[:space:]]*$//')" - _gh_reason="${_gh_err:-gh exited with status ${_gh_rc}}" - printf 'WARNING: open pull request heads not scanned (%s); fork pull request ADR numbers may collide.\n' \ - "${_gh_reason}" >&2 fi fi fi @@ -551,12 +625,12 @@ remote_numbers=() if [ "${REMOTE_OFFLINE}" -eq 0 ]; then while IFS= read -r n; do [ -n "${n}" ] && remote_numbers+=("${n}") - done < <(_collect_remote_branch_numbers "${ls_remote_out}" 2>/dev/null || + done < <(_collect_remote_branch_numbers "${ls_remote_out}" || printf 'WARNING: remote branch scan failed.\n' >&2) fi if [ "${REMOTE_OFFLINE}" -eq 1 ]; then - printf 'WARNING: could not reach origin — remote branch scan skipped.\n' >&2 + printf 'WARNING: could not reach origin — remote branch scan skipped; open pull request heads not scanned.\n' >&2 printf ' Collision risk if other agents are working on separate branches.\n' >&2 printf ' Proceeding with local + master + .git/adr-claims/ only.\n' >&2 fi diff --git a/scripts/adr/tests/test-next-free-remote-aware.sh b/scripts/adr/tests/test-next-free-remote-aware.sh index 82f113a166..d44b1ad682 100644 --- a/scripts/adr/tests/test-next-free-remote-aware.sh +++ b/scripts/adr/tests/test-next-free-remote-aware.sh @@ -18,6 +18,8 @@ # 9. Positive: gh returns 1000 SHAs (limit reached) → incomplete-scan WARNING. # 10. Negative: gh returns 999 SHAs (< limit) → no incomplete-scan WARNING. # 11. Boundary: gh prints 1 SHA + non-SHA stderr line, exit 0, TMPDIR missing. +# 12. Split fetch: branch ADRs survive absent PR head commit SHA. +# 13. VMAF_ADR_PR_REPO override: positive (custom repo), negative (unset), boundary (empty). # # All tests run in a throw-away temporary git repository so they cannot touch # production ADR state. The real next-free.sh is invoked via PATH override with @@ -320,7 +322,7 @@ fi # docs/adr/0700-fork.md. The commit is reachable only as refs/pull/1/head # in the fake origin (no branch on origin). # gh stub outputs the head commit SHA. -# Expected: allocator returns a number above 0700 (0701). +# Expected: allocator returns exactly 0701 (0700 + 1). git -C "${FAKE_REPO}" checkout --quiet -b temp-fork-pr master touch "${FAKE_REPO}/docs/adr/0700-fork.md" @@ -335,23 +337,39 @@ ORIGIN_URL="$(git -C "${FAKE_REPO}" remote get-url origin)" cat >"${STUB_BIN}/gh" <&2 + exit 1 fi -exit 1 +if [ "\${has_repo}" -ne 1 ]; then + printf 'gh stub: missing or invalid -R argument (expected %s)\n' "${ORIGIN_URL}" >&2 + exit 1 +fi +if [ "\${has_state_open}" -ne 1 ]; then + printf 'gh stub: missing --state open\n' >&2 + exit 1 +fi +if [ "\${has_json_head}" -ne 1 ]; then + printf 'gh stub: missing --json headRefOid\n' >&2 + exit 1 +fi +printf '%s\n' "${SHA_FORK}" +exit 0 GHEOF chmod +x "${STUB_BIN}/gh" @@ -359,10 +377,17 @@ result_t5=$(_run --claim test-fork-pr 2>/dev/null) || echo "note: _run exited no _STUBS_T5=("${FAKE_REPO}/docs/adr/${result_t5}-test-fork-pr.md.stub") GIT_COMMON_T5="$(_git_common_dir)" -if [ -n "${result_t5}" ] && [ "$((10#${result_t5}))" -gt 700 ]; then - _pass "test 5: fork PR head with ADR 0700 → allocator returns number above 0700: ${result_t5}" +if [ "${result_t5}" = "0701" ]; then + _pass "test 5a: fork PR head with ADR 0700 → allocator returns 0701" +else + _fail "test 5a: expected 0701, got '${result_t5}'" +fi + +# Negative: prove the strict gh stub rejects calls without --state open +if "${STUB_BIN}/gh" pr list -R "${ORIGIN_URL}" --state closed --json headRefOid 2>/dev/null; then + _fail "test 5b: strict gh stub accepted --state closed" else - _fail "test 5: expected number above 0700, got '${result_t5}'" + _pass "test 5b: strict gh stub rejects call without --state open" fi # Clean up stub, claim sidepointer, and refs/pull/1/head. @@ -373,7 +398,7 @@ git -C "${FAKE_REPO}" update-ref -d "refs/pull/1/head" # ── test 6: negative: gh exits 1 → fallback to branches/master + WARNING ────── # # Scenario: gh stub exits 1 (simulating failure or not logged in). -# Expected: claim succeeds with exit 0, returns next free number, +# Expected: claim succeeds with exit 0, returns next free number (0602), # and stderr contains WARNING with reason text. cat >"${STUB_BIN}/gh" <<'GHEOF' @@ -387,10 +412,10 @@ stderr_t6="$(mktemp)" result_t6=$(_run --claim test-gh-fail-t6 2>"${stderr_t6}") || echo "note: _run exited non-zero" >&2 GIT_COMMON_T6="$(_git_common_dir)" -if printf '%s' "${result_t6}" | grep -qE '^[0-9]{4}$'; then - _pass "test 6a: gh failure still returns 4-digit number: ${result_t6}" +if [ "${result_t6}" = "0602" ]; then + _pass "test 6a: gh failure returns expected number 0602" else - _fail "test 6a: gh failure returned invalid output: '${result_t6}'" + _fail "test 6a: expected 0602, got '${result_t6}'" fi if grep -q "WARNING: open pull request heads not scanned (GraphQL: could not resolve to a Repository); fork pull request ADR numbers may collide" "${stderr_t6}"; then @@ -406,7 +431,7 @@ rm -f "${stderr_t6}" # ── test 7: negative: no gh on PATH → fallback + WARNING with reason ───────── # # Scenario: gh is completely missing from PATH. -# Expected: claim succeeds with exit 0, returns next free number, +# Expected: claim succeeds with exit 0, returns next free number (0602), # and stderr contains WARNING with reason. NO_GH_BIN="${TMPDIR_BASE}/no-gh-bin" @@ -421,17 +446,16 @@ result_t7=$(cd "${FAKE_REPO}" && PATH="${NO_GH_BIN}" bash "${SCRIPT}" --claim te echo "note: _run exited non-zero" >&2 GIT_COMMON_T7="$(_git_common_dir)" -if printf '%s' "${result_t7}" | grep -qE '^[0-9]{4}$'; then - _pass "test 7a: missing gh still returns 4-digit number: ${result_t7}" +if [ "${result_t7}" = "0602" ]; then + _pass "test 7a: missing gh returns expected number 0602" else - _fail "test 7a: missing gh returned invalid output: '${result_t7}'" + _fail "test 7a: expected 0602, got '${result_t7}'" fi -if grep -q "WARNING: open pull request heads not scanned" "${stderr_t7}" && - grep -q "fork pull request ADR numbers may collide" "${stderr_t7}"; then - _pass "test 7b: missing gh prints expected WARNING on stderr" +if grep -q "WARNING: open pull request heads not scanned (gh not found on PATH); fork pull request ADR numbers may collide" "${stderr_t7}"; then + _pass "test 7b: missing gh prints expected WARNING with reason 'gh not found on PATH' on stderr" else - _fail "test 7b: expected WARNING on stderr, got: $(cat "${stderr_t7}")" + _fail "test 7b: expected WARNING with 'gh not found on PATH' on stderr, got: $(cat "${stderr_t7}")" fi rm -f "${FAKE_REPO}/docs/adr/${result_t7}-test-no-gh-t7.md.stub" @@ -441,7 +465,7 @@ rm -f "${stderr_t7}" # ── test 8: boundary: gh returns empty list (no open PRs) ──────────────────── # # Scenario: gh succeeds and prints nothing (no open PRs). -# Expected: no WARNING on stderr, allocator returns next number from branches/master. +# Expected: no WARNING on stderr, allocator returns next number from branches/master (0602). cat >"${STUB_BIN}/gh" <<'GHEOF' #!/usr/bin/env bash @@ -453,10 +477,10 @@ stderr_t8="$(mktemp)" result_t8=$(_run --claim test-empty-pr-t8 2>"${stderr_t8}") || echo "note: _run exited non-zero" >&2 GIT_COMMON_T8="$(_git_common_dir)" -if printf '%s' "${result_t8}" | grep -qE '^[0-9]{4}$'; then - _pass "test 8a: empty PR list returns 4-digit number: ${result_t8}" +if [ "${result_t8}" = "0602" ]; then + _pass "test 8a: empty PR list returns expected number 0602" else - _fail "test 8a: empty PR list returned invalid output: '${result_t8}'" + _fail "test 8a: expected 0602, got '${result_t8}'" fi if grep -q "WARNING: open pull request heads not scanned" "${stderr_t8}"; then @@ -592,6 +616,180 @@ rm -f "${GIT_COMMON_T11}/adr-claims/${result_t11}" git -C "${FAKE_REPO}" update-ref -d "refs/pull/2/head" rm -f "${stderr_t11}" +# ── test 12: split fetch: branch ADRs survive absent PR head commit SHA ─────── +# +# Scenario: an origin branch holds docs/adr/0750-remote.md. +# gh stub returns one valid PR SHA (with docs/adr/0760-pr.md) and one absent +# SHA not present on origin. +# Batch PR fetch fails on the absent SHA and retries per-SHA, warning on +# the absent SHA. +# Expected: branch tip fetch succeeds independently; allocator skips branch +# ADR 0750 and valid PR ADR 0760, returning 0761; +# stderr contains per-SHA warning for the absent PR head; +# stderr does NOT report branch tips failed to fetch. + +git -C "${FAKE_REPO}" checkout --quiet -b branch-t12 master +touch "${FAKE_REPO}/docs/adr/0750-remote.md" +git -C "${FAKE_REPO}" add docs/adr/0750-remote.md +git -C "${FAKE_REPO}" commit --quiet -m "feat: branch adr 0750" +SHA_BRANCH_12="$(git -C "${FAKE_REPO}" rev-parse HEAD)" + +git -C "${FAKE_REPO}" checkout --quiet -b temp-pr-t12 master +touch "${FAKE_REPO}/docs/adr/0760-pr.md" +git -C "${FAKE_REPO}" add docs/adr/0760-pr.md +git -C "${FAKE_REPO}" commit --quiet -m "feat: pr adr 0760" +SHA_PR_12="$(git -C "${FAKE_REPO}" rev-parse HEAD)" + +git -C "${FAKE_REPO}" checkout --quiet master +git -C "${FAKE_REPO}" branch -D branch-t12 temp-pr-t12 >/dev/null + +git -C "${FAKE_REPO}" update-ref "refs/remotes/origin/branch-t12" "${SHA_BRANCH_12}" +git -C "${FAKE_REPO}" update-ref "refs/pull/12/head" "${SHA_PR_12}" + +SHA_ABSENT_12="e00000000000000000000000000000000000000e" + +MASTER_SHA_T12="$(git -C "${FAKE_REPO}" rev-parse master)" +cat >"${STUB_BIN}/git" <"${STUB_BIN}/gh" <"${stderr_t12}") || echo "note: _run exited non-zero" >&2 +GIT_COMMON_T12="$(_git_common_dir)" + +if [ "${result_t12}" = "0761" ]; then + _pass "test 12a: split fetch: branch ADR 0750 survived absent PR head → allocator returns 0761" +else + _fail "test 12a: expected 0761, got '${result_t12}'" +fi + +if grep -q "WARNING: could not fetch open pull request head ${SHA_ABSENT_12}; its ADR numbers are skipped." "${stderr_t12}"; then + _pass "test 12b: per-SHA warning printed for absent PR head commit" +else + _fail "test 12b: expected per-SHA warning on stderr, got: $(cat "${stderr_t12}")" +fi + +if grep -q "could not fetch remote branch tips" "${stderr_t12}"; then + _fail "test 12c: branch tips incorrectly reported as failed: $(cat "${stderr_t12}")" +else + _pass "test 12c: branch tips fetch not affected by bad PR head" +fi + +rm -f "${FAKE_REPO}/docs/adr/${result_t12}-test-split-fetch-t12.md.stub" +rm -f "${GIT_COMMON_T12}/adr-claims/${result_t12}" +git -C "${FAKE_REPO}" update-ref -d "refs/remotes/origin/branch-t12" +git -C "${FAKE_REPO}" update-ref -d "refs/pull/12/head" +rm -f "${stderr_t12}" "${STUB_BIN}/git" + +# ── test 13: VMAF_ADR_PR_REPO environment override ─────────────────────────── +# +# Scenario: +# 13a (positive): VMAF_ADR_PR_REPO is set to custom repo; stub requires -R with that value. +# 13b (negative): VMAF_ADR_PR_REPO is unset; stub requires -R with origin URL. +# 13c (boundary): VMAF_ADR_PR_REPO is empty string; falls back to origin URL. + +git -C "${FAKE_REPO}" checkout --quiet -b temp-override-pr master +touch "${FAKE_REPO}/docs/adr/0770-override.md" +git -C "${FAKE_REPO}" add docs/adr/0770-override.md +git -C "${FAKE_REPO}" commit --quiet -m "feat: override pr adr 0770" +SHA_OVERRIDE="$(git -C "${FAKE_REPO}" rev-parse HEAD)" +git -C "${FAKE_REPO}" checkout --quiet master +git -C "${FAKE_REPO}" branch -D temp-override-pr >/dev/null +git -C "${FAKE_REPO}" update-ref "refs/pull/13/head" "${SHA_OVERRIDE}" + +CUSTOM_REPO="VMAFx/vmafx" + +# 13a: Positive case: VMAF_ADR_PR_REPO set to CUSTOM_REPO +cat >"${STUB_BIN}/gh" <&2 +exit 1 +GHEOF +chmod +x "${STUB_BIN}/gh" + +result_t13a=$(VMAF_ADR_PR_REPO="${CUSTOM_REPO}" _run --claim test-override-pos-t13 2>/dev/null) || echo "note: _run exited non-zero" >&2 +GIT_COMMON_T13="$(_git_common_dir)" + +if [ "${result_t13a}" = "0771" ]; then + _pass "test 13a: positive: VMAF_ADR_PR_REPO override passed to gh -R → allocator returns 0771" +else + _fail "test 13a: expected 0771 with VMAF_ADR_PR_REPO override, got '${result_t13a}'" +fi +rm -f "${FAKE_REPO}/docs/adr/${result_t13a}-test-override-pos-t13.md.stub" +rm -f "${GIT_COMMON_T13}/adr-claims/${result_t13a}" + +# 13b: Negative case: VMAF_ADR_PR_REPO unset → stub receives origin URL +cat >"${STUB_BIN}/gh" <&2 +exit 1 +GHEOF +chmod +x "${STUB_BIN}/gh" + +result_t13b=$( + unset VMAF_ADR_PR_REPO + _run --claim test-override-unset-t13 2>/dev/null +) || echo "note: _run exited non-zero" >&2 +if [ "${result_t13b}" = "0771" ]; then + _pass "test 13b: negative: unset VMAF_ADR_PR_REPO falls back to origin URL → allocator returns 0771" +else + _fail "test 13b: expected 0771 with unset VMAF_ADR_PR_REPO, got '${result_t13b}'" +fi +rm -f "${FAKE_REPO}/docs/adr/${result_t13b}-test-override-unset-t13.md.stub" +rm -f "${GIT_COMMON_T13}/adr-claims/${result_t13b}" + +# 13c: Boundary case: VMAF_ADR_PR_REPO set to empty string → falls back to origin URL +result_t13c=$(VMAF_ADR_PR_REPO="" _run --claim test-override-empty-t13 2>/dev/null) || echo "note: _run exited non-zero" >&2 +if [ "${result_t13c}" = "0771" ]; then + _pass "test 13c: boundary: empty VMAF_ADR_PR_REPO falls back to origin URL → allocator returns 0771" +else + _fail "test 13c: expected 0771 with empty VMAF_ADR_PR_REPO, got '${result_t13c}'" +fi +rm -f "${FAKE_REPO}/docs/adr/${result_t13c}-test-override-empty-t13.md.stub" +rm -f "${GIT_COMMON_T13}/adr-claims/${result_t13c}" +git -C "${FAKE_REPO}" update-ref -d "refs/pull/13/head" + # ── summary ─────────────────────────────────────────────────────────────────── printf '\n%d passed, %d failed\n' "${PASS}" "${FAIL}"