DOC-6951 Detect missing aliases after merge and open a fix PR - #3770
DOC-6951 Detect missing aliases after merge and open a fix PR#3770andy-stark-redis wants to merge 4 commits into
Conversation
🧠 Redis MemoryFound 5 related items from repository history: Memory updated at 4c51436 |
🧠 Redis MemoryFound 5 related items from repository history: Memory updated at 4fda01f |
🧠 Redis MemoryFound 5 related items from repository history:
Memory updated at 9bd76a0 |
| # 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 |
There was a problem hiding this comment.
Exit code one misread as skips
Medium Severity
The workflow misinterprets check_missing_aliases.py's exit code 1. If git log fails early in the script or the content/ tree is clean, the workflow incorrectly reports that aliases were found but couldn't be added automatically, leading to a misleading error message about skipped files.
Reviewed by Cursor Bugbot for commit 9bd76a0. Configure here.
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
9bd76a0 to
4b5b62a
Compare
🧠 Redis MemoryFound 5 related items from repository history:
Memory updated at 4b5b62a |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4b5b62a. Configure here.
| echo '```' | ||
| echo | ||
| echo '</details>' | ||
| } > pr-body.md |
There was a problem hiding this comment.
PR body stale after fix
Low Severity
The fix PR description is built from alias-report.txt, which is the full stdout of a single --all --fix run. The scanner logs “moved with no alias” warnings before it applies fixes, so the embedded report still lists URLs as missing even when the same run added those aliases and the PR diff already contains them.
Reviewed by Cursor Bugbot for commit 4b5b62a. Configure here.
|
|
||
| # `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)" |
There was a problem hiding this comment.
PR lookup ignores head owner
Medium Severity
gh pr list --head matches any open pull request whose head branch name equals auto/missing-aliases, including forks, and head -1 picks one arbitrarily. The workflow then comments, closes, edits, or skips creating the upstream bot PR based on that number without checking that the head repository is this repo.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 4b5b62a. Configure here.


Automates running the alias scanner — item C of DOC-6951. One new workflow file, nothing else.
Post-merge, not a PR check
Deliberately nothing appears on anyone's PR — no check, no annotation, no comment. It runs on push to
maininstead.A PR-time check has to pick between being ignorable and being intrusive, and neither pays here: the fix is always the same mechanical edit, so author intent almost never matters — and the cases where it might, such as a page retired rather than moved, are exactly the ones the scanner already refuses to touch.
Running per-push costs ~3s and needs no Hugo build, so the window where an old URL 404s is minutes rather than the days a scheduled-only sweep would imply.
mainworkflow_dispatchThe PR it opens
Always means one thing: current main plus every alias currently missing. The bot branch is regenerated from
mainand force-pushed each run, never extended, so repeated runs are idempotent and a stale half-fix can't accumulate.Cases needing judgement are reported in the PR body rather than changed — an old URL that's a live page, a URL another page already claims, or a move onto a draft. Closing the PR is a fine answer when a page was retired on purpose; the scanner will simply propose it again if the dead URL is still reachable, and nothing depends on it.
fetch-depth: 0is load-bearingVerified against a
--depth 1clone: the scanner finds no rename records, reports zero moves and exits 0. A shallow checkout would give a permanent green tick that never examines anything — the worst failure mode for a check like this.No other workflow in the repo sets it, and
repo-memory.ymlhas already been caught by the same default once, needingfetch-depth: 2.Two deviations from the house pattern, both deliberate
gh pr list --headinstead of thegh search prsidiom the seven sync workflows use. Search goes through an eventually-consistent index and can miss a PR opened moments earlier, which would produce duplicates.--headis an exact lookup.--fail. The PR is the signal; a red tick onmainwould be noise for something no build depends on.Review round (
4fda01f09)Five Bugbot findings, all valid. One was a certainty, not a risk: the PR body embedded the whole scanner report, and a full report is 117,912 characters against GitHub's 65,536 limit — so
gh pr createwould have failed on the very first run, after force-pushing the branch. The report is now trimmed to 40,000 chars with a note, putting the body near 41,000; the untrimmed version stays in the run log.concurrencygroup, cancelling in-flight older runsref: mainregardless of trigger--fail, capture status viaPIPESTATUS, error when the fixer declined something with nothing to reviewThat last one is the same defect Bugbot found in the scanner earlier and I'd only half-fixed: I made a refused file visible in the exit code, but only under
--fail, which this workflow wasn't passing.Second review round (
9bd76a07a)Two further findings, both about the bot PR drifting from what it claims:
gh pr editon both the already-open and lost-the-race paths, so the body always describes the commit beneath itThe second matters because the diff becomes redundant, not wrong: if the aliases reach
mainsome other way, a stale bot PR sits there inviting someone to merge it weeks later.All five control paths of the final step were exercised as shell — clean/no PR, clean/with PR, changes/no PR, changes/with PR, and the refused-file error.
Third review round (
b72cfc125on #3767,4b5b62acbhere)>1as a hard errorThe second was a TOCTOU: the PR was looked up at the top of the step and acted on minutes later. Closing an already-merged PR is an error, so
set -ewould have reddened a run onmainfor something harmless. Verified as shell across all four states — open, merged, closed, absent — confirming the fall-through routes into the no-PR path rather than aborting.Verification
Dry-run in a fresh full clone: the scan plus the exact git sequence produces a commit on the fix branch matching #3769. YAML parses, permissions are
contents: write+pull-requests: writescoped to the single job with acontents: readdefault. The exit-code contract the workflow now relies on was checked directly —--fix --failexits 0 with nothing skipped and 1 when something is,--failalone exits 1 while gaps remain — and both branches of the new shell gate were exercised.Not verifiable until it's on
main: the trigger, theconcurrencybehaviour, andGITHUB_TOKENhaving enough scope to push the branch and open the PR. Also unexercised: the scanner's own skipped-file exit path, since no file in this corpus is refused any more — that half is reasoned from the code, not observed. Worth watching the first run.🤖 Generated with Claude Code