chore: remove compounded footer blocks, batch 43 of 45 (#3451) - #3662
Merged
Merged
Conversation
Refs #3451 Batch 43 of 45. Pure deletion: 90 files, 0 added. node scripts/dedupe-footers.js --fix --paths-from=small-43.txt The scope split comes from the code, not per-file judgement: isFooterExemptPath() in scripts/agents/includes/footer-policy.js with the keep decision in analyseContent(). 6 of the 90 files are footer-exempt and lost their footer entirely, which is the policy docs/QUIRKY_FOOTERS_GUIDE.md states; the other 84 were collapsed to exactly one clean trailing footer, the last block, which is what ensureFooter() treats as canonical. Covers `.github`. Manifests were derived once before any of this ran and are fixed: batch N is small-NN.txt. Cross-batch overlap is 0, so these pull requests cannot conflict with each other. Verification: - re-running --check over the manifest reports 0/90 affected - independent differential pass, negative-tested: pure order-preserving deletion, every deleted line classified as footer structure, none from frontmatter or a fenced code block, and the classified counts sum to exactly the numstat deletion total - staged paths match the manifest exactly, 0 outside No changelog entry: Markdown-only diff, exempt as docs-only by the gate in changelog-unified.yml. The pre-commit hook is bypassed for this commit, as in every commit in this series. It runs npx lint-staged, dependencies are not installed in this worktree, and #3590 records that the Markdown hook corrupts shields.io badge links. CI does not run lint:md, so this does not diverge from what CI checks.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Contributor
|
Warning Review limit reachedNext included review available in 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: lightspeedwp/.github/.coderabbit.yml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (90)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
No description provided. |
Contributor
PR Template RoutingBranch Type: This PR was automatically routed based on the branch naming strategy. |
This was referenced Sep 29, 2026
eleshar
pushed a commit
that referenced
this pull request
Sep 30, 2026
…essage Three defects in the keep-current path, all of the same shape: state or a guess that does not belong where it was. The log array was declared outside the loop over open pull requests, so each conflict comment carried every earlier pull request's API response as well — publishing one pull request's status on another. The per-pull-request work now lives in processPullRequest behind an injected client, which owns its own log lines, and the workflow is a thin loop. Reintroducing a shared array makes two tests fail, so the defect is covered rather than removed. update-branch answers 422 for three unrelated situations. All were observed here against a deliberately wrong expected_head_sha: 202 Updating pull request branch. 422 There are no new commits on the base branch. 422 merge conflict between base and head 422 head ref does not exist 404 Not Found The third is the text Mergify reported on #3580 and #3662 seconds after each merged; read as a conflict it would have commented on a merged pull request telling a human to resolve a conflict that does not exist. Unknown messages now classify as an error rather than as a conflict, and a throttled call is retryable, because GitHub documents 422 as "Validation failed, or the endpoint has been spammed". Anything that is not a clean update, a no-op or a conflict is raised as a warning and listed in the job summary rather than logged quietly. expected_head_sha is documented as the guard against a head that moved, but it was not enforced when probed: a deliberately wrong value still returned 202 and updated the branch. The comment claimed otherwise and now says what was actually observed, so it reads as defence in depth. Comment lookup is paginated, so a pull request with more than a page of earlier comments still finds its own and updates it instead of duplicating.
eleshar
added a commit
that referenced
this pull request
Sep 30, 2026
* ci(mergify): report update conflicts without a failing check The Mergify `update` rule reported conclusion `failure` whenever it could not merge develop into a pull request branch, and the action's schema accepts only `bot_account`, so nothing in the rule could change that. Observed across the 394 pull requests opened between 2026-09-03 and 2026-09-29: four failures, split between a genuine conflict (#3524, #3532) and an update attempted on a merged pull request whose branch had already been deleted (#3580, #3662). Merging develop into open branches is now done by a workflow using GitHub's update-branch API. A conflict is written as a comment naming mergeable_state instead of being reported as a red check, and the job reports success on every outcome. Same exclusions as the rule it replaces: non-draft, same-repository, non-archived, base develop, any staleness. The up-to-date requirement is unchanged: develop-branch-ruleset still sets strict_required_status_checks_policy, so a branch behind develop that cannot be updated is still blocked from merging. The workflow is not a required check and no ruleset setting was changed. Closes #3574 * ci(mergify): validate the config against Mergify's published schema `npm run validate:mergify` checks .github/mergify.yml against https://docs.mergify.com/mergify-configuration-schema.json and names the offending path on failure. The schema is a 176 kB download, so it is fetched rather than vendored; the test asserts the same thing offline only when a fixture is present. The suite also probes the schema for any option that would change how a failed update is reported. Today `actions.update` rejects every property but `bot_account`, which is the evidence for removing the rule rather than reconfiguring it, and the test to revisit if that ever changes. * ci(mergify): note why the relative require path is correct actions/github-script runs the script with the workspace as its working directory, which is why the relative specifier resolves. Recorded so the next reader does not 'fix' it to an absolute path and break it. * fix(mergify): separate a real conflict from an already-current pull request GitHub answers 422 for both, so status alone cannot classify the result. Observed against this repository on 2026-09-30: {"message": "merge conflict between base and head", "status": "422"} {"message": "There are no new commits on the base branch.", "status": "422"} The second is what most open pull requests return on a push to develop, because most of them are not behind. Reading every 422 as a conflict would post a false conflict comment on every open pull request each time develop is pushed to, so the message is now part of the classification and only a genuine conflict produces a comment. Also captures the API's own message rather than Octokit's wrapper text, since that message is what the classification depends on. * docs(changelog): keep the new entry within the 250 character limit The pr_submission validator reports CHK_MAX_LENGTH as a critical rule, so the entry was adding one new failure against the base branch. Now 10 failures on both base and this branch. * fix(mergify): scope conflict state per pull request and classify by message Three defects in the keep-current path, all of the same shape: state or a guess that does not belong where it was. The log array was declared outside the loop over open pull requests, so each conflict comment carried every earlier pull request's API response as well — publishing one pull request's status on another. The per-pull-request work now lives in processPullRequest behind an injected client, which owns its own log lines, and the workflow is a thin loop. Reintroducing a shared array makes two tests fail, so the defect is covered rather than removed. update-branch answers 422 for three unrelated situations. All were observed here against a deliberately wrong expected_head_sha: 202 Updating pull request branch. 422 There are no new commits on the base branch. 422 merge conflict between base and head 422 head ref does not exist 404 Not Found The third is the text Mergify reported on #3580 and #3662 seconds after each merged; read as a conflict it would have commented on a merged pull request telling a human to resolve a conflict that does not exist. Unknown messages now classify as an error rather than as a conflict, and a throttled call is retryable, because GitHub documents 422 as "Validation failed, or the endpoint has been spammed". Anything that is not a clean update, a no-op or a conflict is raised as a warning and listed in the job summary rather than logged quietly. expected_head_sha is documented as the guard against a head that moved, but it was not enforced when probed: a deliberately wrong value still returned 202 and updated the branch. The comment claimed otherwise and now says what was actually observed, so it reads as defence in depth. Comment lookup is paginated, so a pull request with more than a page of earlier comments still finds its own and updates it instead of duplicating. * docs(validation): correct the stale validate:mergify header The header claimed the script was "not wired into npm run validate:*", which stopped being true when package.json gained the validate:mergify script in this series. It now states what is actually true: it is wired as npm run validate:mergify, it is deliberately not one of the ten steps in validate:all, and no workflow runs it, because it downloads the schema and every validate:all step works offline. * test(mergify): cover the update-branch responses actually observed Replaces the two classifier assertions that rested on unverified messages: - "head branch was modified" for a stale expected_head_sha was never observed on this repository; a deliberately wrong value returned 202 and updated the branch. The test now pins the three 422 texts that were observed, so an unverified guess cannot drift back in. - 403 asserted "gone", which merges two different problems: a token that cannot write the branch, and a throttled call. It is now "denied", with rate-limit text and 429 classified as retryable first. A 422 whose message is none of the three is an error, not a conflict, so an unfamiliar rejection is surfaced rather than posted on a pull request that may have nothing wrong with it. Also asserts the workflow keeps no state outside the per-pull-request loop and delegates the API call to the module instead of duplicating it. * fix(mergify): update branches with the bot App token, not GITHUB_TOKEN A branch update is a push. A push made with the workflow's own GITHUB_TOKEN leaves the new head's pull_request runs unusable, so a pull request this workflow updates would sit BLOCKED with its required checks missing. Measured on a scratch same-repository draft pull request on 2026-09-30, over two separate updates. Before 039ac1b -> 1489e93 and 1489e93 -> 30105ad, each new head had ten pull_request workflow runs, every one of them action_required, and therefore no check runs at all: no "Route PR template and apply labels", no "Validate changelog on PR", no actionlint. That is documented behaviour, not a fault. From https://docs.github.com/en/actions/concepts/security/github_token : "pull_request events with the opened, synchronize, or reopened activity types: when a workflow using GITHUB_TOKEN creates or updates a pull request, the resulting pull_request event creates workflow runs in an approval-required state. The pull request displays a banner in the merge box, and a user with write access to the repository can start the runs by selecting Approve workflows to run." The merge commit is attributed to github-actions[bot], which is what makes the push a GITHUB_TOKEN push. Fixed with the same GitHub App documentation.yml already uses for exactly this reason -- its comment reads "App token (not GITHUB_TOKEN) so the regeneration PR triggers CI and can satisfy develop's required checks" -- with the same two existing repository secrets, BOT_PR_APP_CLIENT_ID and BOT_PR_APP_PRIVATE_KEY. No new secret and no new App permission is required: updateBranch needs contents:write, and upserting a comment needs "Pull requests: write" or "Issues: write" (https://docs.github.com/en/rest/issues/comments), both of which the App already holds. permission-workflows stays absent, as in documentation.yml. The job's own token drops to contents:read so a checkout cannot reach a write-scoped token. The token step is skipped for forks, where repository secrets are not exposed; those pull requests are skipped before any write. * test(mergify): make the update and token assertions mutation-sensitive Three assertions in these files passed on a broken implementation. The upsert test asserted that comment_id was undefined, which is exactly what a call that dropped it would produce, and its fixture comment had no id at all so the two were indistinguishable. Dropping comment_id from the updateComment call left 11 of 11 tests passing. The fixture now carries a real id and the assertion checks for it: the same mutation now fails 1 of 11. The token assertions matched the workflow as text, so the header comment -- which mentions updateBranch, permission-workflows and contents: write while explaining why they are absent -- could satisfy or break them. They now parse the workflow and assert on the parsed step: the token inputs, the permissions granted and not granted, the github-token expression, the fork guard, and that the script body does not call the API itself. Verified by mutation, each reverting one of the three changes above: no app-token step -> 3 failed permission-workflows granted -> 1 failed job token contents: write -> 1 failed * fix(mergify): isolate a comment-write failure to its own pull request paginate, createComment and updateComment were unguarded. The caller loops over every open pull request in one run, so a rejection on the comment path -- a transient 5xx, or a secondary rate limit on the shared token -- escaped processPullRequest, stopped the remaining pull requests, and failed the job, which contradicts the never-red contract this workflow is built on. Reproduced first: with the comment API throwing for pull request 111 while 112 was still to be processed, five tests failed, including the one asserting that the second pull request is still reached. Now 16/16 pass. The conflict itself is still reported. Only the comment is missed, and the miss is visible three ways: a warning annotation, an outcome label in the job summary that reads "conflict (comment not written, HTTP 503)", and commentFailed/commentStatus on the returned result. pulls.get is now split by status. A 404 means the pull request was merged or deleted and is logged as gone; anything else is transient, so it warns and returns retry instead of the previous "unreadable" that logged quietly through info and looked like a clean pass. The workflow's own loop now catches anything that still throws, so a single pull request cannot stop the others even if a future edit leaves a path unguarded. The pulls.list enumeration is guarded for the same reason: it is the one call that decides how many pull requests there are. Also tightens the existing-comment lookup, which matched on body.includes(marker). Other bot comments here are type: Bot and mention this workflow by name -- the Linear review comment carries the branch and workflow names -- so a substring match could adopt and rewrite one of them. Now anchored to the start of the body. Two reviewers have raised this across rounds; the earlier deferral is now closed. * fix(mergify): keep a missing App secret or a failed listing off the red path Four findings from `coderabbit review --agent` on the pending diff, all valid. The App token step gains continue-on-error. Without it, absent or invalid BOT_PR_APP_* secrets fail the job on every run; with it the token is empty, github-token falls back to the read-only GITHUB_TOKEN, the run updates nothing and says so. The pulls.list enumeration is wrapped. It is the one call that decides how many pull requests there are, so leaving it to throw failed the whole job; it now warns and proceeds with an empty list. fetchSchema is bounded. https.get sets no timeout of its own, so a stalled connection left the promise pending forever and the validator hung instead of reporting a failure. The request is destroyed at 30s, which rejects the promise. A periodic `schedule` was considered as a backstop for the gap a `synchronize` push leaves -- a pull request can sit behind develop after its author resolves a conflict -- and rejected on evidence, recorded in the workflow header. With 15 open pull requests, */15 is 96 runs a day and ~1,440 update-branch calls, almost all wasted on the "no new commits" 422. The cost that matters is that develop-branch-ruleset sets dismiss_stale_reviews_on_push: every update moves the head and dismisses the approval, so a fixed cadence would repeatedly void approvals given against an exact head. Scheduled workflows also run only from the default branch and are disabled after 60 days of inactivity, making them the least reliable trigger available. workflow_dispatch is the backstop instead. * docs(mergify): correct the synchronize loop claim An already-current result makes no further push, so a synchronize-triggered run would terminate rather than loop. The reason for excluding it stands -- each update is itself a push and would double the runs for no gain -- but the loop claim was wrong and is corrected, with the de-confliction gap named instead. Raised in review. --------- Co-authored-by: Chris <support@lightspeedwp.agency>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Compounded footer blocks removed — batch 43 of 45
Refs #3451 — not
Closes. The issue stays open until all 45 batches land and the whole-repo run reports zero.Linked issues
Relates to #3451— repo-wide footer duplicate cleanup. 2 batches remain after this one.Why 90 files and not 1,950
CodeRabbit's server-side review only runs under 100 changed files. Every batch of the earlier plan was skipped with
Review skipped: … files exceed the limit of 100, so their review was a local slice rather than a review of the pull request. Batches are capped at 90, leaving headroom forCHANGELOG.mdand keeping the largest pull request at 91 files. The cost is explicit: 45 pull requests.Context
Reproduction
Scoped to this batch's manifest, so the check reports only what this pull request changed:
Before the fix:
90/90 file(s) affected · blocks found 776 · exempt-path files 6, exit 1.After the fix:
0/90 file(s) affected, exit 0.Root Cause
Two defects, both fixed in merged tooling:
header-footer.jsheldFOOTER_PATTERNSprivately, so the end-anchored matcher inensureFooter()could not see an asterisk-wrapped footer. Every automation run appended another copy rather than replacing the existing one, compounding the stack. Fixed by fix: match asterisk-wrapped footers in dedup regex #3443 and feat: add footer duplicate guard and share footer policy (#3451) #3588.docs/QUIRKY_FOOTERS_GUIDE.mdwas documented and mirrored into two config files, but no live code read either. Fixed by feat: add footer duplicate guard and share footer policy (#3451) #3588.Fix Summary
Deletion only, produced by the committed tool over this batch's fixed manifest:
The split is decided by
isFooterExemptPath()inscripts/agents/includes/footer-policy.jsand thekeepdecision inanalyseContent(), not by a judgement call per file. Batch 43 covers.github.Verification
--checkover the manifest after the fix reports0/90 file(s) affected, exit 0 — idempotent.numstatdeletion total, a second independent count of the same number. The checker is negative-tested — an injected content line and a deletion from inside a fence both make it fail.CodeRabbit
Recorded in the running report for this batch: whether the review actually ran on this pull request, or was rate-limited and therefore did not run. A green CodeRabbit tick here means "reviewed" only if that report says so — the check also passes when it declines.
Changelog
None. This is a Markdown-only diff, which the
changelog-unified.ymlgate exempts as docs-only. Theother areas/area-level entry lands on the last pull request for that area.Risk & Rollback
git revertthis commit. The revert is itself a pure insertion.npx lint-staged, dependencies are not installed in the worktree used for this work, and fix: lint:md and lint-staged corrupt shields.io badge links whose URL contains a space #3590 records that the Markdown hook corrupts shields.io badge links. CI does not runlint:md.