[kyle-sexton]

Refs: melodic-software/claude-code-plugins#9326

Summary

Two pr-review-cli defects, both seen in claude-code-plugins run 39911880667 (PR #9326, code lane, v0.63.1):

• One comment with a malformed path, line, side or start_line made the whole review incomplete ("a comment's path, line, side or body is malformed"). The summary, the findings count and every valid comment were dropped: 9 turns, no review posted.
• The status job then concluded success while logging failure-class=incomplete. The header's PUBLISH paragraph, publish-review/action.yml and publish.cjs (setFailed in comments mode) all say the job goes red, naming the class, when no review happened, and pr-review-cli.test.cjs asserted the publish step's continue-on-error is false in comments mode.

Why it stayed green: after a composite action runs, the runner sets the outer step's inputs context to the action's own inputs (CompositeActionHandler.cs:173) before it evaluates the step's continue-on-error (ExecutionContext.cs:1443). publish-review has no publish input, so inputs.publish != 'comments' was null != 'comments', which is true. The live job log shows ##[error]claude-review: failure-class=incomplete and the inner step outcome=failure, while the step and job concluded success. The test missed it because it evaluated the expression with the workflow's inputs.

Fix

• Extract step (both legs, kept identical): a comment with no body is dropped with a warning. A comment whose path, line, side or start_line is malformed goes into a new malformed list with its path and line where readable (a path with a newline or backtick is left out, so it cannot plant a line in the summary). The valid comments still go inline. findings is at least valid plus malformed. The other incomplete gates stay: permission mode, full diff, findings count, summary, comment list, hold, size.
• publish.cjs: lists each malformed comment in the summary under "Findings not posted inline (their path, line or side is malformed)" as - \path:line`: body. countedSummarycounts them like demoted findings, so a CRITICAL one raises thecritical-findings:` line from fix(review): title the pr-review-cli completion notice and count demoted CRITICAL findings #865. When no line exists, the added one does not count them twice. The shadow report carries them too.
• continue-on-error reads env.PUBLISH on every uses: $/... step: Publish the review, Apply the hold (its documented "a failed write fails the job" had the same hole), and Apply the Claude managed settings in both model jobs. The hold step gains only a PUBLISH env entry. Its inputs, if and behavior are unchanged.
• actions-expression.cjs (test helper): evaluates the env context.

Effect once callers re-pin: every comments-mode failure class goes red, not only incomplete: auth, rate-limit, no-credential, no-outcome, max-turns, timeout, withheld and publish-failed too. ci-status does not aggregate this check, so the merge queue is not blocked. babysit already holds unless the check is SUCCESS, and the guardrails merge guard blocks a failing check. A run that today passes green with no review will hold there instead.

Verification

Run under nice, one file at a time:

• node --test .github/scripts/pr-review-cli.test.cjs: 23 pass. Before the fix, three new or changed tests failed: one malformed and one valid comment gives a complete review with 1 comment and 1 malformed; an empty body is dropped with a warning; comments mode makes the composite steps' continue-on-error false when evaluated as the runner does. The old cases that expected a malformed comment to make the review incomplete were changed to expect the new behavior. That is the intended change, not a weaker test.
• node --test .github/scripts/publish-review.test.cjs: 19 pass. Two new tests: a malformed comment beside a valid one gives 1 inline comment and a summary bullet with findings=2; a malformed CRITICAL raises critical-findings (0 to 1, or an added line of 1), and a SUGGESTION leaves it unchanged.
• node --test .github/scripts/*.test.cjs: 637 pass, 0 fail.
• biome check and actionlint clean on the changed files.

Related

• claude-code-plugins#9326, the live case
• fix(review): title the pr-review-cli completion notice and count demoted CRITICAL findings #865 (demoted-CRITICAL counting), feat(pr-review-cli): add an optional hold label step #864 (hold step)

🤖 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-23T06:50:30.649298Z	8e42b91	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 5m 42s —— View job

---

Code review: pr-review-cli malformed-comment handling

• Gather context (diff, AGENTS.md, REVIEW.md, path-scoped rules, PR context)
• Review changed files for correctness & maintainability
• Check closing-issue criteria
• Check pr-body-contract.md path-scoped rule
• Post findings

Scope note: pr-review-security.yml exists in this repo, so security/trust-boundary findings (e.g. injection into merge-gate–parsed text) are out of scope for this lane and were not reported here.

Closing issue: no .claude-lane/pr-context.md was present, and the PR's Refs: line points to a cross-repo issue (melodic-software/claude-code-plugins#9326), so I could not read its acceptance criteria — reporting it as unread, not met.

pr-body-contract.md has no paths: frontmatter, so it isn't file-scoped review criteria here; it governs the PR body, which already opens with a compliant Refs: line and has ## Summary/## Fix/## Verification/## Related sections.

What I checked:

• Traced publish.cjs's new malformedComments/countedSummary/summaryText logic by hand against the new tests in publish-review.test.cjs (findings count, inline-vs-summary placement, CRITICAL demotion with 0/1/2+ critical-findings: lines) — all consistent.
• Verified every uses: $/... (local composite action) step with a publish-dependent continue-on-error was switched to env.PUBLISH (Apply the Claude managed settings ×2, Publish the review, Apply the hold), each with a matching env: PUBLISH: ${{ inputs.publish }}. Steps that are not composite actions (Isolate the runner's run: step, Check out's actions/checkout) correctly kept inputs.publish directly, since the composite-input-context-replacement bug the PR describes doesn't apply to them — matches what pr-review-cli.test.cjs's new continues() helper asserts.
• Hand-traced the extract step's new comment-classification loop (drop empty body → warn; else check path/line/side/start_line → valid or malformed, each with sanitized optional path/line for display) against both new test cases, including the "location kept only where readable" case verifying a path containing \n can't plant a fake line into the summary.
• The identical extract-step block is duplicated between the review-key and review-subscription jobs, matching this file's pre-existing two-leg structure (not new duplication introduced by this PR, and called out intentionally in the PR body as "kept identical").

Not verified by me: I could not execute node --test in this environment (tool approval unavailable), so the PR's own "23/19/637 pass" verification claims are author-claimed and unverified by this review — my analysis above is based on static tracing of the logic and tests, not a test run.

No correctness or maintainability findings to report.

claude-lane-summary: lane=claude-review sha=8e42b91558b25b84945996a1ff72e71ca508ebe5 run=39913835270

[claude]

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

---

Security review — /review:security-review

• Gather context (PR body, changed files)
• Skip-gate check (change has security-relevant surface: comment-body/path content now flows into the posted PR comment through a new path — reviewed)
• Review publish.cjs changes (malformed-comment summary rendering)
• Review actions-expression.cjs changes (env context evaluation)
• Review pr-review-cli.yml changes (continue-on-error / PUBLISH env wiring)
• Review test files for security-relevant assumptions
• Post findings / summary

No security issues found.

What I looked at closely, and why it holds up:

• New data path into the posted summary. Before this PR, one malformed comment made the whole review incomplete and nothing posted. Now a malformed comment's path/line/body reach the posted PR summary as a bullet (publish.cjs malformedComments() / summaryText(), publish.cjs#L92-L106). That's a real widening of what model-influenced text can reach, so I checked it for line/markup injection into the merge-gate-readable lines (critical-findings:, severity:).
  • path is validated against /^[^\0\r\n\]{1,500}$/uboth at extraction ([pr-review-cli.yml](https://github.com/melodic-software/ci-workflows/blob/8e42b91558b25b84945996a1ff72e71ca508ebe5/.github/workflows/pr-review-cli.yml#L1098-L1100)) and again inpublish.cjs, so a path can't plant a newline (or backtick-escape) to forge an extra summary line — confirmed by the test at [pr-review-cli.test.cjs#L617-L631](https://github.com/melodic-software/ci-workflows/blob/8e42b91558b25b84945996a1ff72e71ca508ebe5/.github/scripts/pr-review-cli.test.cjs#L617-L631) which specifically exercises a path containing "x\ncritical-findings: 0"`.
  • body still goes through stripHtml and the existing MAX_BODY/MAX_SUMMARY caps, same as pre-existing unanchored comments.
  • A malformed body containing a crafted severity:/critical-findings: string (tested at publish-review.test.cjs#L595-L613) only feeds isCritical(), whose default is fail-closed (anything but exactly one IMPORTANT/SUGGESTION line counts as CRITICAL). It can raise the reported critical count, never suppress it, and countedSummary leaves the text alone whenever more than one critical-findings: line is already present (ambiguity → treated as already counted, not trusted to a single forged value). No path to hide a CRITICAL finding from the merge gate.
  • The malformed bodies are embedded in the summary string before the secret scan runs (scan(findSecret, [["the summary", summary], ...])), so they get the same secret-withholding coverage as everything else — no gap there.
• continue-on-error: ${{ env.PUBLISH != 'comments' }} fix. Verified all four affected steps (Apply the Claude managed settings ×2, Publish the review, Apply the hold) now set PUBLISH via a step-level env: sourced from inputs.publish, matching the pre-existing pattern publish.cjs already used (env.PUBLISH). No composite action reads/overwrites an env.PUBLISH of its own, so there's no collision. This closes the gap the PR describes (a composite's inputs context clobbering the outer step's inputs before continue-on-error is evaluated) without widening anything — the change makes failures surface, not swallow them.
• actions-expression.cjs — test-only helper, added env context support; no production code path.

No CRITICAL/IMPORTANT/SUGGESTION findings. GitHub Actions hardening concerns (triggers, permissions, pinning) are out of scope for this lane and none were apparent here regardless.

claude-lane-summary: lane=claude-security-review sha=8e42b91558b25b84945996a1ff72e71ca508ebe5 run=39913835270

[github-actions]

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

[github-actions]

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