From 36ccdc28fec3e39eff176145f8109f395f86ec87 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 7 Aug 2026 14:34:42 +0100 Subject: [PATCH 1/9] DOC-6951 Detect missing aliases after merge and open a fix PR Automates remembering to run the alias scanner, which is the only thing item C was ever about. The scan itself has been available as make check_aliases since the tooling landed. Post-merge on purpose, rather than the pull_request check originally planned. A PR-time check has to choose between being ignorable and being intrusive, and neither is worth it here: the fix is always the same mechanical edit, so author intent almost never matters, and the cases where it might -- a page retired rather than moved -- are already the ones the scanner refuses to touch. Running on every push to main instead costs about three seconds, needs no Hugo build, and keeps the window where an old URL 404s down to minutes rather than the days a scheduled-only sweep implies. Nobody sees anything on their own PR. The branch is regenerated from main on every run and force-pushed, so the PR always means the same thing: current main plus every alias currently missing. That makes repeated runs idempotent and stops a stale half-fix accumulating on a long-lived bot branch. An earlier draft tried to reuse and extend the existing fix branch, which meant switching branches with a dirty working tree -- fragile for no benefit. fetch-depth: 0 is load-bearing rather than tidy. The scanner reads git rename records, and in a shallow clone it finds none, reports zero moves and exits 0. Verified against a --depth 1 clone: a permanent green tick that never examines anything, which is the worst possible failure for a check like this. No other workflow in the repo sets it, and repo-memory.yml has already been caught by the same default once, needing fetch-depth: 2. Uses gh pr list --head rather than the gh search prs idiom the sync workflows use, because search goes through an eventually-consistent index and can miss a PR opened moments earlier, which would produce duplicates. Dry-run in a fresh clone: the scan and the exact git sequence produce a 202-file, 430-insertion commit on the fix branch, matching the backfill in #3769 line for line. Learned: checkout defaults to a depth-1 clone, in which this scanner silently passes; any history-reading check needs fetch-depth 0 and a test that it fails when it should Constraint: the fix branch is rebuilt from main and force-pushed every run, never extended, so the PR always represents main plus all currently-missing aliases Rejected: a pull_request check with annotations | intent almost never matters for a mechanical alias, and the cases where it would are already never auto-fixed Directive: merge this after the backfill in #3769, or its first run opens a competing PR containing the same 256 aliases Ticket: DOC-6951 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/alias_check.yaml | 124 +++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 .github/workflows/alias_check.yaml diff --git a/.github/workflows/alias_check.yaml b/.github/workflows/alias_check.yaml new file mode 100644 index 0000000000..c1b7aed900 --- /dev/null +++ b/.github/workflows/alias_check.yaml @@ -0,0 +1,124 @@ +name: alias_check + +# Find pages that moved without gaining an alias for their old URL, and open a +# PR adding the missing aliases. +# +# Deliberately *post-merge* rather than a pull_request check: it adds nothing to +# anyone's PR -- no check, no annotation, no comment. Because the scan takes about +# three seconds and needs no Hugo build, running it on every push to main keeps +# the window in which an old URL 404s down to minutes, rather than the days a +# scheduled-only sweep would imply. +# +# The PR it opens is the only output, and it always represents the same thing: +# current main plus every alias that is currently missing. That makes repeated +# runs idempotent -- the branch is regenerated from scratch each time, so it can +# never accumulate a stale half-fix. +# +# Closing the PR is a perfectly good answer when a page was retired on purpose +# rather than moved. Nothing else depends on it, and the next run will simply +# propose it again if the page is still reachable by a dead URL. +# +# See DOC-6951 and build/check_missing_aliases.py. + +on: + push: + branches: [main] + schedule: + # Belt and braces for quiet periods, and for the small tail of renames git + # records as a delete plus an add rather than a rename. + - cron: '0 4 1 * *' # 04:00 UTC on the 1st of each month + workflow_dispatch: + +# Minimal default; the job widens what it needs. +permissions: + contents: read + +env: + FIX_BRANCH: auto/missing-aliases + PR_TITLE: Add aliases for pages that moved without one + +jobs: + alias_check: + name: Check for pages that moved without an alias + runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: write + steps: + - name: Check out the repo with full history + uses: actions/checkout@v4 + with: + # Required, not merely preferred. The scanner reads git rename records, + # and in a shallow clone it finds none, reports zero moves and exits 0 + # -- a permanent green tick that never examines anything. Verified + # against a --depth 1 clone. + fetch-depth: 0 + + - name: Install dependencies + run: pip3 install "PyYAML==6.0.1" + + - name: Scan for missing aliases and add them + run: | + set -euo pipefail + # --fail is deliberately not passed. The PR is the signal; a red tick + # on main would be noise for something no build depends on. + python3 build/check_missing_aliases.py --all --fix 2>&1 | tee alias-report.txt + + - name: Open or update the fix PR + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + set -euo pipefail + + if [ -z "$(git status --porcelain -- content)" ]; then + echo "No missing aliases. Nothing to do." + exit 0 + fi + + echo "Changed files:" + git diff --stat -- content | tail -1 + + git config user.email "177626021+redisdocsapp[bot]@users.noreply.github.com" + git config user.name "redisdocsapp[bot]" + + # Branch off the commit just built, carrying the working-tree changes + # with us. No branch switching, so nothing can conflict. + git checkout -B "${FIX_BRANCH}" + git add content + git commit --quiet -m "Add aliases for pages that moved without one" \ + -m "Generated by build/check_missing_aliases.py --all --fix." + + # A plain force push: this branch is bot-owned and regenerated from + # main on every run, so there is no history worth preserving on it. + git push --quiet --force origin "${FIX_BRANCH}" + + # `gh pr list --head` is an exact lookup. `gh search prs` goes through + # an eventually-consistent index and can miss a PR opened moments ago. + if [ -n "$(gh pr list --head "${FIX_BRANCH}" --state open --json number --jq '.[].number')" ]; then + echo "Force push updated the PR already open for ${FIX_BRANCH}." + exit 0 + fi + + { + echo "Adds aliases for pages that were renamed without one, so their old URLs stop returning 404." + echo + echo "Generated by \`build/check_missing_aliases.py --all --fix\`. Only \`aliases:\` frontmatter is touched — no prose changes and no page moves." + echo + echo "**If a page here was retired on purpose rather than moved, close this PR.** Nothing depends on it." + echo + echo "Cases needing a human decision are reported below rather than changed: an old URL that is a live page today, a URL another page already claims as its alias, or a move onto a draft." + echo + echo '
Scanner report' + echo + echo '```' + cat alias-report.txt + echo '```' + echo + echo '
' + } > pr-body.md + + gh pr create \ + --title "${PR_TITLE}" \ + --body-file pr-body.md \ + --head "${FIX_BRANCH}" \ + --base main From a109b8403d5689536a9364d925821b96fc0bc287 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 7 Aug 2026 15:02:32 +0100 Subject: [PATCH 2/9] DOC-6951 Harden the alias workflow against races, size and silent skips Five Bugbot findings, all valid, and one of them was a certainty rather than a risk. The PR body embedded the whole scanner report, and a full report runs to 117,912 characters against GitHub's 65,536 limit for a body. So gh pr create would have failed on the very first run, after the branch had already been force-pushed -- leaving a pushed branch and no PR. The report is now trimmed to 40,000 characters with a note saying so, which puts the body around 41,000, and the untrimmed version stays in the run log. Two findings were about concurrency. Overlapping runs both force-push the same branch, so a slower run built from an older main could replace a newer commit; and both could pass the gh pr list check before either created a PR, so the loser would fail under set -e and redden a run on main. There is now a concurrency group that cancels an in-flight older run, since the newest run is always the one whose answer we want, and PR creation tolerates losing the race when a PR turns out to exist. A run cancelled between the push and the create is self-healing: the next one force-pushes again and finds no open PR. The checkout now pins ref: main. A manual dispatch from another branch would otherwise have scanned that branch while still opening a PR against main, so the head would have carried unrelated commits. The last one is the same shape as a defect Bugbot found in the scanner earlier, which I had fixed only halfway. --fix can decline to edit a file, and I had made that visible in the exit code but only under --fail, which the workflow did not pass. So a refused file left the working tree clean for that page and the run printed that there was nothing to do. The workflow now passes --fail, captures the status through the tee with PIPESTATUS, and errors when the fixer declined something it could not put in a PR -- a red run being the only channel anyone would notice for a case with nothing to review. Verified the exit-code contract the workflow now depends on: --fix --fail exits 0 when nothing was skipped and 1 when something was, and --fail without --fix exits 1 while gaps remain. Both branches of the new shell gate were exercised directly. The skipped-file path inside the scanner itself is currently unreachable with this corpus, so that half is reasoned from the code rather than observed. Learned: the PR-body size limit turned a design that looked fine into one that could never have worked once, and only measuring the artifact showed it -- 117,912 characters against a 65,536 cap Constraint: the fix PR body must stay under GitHub's 65,536-character limit, so the scanner report is trimmed and the run log holds the full version Constraint: the workflow always checks out main, whatever ref triggered it, because the fix branch and the PR base both assume main Gaps: the scanner's own skipped-file exit path is unexercised, since no file in this corpus is refused any more Ticket: DOC-6951 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/alias_check.yaml | 79 ++++++++++++++++++++++++------ 1 file changed, 63 insertions(+), 16 deletions(-) diff --git a/.github/workflows/alias_check.yaml b/.github/workflows/alias_check.yaml index c1b7aed900..875fe6e37e 100644 --- a/.github/workflows/alias_check.yaml +++ b/.github/workflows/alias_check.yaml @@ -9,10 +9,9 @@ name: alias_check # the window in which an old URL 404s down to minutes, rather than the days a # scheduled-only sweep would imply. # -# The PR it opens is the only output, and it always represents the same thing: -# current main plus every alias that is currently missing. That makes repeated -# runs idempotent -- the branch is regenerated from scratch each time, so it can -# never accumulate a stale half-fix. +# The PR it opens always represents the same thing: current main plus every alias +# that is currently missing. That makes repeated runs idempotent -- the branch is +# regenerated from scratch each time, so it can never accumulate a stale half-fix. # # Closing the PR is a perfectly good answer when a page was retired on purpose # rather than moved. Nothing else depends on it, and the next run will simply @@ -29,6 +28,15 @@ on: - cron: '0 4 1 * *' # 04:00 UTC on the 1st of each month workflow_dispatch: +# One run at a time. Overlapping runs both force-push the same branch, and the +# loser could replace a newer commit with an older one built from an earlier main. +# The newest run is always the one whose answer we want, so an in-flight older run +# is cancelled rather than queued. A run cancelled between the push and the PR +# creation is self-healing: the next run force-pushes again and finds no open PR. +concurrency: + group: alias_check + cancel-in-progress: true + # Minimal default; the job widens what it needs. permissions: contents: read @@ -36,6 +44,10 @@ permissions: env: FIX_BRANCH: auto/missing-aliases PR_TITLE: Add aliases for pages that moved without one + # GitHub rejects a PR body over 65,536 characters. A full report on a large + # backlog runs to about 118,000, so it is trimmed well below the cap and the + # untrimmed version stays in the run log. + BODY_REPORT_LIMIT: 40000 jobs: alias_check: @@ -45,9 +57,13 @@ jobs: contents: write pull-requests: write steps: - - name: Check out the repo with full history + - name: Check out main with full history uses: actions/checkout@v4 with: + # Always main, whatever ref triggered the run. A manual dispatch from + # another branch would otherwise scan that branch and open a PR whose + # head carried its unrelated commits. + ref: main # Required, not merely preferred. The scanner reads git rename records, # and in a shallow clone it finds none, reports zero moves and exits 0 # -- a permanent green tick that never examines anything. Verified @@ -58,19 +74,36 @@ jobs: run: pip3 install "PyYAML==6.0.1" - name: Scan for missing aliases and add them + id: scan run: | - set -euo pipefail - # --fail is deliberately not passed. The PR is the signal; a red tick - # on main would be noise for something no build depends on. - python3 build/check_missing_aliases.py --all --fix 2>&1 | tee alias-report.txt + set -uo pipefail + # --fail is passed so that a file the fixer *refused* to edit becomes + # visible. With --fix, it exits 1 only when some actionable file was + # skipped, which is otherwise invisible: the working tree would look + # clean for those pages and the run would report nothing to do. + python3 build/check_missing_aliases.py --all --fix --fail 2>&1 | tee alias-report.txt + status="${PIPESTATUS[0]}" + echo "skipped=${status}" >> "$GITHUB_OUTPUT" + if [ "${status}" -gt 1 ]; then + echo "::error::check_missing_aliases failed with exit ${status}" + exit "${status}" + fi - name: Open or update the fix PR env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + SKIPPED: ${{ steps.scan.outputs.skipped }} run: | set -euo pipefail if [ -z "$(git status --porcelain -- content)" ]; then + if [ "${SKIPPED}" = "1" ]; then + # Nothing to open a PR with, yet the fixer declined some files. A + # red run is the only channel anyone would notice, and the report + # above names the files. + echo "::error::Missing aliases were found but could not be added automatically. See the report above." + exit 1 + fi echo "No missing aliases. Nothing to do." exit 0 fi @@ -99,6 +132,14 @@ jobs: exit 0 fi + if [ "$(wc -c < alias-report.txt)" -gt "${BODY_REPORT_LIMIT}" ]; then + head -c "${BODY_REPORT_LIMIT}" alias-report.txt > report-for-body.txt + printf '\n\n[Trimmed at %s characters. The full report is in the workflow run log.]\n' \ + "${BODY_REPORT_LIMIT}" >> report-for-body.txt + else + cp alias-report.txt report-for-body.txt + fi + { echo "Adds aliases for pages that were renamed without one, so their old URLs stop returning 404." echo @@ -106,19 +147,25 @@ jobs: echo echo "**If a page here was retired on purpose rather than moved, close this PR.** Nothing depends on it." echo - echo "Cases needing a human decision are reported below rather than changed: an old URL that is a live page today, a URL another page already claims as its alias, or a move onto a draft." + echo "Cases needing a human decision are reported below rather than changed: an old URL that is a live page today, a URL another page already claims as its alias, a page that was split into a section, or a move onto a draft." echo echo '
Scanner report' echo echo '```' - cat alias-report.txt + cat report-for-body.txt echo '```' echo echo '
' } > pr-body.md - gh pr create \ - --title "${PR_TITLE}" \ - --body-file pr-body.md \ - --head "${FIX_BRANCH}" \ - --base main + # A run cancelled or overlapping despite the concurrency group could + # have opened the PR between the check above and this call. Losing that + # race is not a failure: the force push already updated the branch. + if ! gh pr create --title "${PR_TITLE}" --body-file pr-body.md \ + --head "${FIX_BRANCH}" --base main; then + if [ -n "$(gh pr list --head "${FIX_BRANCH}" --state open --json number --jq '.[].number')" ]; then + echo "A concurrent run opened the PR first; the force push has updated it." + else + exit 1 + fi + fi From 7f52d4b82977fcb9167464106c023ca8dea889ec Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 7 Aug 2026 15:48:04 +0100 Subject: [PATCH 3/9] DOC-6951 Keep the alias fix PR's description and lifecycle honest Two more Bugbot findings, both about the bot PR drifting from what it claims. The description was written once and never rewritten. A later run force-pushed a new commit to the branch, so the diff moved but the body kept the previous run's scanner report -- including its skip and collision notes, which are exactly what a reviewer reads to decide. Both the already-open and the lost-the-race paths now refresh it with gh pr edit, so the body always describes the commit beneath it. The other is the case where nothing is missing any more. The clean-scan path exited without looking for an open PR, so if the aliases reached main some other way -- by hand, or inside someone else's PR -- the bot's PR stayed open with a diff that had become redundant, inviting someone to merge a stale set of edits weeks later. It is now commented and closed with its branch deleted, and the next run opens a fresh one if anything is missing again. Restructuring for that meant looking up the open PR once at the top rather than just before creating one, which also removed a duplicated gh pr list call. All five paths of the step were exercised as shell: clean with no PR, clean with a PR, changes with no PR, changes with a PR, and the refused-file error. The GitHub calls themselves are still unexercised until this is on main. Learned: a bot PR has a lifecycle, not just a creation -- the interesting bugs were in refreshing it and retiring it, neither of which the first version considered Constraint: whenever the fix branch is force-pushed the PR description is rewritten, because it embeds a scanner report that a reviewer uses to make decisions Ticket: DOC-6951 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/alias_check.yaml | 44 ++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/.github/workflows/alias_check.yaml b/.github/workflows/alias_check.yaml index 875fe6e37e..af012a5585 100644 --- a/.github/workflows/alias_check.yaml +++ b/.github/workflows/alias_check.yaml @@ -96,6 +96,10 @@ jobs: run: | set -euo pipefail + # `gh pr list --head` is an exact lookup. `gh search prs` goes through an + # eventually-consistent index and can miss a PR opened moments ago. + existing="$(gh pr list --head "${FIX_BRANCH}" --state open --json number --jq '.[].number' | head -1)" + if [ -z "$(git status --porcelain -- content)" ]; then if [ "${SKIPPED}" = "1" ]; then # Nothing to open a PR with, yet the fixer declined some files. A @@ -104,6 +108,18 @@ jobs: echo "::error::Missing aliases were found but could not be added automatically. See the report above." exit 1 fi + if [ -n "${existing}" ]; then + # Nothing is missing any more, but a fix PR is still open -- the + # aliases reached main some other way, by hand or in someone else's + # PR. Its diff is now redundant, and leaving it open invites someone + # to merge a stale set of edits. Close it rather than let it rot; the + # next run reopens one if anything is missing again. + echo "No missing aliases, but PR #${existing} is still open. Closing it." + gh pr comment "${existing}" --body \ + "Closing automatically: a scan of current \`main\` finds no missing aliases, so these changes are no longer needed. A new PR will open if any page moves without one." + gh pr close "${existing}" --delete-branch + exit 0 + fi echo "No missing aliases. Nothing to do." exit 0 fi @@ -125,13 +141,6 @@ jobs: # main on every run, so there is no history worth preserving on it. git push --quiet --force origin "${FIX_BRANCH}" - # `gh pr list --head` is an exact lookup. `gh search prs` goes through - # an eventually-consistent index and can miss a PR opened moments ago. - if [ -n "$(gh pr list --head "${FIX_BRANCH}" --state open --json number --jq '.[].number')" ]; then - echo "Force push updated the PR already open for ${FIX_BRANCH}." - exit 0 - fi - if [ "$(wc -c < alias-report.txt)" -gt "${BODY_REPORT_LIMIT}" ]; then head -c "${BODY_REPORT_LIMIT}" alias-report.txt > report-for-body.txt printf '\n\n[Trimmed at %s characters. The full report is in the workflow run log.]\n' \ @@ -158,13 +167,26 @@ jobs: echo '' } > pr-body.md + if [ -n "${existing}" ]; then + # The force push has already updated the diff, but the description + # holds the previous run's report -- including its skip and collision + # notes, which may no longer be what a reviewer needs to decide. Rewrite + # it so the body always describes the diff below it. + echo "Refreshing the description of PR #${existing}." + gh pr edit "${existing}" --body-file pr-body.md + exit 0 + fi + # A run cancelled or overlapping despite the concurrency group could - # have opened the PR between the check above and this call. Losing that - # race is not a failure: the force push already updated the branch. + # have opened the PR between the lookup above and this call. Losing that + # race is not a failure: the force push already updated the branch, and + # the winner's body describes the same commit. if ! gh pr create --title "${PR_TITLE}" --body-file pr-body.md \ --head "${FIX_BRANCH}" --base main; then - if [ -n "$(gh pr list --head "${FIX_BRANCH}" --state open --json number --jq '.[].number')" ]; then - echo "A concurrent run opened the PR first; the force push has updated it." + raced="$(gh pr list --head "${FIX_BRANCH}" --state open --json number --jq '.[].number' | head -1)" + if [ -n "${raced}" ]; then + echo "A concurrent run opened PR #${raced} first; refreshing its description." + gh pr edit "${raced}" --body-file pr-body.md else exit 1 fi From 116a72a9b5ae22d53ec21c1952d05585c5bdc01f Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Fri, 7 Aug 2026 16:57:04 +0100 Subject: [PATCH 4/9] DOC-6951 Re-check the fix PR's state before acting on it The open PR was looked up once at the top of the step and then acted on minutes later. Someone can merge or close it inside that window, and closing an already-merged PR is an error, so set -e would have reddened a run on main for something entirely harmless -- or, on the other path, quietly rewritten the description of a PR nobody will read again. The state is now confirmed immediately before each use, and anything other than OPEN is treated as "there is no PR", which lets the ordinary paths take over: open a fresh one, or do nothing. The gh calls stay tolerant of failure anyway, because checking the state narrows the race window without closing it, and a failed comment on a PR that has just been merged is not worth a red run. The companion change to the scanner means exit 2 now identifies a failed scan, so the step's comment no longer implies that a non-zero exit can only mean skipped files. Verified as shell across all four PR states -- open, merged, closed, absent -- with set -e active, confirming the fall-through does not abort, and that a failing tolerant call leaves the run going. Learned: a status read at the start of a job is a guess by the time the job acts on it, and "closed" needs to route into the no-PR path rather than into an error Constraint: confirm the fix PR is still open immediately before commenting, closing or editing it, and keep those calls non-fatal regardless Ticket: DOC-6951 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/alias_check.yaml | 39 +++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/.github/workflows/alias_check.yaml b/.github/workflows/alias_check.yaml index af012a5585..603c5e36b2 100644 --- a/.github/workflows/alias_check.yaml +++ b/.github/workflows/alias_check.yaml @@ -78,9 +78,11 @@ jobs: run: | set -uo pipefail # --fail is passed so that a file the fixer *refused* to edit becomes - # visible. With --fix, it exits 1 only when some actionable file was - # skipped, which is otherwise invisible: the working tree would look - # clean for those pages and the run would report nothing to do. + # visible. With --fix it exits 1 only when some actionable file was + # skipped, which is otherwise invisible: the working tree would look clean + # for those pages and the run would report nothing to do. Exit 2 means the + # scan itself failed and is handled separately below, so a broken scan is + # never reported as a content problem. python3 build/check_missing_aliases.py --all --fix --fail 2>&1 | tee alias-report.txt status="${PIPESTATUS[0]}" echo "skipped=${status}" >> "$GITHUB_OUTPUT" @@ -100,6 +102,18 @@ jobs: # eventually-consistent index and can miss a PR opened moments ago. existing="$(gh pr list --head "${FIX_BRANCH}" --state open --json number --jq '.[].number' | head -1)" + # This job takes minutes, and someone can merge or close the fix PR inside + # that window. Acting on the number we looked up at the start would then + # either fail outright -- closing a merged PR is an error, and `set -e` + # would redden a run on main for something harmless -- or quietly edit a + # PR nobody will read again. So confirm it is still open immediately before + # each use, and treat "no longer open" as "there is no PR", which lets the + # normal paths take over: create a fresh one, or do nothing. + still_open() { + [ -n "${existing}" ] || return 1 + [ "$(gh pr view "${existing}" --json state --jq .state 2>/dev/null)" = "OPEN" ] + } + if [ -z "$(git status --porcelain -- content)" ]; then if [ "${SKIPPED}" = "1" ]; then # Nothing to open a PR with, yet the fixer declined some files. A @@ -108,16 +122,21 @@ jobs: echo "::error::Missing aliases were found but could not be added automatically. See the report above." exit 1 fi - if [ -n "${existing}" ]; then + if still_open; then # Nothing is missing any more, but a fix PR is still open -- the # aliases reached main some other way, by hand or in someone else's # PR. Its diff is now redundant, and leaving it open invites someone # to merge a stale set of edits. Close it rather than let it rot; the # next run reopens one if anything is missing again. echo "No missing aliases, but PR #${existing} is still open. Closing it." + # Both calls tolerate failure: the state check above narrows the race + # window but cannot close it, and neither a missing comment nor an + # already-closed PR is worth reddening a run on main for. gh pr comment "${existing}" --body \ - "Closing automatically: a scan of current \`main\` finds no missing aliases, so these changes are no longer needed. A new PR will open if any page moves without one." - gh pr close "${existing}" --delete-branch + "Closing automatically: a scan of current \`main\` finds no missing aliases, so these changes are no longer needed. A new PR will open if any page moves without one." \ + || echo "::warning::Could not comment on PR #${existing}." + gh pr close "${existing}" --delete-branch \ + || echo "::warning::Could not close PR #${existing}; it may have just been merged or closed." exit 0 fi echo "No missing aliases. Nothing to do." @@ -167,14 +186,16 @@ jobs: echo '' } > pr-body.md - if [ -n "${existing}" ]; then + if still_open; then # The force push has already updated the diff, but the description # holds the previous run's report -- including its skip and collision # notes, which may no longer be what a reviewer needs to decide. Rewrite # it so the body always describes the diff below it. echo "Refreshing the description of PR #${existing}." - gh pr edit "${existing}" --body-file pr-body.md - exit 0 + if gh pr edit "${existing}" --body-file pr-body.md; then + exit 0 + fi + echo "::warning::Could not update PR #${existing}; falling through to open a new one." fi # A run cancelled or overlapping despite the concurrency group could From a4be80ecd8cf568f28026fea6df15f9ce1f2da29 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Mon, 10 Aug 2026 10:48:28 +0100 Subject: [PATCH 5/9] DOC-6951 Scope the bot PR lookup to this repo, and stop the report contradicting the diff Two Bugbot findings on the workflow, one of which could have touched somebody else's pull request. gh pr list --head matches on branch name alone, across forks. A contributor whose branch happened to be called auto/missing-aliases would have matched, and this job goes on from that lookup to comment on, close with --delete-branch, or rewrite the description of whatever it found. Filtering on isCrossRepository restricts it to pull requests whose head is in this repository. The lookup existed in two places, the initial one and the lost-the-race fallback, and only one would have been fixed if I had patched them where they sat, so it is now a single function used by both. The other is a wording problem with a real consequence. report() runs before apply_fixes(), so in --fix mode the report describes gaps the same run is about to close -- and that report is embedded verbatim in the pull request the automation opens. A reviewer reading "add this alias to that file", next to a diff that already contains it, goes looking for work that is done. The report now knows whether a fix follows and says "added" rather than "add", drops the instruction to run the fixer, and words its annotations to match. Covered by a test that captures the log in both modes, since this is the kind of thing that reads fine to whoever wrote it. Also rebased onto main now that the scanner has landed, so this PR is four workflow commits rather than eleven. Learned: a lookup by branch name reaches across forks, so automation that closes or rewrites what it finds needs to check the head repository, not just the branch Constraint: the bot PR lookup filters on isCrossRepository so it can only ever act on a pull request whose head is in this repository Constraint: in --fix mode the report describes what was done rather than what to do, because it is published verbatim in the pull request the fix opens Ticket: DOC-6951 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/alias_check.yaml | 18 +++++++++--- build/check_missing_aliases.py | 33 ++++++++++++++++------ build/test_check_missing_aliases.py | 43 ++++++++++++++++++++++++++++- 3 files changed, 80 insertions(+), 14 deletions(-) diff --git a/.github/workflows/alias_check.yaml b/.github/workflows/alias_check.yaml index 603c5e36b2..b48f194c55 100644 --- a/.github/workflows/alias_check.yaml +++ b/.github/workflows/alias_check.yaml @@ -98,9 +98,19 @@ jobs: run: | set -euo pipefail - # `gh pr list --head` is an exact lookup. `gh search prs` goes through an - # eventually-consistent index and can miss a PR opened moments ago. - existing="$(gh pr list --head "${FIX_BRANCH}" --state open --json number --jq '.[].number' | head -1)" + # `gh pr list --head` is an exact lookup, unlike `gh search prs`, which goes + # through an eventually-consistent index and can miss a PR opened moments + # ago. But it matches on branch *name* alone, so a fork whose branch happens + # to be called auto/missing-aliases would match too -- and this job goes on + # to comment on, close, or rewrite whatever it finds. isCrossRepository + # filters to pull requests whose head is in this repository, so a + # contributor's PR can never be picked up by mistake. + find_bot_pr() { + gh pr list --head "${FIX_BRANCH}" --state open \ + --json number,isCrossRepository \ + --jq '[.[] | select(.isCrossRepository == false) | .number] | first // empty' + } + existing="$(find_bot_pr)" # This job takes minutes, and someone can merge or close the fix PR inside # that window. Acting on the number we looked up at the start would then @@ -204,7 +214,7 @@ jobs: # the winner's body describes the same commit. if ! gh pr create --title "${PR_TITLE}" --body-file pr-body.md \ --head "${FIX_BRANCH}" --base main; then - raced="$(gh pr list --head "${FIX_BRANCH}" --state open --json number --jq '.[].number' | head -1)" + raced="$(find_bot_pr)" if [ -n "${raced}" ]; then echo "A concurrent run opened PR #${raced} first; refreshing its description." gh pr edit "${raced}" --body-file pr-body.md diff --git a/build/check_missing_aliases.py b/build/check_missing_aliases.py index 9558ea6bae..a8f9ba51cc 100644 --- a/build/check_missing_aliases.py +++ b/build/check_missing_aliases.py @@ -703,7 +703,8 @@ def apply_fixes(moves: list[Move]) -> tuple[int, int, list[str]]: # reporting # --------------------------------------------------------------------------- # -def report(moves: list[Move], github: bool, fix_hint: str) -> list[Move]: +def report(moves: list[Move], github: bool, fix_hint: str, + fixing: bool = False) -> list[Move]: missing = [m for m in moves if m.actionable] occupied = [m for m in moves if not m.aliased and m.occupied] drafted = [m for m in moves @@ -753,18 +754,32 @@ def report(moves: list[Move], github: bool, fix_hint: str) -> list[Move]: logger.warning(" claimed by %s", owner) if missing: - logger.warning("Moved with no alias for the old URL:") + # The wording has to know whether a fix follows in the same run. This + # report is embedded in the pull request the automation opens, and reading + # "add this alias" next to a diff that already contains it sends a reviewer + # looking for work that is done. + if fixing: + logger.warning("Moved with no alias for the old URL -- added below:") + else: + logger.warning("Moved with no alias for the old URL:") for move in missing: logger.warning(" %s %s %s", move.date, move.commit, move.old_url) logger.warning(" now at %s", move.new_url) - logger.warning(" add to %s: %s", move.new_path, + logger.warning(" %s %s: %s", + "added to" if fixing else "add to", move.new_path, ALIAS_TEMPLATE.format(url=norm(move.old_url))) if github: - print(f"::warning file={move.new_path}::Page moved from " - f"/{norm(move.old_url)}/ with no alias. Add " - f"'{ALIAS_TEMPLATE.format(url=norm(move.old_url))}' to its " - f"aliases, or run: make check_aliases_fix") - logger.warning("Fix them all with: %s", fix_hint) + alias = ALIAS_TEMPLATE.format(url=norm(move.old_url)) + if fixing: + print(f"::warning file={move.new_path}::Page moved from " + f"/{norm(move.old_url)}/ with no alias. '{alias}' has " + f"been added automatically.") + else: + print(f"::warning file={move.new_path}::Page moved from " + f"/{norm(move.old_url)}/ with no alias. Add '{alias}' to " + f"its aliases, or run: make check_aliases_fix") + if not fixing: + logger.warning("Fix them all with: %s", fix_hint) return missing @@ -782,7 +797,7 @@ def main() -> int: fix_hint = ("make check_aliases_fix" if args.all else "python3 build/check_missing_aliases.py " f"--range {args.rev_range} --fix") - missing = report(moves, args.github, fix_hint) + missing = report(moves, args.github, fix_hint, fixing=args.fix) if args.json_out: with open(args.json_out, "w", encoding="utf-8") as handle: diff --git a/build/test_check_missing_aliases.py b/build/test_check_missing_aliases.py index 1b89d27149..38fd08b78b 100644 --- a/build/test_check_missing_aliases.py +++ b/build/test_check_missing_aliases.py @@ -18,7 +18,8 @@ from check_missing_aliases import ( # noqa: E402 Move, declared_aliases, draft_paths, eligible, insert_aliases, is_published, - is_versioned, norm, order_renames, published_urls, render_never_roots, to_url, + is_versioned, norm, order_renames, published_urls, render_never_roots, report, + to_url, ) @@ -119,6 +120,46 @@ def move(**kwargs): assert move().actionable +def _capture_report(**kwargs) -> str: + """Run report() over one actionable move and return what it logged.""" + import io + import logging as _logging + from check_missing_aliases import logger + + move = Move(old_path="content/old.md", new_path="content/new.md", + old_url="old", new_url="new", date="2026-01-01", commit="abc1234") + stream = io.StringIO() + handler = _logging.StreamHandler(stream) + logger.addHandler(handler) + previous = logger.level + logger.setLevel(_logging.INFO) + try: + report([move], False, "make check_aliases_fix", **kwargs) + finally: + logger.removeHandler(handler) + logger.setLevel(previous) + return stream.getvalue() + + +def test_the_report_says_added_when_a_fix_follows_in_the_same_run(): + """The report is embedded in the PR the automation opens. + + report() runs before apply_fixes(), so in --fix mode it describes gaps that the + same run is about to close. Telling a reviewer to "add" an alias that the diff + beneath already contains sends them looking for work that is done. + """ + imperative = _capture_report() + assert "Moved with no alias for the old URL:" in imperative + assert "add to content/new.md" in imperative + assert "Fix them all with" in imperative + + fixing = _capture_report(fixing=True) + assert "added below" in fixing + assert "added to content/new.md" in fixing + # No instruction to run the fixer: it has just run. + assert "Fix them all with" not in fixing + + def test_a_whitespace_only_line_is_not_a_folded_continuation(): # The folded-scalar guard must not be tripped by trailing whitespace on the # line after a perfectly ordinary single-value scalar. From edbdcba58f6792cff67be5ff8683be961e52b883 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Mon, 10 Aug 2026 11:04:51 +0100 Subject: [PATCH 6/9] DOC-6951 Keep the argparse help line within the file's width A one-line wrap missed when the exit-code constants were added. No behavior change. Ticket: DOC-6951 Co-Authored-By: Claude Opus 5 (1M context) --- build/check_missing_aliases.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build/check_missing_aliases.py b/build/check_missing_aliases.py index a8f9ba51cc..b888001aef 100644 --- a/build/check_missing_aliases.py +++ b/build/check_missing_aliases.py @@ -158,7 +158,7 @@ def parse_args() -> argparse.Namespace: parser.add_argument("--github", action="store_true", help="emit GitHub Actions warning annotations") parser.add_argument("--fail", action="store_true", - help="exit 1 if any move is missing an alias (see EXIT_* below)") + help="exit 1 if any move is missing an alias (see EXIT_*)") return parser.parse_args() From f630c896cf92d4a368901fcf0fa7f34593c47da9 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Mon, 10 Aug 2026 11:32:17 +0100 Subject: [PATCH 7/9] DOC-6951 Report what the fixer did, and tolerate a lost race on the edit Two Bugbot findings, the first of them a correction to my own previous fix. I had changed the report to say "added" in --fix mode, because saying "add this alias" beside a diff that already contains it sends a reviewer after finished work. But --fix can decline a file, so "added" was then a lie in the other direction, and a gap that still needs a person would have read as closed. The honest fix is not better wording but better ordering: --fix now runs first and the report is told which files it declined, so each line says added, or COULD NOT add and fix by hand. Neither version of the wording can be right without that, which is why the first attempt was bound to be wrong whichever phrasing I picked. The second is the last untolerated call on a PR number. The lost-the-race path edited the winner's description without allowing for that PR being merged or closed in the meantime, so set -e could have reddened a run on main after the branch was already pushed. Every gh call that mutates a pull request here is now failure-tolerant, which I checked by auditing all four rather than the one that was reported. Learned: the first fix was unfalsifiable as worded -- a report written before the fix runs cannot describe the fix, so no phrasing was going to be accurate and the ordering had to change Constraint: --fix runs before the report, and the report is given the set of files the fixer declined, so it can distinguish an alias that was added from one that still needs a person Ticket: DOC-6951 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/alias_check.yaml | 7 +++- build/check_missing_aliases.py | 59 ++++++++++++++++++----------- build/test_check_missing_aliases.py | 23 ++++++----- 3 files changed, 56 insertions(+), 33 deletions(-) diff --git a/.github/workflows/alias_check.yaml b/.github/workflows/alias_check.yaml index b48f194c55..6278bc8396 100644 --- a/.github/workflows/alias_check.yaml +++ b/.github/workflows/alias_check.yaml @@ -216,8 +216,13 @@ jobs: --head "${FIX_BRANCH}" --base main; then raced="$(find_bot_pr)" if [ -n "${raced}" ]; then + # Tolerant for the same reason as every other call on a PR number here: + # the PR can be merged or closed between finding it and editing it, and + # the branch has already been pushed either way, so failing the run on + # main achieves nothing. echo "A concurrent run opened PR #${raced} first; refreshing its description." - gh pr edit "${raced}" --body-file pr-body.md + gh pr edit "${raced}" --body-file pr-body.md \ + || echo "::warning::Could not update PR #${raced}; its description may be from an earlier run." else exit 1 fi diff --git a/build/check_missing_aliases.py b/build/check_missing_aliases.py index b888001aef..74fb181ea6 100644 --- a/build/check_missing_aliases.py +++ b/build/check_missing_aliases.py @@ -704,7 +704,7 @@ def apply_fixes(moves: list[Move]) -> tuple[int, int, list[str]]: # --------------------------------------------------------------------------- # def report(moves: list[Move], github: bool, fix_hint: str, - fixing: bool = False) -> list[Move]: + fixing: bool = False, skipped: set[str] | None = None) -> list[Move]: missing = [m for m in moves if m.actionable] occupied = [m for m in moves if not m.aliased and m.occupied] drafted = [m for m in moves @@ -754,30 +754,38 @@ def report(moves: list[Move], github: bool, fix_hint: str, logger.warning(" claimed by %s", owner) if missing: - # The wording has to know whether a fix follows in the same run. This - # report is embedded in the pull request the automation opens, and reading - # "add this alias" next to a diff that already contains it sends a reviewer - # looking for work that is done. - if fixing: - logger.warning("Moved with no alias for the old URL -- added below:") - else: - logger.warning("Moved with no alias for the old URL:") + # This report is embedded in the pull request the automation opens, so it has + # to describe what happened rather than what to do -- "add this alias" next to + # a diff that already contains it sends a reviewer looking for finished work. + # It is therefore printed *after* --fix has run, and told which files the fixer + # declined, because claiming an alias was added when it was skipped is the same + # error in the opposite direction. + declined = skipped or set() + logger.warning("Moved with no alias for the old URL:") for move in missing: + alias = ALIAS_TEMPLATE.format(url=norm(move.old_url)) logger.warning(" %s %s %s", move.date, move.commit, move.old_url) logger.warning(" now at %s", move.new_url) - logger.warning(" %s %s: %s", - "added to" if fixing else "add to", move.new_path, - ALIAS_TEMPLATE.format(url=norm(move.old_url))) + if not fixing: + logger.warning(" add to %s: %s", move.new_path, alias) + elif move.new_path in declined: + logger.warning(" COULD NOT add to %s: %s -- fix by hand", + move.new_path, alias) + else: + logger.warning(" added to %s: %s", move.new_path, alias) if github: - alias = ALIAS_TEMPLATE.format(url=norm(move.old_url)) - if fixing: - print(f"::warning file={move.new_path}::Page moved from " - f"/{norm(move.old_url)}/ with no alias. '{alias}' has " - f"been added automatically.") - else: + if not fixing: print(f"::warning file={move.new_path}::Page moved from " f"/{norm(move.old_url)}/ with no alias. Add '{alias}' to " f"its aliases, or run: make check_aliases_fix") + elif move.new_path in declined: + print(f"::warning file={move.new_path}::Page moved from " + f"/{norm(move.old_url)}/ with no alias, and '{alias}' could " + f"not be added automatically. Add it by hand.") + else: + print(f"::warning file={move.new_path}::Page moved from " + f"/{norm(move.old_url)}/ with no alias. '{alias}' has been " + f"added automatically.") if not fixing: logger.warning("Fix them all with: %s", fix_hint) return missing @@ -797,7 +805,16 @@ def main() -> int: fix_hint = ("make check_aliases_fix" if args.all else "python3 build/check_missing_aliases.py " f"--range {args.rev_range} --fix") - missing = report(moves, args.github, fix_hint, fixing=args.fix) + # Fix first, then report, so the report can say which aliases were actually + # written and which the fixer declined. + missing = [move for move in moves if move.actionable] + skipped: list[str] = [] + added_files = added_aliases = 0 + if args.fix and missing: + logger.info("Adding %d alias(es):", len(missing)) + added_files, added_aliases, skipped = apply_fixes(moves) + + report(moves, args.github, fix_hint, fixing=args.fix, skipped=set(skipped)) if args.json_out: with open(args.json_out, "w", encoding="utf-8") as handle: @@ -805,10 +822,8 @@ def main() -> int: logger.info("Wrote %s", args.json_out) if args.fix and missing: - logger.info("Adding %d alias(es):", len(missing)) - files, aliases, skipped = apply_fixes(moves) logger.info("check_missing_aliases: added %d alias(es) across %d file(s).", - aliases, files) + added_aliases, added_files) if skipped: logger.warning("check_missing_aliases: could not place aliases in %d " "file(s), which still need fixing by hand:", len(skipped)) diff --git a/build/test_check_missing_aliases.py b/build/test_check_missing_aliases.py index 38fd08b78b..0c6e0fcf5e 100644 --- a/build/test_check_missing_aliases.py +++ b/build/test_check_missing_aliases.py @@ -141,23 +141,26 @@ def _capture_report(**kwargs) -> str: return stream.getvalue() -def test_the_report_says_added_when_a_fix_follows_in_the_same_run(): - """The report is embedded in the PR the automation opens. +def test_the_report_describes_what_the_fixer_actually_did(): + """The report is embedded in the PR the automation opens, so it must be accurate. - report() runs before apply_fixes(), so in --fix mode it describes gaps that the - same run is about to close. Telling a reviewer to "add" an alias that the diff - beneath already contains sends them looking for work that is done. + Telling a reviewer to "add" an alias the diff already contains sends them after + finished work; claiming one was added when the fixer declined the file is the same + error pointing the other way, and leaves a real gap looking closed. """ imperative = _capture_report() - assert "Moved with no alias for the old URL:" in imperative assert "add to content/new.md" in imperative assert "Fix them all with" in imperative - fixing = _capture_report(fixing=True) - assert "added below" in fixing - assert "added to content/new.md" in fixing + added = _capture_report(fixing=True) + assert "added to content/new.md" in added + assert "COULD NOT" not in added # No instruction to run the fixer: it has just run. - assert "Fix them all with" not in fixing + assert "Fix them all with" not in added + + declined = _capture_report(fixing=True, skipped={"content/new.md"}) + assert "COULD NOT add to content/new.md" in declined + assert "fix by hand" in declined def test_a_whitespace_only_line_is_not_a_folded_continuation(): From 218810c59989997a23177e8b2b969069821dca6f Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Mon, 10 Aug 2026 12:48:30 +0100 Subject: [PATCH 8/9] DOC-6951 Take errexit off the scan so a refused file survives to the PR step The refused-file mechanism was unreachable, and it failed in the worst direction. Actions runs `run` blocks with `bash -eo pipefail`. `set -uo pipefail` sets -u and pipefail but does not undo the -e, so the deliberate exit 1 that --fix --fail uses to report a declined file aborted the step on the spot: PIPESTATUS was never read, the `skipped` output was never written, the next step never ran, and every alias the run had successfully fixed was thrown away with no pull request opened. So the one scenario the plumbing existed to surface was the one scenario that discarded the work. errexit now comes off around the pipeline and back on afterwards. Verified under `bash --noprofile --norc -eo pipefail` across all three exits: 0 and 1 both reach the PR step with the status captured, and 2 still stops the run with exit 2. Also guarded the two command substitutions that assign a PR number. Under -e a transient API failure there would redden a run on main; treating it as "no PR" means the worst case is a duplicate that the next run's force push folds back together. Worth noting what this says about the earlier fixes in this PR. The exit-code contract, the --fail flag and the skipped plumbing were all correct and all dead, because the shell aborted before any of them ran. Nothing in the workflow file itself hinted at that -- it needed knowing what shell Actions supplies. Learned: Actions supplies `bash -eo pipefail`, and `set -uo pipefail` reads like it replaces those flags while only adding to them, so a deliberate non-zero exit dies before its status can be captured Constraint: disable errexit around any pipeline whose exit code is then inspected, because the step's default shell aborts on failure before the next line runs Ticket: DOC-6951 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/alias_check.yaml | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/.github/workflows/alias_check.yaml b/.github/workflows/alias_check.yaml index 6278bc8396..ea88b0df5a 100644 --- a/.github/workflows/alias_check.yaml +++ b/.github/workflows/alias_check.yaml @@ -83,8 +83,18 @@ jobs: # for those pages and the run would report nothing to do. Exit 2 means the # scan itself failed and is handled separately below, so a broken scan is # never reported as a content problem. + # + # errexit has to come off around the pipeline. Actions runs `run` blocks with + # `bash -eo pipefail`, and `set -uo pipefail` does not undo the -e, so a + # deliberate exit 1 would abort the step before PIPESTATUS is read -- leaving + # the `skipped` output unwritten, the next step unrun, and the aliases that + # *were* fixed discarded with no pull request. That made the whole + # refused-file mechanism unreachable exactly when it mattered. Verified + # against `bash --noprofile --norc -eo pipefail`. + set +e python3 build/check_missing_aliases.py --all --fix --fail 2>&1 | tee alias-report.txt status="${PIPESTATUS[0]}" + set -e echo "skipped=${status}" >> "$GITHUB_OUTPUT" if [ "${status}" -gt 1 ]; then echo "::error::check_missing_aliases failed with exit ${status}" @@ -110,7 +120,10 @@ jobs: --json number,isCrossRepository \ --jq '[.[] | select(.isCrossRepository == false) | .number] | first // empty' } - existing="$(find_bot_pr)" + # Tolerant of a transient API failure: an unreachable listing should not + # redden a run on main, and treating it as "no PR" means the worst case is a + # duplicate that the next run's force push folds back together. + existing="$(find_bot_pr || true)" # This job takes minutes, and someone can merge or close the fix PR inside # that window. Acting on the number we looked up at the start would then @@ -214,7 +227,7 @@ jobs: # the winner's body describes the same commit. if ! gh pr create --title "${PR_TITLE}" --body-file pr-body.md \ --head "${FIX_BRANCH}" --base main; then - raced="$(find_bot_pr)" + raced="$(find_bot_pr || true)" if [ -n "${raced}" ]; then # Tolerant for the same reason as every other call on a PR number here: # the PR can be merged or closed between finding it and editing it, and From 49025d2fb369962f3897d6d0b9778d002fd4d9e1 Mon Sep 17 00:00:00 2001 From: Andy Stark Date: Mon, 10 Aug 2026 13:55:17 +0100 Subject: [PATCH 9/9] DOC-6951 Make the fix PR's report readable, complete and self-consistent Three Bugbot findings, and two of them are consequences of my own earlier change in this PR rather than of the original design. Reordering --fix to run before the report moved the useful part of the output to the end: the run now opens with a line per alias written, which the diff already shows, and closes with the summary and the categories needing a human decision. The body was still keeping the first 40,000 characters, so on a large report it preserved the noise and cut off everything a reviewer actually needs. Demonstrated on a synthetic report: head -c kept neither the summary nor the collision section, tail -c keeps both. It now trims from the front and says so. The summary line still described the pre-fix state. Every detail line said "added to" while the headline above them said "missing an alias" -- and the headline is what anyone skims. It now reports aliases added and aliases that could not be added, which matches the lines beneath it in all three modes, with a test pinning that. The third is a real gap in the close logic. When the tree is clean because the fixer declined every remaining file, the step errored before reaching the stale-PR path, so an open bot PR whose edits had already landed stayed open. That is the stale-PR case the close logic exists for, whether or not other gaps remain. It now closes the PR -- with wording that says gaps remain rather than implying everything is resolved -- and then fails. Learned: reordering the fix ahead of the report changed which end of the output mattered, and the trim that had been correct became exactly backwards; a change of order is a change to everything downstream that assumed it Constraint: the fix PR body keeps the tail of the report, because --fix runs first and the summary and human-decision sections come last Ticket: DOC-6951 Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/alias_check.yaml | 27 +++++++++++++++++++++------ build/check_missing_aliases.py | 25 +++++++++++++++++++------ build/test_check_missing_aliases.py | 17 +++++++++++++++++ 3 files changed, 57 insertions(+), 12 deletions(-) diff --git a/.github/workflows/alias_check.yaml b/.github/workflows/alias_check.yaml index ea88b0df5a..4f9ac4f12b 100644 --- a/.github/workflows/alias_check.yaml +++ b/.github/workflows/alias_check.yaml @@ -139,9 +139,19 @@ jobs: if [ -z "$(git status --porcelain -- content)" ]; then if [ "${SKIPPED}" = "1" ]; then - # Nothing to open a PR with, yet the fixer declined some files. A - # red run is the only channel anyone would notice, and the report - # above names the files. + # Nothing to open a PR with, yet the fixer declined some files. Close + # any open bot PR first: its edits have already reached main, so its + # diff is spent whether or not other gaps remain, and leaving it open + # is the stale-PR case regardless. Then fail, because a red run is the + # only channel anyone would notice for the gaps that are left. + if still_open; then + echo "Closing PR #${existing}: its changes have landed, though gaps remain." + gh pr comment "${existing}" --body \ + "Closing automatically: the aliases in this PR have reached \`main\`, so its diff is spent. Some pages still need aliases added by hand -- see the failing \`alias_check\` run for which." \ + || echo "::warning::Could not comment on PR #${existing}." + gh pr close "${existing}" --delete-branch \ + || echo "::warning::Could not close PR #${existing}." + fi echo "::error::Missing aliases were found but could not be added automatically. See the report above." exit 1 fi @@ -183,10 +193,15 @@ jobs: # main on every run, so there is no history worth preserving on it. git push --quiet --force origin "${FIX_BRANCH}" + # Keep the *end* of the report, not the beginning. Since --fix runs before + # the report, the output starts with a line per alias written -- which the + # diff below already shows -- and ends with the summary and the categories + # that need a human decision. Trimming the head off keeps what a reviewer + # cannot get anywhere else. if [ "$(wc -c < alias-report.txt)" -gt "${BODY_REPORT_LIMIT}" ]; then - head -c "${BODY_REPORT_LIMIT}" alias-report.txt > report-for-body.txt - printf '\n\n[Trimmed at %s characters. The full report is in the workflow run log.]\n' \ - "${BODY_REPORT_LIMIT}" >> report-for-body.txt + printf '[Earlier output trimmed to fit; the full report is in the workflow run log.]\n\n' \ + > report-for-body.txt + tail -c "${BODY_REPORT_LIMIT}" alias-report.txt >> report-for-body.txt else cp alias-report.txt report-for-body.txt fi diff --git a/build/check_missing_aliases.py b/build/check_missing_aliases.py index 74fb181ea6..036e77ed1d 100644 --- a/build/check_missing_aliases.py +++ b/build/check_missing_aliases.py @@ -717,12 +717,25 @@ def report(moves: list[Move], github: bool, fix_hint: str, logger.info("check_missing_aliases: %d URL-changing move(s) found.", len(moves)) if moves: - logger.info(" %d already aliased, %d missing an alias, %d skipped " - "(old URL is a live page), %d skipped (target is a draft), " - "%d need a decision (page split), %d need a decision " - "(collision).", - len(aliased), len(missing), len(occupied), len(drafted), - len(splits), len(collisions)) + # The headline has to agree with the detail lines below it. After --fix the + # actionable moves are no longer missing anything -- they were just written -- + # so counting them as missing contradicts every line that says "added to". + if fixing: + declined_now = [m for m in missing if m.new_path in (skipped or set())] + logger.info(" %d already aliased, %d alias(es) added, %d could not be " + "added, %d skipped (old URL is a live page), %d skipped " + "(target is a draft), %d need a decision (page split), " + "%d need a decision (collision).", + len(aliased), len(missing) - len(declined_now), + len(declined_now), len(occupied), len(drafted), + len(splits), len(collisions)) + else: + logger.info(" %d already aliased, %d missing an alias, %d skipped " + "(old URL is a live page), %d skipped (target is a draft), " + "%d need a decision (page split), %d need a decision " + "(collision).", + len(aliased), len(missing), len(occupied), len(drafted), + len(splits), len(collisions)) if occupied: logger.info("Skipped -- old URL currently resolves, so must not redirect:") diff --git a/build/test_check_missing_aliases.py b/build/test_check_missing_aliases.py index 0c6e0fcf5e..6f401bb65e 100644 --- a/build/test_check_missing_aliases.py +++ b/build/test_check_missing_aliases.py @@ -163,6 +163,23 @@ def test_the_report_describes_what_the_fixer_actually_did(): assert "fix by hand" in declined +def test_the_summary_line_agrees_with_the_detail_lines(): + """The headline count is read far more often than the lines beneath it. + + Counting a move as "missing an alias" after --fix has just written it contradicts + every detail line saying "added to", and the headline is what someone skims. + """ + assert "missing an alias" in _capture_report() + + added = _capture_report(fixing=True) + assert "1 alias(es) added" in added + assert "missing an alias" not in added + + declined = _capture_report(fixing=True, skipped={"content/new.md"}) + assert "0 alias(es) added" in declined + assert "1 could not be added" in declined + + def test_a_whitespace_only_line_is_not_a_folded_continuation(): # The folded-scalar guard must not be tripped by trailing whitespace on the # line after a perfectly ordinary single-value scalar.