fix: test - stop tests writing into the repository and guard against it (#3498) - #3499
Merged
Merged
Conversation
) - phase-2b-validation rewrote the tracked results-phase-2b.json; metrics-collection-orchestrator wrote collection summaries and its config into the repo; metricsCollection wrote snapshots to .github/reports/changelog-metrics (it never passed its temp dir on) and CSVs into tests/fixtures. All now write under os.tmpdir(). - collectMetricsSnapshot accepts an optional metricsDir (default unchanged), so the test can redirect it. - The orchestrator save-summary test asserted only if the file existed; it now always asserts. - Remove two committed CSV test outputs nothing reads. - Jest globalSetup/globalTeardown compare git status before and after the run and fail on any change (ALLOW_TEST_ARTEFACTS=1 bypasses).
…3498) Code review follow-up: with -z, git status emits a rename as two fields, and the second was treated as an entry; skip it. A missing globalSetup snapshot now warns instead of passing silently, distinct from running outside a Git work tree. Adds tests for the guard.
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: lightspeedwp/.github/.coderabbit.yml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughJest now runs a working-tree guard around the test run. Metrics and performance test output use temporary directories, and the changelog metrics collector accepts an optional output directory. ChangesJest working-tree guard
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Jest
participant SetupScript
participant WorkingTreeGuard
participant Git
participant TestSuites
participant TeardownScript
Jest->>SetupScript: run global setup
SetupScript->>WorkingTreeGuard: call setup
WorkingTreeGuard->>Git: read status and hash listed paths
Jest->>TestSuites: run tests
Jest->>TeardownScript: run global teardown
TeardownScript->>WorkingTreeGuard: call teardown
WorkingTreeGuard->>Git: read status and hash listed paths
WorkingTreeGuard-->>Jest: throw if entries changed
Merge Risk: 🔵 Low · up to Test runs now write their output to temporary folders and fail when a test changes repository files. The new check can still miss some changes: a Git error disables it silently, and it does not report deleted files. Separately, metrics collection can report success even when the snapshot was not saved. These gaps affect test-infrastructure accuracy rather than production behavior, so the PR is mergeable with small follow-up fixes. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes chiefly isolate test output. The new working-tree check has gaps that can leave repository changes undetected, but the identified exposure is limited to local and CI test runs. No production attack path was established. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 68.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 9 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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
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. |
github-actions
Bot
added
status:needs-review
priority:normal
type:chore
meta:needs-changelog
and removed
type:chore
area:testing
labels
16 tasks
eleshar
added a commit
that referenced
this pull request
Sep 26, 2026
…#3500) The pass-2 section claimed the remaining findings had no tracker and that the loop simply stopped. That was wrong: #3519 already exists to hold spec 016 and 017 design findings from this very pull request, and it already listed most of them. Mapped the ten findings against the open trackers. Five were duplicates of existing #3519 items, re-raised against the corrected prose: inconclusive classifications, category verdicts in the comment template, the wrong comparison branch, losing the file type when linting, and posting the comment without re-verification. The generated performance fixture is already covered by #3498 and PR #3499. Four were genuinely new and are now appended to #3519 as checklist items, taking it from 13 to 17: excluding CHANGELOG.md from the docs-only bypass, limiting automatic labelling to non-exemption labels, running one validator on both branches, and testing the shipped workflow rather than a local simulation.
eleshar
added a commit
that referenced
this pull request
Sep 26, 2026
…ation specs (#3500) * docs(specs): land CI remediation, changelog agent and agent consolidation specs (#3465) Specs only, so the team can start per-agent spec work from develop: - 017-ci-failure-remediation from audit/governance-audit-implementation, renumbered from 015 (015 is pr-agent-consolidation, 016 is taken); the quoted original request keeps its 015. - 016-changelog-agent-quality and the 014-agents-restructure-consolidate tasks.md and registry-schema.json updates from #3434, unchanged. - CATALOG.md rows for 014-017. * docs(specs): sync 014 and 016 with the agreed #3434 head (b29c26b) #3500 had copied an older revision. Picks up the #3470 decisions: the REST changelog_path/changelog_content table, merged-only PR links, the changelog-unified.yml workflow name and the corrected checklist. * docs(specs): fix five consistency findings in specs 016 and 017 CodeRabbit full review of #3500, tracked in #3519: - 016 checklist: "no implementation details" was both failed and passed, and the summary said all items pass. Recorded it as an accepted exception. - 016 check-links: an unmerged link fails only with --strict, matching the default and the JSON example; the exit-code table now says so. - 016 quickstart: the closing frontmatter delimiter must be a whole line (rejects ---oops; accepts CRLF and end of file). - 016 locking: stale-lock recovery is an atomic compare-and-remove (rename to a per-contender tombstone, verify inode and token, restore with link() if a live lock was moved) plus a token fence before every write. A path-based unlink after re-stat could delete a new owner's lock. - 017 checklist: only User Story 1 was verified; Stories 2-4 are marked incomplete, as the spec's resolution note says. The 016 changes are applied identically to #3434. * docs(specs): address the second CodeRabbit review on #3500 - 014 registry schema: generatedSkill uses the documented model fields (location, agentskills_compliant, compliance_violations, used_by). - 014 tasks: US4 (T058-T069) unchecked; none of its deliverables exist. - 016 CLI contract: document the validator's real JSON error payloads and its full --help output. - 016 data model: lifecycle branches after validation; sample character_count is 86. - 016 quickstart: required-field check uses jq -e with type checks; the documentation check exits non-zero when a file is missing. - 016 spec: validation gate lives in changelog-unified.yml. - 017 checklist: replace the stale /speckit-plan next step. - Catalog: move resolved 017 to a Resolved section. Validator schema enforcement: #3522. Other 014 tasks: #3523. * docs(specs): address the third CodeRabbit review on #3500 - 014 schema: version pattern is full SemVer 2.0.0 (pre-release, build). - 016 data model: ValidationResult records skill_id with skill_version. - 016 quickstart: assert the four planned skill directories; sample output uses their real names. - 016 plan: list the shipped changelog-unified.yml workflow. - 016 spec: Scenario 4 invokes skills through the npm CLI (Phase 1). - 017 data model: terminal state includes GOVERNANCE WORKFLOW. - 017 plan: name the real branch, audit/governance-audit-implementation (#3367); 017-ci-failure-remediation never existed. - Catalog: next-number steps no longer say 013/014. * docs(specs): use the computed number in the catalogue's create-directory step * docs(specs): address the fourth CodeRabbit review on #3500 - 014 schema: the three registry variants exclude each other's defining fields (agents, schema, category), so oneOf matches exactly one. Tested with ajv: each valid variant passes; a category registry carrying the consolidated schema URL is rejected. - 014 tasks: uncheck T033, T034, T036, T038-T040, T042, T044-T047 and T050-T057; their deliverables are absent (the agent template has 4 of its 7 components; the skills/ category folders do not exist). See #3523. - 016 data model: release_date is typed date | "Unreleased"; both entry lifecycles go VALID -> READY_FOR_RELEASE -> MERGED. * docs(specs): close review contract gaps * docs: align the 016 error object and scope the merged-PR check (#3500) Two consistency defects in the changelog-agent quality spec. The error shape was spelled three different ways across the spec set: data-model.md defined error_code, expected_format and actual_value, spec.md used error_type for the same field, and research.md's example used type, actual, expected and a second current_value. data-model.md also called the entity ValidationError in one relationship and ErrorObject in the other two. data-model.md's field table is the contract, so ErrorObject with error_code, expected_format, actual_value, suggestion and severity is now the single shape, and the two examples and the spec prose are aligned to it, including the severity and suggestion fields the examples were missing. The validation rules required every pr_issues reference to be a merged pull request, but pr_issues also carries issue references and an issue has no merged state, so a valid issue reference could never satisfy the rule. The rule and the constraint row now apply the merged check to pull-request references only and state that issue references must merely exist. * docs: finish aligning the 016 error object and sync with develop (#3500) CodeRabbit's two findings on this pull request were addressed in 23cf746, but the error-object contract was only partly aligned: spec.md still summarised the Error Object with `location` and `fix_suggestion` and omitted entry_id, severity and field, and FR-005 described the report in prose that an implementer could read as licence to reintroduce error_type and fix_suggestion. Point both at the canonical field set in data-model.md, so every definition, requirement and example in the specification now names the same fields. The merged-state finding needed no further work: data-model.md already scopes the merged requirement to pull-request references and exempts issues, which have no merged state. Also merge origin/develop, keeping both additive CHANGELOG entries. * docs: correct three 016 spec contradictions against the shipped gate (#3500) A Qodo review of the head found three spec statements that the shipped code contradicts. Each was checked against the code that actually runs. The bypass mechanism was described as automatic for chore/ and deps/ branches. changelog-unified.yml has no such bypass: it skips Dependabot and docs-bot authors, docs-only diffs (every changed file under docs/** or ending in .md), and the meta:no-changelog label, which it refuses for high-impact change types. A chore/ branch with a code diff therefore still needs a changelog entry or the label, so FR-009, Q1, research and the decision table now state the shipped behaviour and say explicitly that branch prefix is not a bypass. The spec required labels meta:has-changelog, meta:needs-changelog-fix and meta:changelog-exempt. None exist; the canonical set has only meta:needs-changelog and meta:no-changelog. Because labels.yml is a locked file this fixes the spec rather than adding labels: passing validation clears meta:needs-changelog, and failing keeps it applied. The label families, the User Story 4 acceptance criteria and the quickstart commands now use the two real names. pr_issues accepted "#123" or "PR-456", but the engine matches only /#(\d+)/ and /issues\/#(\d+)/, so a PR-456 reference matches neither and is counted as no link at all instead of a valid one. The data model now records #123 as the only machine-validated form, keeps PR-456 as a human-readable convention, and says that a full markdown URL is what makes it checkable. * docs: fix four more 016/017 contradictions found by review (#3500) A forced full review of the head surfaced four more statements that the shipped code contradicts; each was checked against the code that runs. spec.md's acceptance scenario promised the tool accepts "#123 or PR-456", but the engine matches only /#(\d+)/, so a PR-456 entry is still reported missing. The scenario now states #123 as the machine-validated format and says a bare PR-456 is not recognised. FR-007 said only "labels from the canonical set with prefix meta:" and never named one, which is how the spec ended up requiring three labels that do not exist. It now names the two that do, meta:needs-changelog and meta:no-changelog, and forbids any other changelog label. The 017 quickstart told readers to run node .github/validation/changelog/validator.js, which does not exist; the entry point is bin/validate.js and it needs --changelog-path. The corrected command was executed and runs. The 017 comment template shipped PR #3367's measurements as if they were reusable: 29 failures, 48/54 entries, HIGH confidence. A template that carries frozen numbers invites a reviewer to act on a stale count, so the figures are now {{placeholders}} under a warning that every number must be recomputed. * fix: close the registry schema gap and the last 017 quickstart/template claims (#3500) A full review of the head found the 014 registry schema accepted invalid skill metadata. No object schema set additionalProperties: false, so every undeclared or misspelled field validated: a skill whose compliance_violations was typed as complianceViolations, or whose agentskills_compliant was misspelled, passed cleanly and would have produced a wrong compliance report. Reproduced all five cases before fixing. Added additionalProperties: false to the eight object schemas and confirmed three well-formed registry shapes still validate while the misspelled and junk-key cases are now rejected. The 017 quickstart also runs two npm scripts that do not exist: validate:mermaid and validate:agent-spec are absent from package.json, so those steps fail immediately. Each is now marked as not yet defined, and the summary block distinguishes the defined scripts from the missing ones. The 017 comment template still carried PR #3367's numbers in its body -- 48/54 entries, 88.9%, 6/54 compliant, "within 48 hours", Q4 2026 -- below the warning added earlier. A warning does not stop anyone copying a stale count, so all 15 figures are now {{placeholders}}. * docs: add AI feedback response for #3500 (#3500) The ai-feedback-validation workflow reported that the pull request had no issue link, so this records the review feedback with a status per item as that workflow requires. Linkage: the pull request now closes #3465, which is the rebase of #3434 onto develop and the resolution of the spec 014/016 conflicts -- exactly what this pull request delivers. #3464 and #3434 stay as "Relates to" because neither is completed here. Tracked: eleven items are marked addressed, each with the commit that fixed it. Two are deferred to existing open issues rather than dropped: the repeat prose findings in specs 016 and 017 go to #3519, and the stricter 014 registry schema not being enforced at runtime goes to #3522, since the validator does not read the loaded schema and that is a code change outside a documentation-only pull request. Verified by running scripts/validation/ai-feedback-helpers.cjs against this file and the updated pull request body: passed, with no invalid status markers and no deferred item lacking an issue reference. * fix: placeholder the 017 comment template conclusions, not just its figures (#3500) Qodo was right that the template was only half fixed. The measurements became placeholders, but the body still asserted the conclusions: "Audit governance implementation is clean", "Classification confidence: HIGH", "Ready for merge", a fixed six-category count, an approval section stating all conditions were met, and instructions written for PR #3367 specifically. A template that asserts those verdicts will tell a reviewer that a different pull request is approved when it is not, and a stale verdict is harder to notice than a stale number because it reads as an assessment rather than a measurement. The verdicts, confidence level, merge readiness, approval section and category count are now placeholders as well, taking the body to 25, and the warning states that conclusions must be derived from evidence for the pull request under review. Only the historical "Usage Context" line still names PR #3367, which is correct: it documents where the example came from and is outside the reusable body. * docs: record the rejected claim that validate:changelog is missing (#3500) Qodo reported at High severity that the 017 quickstart runs a root script the package does not define, so the documented command fails before validation runs. It is defined, at package.json line 103, and running it from the root exits 0 with real output. The rejection is recorded in FEEDBACK_RESPONSE.md with that evidence so the decision is auditable rather than invisible. * docs: address the nine open CodeRabbit findings on #3500 Each finding was checked against the code before changing anything. Two were inconsistencies I had introduced or missed. The link rules still promised a bare `PR-NNN` in the business-rule table and in research.md after the previous commit narrowed the field description to `#123`, and the claim that a full markdown URL makes a reference machine-checkable was itself wrong: neither /#(\d+)/ nor /issues\/#(\d+)/ matches a /pull/456 path, so such a link satisfies only the non-empty-target check. The rule, the field description, the research summary and both error examples now agree that only `#123` and `issues/#123` are resolved. Separately, the branch-prefix bypass survived in research.md's pseudocode and in three places in the requirements checklist; the pseudocode now mirrors changelog-unified.yml (bot author, docs-only diff, or an explicit meta:no-changelog label) and the branch name is never consulted. The recovery mutex had no recovery path: a process exiting while holding it would leave every later contender unable to enter the protocol, so the abandoned lock could never be removed. It now requires an OS-backed lock and the portable stat-and-compare fallback is not used where no such primitive exists. Three quickstart checks could pass without proving anything. The missing-file test used /nonexistent/file.md, which the plan rejects as an absolute path before any read, so it never exercised missing-file handling; CHANGELOG.missing.md does, and returns exit 1 with "not found". The documentation greps were chained with &&, letting an early failure be masked by a later success; they now collect failures and exit nonzero, verified by removing one needle and confirming exit 1. The workflow scenario accepted a precomputed CHANGELOG_CHANGED flag without exercising any documented skip condition; it now classifies each of bot author, docs-only, mixed diff, meta:no-changelog and empty diff, verified across eight cases. Also: the skill name must match its containing directory per the Agent Skills specification; the changelog entry references #3500 rather than the linked issue #3465; and FEEDBACK_RESPONSE.md no longer implies rejected feedback was deferred. * docs: address the ten findings from the CodeRabbit CLI review (#3500) Ran `coderabbit review --agent --base develop` as the review comments suggest. It reviewed 19 files and returned ten findings, nine major. Each was checked against the code before changing anything; all ten held. Two of my own earlier fixes were wrong. The recovery-mutex rule I added previously listed an `O_CREAT | O_EXCL` lock file as an OS-released primitive. It is not: the file survives process exit, so using it would cause exactly the deadlock the rule exists to prevent. The rule now names only `flock`/`fcntl` and Windows named mutexes or kernel semaphores, and requires the operation to be refused outright where no such primitive exists. The example in the entry model also used `PR-3373`, which the link rules do not resolve. The classification guidance contradicted itself across two files. data-model.md called a differing error between branches "likely infrastructure"; the quickstart matrix called it audit-related; the quickstart flow called it ambiguous. All three now read ambiguous, requiring per-failure comparison, which is the only defensible reading. FR-008 and FR-009 overlapped on a changelog-only pull request: the shipped docs-only test accepts any file ending in `.md`, and `CHANGELOG.md` ends in `.md`, so the one pull request that most needs its entries validated is exempt. FR-009 keeps the shipped behaviour and now says so explicitly and names the gap, rather than leaving two requirements in silent conflict. The 017 quickstart compared against the specification branch while attributing the failures to PR #3367, whose branch is `audit/governance-audit-implementation`; it now checks out that branch. It also used the root `validate:changelog` script to establish a 6/54 entry baseline, but that script runs the repository-wide safety audit, which reports no per-entry results; the feature-016 engine is used instead. The undefined `validate:mermaid` and `validate:agent-spec` commands are no longer presented as evidence producers: each is guarded and records UNVERIFIED when absent, and pipefail stops `tee` masking a missing script. The reusable comment template still declared classifications in its section headings and inline "pre-exists on develop" notes, so replacing the numeric placeholders could not have corrected them. Those are placeholders now, 6 headings and 6 inline claims. The 017 plan presented six categories as classified evidence while spec.md records that only User Story 1 was verified and the failures no longer occur; the plan now carries a banner saying it is a historical record. The 016 workflow scenario ended its failure branch with a successful echo, so a blocked pull request still exited 0; it now exits nonzero. * docs: record the CodeRabbit CLI review passes on #3500 Ran the review the comments suggest, following the documented agent workflow at docs.coderabbit.ai/cli/overview: coderabbit review --agent --base develop, then a second pass as step 4 recommends. Pass 1 returned 10 findings across 19 files and all 10 were addressed. Pass 2 returned 10 more. The loop stops after two passes, as that guidance requires. The remaining findings concern the prose of specifications 016 and 017, which describe a system that has not been built, so there is no implementation to verify a claim against and each pass raises further hypotheticals. Recorded rather than chased. Two are called out as independently actionable: the CHANGELOG.md docs-only bypass in FR-009, which needs a change to changelog-unified.yml rather than to this documentation, and the generated performance results fixture. * docs: route the CodeRabbit pass-2 findings to their existing trackers (#3500) The pass-2 section claimed the remaining findings had no tracker and that the loop simply stopped. That was wrong: #3519 already exists to hold spec 016 and 017 design findings from this very pull request, and it already listed most of them. Mapped the ten findings against the open trackers. Five were duplicates of existing #3519 items, re-raised against the corrected prose: inconclusive classifications, category verdicts in the comment template, the wrong comparison branch, losing the file type when linting, and posting the comment without re-verification. The generated performance fixture is already covered by #3498 and PR #3499. Four were genuinely new and are now appended to #3519 as checklist items, taking it from 13 to 17: excluding CHANGELOG.md from the docs-only bypass, limiting automatic labelling to non-exemption labels, running one validator on both branches, and testing the shipped workflow rather than a local simulation. * docs: fix the MD022 failure the specification lint caught (#3500) The rewritten pass-2 paragraph began a line with '#3519 and #3470 ...', which markdownlint parses as an ATX heading, so MD022 fired and Specification Validation failed. Reflowed so no line starts with a hash, and turned the bare issue references into links now that they name the trackers the findings were routed to. Also corrected the stale lead-in, which still said two findings were acted on here: they are checklist items in #3519 and #3498. Verified with markdownlint over the exact 18-file set the workflow lints. An earlier local run covered only three files, which is why this reached CI. * fix: address the four open Qodo findings on #3500 (#3500) Three of the four were defects I had introduced, and one of those I had previously "fixed" in the wrong direction. The baseline comparison read a file nothing writes: the develop step wrote /tmp/develop-changelog-results.json while the count and the cross-branch diff read .txt. The grep pattern could not have matched JSON either. Both branches now run the same engine with --output text and write matching filenames, so the count and the diff operate on the file that exists. This also completes the "run the same entry validator on both branches" item in #3519. I had marked validate:mermaid and validate:agent-spec as UNVERIFIED because those exact script names are absent. That was the wrong response: the repository exposes validate:mermaid-syntax and validate:agents, both of which run. The guards recorded a false gap where a working validator already existed, so all three now call the real scripts, with pipefail preserving a nonzero exit -- which for validate:agents is a genuine finding rather than a missing command. The registry schema still accepted malformed skill metadata. generatedSkill constrained id, category and type to minLength 1 only, so Bad_ID, a category of "Not A Category!" and even ../etc/passwd all validated. id and category now require the same ^[a-z0-9-]+$ slug that legacySkill already uses and type takes the same action/query/transform/utility enum; a valid document still validates. The comment template's fourth category asserted a tentative ENVIRONMENTAL verdict and that the pull request was not blocked, while the template's own rules forbid posting a category that is still unclassified. The verdict and both of its consequences are placeholders now, and the rules state the constraint explicitly: an unclassified category is omitted, or the whole comment is withheld. * docs: record the Qodo pass on the current head (#3500) Qodo resolved its four threads on the push rather than leaving them to close individually, so the reasoning is recorded here instead. Three of the four were defects this pull request had introduced, and one of those had been fixed in the wrong direction: validate:mermaid and validate:agent-spec are genuinely absent, but the repository does expose validate:mermaid-syntax and validate:agents, so wrapping the missing names in UNVERIFIED guards recorded a false gap rather than correcting the call. Noted that the schema tightening has no runtime effect until #3522 lands via PR #3550, and that what the inconclusive-classification standard should be stays with the spec owner in #3519. --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Contributor
Author
|
/agentic_review |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
… changes Qodo found two ways the guard added in this pull request could fail to do its job, and a third place its protection does not reach. status() ran `git status` with no cwd, so it inspected the process directory rather than Jest's repository root. Launched from anywhere else with an absolute --config path, setup recorded `not-a-work-tree`, teardown returned early on that value, and the guard silently disabled itself for the whole run. It now resolves the repository from globalConfig.rootDir, falling back to the process directory only when Jest supplies nothing. hashOf() used `git hash-object`, which covers content only, so a test that chmods a file which was already modified or untracked left the snapshot value unchanged and the change passed unnoticed even though git reports it. The executable bit, which is what git actually records, is now folded into the value. Verified: an untracked file's snapshot value goes from <hash>:0 to <hash>:111 on chmod. The third is a boundary rather than a defect. The guard is registered only in .jest.config.cjs, so the six package-local `test` scripts that invoke a bare jest, and the separate jest.config.js files, do not load it. Each of the six was run and none currently dirties the repository, so this is latent; the header comment now says so explicitly instead of implying wider coverage than exists. Two tests added: one runs the guard from an unrelated working directory with rootDir supplied, the other asserts a mode-only change fails. Both fail against the previous implementation.
Contributor
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 60cd853 |
Qodo found that hashOf() resolved the path through a symlink twice over. Both `git hash-object` and `statSync` follow a link, so the snapshot described whatever the link pointed at rather than the link itself. Two mutations were therefore invisible: - replacing an already modified or untracked regular file with a symlink to a file of identical content - retargeting an existing symlink from one file to another of identical content In both cases the status key was unchanged and the computed value was unchanged, so teardown accepted the repository mutation. Reproduced both before fixing: the value stayed `c2981a99...:0` across the substitution and across the retarget. hashOf() now calls lstatSync first, which does not follow, and returns `symlink:<link text>` for a link. Verified: the value becomes `symlink:target.txt` then `symlink:other.txt`, so the substitution and the retarget are both caught. The executable-bit detection for regular files is unaffected and still reports `<hash>:0` to `<hash>:111` on chmod. Two tests added. Both fail against the previous implementation.
Contributor
Author
Qodo follow-up: "Symlink changes escape the test guard"Correct — and it is a gap in the fix I made an hour ago, not a pre-existing one. I added the executable bit with I reproduced both of Qodo's cases before changing anything, in a scratch repository:
In both cases the status key was unchanged and the computed value was unchanged, so Fix —
|
| state | value after the fix |
|---|---|
| regular file | c2981a99…:0 |
symlink → target.txt |
symlink:target.txt |
symlink → other.txt |
symlink:other.txt |
So the substitution and the retarget are both caught. The executable-bit path is unchanged and still reports
<hash>:0 → <hash>:111 on chmod, so this does not regress the earlier finding.
Tests
Two added, one per case. Against the previous implementation: 2 failed, 9 passed; after: 11 passed.
Why this was the third gap in one field
The snapshot value has now been wrong in three separate ways: content-only hashing missed a chmod; then statSync
followed links; the link type and target were invisible. lstatSync plus the content hash now covers type, link
target, content and the executable bit. A further gap in this field would need a different mechanism rather than
another field appended here.
I have not re-run a full review loop on this; the previous round's two findings and these two are all addressed, and
the guard now has 11 tests pinning its behaviour.
Contributor
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 7adf043 |
This was referenced Sep 26, 2026
eleshar
added a commit
that referenced
this pull request
Sep 27, 2026
… --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.
10 tasks
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.
Bugfix Pull Request
Linked issues
Closes #3498
Context
Reproduction
npx jest --config .jest.config.cjson a clean develop checkout.git status --porcelainshows:M scripts/automation/__tests__/performance/results-phase-2b.json?? .github/reports/changelog-metrics/?? .github/reports/metrics/collection-summary-<date>.jsonRoot Cause
Each artefact was attributed by running the suites one at a time.
phase-2b-validation.test.js: the persistence test saved to the trackedresults-phase-2b.jsonnext to itself.metrics-collection-orchestrator.test.js: its config setstorage.basePathto the real.github/reports/metrics, and it wrote its config file into__tests__/. The save-summary test asserted onlyifthe file existed.metricsCollection.test.js(changelog agent): it created a temporary metrics directory but never passed it on.collectMetricsSnapshothad no way to change its output folder, so snapshots landed in.github/reports/changelog-metrics/. Its CSV exports were written intotests/fixtures/, and two header-only copies had been committed.Fix Summary
os.tmpdir()(mkdtempSync) and clean up afterwards.collectMetricsSnapshotaccepts an optionalmetricsDir; the default is unchanged.globalSetup/globalTeardownguard comparesgit status(entries and content hashes) before and after the run. It fails the run on any file the tests created or changed, and names each one.ALLOW_TEST_ARTEFACTS=1bypasses it for deliberate report regeneration.Verification
git statusstays clean and the guard does not fire. Test counts match develop exactly (5198 passed / 5215 total).zz-guard-probe.txtmakes the run exit 1 and names the file;ALLOW_TEST_ARTEFACTS=1exits 0.-z; warn when the setup snapshot is missing). /security-review: no findings. Semgrep: 61 rules, 0 findings.Risk & Rollback
Changelog
Fixed
Summary by CodeRabbit