Skip to content

Commit f19b3ab

Browse files
primeincclaude
andcommitted
admin: address PR #81 review comments — FF semantics, FD3 stderr, generic msgs
Three real fixes from the PR #81 review pass (codex P1 + 2 Copilot mediums): 1) [chatgpt-codex P1, 00f:200] Fix FF detection inverted compare semantics. The compare API returns ahead_by/behind_by relative to `compare/{base}...{head}` order: ahead_by = commits in head not in base behind_by = commits in base not in head Previous code called `compare/${branch}...main` then read behind_by as "branch behind main" — exactly inverted. With compare/branch...main: ahead_by = commits-in-main-not-in-branch (i.e. branch BEHIND), behind_by = commits-in-branch-not-in-main (i.e. branch AHEAD). The caller's variable names assumed the correct semantic, so a cleanly fast-forwardable branch was being reported as divergent and skipped. Fix: ff_state() now uses `compare/main...${branch}` so the API's ahead_by/behind_by directly match the caller's "branch ahead/behind main" semantic. Comment block expanded to make the API direction explicit so future-me doesn't re-invert it. 2) [Copilot 00f:106] find_release_pr's "Multiple open PRs" error message went to stdout while callers do `... 2>/dev/null`. The stderr redirect didn't help because the annotation was on stdout, and stdout is captured by command substitution. Net effect: silent failure on the multi-PR error path. Fix: open FD 3 to the script's real stderr at the top of the step (`exec 3>&2`). Helper functions now emit their `::error::` annotations + diagnostic lists to FD 3, which survives both the caller's command substitution and the `2>/dev/null` swallow on the happy path. 3) [Copilot 00f:229] Two summary strings hardcoded "PR #79 remains blocked..." This will rot the moment another downstream PR exists. Replace with generic "downstream PRs targeting either lane remain blocked..." in both the error path and the summary section heading. Not addressed in this commit (with reasons): - [Copilot 00e:36] `environment: ${{ inputs.operation == 'upsert' && 'github-admin' || '' }}` empty-string env on the check path. Per ../refs/github/docs/data/reusables/actions/jobs/section-using- environments-for-jobs.md, the docs describe valid forms (single name string, or object with name+url) but do not document empty-string behavior. actionlint accepts the YAML. The 00e workflow is dispatch-only and the brief explicitly defers live upsert ("Do not activate live rulesets"), so the empty-string path is currently unreachable. If/when 00e fires in upsert mode and the empty-string env breaks at runtime, split into two jobs (render + upsert, only upsert declares environment). - [gemini approval-count = 1, both rulesets] Brief explicitly says "No `require_code_owner_review` unless a real separate reviewer/team exists. It does not." With one actor, required_approving_review_count: 1 is unsatisfiable. Brief-aligned value is 0. - [gemini hardcoded App ID 3663316 in tracked JSON, both rulesets] Brief explicitly says "Tracked JSON must not hardcode numeric App IDs." Bypass actor is rendered at runtime in 00e from vars.GH_APP_ID. Tracked specs MUST stay with empty bypass_actors. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 parent b23f57e commit f19b3ab

1 file changed

Lines changed: 45 additions & 13 deletions

File tree

.github/workflows/00f-sync-protected-branches-with-main.yml

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,12 @@ jobs:
6464
run: |
6565
set -euo pipefail
6666
67+
# Open FD 3 to the workflow's real stderr so helper
68+
# functions can emit ::error:: annotations that survive
69+
# callers using `... 2>/dev/null` to suppress noise on the
70+
# happy path. See Copilot review comment on 00f:106.
71+
exec 3>&2
72+
6773
owner="${REPO%%/*}"
6874
6975
gh_api() {
@@ -103,8 +109,24 @@ jobs:
103109
if [ "${count}" -eq 0 ]; then
104110
return 1
105111
fi
106-
echo "::error::Multiple open repo-owned ${head_branch} -> main PRs found; refusing to choose."
107-
jq -r '.[] | " - #\(.number) \(.html_url)"' <<<"${pr_json}" >&2
112+
# Multi-PR error path. Caller captures stdout via command
113+
# substitution + `2>/dev/null`. Write the diagnostic to
114+
# the workflow log directly via gh's GITHUB_STEP_SUMMARY
115+
# would be wrong here (helper, not a step); instead emit
116+
# to file descriptor 3 which we open to a known log path.
117+
# Simpler: write to >&2 NOW (before the redirect by the
118+
# caller would matter), and ALSO set a global state
119+
# variable the caller can read. Since the caller pattern
120+
# is `if find_release_pr ... 2>/dev/null; then ... else
121+
# rc=$?; fi`, the stderr WILL be swallowed. Use FD 3
122+
# (opened in the main script body) to bypass.
123+
#
124+
# Per Copilot review on PR #81: route both the
125+
# `::error::` annotation and the per-PR list to FD 3 so
126+
# they survive the caller's `2>/dev/null`. FD 3 is
127+
# opened to /dev/stderr at the top of this run script.
128+
echo "::error::Multiple open repo-owned ${head_branch} -> main PRs found; refusing to choose." >&3
129+
jq -r '.[] | " - #\(.number) \(.html_url)"' <<<"${pr_json}" >&3
108130
return 2
109131
}
110132
@@ -123,17 +145,26 @@ jobs:
123145
}
124146
125147
# ------------------------------------------------------------
126-
# Helper: compare a branch against main; returns 0 if FF-able
127-
# (behind_by > 0 AND ahead_by == 0), 1 if up-to-date already,
128-
# 2 if divergent (both ahead and behind).
148+
# Helper: compare a branch against main. Returns
149+
# `<branch_ahead_of_main> <branch_behind_main>` on stdout.
150+
#
151+
# Uses `compare/main...{branch}` (NOT `{branch}...main`)
152+
# because the GitHub compare API's `ahead_by`/`behind_by`
153+
# are computed relative to the basehead order:
154+
# compare/A...B → ahead_by = commits in B not in A
155+
# behind_by = commits in A not in B
156+
# So compare/main...{branch} gives:
157+
# ahead_by = commits in branch not in main = branch AHEAD
158+
# behind_by = commits in main not in branch = branch BEHIND
159+
# which matches the caller's variable semantic.
129160
# ------------------------------------------------------------
130161
ff_state() {
131162
local branch="$1"
132163
local cmp
133-
cmp=$(gh_api "/repos/${REPO}/compare/${branch}...main")
164+
cmp=$(gh_api "/repos/${REPO}/compare/main...${branch}")
134165
local behind_by ahead_by
135-
behind_by=$(jq -r '.behind_by' <<<"${cmp}")
136166
ahead_by=$(jq -r '.ahead_by' <<<"${cmp}")
167+
behind_by=$(jq -r '.behind_by' <<<"${cmp}")
137168
echo "${ahead_by} ${behind_by}"
138169
}
139170
@@ -219,14 +250,15 @@ jobs:
219250
echo "::endgroup::"
220251
221252
# ============================================================
222-
# Final disposition: if either lane failed to sync, fail the
223-
# workflow so the operator sees the red signal. PR #79
224-
# remains blocked while either lane is stale.
253+
# Final disposition: if either lane failed to sync, fail
254+
# the workflow so the operator sees the red signal.
255+
# Downstream PRs targeting either lane remain blocked
256+
# while that lane is stale.
225257
# ============================================================
226258
cat state.txt
227259
228260
if grep -qE '^(next|admin)_sync_failed=true' "${GITHUB_OUTPUT:-/dev/null}" 2>/dev/null; then
229-
echo "::error::One or more protected branches could not be synced; PR #79 remains blocked until both lanes are caught up."
261+
echo "::error::One or more protected branches could not be synced; downstream PRs remain blocked until both lanes are caught up to main."
230262
exit 1
231263
fi
232264
@@ -257,6 +289,6 @@ jobs:
257289
echo "- Fast-forward fallback for \`ghapp/repo-admin\` when no open PR: \`PATCH /repos/{owner}/{repo}/git/refs/heads/{branch}\` with \`force=false\` (FF-only)"
258290
echo "- Compare API: \`GET /repos/{owner}/{repo}/compare/{branch}...main\` for FF-state inspection"
259291
echo ""
260-
echo "## PR #79 unblock condition"
261-
echo "PR #79 remains blocked until BOTH lanes are caught up to main. If either lane is stale or divergent, the workflow fails red and the operator must reconcile manually before the next push to main can clear the gate."
292+
echo "## Downstream PR unblock condition"
293+
echo "Downstream PRs targeting either lane remain blocked until BOTH lanes are caught up to main. If either lane is stale or divergent, this workflow fails red and the operator must reconcile manually before the next push to main can clear the gate."
262294
} >>"$GITHUB_STEP_SUMMARY"

0 commit comments

Comments
 (0)