fix: repair a malformed title prefix instead of doubling it - #3580
Merged
Merged
Conversation
isAlreadyPrefixed() correctly requires whitespace after the colon, but normalizeTitle() handled that rejection by prepending the prefix again, so a malformed title was mutated into a worse one: decision:Adopt GraphQL -> decision: decision:Adopt GraphQL Recognise a leading prefix independently of its spacing, insert the missing space, and keep prepending only when the title carries no recognised prefix. The author's capitalisation is preserved and a repaired title is stable, so a second pass is a no-op. The recognised prefix list moves into one constant that both regexes are built from, so the spacing check and the leading-prefix check cannot drift apart again. Applied to the .js twin as well; the two files stay byte-identical.
Contributor
|
Warning Review limit reachedNext included review available in 12 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 (4)
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 |
PR Summary by QodoRepair malformed title prefixes without duplication
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Contributor
PR Template RoutingBranch Type: This PR was automatically routed based on the branch naming strategy. |
Contributor
📋 Changelog Quality Validation
Status✅ Validation PASSED - No new failures introduced by this PR. No action required. |
Resolves the CHANGELOG.md conflict in Unreleased > Fixed: both entries kept, newest first.
8 of 10 tasks
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.
Linked issues
Fixes #3578
Found by Qodo review of #3573.
Context
scripts/automation/normalize-issue-pr-titles.cjsand.js. The doubling behaviour predates fix: correct stale issue-type inference and CommonJS title normalisation #3568 and was asserted by an existing test.Reproduction
normalizeTitle("decision:Adopt GraphQL", "decision").decision: Adopt GraphQL. Actualdecision: decision:Adopt GraphQL.Root Cause
isAlreadyPrefixed()correctly requires whitespace after the colon — the Conventional Commits spec makes the terminal colon-and-space required, and both implementations enforce it. ButnormalizeTitle()treated that rejection as "no prefix present" and prepended one, doubling it. The ecosystem norm is to detect a malformed header rather than silently rewrite it (commitlint fails withtype-enuminstead of editing), anddocs/ISSUE_PR_TITLE_GOVERNANCE.md§7 establishes enforcement by validator as this repo's pattern.Fix Summary
DECISION:Adopt GraphQL→DECISION: Adopt GraphQL..jstwin; the two files remain byte-identical.Verification
Tests added/updated to cover the bug
Manual verification steps — N/A, no UI; verified by executing both implementations
Negative/edge cases checked
Updated the two assertions that encoded the doubling (
fix:Add something, barefeat:/fix:), and added three parity cases: repair in both implementations, never a doubled prefix across five prefixes with a second-pass no-op check, and capitalisation preservation.Suite grew 96 → 98 tests, all passing. Full suite 282 suites / 5638 tests passed.
diffconfirms the.cjsand.jsfiles are byte-identical.semgrep0 findings,markdownlint0 issues,eslint0 errors.Changelog gate: 10 failing entries with and without the new entry — 0 new failures.
Risk & Rollback
feat:→feat:(one prefix, now stable);feature: Some titlestill gains a prefix, becausefeatureis not a recognised prefix;HTTP: protocolstill gains one; a colon mid-title is untouched.Changelog