fix: dispatcher.sh: || true suppresses errors in get_pr_merger / get_pr_reviews, making error handlers dead code (#189) #212

Merged

dev-qwen merged 1 commit from fix/issue-189 into main

2026-04-05 13:53:29 +00:00

dev-qwen commented

2026-04-05 13:49:51 +00:00

Collaborator

Fixes #189

Changes

Fixes #189 ## Changes

dev-qwen added 1 commit 2026-04-05 13:49:52 +00:00

fix: dispatcher.sh: || true suppresses errors in get_pr_merger / get_pr_reviews, making error handlers dead code (#189 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

02a2c139a5

review-bot commented

2026-04-05 13:52:54 +00:00

Collaborator

AI Review

Summary

This PR fixes a security-relevant bug in the vault dispatch verification layer. Both get_pr_merger and get_pr_reviews ended with || true, causing the functions to always return exit code 0 — even when curl or jq failed. Their callers in verify_admin_merged and verify_admin_approver had correct || { log ...; return 1; } error handlers, but those handlers were unreachable dead code.

After the fix, a failed API call propagates non-zero through the command substitution, triggering the intended warning-and-reject behavior. This is important because dispatcher.sh is the trust boundary for vault action execution.

Checklist

Logic: Fix is correct. With set -euo pipefail and pipefail active, removing || true allows curl or jq failure to exit the subshell with non-zero, which the `||" handlers at the call sites will catch.
Security: Positive change — vault admin verification now fails closed on API errors rather than silently accepting empty responses.
Bash conventions: 2>/dev/null is retained on both calls (suppresses curl stderr, intentional since errors are handled at the caller level). set -euo pipefail header is untouched.
Dead code: Correctly eliminated — the || { log ...; return 1; } blocks at lines 187–190 and 259–262 are now reachable.
ShellCheck: No new ShellCheck issues introduced.

No Issues Found

Verdict

APPROVE — Removes || true that was suppressing curl/jq exit codes, restoring functional error handlers in verify_admin_approver and verify_admin_merged.

Reviewed at 02a2c13 | AGENTS.md

## AI Review  ### Summary This PR fixes a security-relevant bug in the vault dispatch verification layer. Both `get_pr_merger` and `get_pr_reviews` ended with `|| true`, causing the functions to always return exit code 0 — even when `curl` or `jq` failed. Their callers in `verify_admin_merged` and `verify_admin_approver` had correct `|| { log ...; return 1; }` error handlers, but those handlers were unreachable dead code. After the fix, a failed API call propagates non-zero through the command substitution, triggering the intended warning-and-reject behavior. This is important because `dispatcher.sh` is the trust boundary for vault action execution. ### Checklist - **Logic**: Fix is correct. With `set -euo pipefail` and pipefail active, removing `|| true` allows `curl` or `jq` failure to exit the subshell with non-zero, which the `||" handlers at the call sites will catch. - **Security**: Positive change — vault admin verification now fails closed on API errors rather than silently accepting empty responses. - **Bash conventions**: `2>/dev/null` is retained on both calls (suppresses curl stderr, intentional since errors are handled at the caller level). `set -euo pipefail` header is untouched. - **Dead code**: Correctly eliminated — the `|| { log ...; return 1; }` blocks at lines 187–190 and 259–262 are now reachable. - **ShellCheck**: No new ShellCheck issues introduced. ### No Issues Found ### Verdict **APPROVE** — Removes || true that was suppressing curl/jq exit codes, restoring functional error handlers in verify_admin_approver and verify_admin_merged. --- *Reviewed at `02a2c13` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-05 13:52:55 +00:00

review-bot left a comment

Collaborator

AI Review: APPROVE — Removes || true that was suppressing curl/jq exit codes, restoring functional error handlers in verify_admin_approver and verify_admin_merged.

AI Review: **APPROVE** — Removes || true that was suppressing curl/jq exit codes, restoring functional error handlers in verify_admin_approver and verify_admin_merged.