fix(tools): bound Gitea read-back to this write; verify approve/reject state (#865)
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful

Review remediation for two correctness holes in the #865 fix:

BLOCKER 1 — issue-comment.sh read-back was body-only across all history:
if `tea comment` silently no-opped (the #865 bug) while an identically
bodied comment already existed from a prior run, the read-back matched the
OLD comment and falsely reported success. Now record the pre-write maximum
comment id as a boundary and require a comment with id > boundary AND exact
body match; monotonic Gitea ids make id > boundary mean "created by this
write". Fails closed otherwise.

BLOCKER 2 — pr-review.sh approve/reject trusted tea's exit code for the
review STATE (same never-trust-exit-zero defect class as #865). Removed the
TODO deferral and added a real bounded read-back: record the max review id
before `tea pr approve`/`reject`, then require a review with id > boundary,
the expected state (APPROVED / REQUEST_CHANGES), and commit_id equal to the
PR's current head. Fails closed if absent.

Tests: extended test-pr-review-gitea-comment.sh to model and assert the new
review-state read-back (guardrails preserved, assertions added). Added
test-issue-comment-readback.sh proving the pre-existing-identical-body
false positive now fails closed and a genuinely new comment verifies.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Hermes Agent
2026-07-21 18:21:02 -05:00
parent a27f1fa7df
commit 10fdd49e32
5 changed files with 529 additions and 43 deletions

View File

@@ -145,7 +145,33 @@ case "${PR_REVIEW_TEST_MODE:-}" in
write_response 500 '{"message":"simulated rejection"}'
;;
approve|request-changes|comment-success|http-success|prefix-success|subpath-success|port-success|scp-ssh-success|url-ssh-success|ssh-transport-port-success|explicit-default-port-success|readback-failure)
if [[ "$method" == "POST" && "$url" == "$PR_REVIEW_EXPECTED_API_BASE/issues/123/comments" ]]; then
if [[ "$method" == "GET" && "$url" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123/reviews" ]]; then
# First GET = pre-write review-id BOUNDARY; second GET = post-write
# read-back that must surface a strictly-newer review created by
# THIS action (id 200 > boundary 100), with the expected state and
# pinned to the PR's current head commit.
calls_file="${PR_REVIEW_REVIEW_CALLS:-/dev/null}"
if [[ -f "$calls_file" ]]; then
state="APPROVED"
[[ "$PR_REVIEW_TEST_MODE" == "request-changes" ]] && state="REQUEST_CHANGES"
response=$(PR_REVIEW_STATE="$state" python3 - <<'PY'
import json
import os
print(json.dumps([
{"id": 100, "state": "COMMENT", "commit_id": "oldsha0000"},
{"id": 200, "state": os.environ["PR_REVIEW_STATE"], "commit_id": "HEADSHA_FEEDFACE"},
]))
PY
)
else
: > "$calls_file"
response='[{"id":100,"state":"COMMENT","commit_id":"oldsha0000"}]'
fi
write_response 200 "$response"
elif [[ "$method" == "GET" && "$url" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123" ]]; then
write_response 200 '{"head":{"sha":"HEADSHA_FEEDFACE"}}'
elif [[ "$method" == "POST" && "$url" == "$PR_REVIEW_EXPECTED_API_BASE/issues/123/comments" ]]; then
PR_REVIEW_PAYLOAD="$payload" python3 - <<'PY'
import json
import os
@@ -201,12 +227,14 @@ run_review() {
: > "$TEA_LOG"
: > "$CURL_LOG"
: > "$OUTPUT_FILE"
rm -f "$WORK_DIR/review_calls"
(
cd "$REPO_DIR"
PATH="$BIN_DIR:$PATH" \
MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \
PR_REVIEW_TEA_LOG="$TEA_LOG" \
PR_REVIEW_CURL_LOG="$CURL_LOG" \
PR_REVIEW_REVIEW_CALLS="$WORK_DIR/review_calls" \
PR_REVIEW_TEST_MODE="$mode" \
PR_REVIEW_EXPECTED_BODY="$comment" \
PR_REVIEW_EXPECTED_API_BASE="$expected_api_base" \
@@ -216,7 +244,11 @@ run_review() {
run_review approve approve
grep -q '^pr approve 123 --repo mosaicstack/stack --login mosaicstack$' "$TEA_LOG"
grep -q 'Approved Gitea PR #123' "$OUTPUT_FILE"
grep -q 'Approved and verified Gitea PR #123 (review ID 200)' "$OUTPUT_FILE"
# #865: the approval STATE itself is read back — a pre-write boundary GET and a
# post-write read-back GET on the reviews endpoint, plus a PR head lookup.
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews$' "$CURL_LOG"
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123$' "$CURL_LOG"
if grep -q 'comment' "$TEA_LOG"; then
echo "Plain approve (no review body) unexpectedly touched comment persistence" >&2
exit 1
@@ -227,7 +259,7 @@ fi
# comment REST API instead of being passed to `tea` directly.
run_review approve approve approve-note
grep -q '^pr approve 123 --repo mosaicstack/stack --login mosaicstack$' "$TEA_LOG"
grep -q 'Approved Gitea PR #123' "$OUTPUT_FILE"
grep -q 'Approved and verified Gitea PR #123 (review ID 200)' "$OUTPUT_FILE"
grep -q '^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/123/comments$' "$CURL_LOG"
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/comments/456$' "$CURL_LOG"
grep -q 'Added and verified review comment on Gitea PR #123 (comment ID 456)' "$OUTPUT_FILE"
@@ -235,7 +267,8 @@ grep -q 'Added and verified review comment on Gitea PR #123 (comment ID 456)' "$
# #835: same for `pr reject` (request-changes), where a comment is required.
run_review request-changes request-changes changes-required
grep -q '^pr reject 123 --repo mosaicstack/stack --login mosaicstack$' "$TEA_LOG"
grep -q 'Requested changes on Gitea PR #123' "$OUTPUT_FILE"
grep -q 'Requested changes and verified on Gitea PR #123 (review ID 200)' "$OUTPUT_FILE"
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews$' "$CURL_LOG"
grep -q '^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/123/comments$' "$CURL_LOG"
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/comments/456$' "$CURL_LOG"
grep -q 'Added and verified review comment on Gitea PR #123 (comment ID 456)' "$OUTPUT_FILE"