[kyle-sexton]

No related issue: repair standards main after ci-workflows#733

Summary

check-pr-convention-copies has been red on main since melodic-software/ci-workflows#733 (merged 2026-10-11 17:13Z) changed the pr-contract composite's run.sh, for example run 38159506436. The gate's behavior with the default pattern is unchanged; the drift checker was reading the old source shape.

Fix

#733 changed four things the checker anchored on:

Checker expectation	run.sh after #733
match(chunk, /(close|...)[ \t]*:?[ \t]*([a-z0-9_.-]+\/[a-z0-9_.-]+)?#[0-9]+/)) break	match(chunk, closing_regex)) break, with closing_regex = "(close|...)" separator ref and ref from ENVIRON["ISSUE_REF_PATTERN"] (default DEFAULT_ISSUE_REF_PATTERN='([a-z0-9_.-]+/[a-z0-9_.-]+)?#[0-9]+')
rest ~ /^(refs|relates[ \t]+to):...#[0-9]+[ \t]*$/)	rest ~ non_closing_regex) with non_closing_regex = "^(refs|relates[ \t]+to):[ \t]*" ref "[ \t]*$"
linkage_errors+=('Missing a native closing keyword	linkage_errors+=("Missing a ${keyword_kind} closing keyword
analyzer runs with no env	analyzer reads ISSUE_REF_PATTERN; unset, ref became () and 347 sample bodies (xcloses #12, Closes #12x, ...) read as closing

Changes, all in components/pr-convention-policy/:

• The extractors accept both shapes. Consumers pinned before ci: repin ci-workflows to v0.39.0 #733 still run the inline literal, so the pin audit must keep reading it. The new shape is rebuilt from the keyword alternation, the default separator and DEFAULT_ISSUE_REF_PATTERN.
• The behavioral run passes run.sh's default as ISSUE_REF_PATTERN/DEFAULT_ISSUE_REF_PATTERN. A run.sh that reads the variable without declaring, falling back to and exporting the default is drift.
• New check: action.yml's issue-ref-pattern default must equal run.sh's default (or be empty). Any other value flips the analyzer to its custom-pattern separator for every caller that leaves the input unset.
• Fixtures: check-contract-run.sh.txt is now ci-workflows 8b3cc4bf28. It is verbatim apart from one spellchecker:ignore-next-line awk comment, needed because this repository's typos lane flags ture in an upstream comment. The previous fixture is kept as check-contract-run-literal-ref.sh.txt for the older-pin shape.

Verification

• npm run test:pr-convention-lockstep: 90 pass, 0 fail. Before the fix, the new fixture produced 37 failures.
• npm run lint:pr-convention-copies against live ci-workflows main: every copy matches policy.json.
• Real drift still fails, with new tests for: a missing or unexported default, a default without the #N grammar (both the enforcement-pattern probe and the behavior check catch it), a default separator losing the optional colon (the behavior check catches it), and a mismatched action.yml default. The existing mutation tests still pass against the new fixture.
• Biome, typos, gitleaks and editorconfig pre-commit hooks pass.

Related

• feat(pr-contract): accept a per-repo issue-ref-pattern for linkage ci-workflows#733 (the change being tracked)
• feat(claude-lanes): pass allowed-bot-sender-ids to the review lanes at ci-workflows v0.40.0 #773 (blocked by the red main)

🤖 Generated with Claude Code

[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-10-11T17:54:33.020015Z	7f7cb4f	Draft marked ready

ℹ️ 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 8m 11s —— View job

---

Code review complete

• Gather context (diff, CLAUDE.md, REVIEW.md, changed files)
• Understand the request (CI code-review lane per /review:code-review)
• Review changed files for correctness, maintainability, test coverage
• Adversarially verify candidate findings
• Post findings (inline comment + summary)

Scope: all 5 changed files in components/pr-convention-policy/ (the lockstep drift checker gaining support for ci-workflows#733's ISSUE_REF_PATTERN shape).

I traced the new parseCompositeIssueRef / compositeKeywordPattern / assertCompositeIssueRefDefault logic against the vendored check-contract-run.sh.txt fixture line-by-line (separator, closing_regex, non_closing_regex, ref assembly) and against the literal-ref fixture, and confirmed check-contract-run-literal-ref.sh.txt is byte-identical to the pre-PR fixture, matching the PR's claim that it preserves the older-pin shape.

Finding (posted inline on lockstep-drift.mjs:1048)

One root cause — a composite's run.sh reading ENVIRON["ISSUE_REF_PATTERN"] without correctly declaring/exporting DEFAULT_ISSUE_REF_PATTERN — is independently detected and reported by three separate call sites sharing the same parseCompositeIssueRef (lockstep-behavior.mjs:394-409):

• lockstep-drift.mjs:1028 (parseCompositePatterns → compositeKeywordPattern)
• lockstep-drift.mjs:1048 (assertCompositeIssueRefDefault)
• lockstep-drift.mjs:1049 (checkCompositeBehavior, via lockstep-behavior.mjs:444)

In checkPinnedComposite, lines 1028 and 1048 pass the identical location string, so this specific defect produces two byte-identical drift: lines plus a third (behavior)-suffixed one — in the exact tool whose job is a clean, actionable fleet-scan report. The analogous checkCopies path (lines 860, 971-975, 977) produces three near-duplicate lines differing only by a location suffix. Not a false negative, and the existing test only asserts errors.some(...) so it doesn't surface the duplication. Details and a suggested fix are in the inline comment.

Other notes

• The default-pattern reconstruction in compositeKeywordPattern (rebuilding closing_regex from assembled[1] + separator[1] + issueRef) matches the fixture's runtime assembly exactly for the shipped default; I did not find a case where it diverges from what the real awk program computes.
• assertCompositeIssueRefDefault's pinned-consumer path fetches runSh and actionYml from the same source/ref (lockstep-drift.mjs:1277-1278), so the two sides of the comparison can't be pulled from mismatched pins.
• I could not execute npm run test:pr-convention-lockstep or node --test in this sandbox (command execution requires approval I can't grant here), so the PR description's test-pass counts and lint/pre-commit results are author-claimed and unverified by me — the review above is from static reading of the diff and fixtures only.
  · Branch

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Review changed files for security-relevant surface
• Check for instruction-surface deletions (CLAUDE.md/AGENTS.md/rules)
• Adversarial validation of any candidate findings
• Post final results

Scope: components/pr-convention-policy/lockstep-drift.mjs, lockstep-behavior.mjs, lockstep-drift.test.mjs, and the two vendored fixtures/lockstep/*.sh.txt fixtures.

No security issues found.

Notes from the review:

• lockstep-drift.mjs adds compositeKeywordPattern, which assembles a RegExp from substrings scraped out of a remote pinned composite's run.sh and later evaluates it with new RegExp(keyword). This reuses the same trust model the file already had pre-PR (new RegExp(keyword[1]) from a single literal extracted from fetched content) — it doesn't widen the trust boundary or add a new remote-input path, so it's not a new finding, just worth noting for future review if this parsing grows further.
• lockstep-behavior.mjs now threads a fetched DEFAULT_ISSUE_REF_PATTERN value into gawk as an environment variable (ISSUE_REF_PATTERN) rather than shell-interpolating it — spawnSync is called with an args array (no shell: true), so there's no command-injection path, and the awk program itself still runs under gawk --sandbox exactly as before this change.
• No changes to .claude/rules/, AGENTS.md, or any other instruction-bearing file in this diff, so the instruction-surface-deletion check doesn't apply here.
• The diff is maintenance/test tooling (a drift checker between this repo's policy and ci-workflows' composite action) with no changes to request handling, authz, secrets, or the production pr-contract gate's runtime behavior itself.

[github-actions]

claude-security-review has reviewed this pull request through 2db3648; a later push is reviewed from there.

[github-actions]

claude-review has reviewed this pull request through 2db3648; a later push is reviewed from there.

[claude]

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

---

Security review complete

• Gather context (incremental diff since 7f7cb4f, PR description, prior review comments)
• Review changed files for security-relevant surface (incremental scope: lockstep-behavior.mjs, lockstep-drift.mjs, lockstep-drift.test.mjs)
• Check for instruction-surface deletions
• Adversarial validation of any candidate findings
• Post final results

Scope: incremental — only the delta since this lane's last review at 7f7cb4f, i.e. commit 2db3648 ("report a broken issue-ref default once"), across lockstep-behavior.mjs, lockstep-drift.mjs, and lockstep-drift.test.mjs.

No security issues found.

This commit is the fix for the duplicate-reporting finding from the prior review round (three call sites independently raising the same DriftError for one broken DEFAULT_ISSUE_REF_PATTERN wiring). The change:

• lockstep-drift.mjs#L128-L131 — collect() now dedupes verbatim DriftError messages (errors.includes(error.message)) instead of always pushing. Since each message is prefixed with its location (e.g. "gate composite: ..." vs. "caller codex-plugins pin ddddddd: ..."), dedup can't merge findings that come from genuinely different composites/pins — only truly identical strings from the same root cause collapse.
• lockstep-behavior.mjs#L442-L449 and lockstep-drift.mjs#L973-L978 / #L1048-L1051 — checkCompositeBehavior gains a wiringLocation parameter so its internal parseCompositeIssueRef call reports under the same location string the two static checks already use, making the three call sites' error text byte-identical so the new dedup actually collapses them.
• lockstep-drift.test.mjs updates pin this exact behavior (assert.deepEqual to a single message; a new checkPinnedComposite assertion that a broken pin produces exactly one finding).

No new input-handling, injection surface (command/path/template), secret/token handling, authz logic, or trust-boundary change is introduced — this is purely error-message plumbing inside a maintenance drift checker. No CLAUDE.md/AGENTS.md/rules-file changes in this delta, so the instruction-surface-deletion check doesn't apply.

Every other file in this PR was reviewed at 7f7cb4f and is unchanged since.
· Branch

[claude]

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

---

Code review complete

• Gather context (incremental diff since 7f7cb4f, prior review state)
• Understand the request (incremental re-review of the 3 files touched since the last review round)
• Review changed hunks for correctness, maintainability, test coverage
• Adversarially verify candidate findings
• Post findings (none required)

Scope: incremental diff only, per the review-scope header — lockstep-behavior.mjs, lockstep-drift.mjs, lockstep-drift.test.mjs, as changed by 2db3648 since the last-reviewed commit 7f7cb4f. The other two files in this PR are unchanged since then and were covered by the prior round.

Result: no findings. This commit is a direct, well-targeted fix for the only finding raised in the last review round (the duplicate/triplicate drift: lines for one root cause).

What changed and why it holds up:

• checkCompositeBehavior gained a wiringLocation = location parameter so its parseCompositeIssueRef call can be told to use the same location string another check already used, rather than always minting an (behavior)-suffixed one.
• collect() now dedupes by exact message before pushing. I checked the blast radius of this: errors is a fresh local array per checkCopies/checkPinnedComposite call (not shared across consumers via checkFleet, which concatenates finished per-consumer arrays with errors.push(...) rather than routing through collect), so the dedup can't mask a distinct consumer's or distinct check's finding — it only collapses byte-identical messages produced within one composite's own check run.
• I traced all three call sites that invoke parseCompositeIssueRef for the gate composite (parseCompositePatterns at line 863, assertCompositeIssueRefDefault at line 973, checkCompositeBehavior at line 975) and the equivalent trio in checkPinnedComposite (lines 1027, 1047, 1048) — all now pass the identical location string into parseCompositeIssueRef, so the thrown DriftError text is byte-identical and collapses to one entry.
• The updated test (lockstep-drift.test.mjs:438-460) now asserts the deduped output with assert.deepEqual (not just errors.some(...)), for both checkCopies and checkPinnedComposite, closing the gap the prior review called out ("the duplication wasn't visible from the test"). The adjacent "drops the #N grammar" test two cases below still expects two distinct messages ((enforcement patterns) vs (behavior)) and is correctly left alone, since those are genuinely different findings, not duplicates.

I could not execute npm run test:pr-convention-lockstep or node --test in this sandbox (command execution requires approval I can't grant here), so this is from static reading of the diff and the fixture/test logic only — I did not independently run the suite to confirm the new assertions pass.
· Branch