fix(pr-convention-policy): parse the scan_linkage hook validator in lockstep-drift

[kyle-sexton]

Closes #633

Summary

pr-convention-lockstep failed on every open PR with drift: hook validator: KEYWORD_ERE / NO_ISSUE_ERE declarations not found. melodic-software/claude-code-plugins#4636 rewrote the hook validator around a per-line scan_linkage and renamed its closing pattern to CLOSING_ERE, so parseValidatorPatterns could not find it. The validator and the ci-workflows pr-contract composite still agree, so this was a parse failure, not drift.

Fix

• parseValidatorPatterns reads CLOSING_ERE (plus the unchanged NO_ISSUE_ERE) and translates [[:blank:]] as well as [[:space:]].
• The new EREs are lowercase with no i flag, matched against ${line,,}. The parser now asserts that lowercasing is present and the probes lowercase their input, the same way the composite check already asserts lower = tolower(line).
• The test fixture takes the new validator shape; a new test fails when the validator stops lowercasing.
• policy.json is unchanged. Every closing keyword and no-issue marker it lists is still accepted by both the composite (run.sh scan_line and the no-issue test) and the validator. The new Refs: / Relates to: acceptance and negated-closer rejection are enforced by both, but policy.json has no field for them and pr-convention-policy.mjs and the pinned reusables do not implement them, so recording them would be a separate change.

Verification

• npm run test:pr-convention-lockstep: 40/40 pass.
• npm run test:pr-convention-policy: 11/11 pass.
• npm run lint:pr-convention-lockstep against live sources with a token: pr-convention lockstep: all copies and consumer pins match policy.json, exit 0.
• biome check on both touched files: clean.

Related

• fix(source-control): accept Refs: markers and block negated closers in PR-body gates (#4183) claude-code-plugins#4636 (the validator rewrite)
• fix(pr-issue-linkage): accept Refs/Relates markers and fail negated closers ci-workflows#544 (the pr-contract negation and Refs: change the validator mirrors)
• Unblocks docs(lefthook-powershell): correct the PSScriptAnalyzer failure mechanism #628, docs(review-instructions): drop maintainer history from REVIEW.md #630, chore: remove stale select-runner references after ci-perf Phase 7 #631, docs: remove em dashes from synced components #632

🤖 Generated with Claude Code

https://claude.ai/code/session_01LCEBhuXxZAT8K619tC6T5f

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-27T23:26:09.754250Z	55d33cc	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 2m 53s —— View job

---

Code review complete

• Gather context (PR diff, changed files)
• Review lockstep-drift.mjs changes
• Review lockstep-drift.test.mjs changes
• Post findings

No blocking findings.

Reviewed the diff in components/pr-convention-policy/lockstep-drift.mjs and lockstep-drift.test.mjs against origin/main (commit 55d33cc).

parseValidatorPatterns now reads CLOSING_ERE instead of the renamed-away KEYWORD_ERE, and adds a lower="${line,,}" presence assertion mirroring the existing parseCompositePatterns guard for lower = tolower(line) (lockstep-drift.mjs#L151-L162). Traced through the logic by hand:

• toJs now translates both [[:space:]] and [[:blank:]], and drops the i flag in favor of the { lowercaseProbe: true } option added at the two checkCopies call sites for the hook validator (lockstep-drift.mjs#L652-L658) — this is the same case-handling design already used for the composite's awk patterns, not a new pattern.
• Manually verified the probe strings (closes #12, no related issue: none, etc., lowercased) still match the translated CLOSING_ERE / NO_ISSUE_ERE from the updated GOOD_VALIDATOR fixture.
• The new regression test (lockstep-drift.test.mjs#L441-L448) correctly isolates to exactly one error when lowercasing is removed, since parseValidatorPatterns throws before assertPatternsEnforce runs.
• The deliberate scope cut — not wiring up NON_CLOSING_ERE (negated-closer rejection) since policy.json has no field for it — is disclosed in the PR body and consistent with what's actually implemented.

Two caveats on what I could not independently verify in this sandbox (flagging per review policy rather than restating author claims as confirmed):

• No network access to fetch the live claude-code-plugins pr-linkage-validator.sh, so I could not confirm the literal lower="${line,,}" string and CLOSING_ERE/NO_ISSUE_ERE declarations match the real upstream file byte-for-byte — only that the fixture and logic are internally consistent.
• No permission to execute node --test / biome check in this environment, so the author's reported 40/40, 11/11, and clean-biome results are author-claimed and unverified by me.

Both are environment limitations, not defects found in the diff.

[claude]

Claude finished @kyle-sexton's task in 1m 0s —— View job

---

Security review — PR #634

• Gather context (diff, changed files)
• Run /review:security-review criteria against the diff
• Post results

Scope: components/pr-convention-policy/lockstep-drift.mjs, lockstep-drift.test.mjs (diff against origin/main).

Findings: none.

This PR only changes the regex-based parser inside the lockstep-drift checker (parseValidatorPatterns in lockstep-drift.mjs) and its test fixture to match the renamed CLOSING_ERE/scan_linkage shape of the upstream hook validator. No new external input path, no change to how content is fetched (still the existing contentsUrl/cachedText flow using the org GitHub App token), no new code execution, no permission or workflow changes, and no handling of secrets/credentials. The RegExp construction from fetched validator text is pre-existing behavior, not introduced by this diff, and the sources are org-controlled repositories reached through the same authenticated API calls as before. This tool is an advisory drift-checker (per .claude/rules/pr-body-contract.md, it "is reported, not gating"), so even a logic mistake here has no access-control impact.

No CRITICAL / IMPORTANT / SUGGESTION security findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55d33cc17b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 50s —— View job

---

Security review — PR #634 (HEAD 39017d1)

• Gather context (diff, changed files)
• Run /review:security-review criteria against the diff
• Post results

Scope: components/pr-convention-policy/lockstep-drift.mjs, lockstep-drift.test.mjs — diff origin/main...HEAD (includes commit 39017d1, which was pushed after the prior automated reviews on 55d33cc and adds the operand-lowercasing check requested by Codex's P2 comment on that earlier commit).

Findings: none.

This PR only touches the regex-based parser in parseValidatorPatterns (lockstep-drift.mjs#L303-L324) and its test fixtures, to track the renamed CLOSING_ERE/scan_linkage shape of the upstream hook validator. No new external input path, no change to how validator text is fetched (still the existing authenticated contentsUrl/cachedText flow against org-controlled repos), no code execution, no permission or workflow changes, no secret/credential handling.

Checked specifically for the class of bug Codex flagged on the prior commit (a lockstep check that "passes" while the live validator has silently stopped lowercasing, which would make the tool certify a validator that rejects capitalized Closes/No related issue forms — a false-negative in an advisory check, not an access-control gap):

• The new VALIDATOR_OPERANDS loop (lockstep-drift.mjs#L309-L320) now requires each =~ $CLOSING_ERE / =~ $NO_ISSUE_ERE use-site to read the specific lowercased operand name (chunk / lower) and requires the lowercasing source lines to literally be present in the fetched text — not just present anywhere, but tied to the operand actually matched against. A validator that keeps the lower="${line,,}" assignment but matches $line directly (the exact bypass Codex described) now fails this check, since uses.every((use) => use[1] === operand) requires every capturing use of that ERE name to bind the lowercased variable.
• new RegExp(...) construction is still built from ERE fragments extracted from org-controlled validator source, same trust boundary as before this PR; no new attacker-influenced input reaches RegExp.
• sources.every((source) => shellText.includes(source)) is a plain substring check against fixed literal strings — no injection surface (the name used to build the use-site regex is one of the two fixed object keys, not attacker data).

This tool is an advisory drift-checker (.claude/rules/pr-body-contract.md: it "is reported, not gating"), so even a residual logic gap here has no access-control impact — consistent with the assessment on the prior commit.

No CRITICAL / IMPORTANT / SUGGESTION security findings.

[claude]

Claude finished @kyle-sexton's task in 4m 20s —— View job

---

Code review complete

• Gather context (PR diff, changed files)
• Review lockstep-drift.mjs changes
• Review lockstep-drift.test.mjs changes
• Post findings

Reviewed git diff origin/main...HEAD at 39017d1 (both commits: the scan_linkage parse fix and the lowercased-operand tightening that already resolves Codex's earlier P2 comment on 55d33cc — the check now requires every =~ $CLOSING_ERE / =~ $NO_ISSUE_ERE use to read the designated lowercased operand, not just that a lowercasing assignment exists somewhere in the file).

1 finding, posted inline on components/pr-convention-policy/lockstep-drift.mjs:304:

• The new keyword extraction regex, /CLOSING_ERE='([^']+)'/, has no boundary before the name. This diff's own test fixture adds a sibling declaration, NON_CLOSING_ERE='...', to the same script, and "NON_CLOSING_ERE='...'" contains "CLOSING_ERE='...'" as a literal substring. If the real upstream validator ever declares NON_CLOSING_ERE before CLOSING_ERE, .match() silently returns NON_CLOSING_ERE's value mislabeled as CLOSING_ERE's, producing a misleading "declared pattern rejects closing keyword" drift error instead of the true (harmless) cause. Today's fixture order avoids it, and no new test exercises declaration order, so the coupling is silent. Suggested a one-character \b anchor fix inline.

Everything else checked out on read-through:

• toJs's new [[:blank:]] → [ \t] translation and dropped i flag pair correctly with the fixture's lowercase-only EREs and lowercaseProbe: true at both checkCopies call sites.
• The VALIDATOR_OPERANDS lowercasing assertion correctly requires all =~ uses of each ERE to read the designated operand (verified by hand against the new regression test's four mutations plus the "extra raw-line match" case) — this is a real strengthening over 55d33cc, not just cosmetic.
• The deliberate scope cut (not implementing NON_CLOSING_ERE / negated-closer rejection, since policy.json has no field for it) is disclosed accurately and matches what's implemented.

Not independently verified (environment limitations, not defects found):

• Could not run node --test components/pr-convention-policy/lockstep-drift.test.mjs, npm run lint:pr-convention-lockstep, or biome check — command execution required approval not available in this session. The author's reported 40/40, 11/11, and clean-biome results are author-claimed and unverified by me.
• No network access to fetch the live upstream pr-linkage-validator.sh, so the CLOSING_ERE/NON_CLOSING_ERE/NO_ISSUE_ERE declaration order in the real file (relevant to the finding above) is unconfirmed either way.
  · branch fix/lockstep-hook-validator-parse

[claude]

Claude finished @kyle-sexton's task in 2m 34s —— View job

---

Code review complete

• Gather context (PR diff vs origin/main, CLAUDE.md)
• Review lockstep-drift.mjs changes
• Review lockstep-drift.test.mjs changes
• Post findings

No blocking findings.

Reviewed git diff origin/main...HEAD at 7fba499 (all three commits: the CLOSING_ERE/scan_linkage rename fix, the lowercased-operand tightening, and the line-start anchor).

This PR has already been through two full rounds of automated review on this thread, each surfacing one real issue that got fixed in a follow-up commit:

• Codex's P2 on 55d33cc (presence-only lowercasing check didn't verify the ERE was actually matched against the lowercased variable) → fixed in 39017d1's VALIDATOR_OPERANDS operand-binding check (lockstep-drift.mjs#L297-L321).
• My own finding on 39017d1 (CLOSING_ERE=' unanchored match could read the sibling NON_CLOSING_ERE=' value if declaration order ever flipped upstream) → fixed in 7fba499 by anchoring both declarations to line start (lockstep-drift.mjs#L305-L306), and the new regression test at lockstep-drift.test.mjs#L447-L452 now exercises the swapped-order case directly.

Re-verified the current HEAD from scratch rather than re-trusting the prior passes:

• The VALIDATOR_OPERANDS loop correctly requires every "$X" =~ $CLOSING_ERE / $NO_ISSUE_ERE use-site in the fetched text to bind the specific lowercased operand (chunk / lower), not just that a lowercasing assignment exists somewhere — confirmed by hand against all five mutation cases in the new test (lockstep-drift.test.mjs#L456-L479), including the "extra raw-$line match alongside the correct one" case, which is the scenario my prior finding on this PR was about for the closing regex extraction (uses.every fails, not just uses.some).
• \$CLOSING_ERE\b extraction cannot false-match inside $NON_CLOSING_ERE text (the character after $ differs), independent of the line-anchor fix — the two protections (name-extraction anchor, operand =~ regex) address different collision risks and both hold.
• toJs's new [[:blank:]] → [ \t] translation and dropped i flag pair correctly with lowercaseProbe: true now passed at both checkCopies call sites for the hook validator.
• The scope cut (not implementing NON_CLOSING_ERE / negated-closer rejection, since policy.json has no field for it) is accurately disclosed in the PR body and matches what's implemented.

One residual, non-blocking observation, not raised as a finding because it's speculative rather than demonstrated: the new ^CLOSING_ERE='...'/^NO_ISSUE_ERE='...' line-start anchors (lockstep-drift.mjs#L305-L306) would report a false "declarations not found" drift if the live upstream ever prefixed either declaration with readonly/declare -r/indentation. The PR body's author-claimed live-lint run (pr-convention lockstep: all copies and consumer pins match policy.json, exit 0) implies today's upstream declares both at column 0 with no prefix, but I have no network access in this sandbox to confirm that independently, so it's unverified rather than a bug I can demonstrate against current upstream text.

Not independently verified (environment limitations, not defects found in the diff):

• Could not run node --test components/pr-convention-policy/lockstep-drift.test.mjs, npm run lint:pr-convention-lockstep, or biome check — command execution required approval not available in this session. The author's reported 40/40, 11/11, and clean-biome results are author-claimed and unverified by me.
• No network access to fetch the live upstream pr-linkage-validator.sh, so the real declaration indentation/prefix noted above is unconfirmed either way.
  · branch fix/lockstep-hook-validator-parse

[claude]

Claude finished @kyle-sexton's task in 48s —— View job

---

Security review — PR #634 (HEAD 7fba499)

• Gather context (diff, changed files)
• Run /review:security-review criteria against the diff (origin/main...HEAD)
• Post results

Scope: components/pr-convention-policy/lockstep-drift.mjs, lockstep-drift.test.mjs — full diff against origin/main, including the newest commit 7fba499 (anchors CLOSING_ERE/NO_ISSUE_ERE extraction to line start with ^...+/m).

Findings: none.

This commit only changes parseValidatorPatterns: the keyword/marker extraction regexes gain a ^/m anchor, which is the fix for the substring-collision finding raised earlier in review (a NON_CLOSING_ERE='...' declaration could otherwise be mismatched as CLOSING_ERE). Verified the fix is correct and complete:

• /^CLOSING_ERE='([^']+)'/m (line 305) and /^NO_ISSUE_ERE='([^']+)'/m (line 306) now require the match to start at a line boundary, so NON_CLOSING_ERE='...' (which contains CLOSING_ERE='...' only mid-string, right after NON_) can no longer be picked up by .match() regardless of declaration order in the upstream validator.
• The added regression test (lockstep-drift.test.mjs — "validator CLOSING_ERE is read from its own declaration, not NON_CLOSING_ERE") swaps declaration order and confirms the correct pattern is still extracted, plus that a CLOSING_ERE-only-as-substring text throws instead of silently matching.
• The VALIDATOR_OPERANDS name used to build the per-use regex (new RegExp(`"\\$(\\w+)" =~ \\$${name}\\b`, "g"), line 311) is always one of the two fixed object keys (CLOSING_ERE / NO_ISSUE_ERE), never attacker- or fetched-text-controlled, so there's no regex-injection surface there.

No new external input path, no change to how validator text is fetched (still the existing authenticated contentsUrl/cachedText flow against org-controlled repos), no code execution, no permission or workflow changes, no secret/credential handling. This is the same trust boundary already assessed on the prior two commits in this PR (55d33cc, 39017d1), unchanged by this fix. Per .claude/rules/pr-body-contract.md, this tool is advisory (pr-convention-lockstep "is reported, not gating"), so even a residual logic gap here would have no access-control impact — moot in any case, since no such gap was found.

No CRITICAL / IMPORTANT / SUGGESTION security findings.
· branch fix/lockstep-hook-validator-parse