Skip to content

fix: validate decision field after extract_verdict_json in single-review loop #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
don-petry merged 4 commits into from
May 17, 2026
Merged

fix: validate decision field after extract_verdict_json in single-review loop #153

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d50fc7a
fix: validate decision field after extract_verdict_json in single-rev…
May 13, 2026
3747c19
Merge branch 'main' into fix/single-review-empty-decision
don-petry May 17, 2026
b1e01f5
Merge branch 'main' into fix/single-review-empty-decision
don-petry May 17, 2026
c09cfbd
Merge branch 'main' into fix/single-review-empty-decision
don-petry May 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions scripts/review-one-pr.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,11 +402,19 @@ if [ "$TRIAGE_ESCALATE" = "false" ]; then
fi

if [ "$single_rc" -eq 0 ] && extract_verdict_json "$VERDICT_JSON.raw" "$VERDICT_JSON"; then
single_ok=true
break
EXTRACTED_DECISION=$(jq -r '.decision // ""' "$VERDICT_JSON" 2>/dev/null || true)

@gemini-code-assist gemini-code-assist Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To improve robustness against potential LLM formatting variability (such as unexpected leading or trailing whitespace in the JSON string), it is recommended to trim the extracted decision value before validation. This ensures that values like " approve" are correctly matched by the case statement.

Additionally, since this script implements a retry loop for LLM-based tasks, ensure that rate limits are detected immediately by checking exit codes and scanning output for rate-limit indicators. Per repository standards, if a rate limit is detected, the script should exit with code 2 to enable engine fallback mechanisms in the calling process.

For a more complete fix, consider centralizing this validation, sanitization, and rate-limit detection within the extract_verdict_json function in scripts/engine.sh so that all tiers benefit.

Suggested change
EXTRACTED_DECISION=$(jq -r '.decision // ""' "$VERDICT_JSON" 2>/dev/null || true)
EXTRACTED_DECISION=$(jq -r '.decision // ""' "$VERDICT_JSON" 2>/dev/null | tr -d '[:space:]' || true)
References
  1. In shell scripts implementing retry loops for LLM-based tasks, detect rate limits immediately and exit with code 2 to enable engine fallback mechanisms.

case "$EXTRACTED_DECISION" in
approve|escalate|skip)
single_ok=true
break
;;
*)
echo " [approve] single-review attempt $single_attempt/$SINGLE_REVIEW_MAX_RETRIES produced invalid decision '$EXTRACTED_DECISION' — will retry"
;;
esac
else
echo " [approve] single-review attempt $single_attempt/$SINGLE_REVIEW_MAX_RETRIES produced no valid JSON (exit $single_rc)"
fi

echo " [approve] single-review attempt $single_attempt/$SINGLE_REVIEW_MAX_RETRIES produced no valid JSON (exit $single_rc)"
head -20 "$VERDICT_JSON.raw" 2>/dev/null || true
[ -n "$SINGLE_STDERR" ] && echo " stderr: $SINGLE_STDERR"
if [ "$single_attempt" -lt "$SINGLE_REVIEW_MAX_RETRIES" ]; then
Expand Down
Loading