Found by the repo-wide /code-tidying:batch-simplify sweep and confirmed by reading the file. Reported, not fixed — the sweep is behavior-preserving, and a fix changes exit-status behavior.
What happens
plugins/source-control/scripts/fetch-all-pr-comments.sh ends like this:
{
[[ -n "$GENERAL" ]] && printf '%s\n' "$GENERAL"
[[ -n "$REVIEWS" ]] && printf '%s\n' "$REVIEWS"
[[ -n "$INLINE" ]] && printf '%s\n' "$INLINE"
} | jq -s 'sort_by(.created_at)'
exit 0The exit 0 discards the pipeline's status. If the jq -s merge fails — malformed JSON from any of the three sources, a jq error, a truncated response — the script still exits 0 with partial output or no output at all.
Why it matters
This is a silent-wrong-answer failure, not a noisy one. Every caller reads the exit status to decide whether it has the full comment set. A caller that gets exit 0 has no way to distinguish "this PR has these comments" from "the merge blew up and you are looking at nothing". On the babysit-PRs path that means triage decisions get made against a comment set the script never actually assembled.
The repo already treats this class as load-bearing elsewhere: validate.mjs in the linear schema-check tree carries a comment making exactly this argument — "a check that prints FAIL and exits 0 cannot be a regression check: every caller reads the status, and a green status over red output is exactly the vacuous pass this harness exists to rule out."
Suggested direction
Propagate the pipeline's status rather than discarding it. set -o pipefail is worth checking for here — the three [[ -n … ]] && printf compound commands inside the group each return non-zero when their variable is empty, which is a legitimate state, so a naive pipefail addition would need those arms restructured (a plain if, or a trailing :) to avoid turning an empty section into a spurious failure.
Whatever shape the fix takes, it changes behavior: invocations where the merge currently fails silently would start failing loudly, which is the point.
Found by the repo-wide
/code-tidying:batch-simplifysweep and confirmed by reading the file. Reported, not fixed — the sweep is behavior-preserving, and a fix changes exit-status behavior.What happens
plugins/source-control/scripts/fetch-all-pr-comments.shends like this:{ [[ -n "$GENERAL" ]] && printf '%s\n' "$GENERAL" [[ -n "$REVIEWS" ]] && printf '%s\n' "$REVIEWS" [[ -n "$INLINE" ]] && printf '%s\n' "$INLINE" } | jq -s 'sort_by(.created_at)' exit 0The
exit 0discards the pipeline's status. If thejq -smerge fails — malformed JSON from any of the three sources, a jq error, a truncated response — the script still exits 0 with partial output or no output at all.Why it matters
This is a silent-wrong-answer failure, not a noisy one. Every caller reads the exit status to decide whether it has the full comment set. A caller that gets exit 0 has no way to distinguish "this PR has these comments" from "the merge blew up and you are looking at nothing". On the babysit-PRs path that means triage decisions get made against a comment set the script never actually assembled.
The repo already treats this class as load-bearing elsewhere:
validate.mjsin the linear schema-check tree carries a comment making exactly this argument — "a check that prints FAIL and exits 0 cannot be a regression check: every caller reads the status, and a green status over red output is exactly the vacuous pass this harness exists to rule out."Suggested direction
Propagate the pipeline's status rather than discarding it.
set -o pipefailis worth checking for here — the three[[ -n … ]] && printfcompound commands inside the group each return non-zero when their variable is empty, which is a legitimate state, so a naivepipefailaddition would need those arms restructured (a plainif, or a trailing:) to avoid turning an empty section into a spurious failure.Whatever shape the fix takes, it changes behavior: invocations where the merge currently fails silently would start failing loudly, which is the point.