fix(source-control): propagate fetch-all-pr-comments jq merge status

[kyle-sexton]

Closes #3483

Summary

fetch-all-pr-comments.sh ended with exit 0 after a jq merge pipeline, so a failed merge still looked like a complete fetch.

Fix

Propagate the merge pipeline status. Empty surfaces stay success (if instead of [[ -n ]] && printf). jq merge failure maps to exit 2.

Verification

fetch-all-pr-comments.test.sh: jq merge failure exits 2; subset merge with INLINE empty still exits 0. All 29 checks passed. scripts/affected-tests.sh --run: 7 shell suites passed.

Related

N/A

  

[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-02T07:13:09.364325Z	08fe388	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 45s —— View job

---

Security review — no issues found

• Gather context (PR diff, changed files)
• Check skip gate
• Review changed files for security-relevant issues
• Adversarial validation of candidate findings
• Post final findings

This PR touches fetch-all-pr-comments.sh, its test file, the plugin manifest version, and the changelog. The functional change is narrowly scoped to exit-code propagation for the final jq -s 'sort_by(.created_at)' merge pipeline:

• The [[ -n ]] && printf → if ... printf; fi swap only changes truthiness semantics under pipefail; it does not change what data flows where.
• $GENERAL / $REVIEWS / $INLINE (which can carry attacker-influenced content, since PR comment bodies are attacker-controlled) are still passed as printf '%s\n' "$VAR" — the format string is the literal '%s\n', not the variable, so this remains safe against format-string injection, same as before the change.
• PR_NUMBER interpolation into the gh api endpoint path (issues/$PR_NUMBER/comments, etc.) is unchanged by this diff — that surface isn't touched here.
• No new external input, no new command construction, no permissions or CI workflow changes, no secrets handling.

No security-relevant surface was introduced or altered by this diff. Skip gate condition 2 applies (no security-relevant surface after reading the diff) — no findings to report.

[github-actions]

Last security-reviewed head: 08fe388ca973ea6799c524cce664ce7896e37009. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[claude]

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

---

Code review complete

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Understand the request (invoke /review:code-review per instructions)
• Review fetch-all-pr-comments.sh diff for correctness
• Review fetch-all-pr-comments.test.sh diff for coverage
• Review plugin.json / CHANGELOG.md changes
• Post findings (inline + summary)

No blocking findings.

Summary

The fix correctly addresses #3483. Walking through the new merge block in
fetch-all-pr-comments.sh#L214-L221:

• The { if ...; if ...; if ...; } group always exits 0 regardless of which surfaces are empty (an if with a false condition and no else returns 0), so under set -o pipefail an empty surface no longer masquerades as a pipeline failure — this is the fix for the "subset merge" case (case 14 in the test).
• Because the group's exit status is effectively always 0, pipefail correctly surfaces jq -s's exit status as the pipeline's status when the merge itself fails, and the new || { ...; exit 2; } catches exactly that case (case 13) without swallowing it in a trailing unconditional exit 0 as before.
• Mapping the merge failure to 2 (rather than propagating jq's native code, e.g. 5) avoids colliding with this script's own "prerequisite missing" exit code — a sensible, documented choice.
• The two new test cases are genuinely discriminating: case 13 stubs only jq -s (surface-level jq -c calls still run against the real binary), so it can't be satisfied by an unrelated surface-parse exit 2; case 14 exercises the specific [[ -n ]] && printf → if regression (INLINE empty, others populated).
• Version bump (0.55.35 → 0.55.36) and CHANGELOG.md entry are consistent and accurately describe the change; no other file references the old version string.

I was not able to execute the shell test suite myself in this sandbox (Bash execution required interactive approval that wasn't available here), so the PR's "29 checks passed" / affected-tests.sh results are author-claimed and unverified by me — the static read-through of both the script and test logic above is independent verification that the logic is sound, but I'd flag that the live run itself is unconfirmed on my end.

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @cursor[bot]'s task in 1m 17s —— View job

---

Code review skipped

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:code-review and follow its instructions
• Check skip gate

Skip gate condition 1 applies: PR is closed or not open. The pre-fetched context for this trigger shows PR State: MERGED, and the local checkout confirms it — git diff origin/main...HEAD is empty because this branch's HEAD already matches origin/main's tip, and the fetch-all-pr-comments.sh jq-merge fix described in this PR's body is already present in that tip commit. There is no open PR surface left to review or anchor inline comments to.

Per /review:code-review's skip gate, I'm stopping here and posting no inline findings. For reference, this PR already received three prior review passes while open (Codex, and two Claude runs for security and code review) that found no blocking issues.