refactor(workflows): repoint the review lanes at the claude-lane composites (PR-A2)

[kyle-sexton]

Summary

PR-A2 of the claude-review-lanes plan (docs/topics/claude-review-lanes/PLAN.md, Phase 2a): both reusable review lanes replace their embedded freshness / outcome / marker-comment steps with the composite trio merged in PR-A1 (#274), pinned at its merge SHA b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd. Local ./ refs cannot work from a reusable workflow (they resolve against the caller's checkout), so the references are full-SHA self-reference pins with # <short-sha> <date> comments.

• claude-review.yml + claude-security-review.yml: freshness, outcome, and both marker-comment steps become composite references; downstream gates move to the composites' kebab-case outputs (review-failed, failure-class, review-detail).
• The four generated-embed classifier files retire (classify-infra-failure.sh, its .test.sh, render-classify-infra-failure.cjs, classify-infra-failure-render.test.cjs), with their ci.yml test step and selector-conformance.yml path triggers + test step.
• Security lane keeps ci(claude-security-review): infra-failed review reports success and satisfies a required context, authorizing merge without a security pass #266's fail-closed contract: the outcome composite records failure without failing; a new inline Fail closed on an in-scope non-run step owns the required-check red under the same pull_request-only carve-out. Its expanded marker body rides body-copy as blockquote-continuation lines (input description updated — docs-only, behavior identical at the pinned SHA).
• Test suites re-pinned: superseded-guard asserts the freshness composite pin; the fail-closed suite drops executed-bash classifier cases (owned by classify.test.cjs's ported corpus) and pins the new wiring shape.
• PLAN.md: Phase 1 marked DONE with merged-main sanity evidence; 2a-addendum records this execution shape.

Verified locally: node --test .github/scripts/*.test.cjs (276 pass), node --test .github/actions/claude-lane-outcome/*.test.cjs, actionlint on all four touched workflows, zizmor findings identical to the pre-change baseline, markdownlint clean.

No linked issue.

Related

• feat(actions): extract the Claude lane composite trio (PR-A1) #274 (PR-A1 — composite extraction this repoints at)
• ci(claude-security-review): infra-failed review reports success and satisfies a required context, authorizing merge without a security pass #266 (fail-closed contract preserved through the repoint)
• standards#278 (Phase 1, merged)

🤖 Generated with Claude Code

[claude]

PR Review in Progress

• Gather context and understand the request
• Review the PR diff
• Examine changed workflow files
• Examine changed test files
• Check security alignment with CLAUDE.md
• Post comprehensive review feedback

View job run

[claude]

Security Review

• Gather PR diff and changed files
• Read claude-review.yml and claude-security-review.yml changes
• Read claude-lane-marker-comment/action.yml changes
• Read test file changes
• Analyze trust boundaries, data flow, token handling, injection surfaces
• Report findings

View job run

[claude]

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

---

PR Review

• Gather context and understand the request
• Review the PR diff and workflow files
• Review test file changes
• Check security alignment with CLAUDE.md
• Post comprehensive review feedback

---

Summary

This is a clean, mechanically sound refactor. The three embedded inline blocks (freshness check, outcome reporting, marker-comment lifecycle) are correctly replaced by the composite trio merged in PR-A1, all pinned to the right merge SHA. The fail-closed contract for the security lane is preserved and improved by separating concerns into a dedicated step. No security regressions found.

---

Correctness

Output naming rename (review_failed → review-failed) — confirmed correct.
The old inline run: step set review_failed (underscore) via $GITHUB_OUTPUT. The new composite exports review-failed (hyphen) as its outputs: key. Both consuming conditions in both workflows were updated to match. The fail-closed test at line 174 pins the exact hyphenated key:

/^ {8}if: >-\n {10}always\(\) && steps\.review-outcome\.outputs\.review-failed == 'true' &&/mu

✓ Consistent rename throughout.

Freshness step if: condition moved into the composite — functionally equivalent.
Old code had if: github.event.pull_request.number != '' on the step. New code removes that condition from the workflow step; the composite's internal step carries the identical condition (see claude-lane-freshness/action.yml:31). For non-PR events: the composite runs but its inner step skips → superseded output is empty → downstream != 'true' gates pass through as fresh. The fail-open contract is unchanged.

Fail-closed separation — improved.
Previously the Report review outcome inline shell step contained both failure reporting AND an exit 1. Now the composite handles reporting without failing, and Fail closed on an in-scope non-run owns the exit 1. This is the architecturally correct split: it makes the ownership explicit and testable in isolation. The test at line 164 pins both that continue-on-error is absent and that exit 1 is present.

body-copy multi-line rendering — correct.
The composite prepends > to the start of the entire BODY_COPY env var string (line 119 of action.yml). The YAML |- block in claude-security-review.yml correctly carries self-owned > prefixes on every continuation line, so the rendered blockquote is well-formed. The description update on action.yml clarifies this contract, which was previously undocumented. The rendered output matches what the PR comments already show.

Superseded-guard test gate count — correct.
The test asserts exactly 7 occurrences of steps.freshness.outputs.superseded != 'true' in claude-review.yml. Verified by manual count across the 7 gated steps: checkout, standards token, standards mount path clear, standards checkout, args composition, review itself, and outcome reporting.

---

Security

SHA pinning. All three composite references pin b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd, confirmed as the actual PR-A1 merge SHA in git log. ✓

pull_request_target / workflow_run tripwires. Untouched in both workflows. ✓

Token handling. No composite receives CLAUDE_CODE_OAUTH_TOKEN or any secret. The token exposure surface is unchanged. ✓

Fork-PR guard. Preserved inside the claude-lane-marker-comment composite (action.yml:82–85). The security lane's job-level fork skip is also preserved. ✓

Fail-closed carve-out is still scoped to pull_request. The new Fail closed on an in-scope non-run step gates on github.event.pull_request.number != '', same as the old embedded IS_PULL_REQUEST guard. Non-PR events (merge_group, workflow_dispatch, schedule) pass through without reddening. ✓

display_report hardcoded off in the security lane. Unchanged; not a flippable consumer input. ✓

---

Minor observations

1. Dependabot coverage on self-reference pins.
The CLAUDE.md note "Dependabot covers only those uses: SHAs" implies Dependabot will scan these pins. Since the composites have no semantic version tags, Dependabot won't suggest a bump — the pins will stay at b5d54bf until a manual update. This is expected and acceptable for intra-repo composite refs, but worth confirming your Dependabot config isn't expected to produce PRs for these. No action required unless you want to document this explicitly.

2. Comment format # b5d54bf 2026-07-27 vs # vX.Y.Z.
CLAUDE.md's "trailing # vX.Y.Z comment" convention is written for external actions with version tags. These self-references don't have tags, so the # <short-sha> <date> convention is the right substitute. No issue.

3. selector-conformance.yml still triggers on claude-review.yml and claude-security-review.yml.
The deleted classifier files' path triggers are removed cleanly. The action path triggers for the composite trio are presumably in a separate entry (not modified here). Worth a quick sanity check that the conformance workflow still gets a path trigger for the composites under .github/actions/claude-lane-* if it needs one. (If it already had them before this PR, they're unchanged.)

---

Verdict

No blocking issues. The PR correctly executes the Phase 2a plan: the inline duplicated blocks are gone, behavior is preserved, security invariants hold, and tests pin the new wiring shape. The separation of the fail-closed exit 1 into its own step is a genuine improvement in clarity and testability over the previous embedded approach.

[claude]

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

---

Security Review

• Gather PR context
• Read claude-review.yml changes
• Read claude-security-review.yml changes
• Read claude-lane-marker-comment/action.yml changes
• Read test file changes
• Analyze trust boundaries, data flow, token handling, fail-closed logic, injection surfaces
• Post findings

---

No security issues found. The changes are a clean structural refactoring from inline steps to the pinned composite trio. All load-bearing security invariants are preserved. Detailed reasoning follows.

Trust Boundary and Fail-Closed Integrity

The most security-sensitive change is the migration of the fail-closed logic in claude-security-review.yml. The previous shape embedded the "in scope and did not run → exit 1" decision inside a bash script that received IS_PULL_REQUEST as an env variable. The new shape splits this into two layers:

1. claude-lane-outcome composite (at the pinned SHA): records review-failed=true/false without exiting non-zero itself.
2. New Fail closed on an in-scope non-run step: evaluates steps.review-outcome.outputs.review-failed == 'true' && github.event.pull_request.number != '' directly in the YAML if: expression.

This is strictly more robust than the old approach. The old bash check treated an unset IS_PULL_REQUEST variable as a PR context (fail-closed), which was a belt-and-braces guard against a wiring omission. The new if: condition evaluates github.event.pull_request.number != '' directly from the GitHub Actions context — this expression is evaluated by the runner before the step body executes, cannot be "unset," and cannot be accidentally omitted without removing the entire step. The test suite correctly drops the "unset IS_PULL_REQUEST" case and replaces it with a structural assertion that the step exists and carries exactly this condition.

The always() gate is correctly placed on the fail-closed step: it fires even when the Claude security review action step exited non-zero (which was the design intent). The Report review outcome composite has no continue-on-error, so a composite crash directly fails the job — the correct fail-closed behavior without the explicit step needing to fire.

The superseded-head path is correct: if freshness marks the run superseded, Report review outcome is skipped, its review-failed output is empty, '' == 'true' evaluates to false, and the fail-closed step skips. A stale run cannot raise a spurious red.

Output Name Change (snake_case → kebab-case)

The composite exposes review-failed, failure-class, review-detail (kebab-case) by remapping from core.setOutput("review_failed", ...) (snake_case, required by the github-script runtime) through the composite outputs: block. All downstream references in both workflows (steps.review-outcome.outputs.review-failed, etc.) use the correct kebab-case names. No mismatch.

Composite Pin Security

Both workflows reference the composites at the full 40-char SHA b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd. This is an immutable reference to the repo's own merged history; a PR author cannot influence what code that SHA points to. The comments explain why ./ local refs are unworkable in reusable workflow contexts (they resolve against the caller's checkout, not this repo's).

Tripwire Preservation

Both Reject privileged triggers steps — the load-bearing pull_request_target / workflow_run hard-fail from CLAUDE.md — are unchanged in both workflows (claude-review.yml L191–197, claude-security-review.yml L400–406).

Fork Guard Preservation

The fork-scoped job if: condition in claude-security-review.yml (github.event.pull_request.head.repo.full_name == github.repository) is unchanged. The marker-comment composite independently enforces the same guard at the API call level. Both layers present.

Deleted Code Surface

Removing classify-infra-failure.sh, its test, and the two render files eliminates ~550 lines of bash and JS that were the previous source of the classification logic. The logic now lives exclusively in classify.cjs inside the composite. This is a net reduction in attack surface.

Prompt Construction

The ${{ inputs.prompt }} interpolation in both workflow prompt: blocks is unchanged from the pre-PR state. It is set by the caller's workflow YAML (not by PR metadata), so no new injection surface is introduced.

body-copy Multi-line Expansion

The body-copy input to claude-lane-marker-comment is a hardcoded YAML literal (the |- block scalar in claude-security-review.yml L617–624). No PR-controlled data is interpolated into it. The composite prepends > only to the first line; the subsequent lines carry their own > prefixes as hardcoded content. No injection risk.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b3b292bcbd

ℹ️ 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".