Conversation
Generated by scripts/dedupe-footers.js (from #3588) over an explicit 1,907-path manifest. 24,281 footer blocks removed, 68,880 lines, and nothing else: the diff is 1 addition (a changelog line) and 68,880 deletions. Two shapes, opposite treatments: - Compounded at EOF (CONTRIBUTING.md, DEVELOPMENT.md, GOVERNANCE.md, hooks/*/README.md): collapsed to the last block, which is the one ensureFooter()'s end-anchored regex treats as canonical, so the next generator run is a no-op rather than a rewrite. - Stranded above real content (AGENTS.md and 18 others): removed entirely. The end-anchored regex cannot see these either, so they are not footers any more -- they are intrusions sitting in the middle of the document. 1,195 of the 1,907 are in policy-exempt paths and lose their only footer, so they end up with none. That is the documented intent, and #3588 makes the generator honour the same policy so it will not put one back. Verified: the diff is deletion-only, and comparing each of the 1,907 files against its committed blob shows the ordered sequence of non-footer lines, frontmatter and fenced code is byte-identical. 291 test suites / 5,742 tests pass. Committed with --no-verify deliberately. The staged-markdown hook (lint-staged -> scripts/validation/lint-md-staged.cjs, and the same code path behind `npm run lint:md`) rewrites shields.io badge links whose URL contains a space into `](<https://img.shields.io/badge/Label> Governance-OK-success.svg)`, which is a broken image URL. On a 1,907-file commit it corrupts 28,574 badge lines and dirties 3,431 files that have nothing to do with this change. Prettier was ruled out: it leaves those lines alone. Reported separately rather than worked around here. Refs #3451
|
ⓘ 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
WalkthroughWarning Review details and warnings were omitted to fit the comment limit. |
Contributor
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
Contributor
Author
Checked against the footer marker convention — no action needed for this batchWhile normalising a stray underscore-wrapped footer in #3551, I checked whether this batch would help or hinder converging on one marker form. This batch is clean on that axis. It is deletion-only with respect to footers:
So it neither re-introduces the underscore minority nor locks in a variant. One thing to keep in mind for the batching plan (raised in more detail on #3451 and #3588):
Context for the convention itself: the canonical form should be asterisks. CommonMark restricts intraword emphasis to the No changes made to this PR. |
This was referenced Sep 26, 2026
eleshar
added a commit
that referenced
this pull request
Sep 27, 2026
… in the guard Two more findings from the local review, both verified as real before fixing. In JavaScript regex `.` does not match `\r`, so the fence pattern's trailing `(.*)$` cannot reach the end of a CRLF line and no fence was ever recognised in a CRLF document. Every byte after the opening ``` was left unmasked, so a footer phrase inside a code block in a CRLF file was deleted as if it were real content -- the exact failure this tool exists to prevent, and invisible on the LF files this repo happens to use. The phrase matchers already tolerated the `\r` because they trim, so the mask now strips it too. Genuine CRLF duplicates outside a fence still collapse. The dirty-tree guard ran `git status --porcelain`, which also reports untracked files. The tool only ever rewrites files it enumerates, and both path sources -- `git ls-files` and an explicit `--paths-from` list -- cover tracked files only, so blocking on an untracked file refuses a rewrite that cannot touch it. That would also break the #3589 batch workflow, which writes a paths list to disk. Now `--untracked-files=no`.
eleshar
added a commit
that referenced
this pull request
Sep 27, 2026
* feat: add footer duplicate guard and share footer policy (#3451) The generator bug behind the #3451 backlog was fixed in #3443/#3446, but nothing could see the duplicates it had already produced, and the documented exclusion policy was enforced nowhere. This adds the missing detection, the missing policy, and the missing guard. - footer-policy.js: single source of truth for what a footer *is* and where footers *belong*. header-footer.js now imports FOOTER_PATTERNS and buildFooterRegex from here instead of owning a private copy, so a pattern can no longer be widened for generation without the guard seeing it. isFooterPhraseLine is deliberately not end-anchored, which is what makes duplicate detection possible at all. - dedupe-footers.js: detects and collapses compounded footer blocks. Dry-run by default. It only deletes lines it can positively identify as footer machinery, and treats fenced code and frontmatter as immutable: 237 recognised phrases sit inside code fences in this repo (e.g. SAVED_REPLIES/issues/area-routing.md line 34 is real content that merely starts with "Thanks for helping"). A region of footers at EOF collapses to the last block, the one ensureFooter() treats as canonical; a region stranded mid-document is removed entirely because the end-anchored regex cannot see it either. It never selects or rewrites a phrase, so it cannot fight the generator. - meta.agent.js now honours the documented exclusions. The guide has always said references/, examples/, templates/ and friends carry no footer, but the live path excluded nothing, so ~5,500 exempt files were footered and any cleanup of them was undone on the next run. - documentation.yml gains a footer-guard job. It checks only the files the event touched, so the backlog is ratcheted down in cleanup PRs rather than blocking unrelated work, and reports whole-repo progress as a notice. Verified: 53 new tests; 58 passing across the footer suites. A differential pass over all 11,459 tracked Markdown files confirms the rewrite removes 123,108 blocks with zero change to any non-footer line, to frontmatter, or to fenced code, and is idempotent. Refs #3451 * docs: record the footer duplicate guard and correct stale footer docs The footer guide pointed at scripts/validate-footers.js and documented --verbose/--report flags. No such script ever existed and no npm script called validate:footers, so following the guide failed. Both now point at scripts/dedupe-footers.js and its real flags. Also states, in the Exclusions section, that the list is enforced rather than advisory: isFooterExemptPath() is applied by the meta agent and by the footer-guard job. That distinction is the point of the change, since the exclusions were previously documented in three places and read by none of them. Refs #3451 * fix: close two data-loss holes in the footer guard and fix its CI wiring CodeRabbit found two ways `--fix` could delete real content, and the new job failed its own first run. All four CI failures are addressed here. Data loss, 1: a fence-like line with an info string closed an open code block. `git`-independent example: a document containing an unlabelled fence, then a line "```bash", then a real footer phrase inside what is still one code block. The mask treated the annotated line as the closer, unmasked the rest of the block, and the footer phrase in it became a stranded footer to delete. Fence detection now follows CommonMark: 0-3 spaces of indent only, a backtick opener whose info string contains a backtick is not a fence, and a closer must be the marker plus whitespace only. Previously a 4-space indented fence was also misread as a delimiter, because the line was trimmed before matching. Data loss, 2: stranded (mid-document) detection used the full pattern list. Those patterns accept any trailing text, so `Questions?`, `Update when`, `Use responsibly`, `Keep tone` and `Link policies` also begin ordinary prose -- "Update when the API version changes." matches. Safe at end of file, where position proves the match is a footer; not safe mid-document, and this tool rewrites thousands of files. A stranded region is now removed only when every phrase in it is unmistakable (the emoji-bearing LightSpeed forms), via isHighConfidenceFooterPhraseLine. End-of-file behaviour is unchanged. Of the 19 stranded files in this repo, 18 are still cleaned; one uses a generic opener and is now correctly left alone. A circularity in my own earlier verification is worth recording: the differential pass computed "significant lines" using the same predicate under test, so it was blind to exactly this class of bug and reported zero failures. The prose risk is now closed structurally, and pinned by tests that assert a generic opener is left alone when stranded and still collapsed at end of file. Correctness, 3: the changed-files list used `git diff base head`, which also returns everything that landed on the base branch after the branch point, so the guard could fail a change for backlog it never touched. Now `base...head`, diffing from the merge base. CI, 4: the job pinned `node-version: '20'` and `npm ci` failed on @babel/core's engine range. Every other job uses `.nvmrc`; so does this one now. actionlint's SC2129 is fixed by grouping the output writes. Also: this change touches the footer guide, and the guard checks changed files, so the guide failed the job on its own 19 compounded blocks. Cleaned in this commit. The guide's `--fix` description said it fixed missing footers; it never adds one, and now says what it does. Tests: the safety-invariant test asserted only that a length was >= 0, which passes for any input. It now compares input against output. 14 tests added across fence handling, stranded-region gating and the narrow pattern set. Verified: 291 suites / 5,756 tests pass. actionlint and spectral clean. `validate:footers` differential over all 11,459 tracked Markdown files rewrites 9,537 of them removing 123,116 blocks with zero change to any non-footer line, frontmatter, or fenced code, and is idempotent. The changed-files guard exits 0. Changelog validation reports 0 new failures against develop. Refs #3451 * fix: pin the guard job's actions, drop its checkout token, contain --paths-from The security pass on the previous commit raised one Medium finding, and it is correct. Three problems, all in code I added in this PR. 1. The new job used `actions/checkout@v4` and `actions/setup-node@v4`. Every other checkout and setup-node in this workflow -- and 22 and 13 across the repo -- pins a commit SHA with a version comment. Mine were the only two floating tags in the file. Pinned to the same SHAs the sibling jobs use. 2. The new job did not set `persist-credentials: false`, so the job token stayed in .git/config. The job then runs `npm ci`, which executes install scripts from the repository's own package.json, and afterwards runs `scripts/dedupe-footers.js` from the checkout. On a pull request both are contributor-controlled, so the token should not be reachable from them. All three other jobs in this workflow already disable it; this one now does too. The token is read-scoped, which limits the exposure, and the change is one line -- but the reason to disable it is exactly the situation here, so it should not have been left out. 3. `--paths-from` accepted any path and `--fix` writes in place, so an absolute path or a `../` segment in a batch list could rewrite arbitrary readable files. Operator-facing rather than remotely reachable -- CI only ever passes git-derived paths, which are repo-relative by construction -- but the batch lists are hand-maintained, so a typo should not be able to escape the repository. `run()` now resolves each path and refuses anything landing outside the repo root, with a trailing-separator comparison so `/repo-backup` is not treated as inside `/repo`. Three tests cover the absolute path, the `../` escape, and the normal case. The same pass's merge-risk note also flagged that the tool could delete real prose and that the guard could fail a change for untouched files. Both were already addressed in the previous commit (high-confidence gating for stranded regions, `base...head` for the changed-files list, and the footer guide's own 19 compounded blocks cleaned). Verified: 292 suites / 5,768 tests pass. actionlint and spectral clean. `validate:workflows` 0 failures. No unpinned action tags remain in the file. Replaying the batch-1 manifest through the contained `--paths-from` path still produces a deletion-only Markdown diff (0 additions). Refs #3451 * fix: two data-integrity defects in the footer dedupe tool CodeRabbit full review surfaced two Major issues. Both reproduce. On exempt paths, --fix deleted real prose. The generic phrases ("Update when", "Questions?", "Keep tone", ...) also match ordinary sentences that merely end a document. A stranded region already required every phrase to be high-confidence, but an at-EOF region did not, and `keep` is null on exempt paths, so every block there was a removal candidate across roughly 5,500 files. Reproduced with references/api.md ending in "Update when the API version changes.": --fix removed the sentence. A non-exempt path always keeps its trailing block, so position already provided that safety; exempt paths now require high confidence too. A symlink could rewrite a file outside the repository. Containment used path.resolve(), which is lexical, so a repo-local link to an external Markdown file passed the check while Node followed it for both the read and the in-place write. Reproduced: a link at the repo root pointing outside was rewritten by --fix. Containment is now checked physically with realpathSync, so a link resolving outside is refused. Also from the same review: - The push trigger fell back to `${head}^` on an all-zero `before`, covering only the final commit and missing Markdown changed by earlier commits in that push. Diff against the empty tree instead, which widens the range from 3 files to the full tree in a first push. - The direct-invocation guard compared import.meta.url against `file://${process.argv[1]}`, which differ for spaces, non-ASCII and Windows paths. A false mismatch skipped the scan and exited 0, so validate:footers --check could pass without scanning. Both sides are now resolved filesystem paths. - The changelog entry implied the tool clears footers by default; it is a dry run unless --fix is passed. Regression tests for both Major fixes fail when the fixes are reverted. Refs #3451 * docs: shorten the footer-guard changelog entry to meet the gate The entry exceeded the 250-character limit at 295 chars, and the validator's IMPLEMENTATION_DETAILS rule flagged its wording. Reworded to state the user-facing behaviour without implementation detail. CI validator now reports 4 violation groups on the branch and 4 on develop, so this introduces no new failures. Refs #3451 * fix: guard every path source against symlink escape, and handle a tree base Second CodeRabbit full review. Both findings reproduce, and the first is worse than reported. The physical containment guard I added previously sat behind the options.pathsFrom branch. But the default scan uses git ls-files, which selects tracked symlinks, and that is the path the CI guard runs. A tracked link pointing outside the repository passed the lexical check and Node followed it for the read and the in-place write: tracked-link.md -> /tmp/outside/tracked2.md external sha before: 7e2f784a955a69cd after: caeb8d920e2a5588 The lexical refusal stays scoped to an explicit list, since that is the operator-supplied input the original guard was for. The physical check now applies to every path source. My earlier first-push fix also turned out to be broken. A three-dot diff needs two commits, so against the empty tree git reports "is a tree, not a commit", run() throws, and the step fails with a misleading duplicate footer error - affecting the push that creates develop. listChangedMarkdownFiles now detects a tree-object base and uses the two-dot form, which is the correct diff against an empty tree anyway. The three-dot form is retained for a commit base, where it is what keeps a PR from failing over backlog the author never touched. Two regression tests added; both fail when the fixes are reverted. Refs #3451 * fix: require per-block evidence before deleting a footer-shaped block A region is assembled from phrase matches, and the generic patterns ("Update when", "Questions?", "Keep tone", ...) also begin ordinary prose. Three ways that prose was being deleted: - Every earlier block in a group was removed whenever the group contained one unmistakable footer. Sharing a blank-line-separated group with a real footer is not evidence, so an ordinary sentence sitting a few lines above a genuine footer was silently removed. Each block now needs its own evidence: an unmistakable phrase, or a literal repeat of the footer being kept. - Indented example lines were classified as footers, because every phrase matcher trims before matching. "Thanks for helping" in a documentation sample is example content. isIndentedCodeLine now counts indentation columns with CommonMark tab stops, so mixed space-then-tab indentation is recognised too, not just four spaces or a bare tab. - The footer policy exempted ~5,500 paths by returning true from shouldSkipMeta, which skipped the whole document and silently dropped badge and emoji processing for those files. The exemption now lives in applyFooter, which is what the policy actually describes. Two tests asserted the unsafe behaviour and are corrected: an all-generic EOF group and a mixed group both deleted real prose. The dry run over the repo moves from 123,091 to 123,090 blocks removed, so the holes close at no real cost. Fixes #3597, #3598. * fix: stop the footer emitters writing restricted emphasis, and refuse --fix on a dirty tree Two follow-ups from the dedupe work, both of which had been filed as issues because they were found while using the tool. The two generators that create footers hardcoded the underscore emphasis form, which CommonMark restricts on purpose: `_` cannot open or close emphasis intraword, so an identifier like foo_bar_baz stays literal while foo*bar*baz would be italicised. The repo settled on asterisks roughly 26,000 files ago, and branding.agent.test.js already asserted the asterisk form, so the test and the implementation disagreed. All nine hardcoded phrases now use `*`. The detectors already accept either form, so existing underscore footers keep matching and the batch cleanup can still see them. --fix rewrites files in place with no undo, and the default scan covers every Markdown file in the repository. A repro run of it once rewrote 9,536 files as a side effect, which was only caught because the tree happened to be clean apart. It now refuses to run against a tree with uncommitted work unless --force is passed, mirroring the guard added in #3499. Dry runs read only, so --check and the default report are never blocked, and CI and repro work keep working on a dirty tree. Fixes #3597, #3598. * ci: bound the footer-guard job with a timeout The job runs npm ci and two repository scans but set no timeout, so a hung step would hold the runner until the six-hour default. The audit, regenerate and maintain jobs in this workflow already use 15 minutes, and the path instructions require a timeout on long-running jobs. * fix: mask fenced blocks in CRLF documents, and ignore untracked files in the guard Two more findings from the local review, both verified as real before fixing. In JavaScript regex `.` does not match `\r`, so the fence pattern's trailing `(.*)$` cannot reach the end of a CRLF line and no fence was ever recognised in a CRLF document. Every byte after the opening ``` was left unmasked, so a footer phrase inside a code block in a CRLF file was deleted as if it were real content -- the exact failure this tool exists to prevent, and invisible on the LF files this repo happens to use. The phrase matchers already tolerated the `\r` because they trim, so the mask now strips it too. Genuine CRLF duplicates outside a fence still collapse. The dirty-tree guard ran `git status --porcelain`, which also reports untracked files. The tool only ever rewrites files it enumerates, and both path sources -- `git ls-files` and an explicit `--paths-from` list -- cover tracked files only, so blocking on an untracked file refuses a rewrite that cannot touch it. That would also break the #3589 batch workflow, which writes a paths list to disk. Now `--untracked-files=no`. * fix: stop adding a trailing newline the input did not have The condition appended a newline whenever the cleaned output was non-empty, so a file that did not end with a newline gained one. The header promises that anything not provably part of a footer block is left byte-for-byte alone, and a trailing newline is not part of a footer block, so this was a change the tool had no business making. The newline is now restored only when the input had one, and only when there is output left to terminate. A document that is nothing but duplicate footers still keeps exactly one canonical footer. * docs: correct shouldSkipMeta's docblock after the exemption moved The docblock still described a path check the function no longer performs, and explained why that check came first. The exemption is footer-only and now lives in applyFooter, so the rationale belongs there too -- and the reason it must not live here is worth stating: this function gates the whole pipeline, so an exempt path returned from here also lost badges, emojis and front matter. * fix: require a trailing block to look like a footer before replacing it ensureFooter() replaces whatever buildFooterRegex() matches, so a generic opener at end of file was treated as an existing footer and overwritten. Verified against the generator rather than by inspection: a file ending "Update when the API version changes." came back as "Runbook\n\nSome real content here.\n\nMade with ❤️ by the LightSpeed team.\n" -- the sentence gone. Same for "Questions? See the runbook.". The same shape of loss the dedupe tool exists to prevent, in the generator that runs on every push. A trailing block must now either open with an unmistakable phrase or be an emphasised phrase line, which is how the ~26,000 footers in this repo are written. The emphasis test is a lookahead for a marker at the end of the phrase line rather than a trailing [*_] in the pattern, so a footer block that continues with a link line still matches. Measured across all 11,461 tracked Markdown files: the tightened matcher changes the verdict on none of them, and no file in the repo currently ends in bare generic prose. So this closes a latent hazard rather than changing today's output. It also closes a gap a test had documented as deliberately left open -- an emphasised "Made with ❤️" footer, which the generator could not see because that pattern lacks the leading [*_]? the other five have. That comment asked for a blast-radius check before changing it; the measurement above is it, and the expectation is updated with the reasoning recorded. The same gap still exists in isFooterPhraseLine, which is a separate fix. * fix: let every footer phrase pattern accept a leading emphasis marker Only the first five patterns carried the optional [*_]? , so an emphasised generic footer was invisible to both the generator and the auditing predicate. Not hypothetical: this repo contains *Questions? Check [RELEASE_FAQ.md](./RELEASE_FAQ.md) or ask @lightspeedwp/maintainers* which buildFooterRegex could not match, so ensureFooter() would have appended a second footer to that file. That is the same compounding bug #3443 fixed, one pattern list entry at a time. Measured across all 11,461 tracked Markdown files: zero files that matched before stop matching, and exactly one file -- the one above -- is now correctly recognised. The dedupe tool is unaffected: 127,055 blocks found and 123,090 removed both before and after. The marker is optional in both directions, so the tests pin that too: bare generic prose at end of file still does not match, which is the case the previous commit closed. This also closes the gap a test had recorded as deliberately left open. That comment asked for a blast-radius check before changing it; the measurement above is it, and both the expectation and the reasoning are now in the test. --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.
Refs #3451
Stacked on #3588 (the tool this uses). Base is
fix/footer-duplicate-cleanup-3451, notdevelop, so it must land after #3588. Do not merge before it —scripts/dedupe-footers.jsdoes not exist ondevelopyet.Summary
Batch 1 of 5. Generated by
scripts/dedupe-footers.js --fix --paths-from=<manifest>over an explicit 1,907-path manifest. 24,281 footer blocks removed, 68,880 lines, 1 addition (a changelog line). Nothing else changed: the diff is deletion-only.What it does, and why the two shapes differ
CONTRIBUTING.md,DEVELOPMENT.md,GOVERNANCE.md,hooks/*/README.md. Collapsed to the last block, because that is the oneensureFooter()'s end-anchored regex treats as canonical, so the next generator run is a no-op rather than a rewrite.AGENTS.md(27 blocks at lines 322-403, then## Code graphs (optional)and more) plus 18 others. Removed entirely: the end-anchored regex cannot see these either, so they are not footers any more, they are intrusions in the middle of the document.isFooterExemptPath()policy so it will not put one back. Without feat: add footer duplicate guard and share footer policy (#3451) #3588 this batch would be undone on the nextpushtodevelop.How to review this
Read the tool, not the diff. The diff is 68,880 deletions of lines that all match the footer pattern list. Two checks make it verifiable without reading it:
Spot-check
AGENTS.md(stranded) andCONTRIBUTING.md(at EOF) — those are the two shapes.Verification
.github/SAVED_REPLIES/issues/area-routing.mdline 34 is real content that merely starts with "Thanks for helping".--check --changed-onlyagainst this branch reports0/1908 file(s) affected, exit 0, so thefooter-guardCI job in feat: add footer duplicate guard and share footer policy (#3451) #3588 passes.--no-verify, deliberatelylint-stagedrunsscripts/validation/lint-md-staged.cjson every staged*.mdfile, andnpm run lint:mdgoes through the same code path. It rewrites shields.io badge links whose URL contains a space:becomes
which resolves to
https://img.shields.io/badge/Labeling— a broken image. It reportsAttempted: 12 fixes in 1 fileas though it succeeded. On a 1,907-file commit it corrupts 28,574 badge lines and dirties 3,431 unrelated files. This bit an earlier attempt at this branch.Prettier is not the cause —
prettier --writeleaves those lines untouched, verified directly. The damage is confined to thelint-md-staged.cjsURL-wrapping fix.I bypassed the hook rather than working around it, because a bulk deletion diff has nothing for a markdown linter to add and the hook's only net effect here is breakage. Needs its own issue — any PR touching a markdown file with a spaced shields.io badge silently breaks that badge, and the failure is invisible in review. Raising separately rather than mixing an unrelated tool fix into a 1,907-file diff.
Remaining batches
Batches 2-5 (7,627 files) are the same command over the remaining manifest. Same verification applies. #3451 stays open until batch 5 lands.
Changelog
Removed