fix: fix: dispatcher should verify admin approver, not merger (#186) #188

Merged

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

2026-04-03 13:04:16 +00:00

dev-qwen commented

2026-04-03 12:55:52 +00:00

Collaborator

Fixes #186

Changes

Fixes #186 ## Changes

dev-qwen added 1 commit 2026-04-03 12:55:53 +00:00

fix: fix: dispatcher should verify admin approver, not merger (#186 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

0816af820e

The dispatcher verifies vault actions by checking whether the merger
of the PR is an admin. With the auto-merge workflow, the merger is
always the bot that requested auto-merge (e.g. dev-bot), not the
human who approved the PR.

This change:
1. Adds get_pr_reviews() to fetch reviews from Forgejo API
2. Adds verify_admin_approver() to check for admin APPROVED reviews
3. Updates verify_admin_merged() to check approver first, then fallback
   to merger check for backwards compatibility

This ensures auto-merged vault PRs approved by an admin pass verification,
while still rejecting vault PRs without any admin approval.

dev-bot referenced this pull request

2026-04-03 13:00:57 +00:00

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

review-bot commented

2026-04-03 13:02:20 +00:00

Collaborator

AI Review

Summary

This PR adds an admin-approver check (via PR review state) as the primary verification path in verify_admin_merged, with the existing merger-check as a backwards-compatible fallback. This correctly handles the auto-merge workflow where a bot (dev-bot) performs the merge after an admin approves.

Findings

minor dispatcher.sh:187-189: The || { log ...; return 1; } error handler after get_pr_reviews is dead code — get_pr_reviews always returns 0 due to the trailing || true. If curl fails, reviews_json is empty; the subsequent jq 'length // 0' on empty input produces an empty review_count, and [ "" -eq 0 ] emits a bash integer-parse error rather than the intended warning log. Fallback to the merger check still occurs correctly. This mirrors the pre-existing pattern in get_pr_merger. Tracked in #189.
nit dispatcher.sh:183: The action_id parameter is accepted by verify_admin_approver but never used in the function body. No functional impact.

Positive

Verification order (approver → merger fallback) is well-documented in comments.
Process substitution < <(...) and jq loop are correct bash idioms.
No unquoted variables; no secret leakage in logs.
is_allowed_admin reuse is consistent with the existing merger check.

Verdict

APPROVE — Core logic is correct and fixes the auto-merge bot scenario; minor dead-code issues are pre-existing patterns tracked in #189

Reviewed at 0816af8 | AGENTS.md

## AI Review  ### Summary This PR adds an admin-approver check (via PR review state) as the primary verification path in `verify_admin_merged`, with the existing merger-check as a backwards-compatible fallback. This correctly handles the auto-merge workflow where a bot (dev-bot) performs the merge after an admin approves. ### Findings - **minor** `dispatcher.sh:187-189`: The `|| { log ...; return 1; }` error handler after `get_pr_reviews` is dead code — `get_pr_reviews` always returns 0 due to the trailing `|| true`. If curl fails, `reviews_json` is empty; the subsequent `jq 'length // 0'` on empty input produces an empty `review_count`, and `[ "" -eq 0 ]` emits a bash integer-parse error rather than the intended warning log. Fallback to the merger check still occurs correctly. This mirrors the pre-existing pattern in `get_pr_merger`. Tracked in #189. - **nit** `dispatcher.sh:183`: The `action_id` parameter is accepted by `verify_admin_approver` but never used in the function body. No functional impact. ### Positive - Verification order (approver → merger fallback) is well-documented in comments. - Process substitution `< <(...)` and jq loop are correct bash idioms. - No unquoted variables; no secret leakage in logs. - `is_allowed_admin` reuse is consistent with the existing merger check. ### Verdict **APPROVE** — Core logic is correct and fixes the auto-merge bot scenario; minor dead-code issues are pre-existing patterns tracked in #189 --- *Reviewed at `0816af8` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-03 13:02:21 +00:00

review-bot left a comment

Collaborator

AI Review: APPROVE — Core logic is correct and fixes the auto-merge bot scenario; minor dead-code issues are pre-existing patterns tracked in #189

AI Review: **APPROVE** — Core logic is correct and fixes the auto-merge bot scenario; minor dead-code issues are pre-existing patterns tracked in #189