Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
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
5 changes: 4 additions & 1 deletion prompts/cascade-action.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ ESCALATE_TO_AI=$(jq -r 'if .decision == "escalate" and .risk != "HIGH" then "tru
# a metadata digest so the demanded metadata fix (which mints no commit) re-arms the
# re-review; a false value keeps commit-only re-arm. Default false.
METADATA_ONLY=$(jq -r 'if .decision == "escalate" and (.metadata_only == true) then "true" else "false" end' "$FINAL_RESULT")
# Marker vocabulary: "approved"/"escalated" — the posting guard and review-cycle
# consumers expect these exact strings, never the raw verdict word (#2169)
DECISION_MARKER=$([ "$DECISION" = "approve" ] && echo "approved" || echo "escalated")
```

2. Compose the review body. Write it to a temp file to avoid shell quoting issues:
Expand Down Expand Up @@ -80,7 +83,7 @@ Replace each PLACEHOLDER with the actual values using sed. Then substitute the r
HEADING=$([ "$DECISION" = "approve" ] && echo "APPROVED ✓" || echo "NEEDS HUMAN REVIEW")
sed -i \
-e "s|PLACEHOLDER_SHA|$PR_HEAD_SHA|g" \
-e "s|PLACEHOLDER_DECISION|$DECISION|g" \
-e "s|PLACEHOLDER_DECISION|$DECISION_MARKER|g" \
-e "s|PLACEHOLDER_RISK|$RISK|g" \
-e "s|PLACEHOLDER_HEADING|$HEADING|g" \
-e "s|PLACEHOLDER_TIER|$FINAL_TIER|g" \
Expand Down
25 changes: 25 additions & 0 deletions scripts/post-pr-review.sh
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,31 @@ BODY=$(jq -r '.body // ""' "$VERDICT_JSON")
# body edit (AC3).
METADATA_ONLY=$(jq -r '.metadata_only // false' "$VERDICT_JSON")

# Derive the approval marker word from the verdict, not from model-written text
# (issue #2169). The verdict vocabulary is `approve`, but every marker consumer
# (the #1754 guard below, review-one-pr.sh, carry-forward, the miss-rate metric)
# expects the past tense `decision=approved`. cascade-action.md substituted the
# raw verdict word, so a valid approve verdict failed the guard on formatting
# alone and nothing was posted. Normalize only the present-tense word inside the
# head-SHA v1 marker; a missing or contradictory decision still fails closed.
if [ "$DECISION" = "approve" ]; then
# -z treats the body as one record so a marker wrapped across lines still
# matches. Detect changes without command substitution stripping newlines: write
# to temp files and compare via cmp, so a trailing-newline-only difference
# doesn't falsely trigger a substitution when sed made no actual change (#2169).
BODY_FILE="$(mktemp)"
NORMALIZED_FILE="$(mktemp)"
printf '%s' "$BODY" > "$BODY_FILE"
sed -E -z \
"s/(<!-- pr-review-agent v1 sha=${PR_HEAD_SHA}[[:space:]][^>]*decision=)approve([[:space:]]|-->)/\1approved\2/" \
"$BODY_FILE" > "$NORMALIZED_FILE"
if ! cmp -s "$BODY_FILE" "$NORMALIZED_FILE"; then
echo " approval marker said decision=approve — normalized to decision=approved from the verdict (#2169)"
BODY="$(cat "$NORMALIZED_FILE")"
fi
rm -f "$BODY_FILE" "$NORMALIZED_FILE"
fi

# Marker keying the single human-escalation artifact (issue #1754 AC2). An
# escalation must leave a visible, updatable comment on the PR — not just a
# label. The comment is upserted (created once, then patched in place) so a
Expand Down
73 changes: 73 additions & 0 deletions tests/test_post_pr_review_escalation.bats
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,79 @@ write_approve_verdict() {
[ "$(cat "$POSTED_REVIEWS_FILE")" = "[]" ]
}

# ── #2169: the marker word is derived from the verdict, not model text ──────
# prompts/cascade-action.md substituted the verdict's raw `decision` value
# (`approve`) into the marker, while the #1754 guard requires `decision=approved`.
# A valid approve verdict then failed on formatting alone and nothing was posted.

@test "#2169: approve verdict whose marker says decision=approve is submitted as decision=approved" {
local body="<!-- pr-review-agent v1 sha=${SHA} decision=approve risk=MEDIUM -->

## Automated review — APPROVED ✓"
local vf; vf=$(write_approve_verdict "$body")
run bash "$POST_SCRIPT" "$PR_URL" "$vf" "false"
echo "$output" >&2
[ "$status" -eq 0 ]
[[ "$output" != *"lacks a complete approval marker"* ]]
grep -qF "<!-- pr-review-agent v1 sha=${SHA} decision=approved risk=MEDIUM -->" "$REVIEW_OUT"
! grep -qE 'decision=approve([[:space:]]|-->)' "$REVIEW_OUT"
}

@test "#2169: decision=approve directly before the marker close is normalized too" {
local body="<!-- pr-review-agent v1 sha=${SHA} risk=LOW decision=approve-->

## Automated review — APPROVED ✓"
local vf; vf=$(write_approve_verdict "$body")
run bash "$POST_SCRIPT" "$PR_URL" "$vf" "false"
echo "$output" >&2
[ "$status" -eq 0 ]
grep -qF "decision=approved-->" "$REVIEW_OUT"
}

@test "#2169: decision=approve for a STALE sha still fails closed" {
local old="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
local body="<!-- pr-review-agent v1 sha=${old} decision=approve risk=LOW -->

## Automated review — APPROVED ✓"
local vf; vf=$(write_approve_verdict "$body")
run bash "$POST_SCRIPT" "$PR_URL" "$vf" "false"
echo "$output" >&2
[ "$status" -eq 1 ]
[ ! -s "$REVIEW_OUT" ]
[ "$(cat "$POSTED_REVIEWS_FILE")" = "[]" ]
}

@test "#2169: an approve verdict whose marker contradicts it (decision=escalate) still fails closed" {
local body="<!-- pr-review-agent v1 sha=${SHA} decision=escalate risk=HIGH -->

## Automated review — NEEDS HUMAN REVIEW"
local vf; vf=$(write_approve_verdict "$body")
run bash "$POST_SCRIPT" "$PR_URL" "$vf" "false"
echo "$output" >&2
[ "$status" -eq 1 ]
[[ "$output" == *"lacks a complete approval marker"* ]]
[ ! -s "$REVIEW_OUT" ]
[ "$(cat "$POSTED_REVIEWS_FILE")" = "[]" ]
}

@test "#2169: decision=approve outside the v1 marker (prose) is not rewritten and still fails closed" {
local body="<!-- pr-review-agent v1 sha=${SHA} risk=LOW -->

The verdict was decision=approve in prose only."
local vf; vf=$(write_approve_verdict "$body")
run bash "$POST_SCRIPT" "$PR_URL" "$vf" "false"
echo "$output" >&2
[ "$status" -eq 1 ]
[ ! -s "$REVIEW_OUT" ]
}

@test "#2169: cascade-action prompt maps the decision word before substituting the marker" {
local prompt="$REPO_ROOT/prompts/cascade-action.md"
grep -qF 'DECISION_MARKER=$([ "$DECISION" = "approve" ] && echo "approved" || echo "escalated")' "$prompt"
grep -qF 's|PLACEHOLDER_DECISION|$DECISION_MARKER|g' "$prompt"
! grep -qF 's|PLACEHOLDER_DECISION|$DECISION|g' "$prompt"
}

@test "AC2: exit 100 (not 101) when no escalation comment can be confirmed" {
export COMMENT_FAILS=1
local vf; vf=$(write_escalate_verdict)
Expand Down
Loading