fix(review-utils): guard unbound SOURCE_HEAD_SHA in metrics-gate script

[nsheaps]

Bug

read-metrics-and-compute-conclusion.sh line 81 (the fail-closed "no metrics + no matching review" error message) interpolates $SOURCE_HEAD_SHA directly, without the ${VAR:-} default used everywhere else it's referenced in this script. Under set -euo pipefail, if SOURCE_HEAD_SHA is genuinely unset in the calling environment, this crashes with unbound variable instead of reaching the intended fail-closed output.

Impact

Instead of the specific title "Review agent finished but metrics missing" (with a useful ::error:: annotation), the check-run shows the generic "The review agent failed to run" — losing diagnostic signal and masking what's actually a normal (if unwanted) fail-closed outcome as an infra-looking crash.

Root cause

Observed live on #333 and on #334's own self-dispatch review: both ran against .ai-agent-henry's repo-dispatch.yaml, which pins review-receiver.yaml at a commit predating #334's SOURCE_HEAD_SHA/APP_SLUG env plumbing — so the env var was never set at all. Companion fix: nsheaps/.ai-agent-henry#(pin bump PR) bumps that pin so the fallback actually has the data it needs going forward. This PR is the defensive half — the script should never crash on missing optional env, regardless of which caller/pin is in play.

Verification

• bash -n — syntax OK.
• Traced: this is the only unguarded reference to $SOURCE_HEAD_SHA; the other two (line ~43, ~50) are inside the if [ -n "${SOURCE_HEAD_SHA:-}" ]-guarded block where it's guaranteed non-empty.

---

Generated by Claude Code

[henry-nsheaps]

Review Summary

✅ APPROVE — Correct, minimal, well-motivated one-line defensive fix. Ship it.

Assessment

This PR patches the last unguarded $SOURCE_HEAD_SHA reference in read-metrics-and-compute-conclusion.sh (line 81, inside the fail-closed ::error:: message emitted when both the metrics file and the review-API fallback fail to yield a verdict). Under set -euo pipefail, if SOURCE_HEAD_SHA was truly unset in the receiver's env — which was the observed case on #333 and #334 where the caller's repo-dispatch.yaml pinned an older review-receiver.yaml predating the SOURCE_HEAD_SHA/APP_SLUG plumbing — the script would crash with unbound variable at the very moment it was trying to emit its structured fail-closed diagnostic. Net effect: the check-run surfaced as generic "The review agent failed to run" (infra-looking) instead of the intended "Review agent finished but metrics missing" title with ::error:: annotation. The diagnostic signal was being eaten by the diagnostic path itself.

How I arrived at the verdict

Correctness (✅): Verified by grep on the head ref that SOURCE_HEAD_SHA appears in exactly 5 non-comment sites:

• Line 32: guard — [ -n "${SOURCE_HEAD_SHA:-}" ] — safe
• Line 43: inside the guarded block — safe (guard proved non-empty)
• Line 50: inside if [ -n "$review_state" ], which is only reachable if the guard on line 32 passed — safe
• Line 81: previously the sole unguarded reference — now fixed

The fix uses ${SOURCE_HEAD_SHA:-<unset>} rather than ${SOURCE_HEAD_SHA:-} — a small but nice touch, since the literal <unset> in the error output tells whoever reads the annotation exactly why the SHA is missing (env var absent, not e.g. a git detach). More diagnostic than an empty string, at zero cost.

Consistency (✅): The ${VAR:-} idiom is already the house style throughout this script for optional env; this line was the outlier. Fix brings it into line.

Scope discipline (✅): Companion pin-bump PR is called out in the description and correctly framed as separate work — this PR is explicitly the defensive half so the script never crashes regardless of caller pin. Good KISS + separation of concerns.

Test coverage: No new tests, and none warranted — this is a one-line string change in a shell script's error branch. The bash -n syntax check + manual trace of unguarded references (which I re-verified above) is the appropriate bar here.

PR description accuracy (✅): Bug, impact, root cause, and verification section all match what I observe in the code.

Risk: Effectively zero. The change is a strict superset in behavior — where the old code crashed, the new code prints an annotation with a placeholder; where the old code worked, the new code prints the identical annotation.

Strengths

• 🎯 Surgical fix — one line, exactly the site that needed it, no drive-by changes.
• 📝 Excellent PR description: bug / impact / root cause / verification structure with links to the concrete failing runs.
• 🧠 <unset> placeholder chosen over empty string — actively useful when reading the error annotation cold.
• 🔗 Companion PR for the underlying pin drift is correctly scoped to a separate change.

Follow-ups

None. All the "should we also…" thoughts I had (add a shellcheck job, add set -x tracing, refactor the metrics reader) are out of scope for this PR and would violate the KISS/YAGNI framing the author is (correctly) applying.

---

_{Reviewed by Claude · opus-4-7}

Dismissed: new commits since last approval require re-review.

[henry-nsheaps]

Review Summary

✅ APPROVE — Correct, minimal, well-motivated one-line defensive fix. Ship it.

Assessment

This PR patches the last unguarded $SOURCE_HEAD_SHA reference in .github/scripts/review-receiver/read-metrics-and-compute-conclusion.sh (line 81, inside the fail-closed ::error:: message emitted when both the metrics file and the review-API fallback fail to yield a verdict). Under set -euo pipefail, if SOURCE_HEAD_SHA was truly unset in the receiver's env — the observed case on #333 and #334, where the caller's repo-dispatch.yaml pinned an older review-receiver.yaml predating the SOURCE_HEAD_SHA/APP_SLUG plumbing — the script would crash with unbound variable at the very moment it was trying to emit its structured fail-closed diagnostic. Net effect: the check-run surfaced as generic "The review agent failed to run" (infra-looking) instead of the intended "Review agent finished but metrics missing" title with ::error:: annotation. The diagnostic signal was being eaten by the diagnostic path itself.

How I arrived at the verdict

Correctness (✅): Verified by grep on the head ref that SOURCE_HEAD_SHA appears in exactly 5 sites in the script:

• Line 10: docstring — n/a
• Line 32: guard — [ -n "${SOURCE_HEAD_SHA:-}" ] — safe
• Line 43: inside the guarded block — safe (guard proved non-empty)
• Line 50: inside if [ -n "$review_state" ], which is only reachable when the line-32 guard passed — safe
• Line 81: previously the sole unguarded reference — now fixed

The fix uses ${SOURCE_HEAD_SHA:-<unset>} rather than ${SOURCE_HEAD_SHA:-} — a small but nice touch, since the literal <unset> in the error output tells whoever reads the annotation exactly why the SHA is missing (env var absent, not e.g. an empty string from upstream). More diagnostic than an empty string, at zero cost.

Consistency (✅): The ${VAR:-} idiom is already the house style throughout this script for optional env; this line was the outlier. Fix brings it into line.

Scope discipline (✅): Companion pin-bump PR is called out in the description and correctly framed as separate work — this PR is explicitly the defensive half so the script never crashes regardless of caller pin. Good KISS + separation of concerns.

Test coverage: No new tests, and none warranted — this is a one-line string change in a shell script's error branch. The bash -n syntax check + manual trace of unguarded references (which I re-verified above) is the appropriate bar here.

PR description accuracy (✅): Bug, impact, root cause, and verification sections all match what I observe in the code.

Risk: Effectively zero. The change is a strict superset in behavior — where the old code crashed, the new code prints an annotation with a <unset> placeholder; where the old code worked, the new code prints the identical annotation.

Strengths

• 🎯 Surgical fix — one line, exactly the site that needed it, no drive-by changes.
• 📝 Excellent PR description: bug / impact / root cause / verification structure with links to the concrete failing runs.
• 🧠 <unset> placeholder chosen over empty string — actively useful when reading the error annotation cold.
• 🔗 Companion PR for the underlying pin drift is correctly scoped to a separate change.

Follow-ups

None. All the "should we also…" thoughts (add a shellcheck job, add set -x tracing, refactor the metrics reader) are out of scope for this PR and would violate the KISS/YAGNI framing the author is (correctly) applying.

---

_{Reviewed by Claude · opus-4-7}