fix(tools): write Gitea reviews/comments via REST POST and verify by exact created id (#865)
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Replace the tea-based write + boundary/author read-back with a direct Gitea
REST POST that returns the created record's id, and verify that exact record.
BLOCKER 2 (credential ordering): resolve the acting identity, the write token,
and the read-back token from the SAME effective login. A --login override now
selects the credential used for the POST, GET /user, and the GET-by-id
read-back, so an overridden write is verified against the identity that
performed it -- not the host default. Login-name resolution is best-effort and
non-fatal (the override always wins; otherwise fall back to the host
credential), so exotic/ported hosts still resolve a token.
BLOCKER 1+3 (attribution + tautological tests): the write is now
POST /issues/{n}/comments or POST /pulls/{n}/reviews (event + body + commit_id
== PR head), parsing the provider-returned created id. Verification GETs that
exact id and checks author == acting identity and body (comments) or state +
commit_id (reviews). Keying on the created id closes the concurrency window:
a no-op create yields no id and fails closed with no list-scan fallback, and a
concurrent same-identity record has a different id. The review body travels in
the review submit, removing the separate detached comment.
Tests: the curl stub now models a real server with persistent on-disk
review/comment state -- a POST actually creates+persists a record and returns
its id, and the read-back reads that same state (no fabricated record for the
wrapper to find). Adds same-identity no-op-concurrent and author-mismatch
fail-closed cases for both comments and reviews, and >page-1 pagination
coverage for both. README "Durable review provenance" refreshed for the REST
mechanism.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -1,11 +1,23 @@
|
||||
#!/usr/bin/env bash
|
||||
# Regression harness for durable Gitea PR review comments (#812) and for the
|
||||
# approve/reject `--comment` flag removal (#835). The `tea` stub below rejects
|
||||
# any `-comment`/`--comment` flag on `pr approve`/`pr reject` exactly like real
|
||||
# `tea` v0.11.1 does ("flag provided but not defined: -comment"), so this
|
||||
# harness fails RED against the pre-#835 wrapper (which passed that flag) and
|
||||
# only passes once the wrapper routes the review body through the durable
|
||||
# comment REST API instead.
|
||||
# Regression harness for pr-review.sh's Gitea review + comment writes (#865,
|
||||
# #812, #835).
|
||||
#
|
||||
# The #865 defect class: tea 0.11.1 can silently no-op while exiting 0 and
|
||||
# cannot emit the id of a record it creates, so its exit code is worthless as
|
||||
# proof of a durable write. The wrapper therefore does NOT write reviews or
|
||||
# comments via tea. approve/request-changes POST to /pulls/{n}/reviews (with the
|
||||
# event, the PR head commit_id, and the review body) and read the created review
|
||||
# back by its EXACT provider-returned id; the `comment` action POSTs to
|
||||
# /issues/{n}/comments and reads that created comment back by its exact id.
|
||||
# Because verification keys on the id the create returned, no concurrent record
|
||||
# can masquerade as this write and a no-op create fails closed. tea is only ever
|
||||
# consulted for the login list.
|
||||
#
|
||||
# The curl stub models a REAL server with persistent review/comment state on
|
||||
# disk: a POST actually CREATES and PERSISTS a record and returns its id, and
|
||||
# the read-back reads that same state. There is no independently fabricated
|
||||
# record for the wrapper to "find" — verification passes only when the POST
|
||||
# genuinely created the record the read-back retrieves.
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
@@ -13,6 +25,11 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
|
||||
WORK_DIR="${MOSAIC_TEST_WORK_DIR:-$PWD/.mosaic-test-work/pr-review-gitea-comment}"
|
||||
REPO_DIR="$WORK_DIR/repo"
|
||||
BIN_DIR="$WORK_DIR/bin"
|
||||
XDG_DIR="$WORK_DIR/xdg"
|
||||
STATE_DIR="$WORK_DIR/state"
|
||||
REVIEWS_FILE="$STATE_DIR/reviews.json"
|
||||
COMMENTS_FILE="$STATE_DIR/comments.json"
|
||||
SUBMIT_PAYLOAD_FILE="$STATE_DIR/review_payload.json"
|
||||
TEA_LOG="$WORK_DIR/tea.log"
|
||||
CURL_LOG="$WORK_DIR/curl.log"
|
||||
OUTPUT_FILE="$WORK_DIR/output.log"
|
||||
@@ -25,8 +42,9 @@ trap cleanup EXIT
|
||||
|
||||
ACTING_LOGIN="review-bot"
|
||||
FOREIGN_LOGIN="other-writer"
|
||||
HEAD_SHA="HEADSHA_FEEDFACE"
|
||||
|
||||
mkdir -p "$REPO_DIR" "$BIN_DIR"
|
||||
mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$STATE_DIR"
|
||||
git -C "$REPO_DIR" init -q
|
||||
git -C "$REPO_DIR" remote add origin https://git.mosaicstack.dev/mosaicstack/stack.git
|
||||
|
||||
@@ -49,6 +67,9 @@ with open(sys.argv[1], "w", encoding="utf-8") as credentials:
|
||||
PY
|
||||
}
|
||||
|
||||
# tea stub: only ever answers the login list. The wrapper must never write a
|
||||
# review or comment through tea (#865 defect class); any other tea invocation is
|
||||
# an error.
|
||||
cat > "$BIN_DIR/tea" <<'SH'
|
||||
#!/usr/bin/env bash
|
||||
set -euo pipefail
|
||||
@@ -56,42 +77,17 @@ set -euo pipefail
|
||||
printf '%s\n' "$*" >> "$PR_REVIEW_TEA_LOG"
|
||||
|
||||
if [[ "$*" == "login list --output json" ]]; then
|
||||
printf '%s\n' '[{"name":"mosaicstack","url":"https://git.mosaicstack.dev"}]'
|
||||
printf '[{"name":"mosaicstack","url":"%s"}]\n' "$PR_REVIEW_LOGIN_URL"
|
||||
exit 0
|
||||
fi
|
||||
|
||||
# tea v0.11.1 defines no --comment/-comment flag on `pr approve` or `pr
|
||||
# reject`; it fails closed with this exact message and a nonzero exit. Any
|
||||
# regression that reintroduces the flag on those subcommands must hit this
|
||||
# branch and fail RED (#835).
|
||||
if [[ "$*" == *" -comment "* || "$*" == *" --comment "* || "$*" == *" -comment" || "$*" == *" --comment" ]]; then
|
||||
echo "flag provided but not defined: -comment" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
case "${PR_REVIEW_TEST_MODE:-}" in
|
||||
approve|paginated-approve|foreign-review)
|
||||
[[ "$*" == "pr approve 123 --repo mosaicstack/stack --login mosaicstack" ]] || exit 90
|
||||
;;
|
||||
request-changes)
|
||||
[[ "$*" == "pr reject 123 --repo mosaicstack/stack --login mosaicstack" ]] || exit 91
|
||||
;;
|
||||
legacy-fallback|comment-success|http-success|prefix-success|subpath-success|port-success|scp-ssh-success|url-ssh-success|ssh-transport-port-success|explicit-default-port-success|write-transport-failure|write-http-failure|readback-failure)
|
||||
if [[ "$*" == pr\ comment* ]]; then
|
||||
# tea v0.11.1 treats the nonexistent subcommand as `tea pr list` and exits 0.
|
||||
printf '%s\n' 'INDEX TITLE STATE'
|
||||
exit 0
|
||||
fi
|
||||
echo "Unexpected tea command: $*" >&2
|
||||
exit 92
|
||||
;;
|
||||
*)
|
||||
exit 95
|
||||
;;
|
||||
esac
|
||||
echo "Unexpected tea command (wrapper must not write via tea): $*" >&2
|
||||
exit 92
|
||||
SH
|
||||
chmod +x "$BIN_DIR/tea"
|
||||
|
||||
# curl stub: a small REST server backed by persistent on-disk review/comment
|
||||
# state.
|
||||
cat > "$BIN_DIR/curl" <<'SH'
|
||||
#!/usr/bin/env bash
|
||||
set -euo pipefail
|
||||
@@ -102,40 +98,19 @@ payload=""
|
||||
url=""
|
||||
while [[ $# -gt 0 ]]; do
|
||||
case "$1" in
|
||||
-o)
|
||||
output_file="$2"
|
||||
shift 2
|
||||
;;
|
||||
-w|-H)
|
||||
shift 2
|
||||
;;
|
||||
-X)
|
||||
method="$2"
|
||||
shift 2
|
||||
;;
|
||||
-d|--data)
|
||||
payload="$2"
|
||||
shift 2
|
||||
;;
|
||||
-s|-S|-sS)
|
||||
shift
|
||||
;;
|
||||
http://*|https://*)
|
||||
url="$1"
|
||||
shift
|
||||
;;
|
||||
*)
|
||||
shift
|
||||
;;
|
||||
-o) output_file="$2"; shift 2 ;;
|
||||
-w|-H) shift 2 ;;
|
||||
-X) method="$2"; shift 2 ;;
|
||||
-d|--data) payload="$2"; shift 2 ;;
|
||||
-s|-S|-sS) shift ;;
|
||||
http://*|https://*) url="$1"; shift ;;
|
||||
*) shift ;;
|
||||
esac
|
||||
done
|
||||
|
||||
# Strip any query string so pagination params (?limit=&page=) don't defeat
|
||||
# path matching, but keep the full URL in the log so tests can assert that the
|
||||
# read-back paginated.
|
||||
path="${url%%\?*}"
|
||||
page="${url##*page=}"
|
||||
[[ "$page" == "$url" ]] && page=1
|
||||
query="${url#*\?}"
|
||||
[[ "$query" == "$url" ]] && query=""
|
||||
printf '%s %s\n' "$method" "$url" >> "$PR_REVIEW_CURL_LOG"
|
||||
|
||||
write_response() {
|
||||
@@ -145,137 +120,202 @@ write_response() {
|
||||
printf '%s' "$status"
|
||||
}
|
||||
|
||||
case "${PR_REVIEW_TEST_MODE:-}" in
|
||||
legacy-fallback|write-transport-failure)
|
||||
echo "simulated transport failure" >&2
|
||||
exit 7
|
||||
;;
|
||||
write-http-failure)
|
||||
write_response 500 '{"message":"simulated rejection"}'
|
||||
;;
|
||||
approve|request-changes|paginated-approve|foreign-review|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" == "GET" && "$path" == "$PR_REVIEW_API_ROOT/user" ]]; then
|
||||
# Acting reviewer identity used for invocation attribution.
|
||||
write_response 200 "$(PR_REVIEW_LOGIN="$PR_REVIEW_ACTING_LOGIN" python3 - <<'PY'
|
||||
emit() {
|
||||
# Split a two-line "status\n<json body>" python result into the response.
|
||||
local result="$1"
|
||||
write_response "$(printf '%s' "$result" | head -n1)" "$(printf '%s' "$result" | tail -n +2)"
|
||||
}
|
||||
|
||||
mode="${PR_REVIEW_TEST_MODE:-}"
|
||||
|
||||
if [[ "$method" == "GET" && "$path" == "$PR_REVIEW_API_ROOT/user" ]]; then
|
||||
write_response 200 "$(PR_REVIEW_LOGIN="$PR_REVIEW_ACTING_LOGIN" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
print(json.dumps({"login": os.environ["PR_REVIEW_LOGIN"]}))
|
||||
PY
|
||||
)"
|
||||
elif [[ "$method" == "GET" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123/reviews" ]]; then
|
||||
# First (boundary) call precedes the write; later calls are the
|
||||
# post-write read-back that must surface a strictly-newer review
|
||||
# created by THIS action (id 200 > boundary 100), authored by the
|
||||
# acting identity, with the expected state and pinned to the PR's
|
||||
# current head commit. The list is served PAGINATED so a target
|
||||
# beyond page 1 is only found by a fully-paginating read-back.
|
||||
calls_file="${PR_REVIEW_REVIEW_CALLS:-/dev/null}"
|
||||
if [[ -f "$calls_file" ]]; then
|
||||
phase="post"
|
||||
else
|
||||
: > "$calls_file"
|
||||
phase="boundary"
|
||||
fi
|
||||
state="APPROVED"
|
||||
[[ "$PR_REVIEW_TEST_MODE" == "request-changes" ]] && state="REQUEST_CHANGES"
|
||||
response=$(PR_REVIEW_PHASE="$phase" PR_REVIEW_PAGE="$page" \
|
||||
PR_REVIEW_MODE="$PR_REVIEW_TEST_MODE" PR_REVIEW_STATE="$state" \
|
||||
PR_REVIEW_ACTING_LOGIN="$PR_REVIEW_ACTING_LOGIN" \
|
||||
PR_REVIEW_FOREIGN_LOGIN="$PR_REVIEW_FOREIGN_LOGIN" python3 - <<'PY'
|
||||
elif [[ "$method" == "GET" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123" ]]; then
|
||||
write_response 200 "$(PR_REVIEW_HEAD_SHA="$PR_REVIEW_HEAD_SHA" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
print(json.dumps({"head": {"sha": os.environ["PR_REVIEW_HEAD_SHA"]}}))
|
||||
PY
|
||||
)"
|
||||
elif [[ "$method" == "POST" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123/reviews" ]]; then
|
||||
printf '%s' "$payload" > "$PR_REVIEW_SUBMIT_PAYLOAD"
|
||||
emit "$(PR_REVIEW_PAYLOAD="$payload" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
|
||||
phase = os.environ["PR_REVIEW_PHASE"]
|
||||
page = int(os.environ["PR_REVIEW_PAGE"])
|
||||
mode = os.environ["PR_REVIEW_MODE"]
|
||||
state = os.environ["PR_REVIEW_STATE"]
|
||||
state_path = os.environ["PR_REVIEW_REVIEWS"]
|
||||
mode = os.environ["PR_REVIEW_TEST_MODE"]
|
||||
acting = os.environ["PR_REVIEW_ACTING_LOGIN"]
|
||||
foreign = os.environ["PR_REVIEW_FOREIGN_LOGIN"]
|
||||
submitted = json.loads(os.environ["PR_REVIEW_PAYLOAD"])
|
||||
|
||||
with open(state_path, encoding="utf-8") as handle:
|
||||
reviews = json.load(handle)
|
||||
|
||||
def review(review_id, review_state, commit, login):
|
||||
return {
|
||||
"id": review_id,
|
||||
"state": review_state,
|
||||
"commit_id": commit,
|
||||
"user": {"login": login},
|
||||
}
|
||||
# no-op-concurrent-review: the wrapper's own submit is SUPPRESSED (200, no
|
||||
# created object) even though a concurrent same-identity, same-state review at
|
||||
# the same head already exists. Nothing is persisted; no created id to verify.
|
||||
if mode == "no-op-concurrent-review":
|
||||
print("200")
|
||||
print(json.dumps({}))
|
||||
raise SystemExit(0)
|
||||
|
||||
author = foreign if mode == "author-mismatch-review" else acting
|
||||
new_id = (max((r["id"] for r in reviews), default=0)) + 1
|
||||
record = {
|
||||
"id": new_id,
|
||||
"state": submitted.get("event"),
|
||||
"commit_id": submitted.get("commit_id"),
|
||||
"body": submitted.get("body"),
|
||||
"user": {"login": author},
|
||||
}
|
||||
reviews.append(record)
|
||||
with open(state_path, "w", encoding="utf-8") as handle:
|
||||
json.dump(reviews, handle)
|
||||
print("201")
|
||||
print(json.dumps(record))
|
||||
PY
|
||||
)"
|
||||
elif [[ "$method" == "GET" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE"/pulls/123/reviews/* ]]; then
|
||||
emit "$(PR_REVIEW_GET_ID="${path##*/}" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
|
||||
if phase == "boundary":
|
||||
records = [review(100, "COMMENT", "oldsha0000", acting)] if page == 1 else []
|
||||
elif mode == "paginated-approve":
|
||||
# A full first page (50 non-matching records) forces the read-back to
|
||||
# request page 2, where the genuine matching review lives.
|
||||
if page == 1:
|
||||
records = [review(101 + i, "COMMENT", "oldsha0000", acting) for i in range(50)]
|
||||
elif page == 2:
|
||||
records = [review(200, state, "HEADSHA_FEEDFACE", acting)]
|
||||
else:
|
||||
records = []
|
||||
elif mode == "foreign-review":
|
||||
# A concurrent APPROVED review at the current head, but authored by a
|
||||
# DIFFERENT identity. tea created nothing; attribution must reject this.
|
||||
records = [review(200, state, "HEADSHA_FEEDFACE", foreign)] if page == 1 else []
|
||||
state_path = os.environ["PR_REVIEW_REVIEWS"]
|
||||
wanted = int(os.environ["PR_REVIEW_GET_ID"])
|
||||
with open(state_path, encoding="utf-8") as handle:
|
||||
reviews = json.load(handle)
|
||||
match = next((r for r in reviews if r["id"] == wanted), None)
|
||||
if match is None:
|
||||
print("404")
|
||||
print(json.dumps({"message": "not found"}))
|
||||
else:
|
||||
if page == 1:
|
||||
records = [
|
||||
review(100, "COMMENT", "oldsha0000", acting),
|
||||
review(200, state, "HEADSHA_FEEDFACE", acting),
|
||||
]
|
||||
else:
|
||||
records = []
|
||||
print(json.dumps(records))
|
||||
print("200")
|
||||
print(json.dumps(match))
|
||||
PY
|
||||
)
|
||||
write_response 200 "$response"
|
||||
elif [[ "$method" == "GET" && "$path" == "$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'
|
||||
)"
|
||||
elif [[ "$method" == "GET" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123/reviews" ]]; then
|
||||
emit "$(PR_REVIEW_QUERY="$query" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
from urllib.parse import parse_qs
|
||||
|
||||
state_path = os.environ["PR_REVIEW_REVIEWS"]
|
||||
params = parse_qs(os.environ["PR_REVIEW_QUERY"])
|
||||
limit = int(params.get("limit", ["50"])[0])
|
||||
page = int(params.get("page", ["1"])[0])
|
||||
with open(state_path, encoding="utf-8") as handle:
|
||||
reviews = json.load(handle)
|
||||
start = (page - 1) * limit
|
||||
print("200")
|
||||
print(json.dumps(reviews[start:start + limit]))
|
||||
PY
|
||||
)"
|
||||
elif [[ "$method" == "POST" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/issues/123/comments" ]]; then
|
||||
case "$mode" in
|
||||
write-transport-failure)
|
||||
echo "simulated transport failure" >&2
|
||||
exit 7
|
||||
;;
|
||||
write-http-failure)
|
||||
write_response 500 '{"message":"simulated rejection"}'
|
||||
;;
|
||||
*)
|
||||
emit "$(PR_REVIEW_PAYLOAD="$payload" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
|
||||
assert json.loads(os.environ["PR_REVIEW_PAYLOAD"]) == {"body": os.environ["PR_REVIEW_EXPECTED_BODY"]}
|
||||
PY
|
||||
response=$(python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
|
||||
print(json.dumps({"id": 456, "body": os.environ["PR_REVIEW_EXPECTED_BODY"]}))
|
||||
PY
|
||||
)
|
||||
write_response 201 "$response"
|
||||
elif [[ "$method" == "GET" && "$url" == "$PR_REVIEW_EXPECTED_API_BASE/issues/comments/456" ]]; then
|
||||
if [[ "$PR_REVIEW_TEST_MODE" == "readback-failure" ]]; then
|
||||
body="different-body"
|
||||
else
|
||||
body="$PR_REVIEW_EXPECTED_BODY"
|
||||
fi
|
||||
response=$(PR_REVIEW_BODY="$body" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
|
||||
print(json.dumps({
|
||||
state_path = os.environ["PR_REVIEW_COMMENTS"]
|
||||
acting = os.environ["PR_REVIEW_ACTING_LOGIN"]
|
||||
base = os.environ["PR_REVIEW_EXPECTED_API_BASE"]
|
||||
body = json.loads(os.environ["PR_REVIEW_PAYLOAD"]).get("body")
|
||||
record = {
|
||||
"id": 456,
|
||||
"body": os.environ["PR_REVIEW_BODY"],
|
||||
"issue_url": os.environ["PR_REVIEW_EXPECTED_API_BASE"] + "/issues/123",
|
||||
}))
|
||||
"body": body,
|
||||
"user": {"login": acting},
|
||||
"issue_url": f"{base}/issues/123",
|
||||
}
|
||||
with open(state_path, "w", encoding="utf-8") as handle:
|
||||
json.dump([record], handle)
|
||||
print("201")
|
||||
print(json.dumps(record))
|
||||
PY
|
||||
)
|
||||
write_response 200 "$response"
|
||||
else
|
||||
echo "Unexpected curl request: $method $url" >&2
|
||||
exit 97
|
||||
fi
|
||||
;;
|
||||
*)
|
||||
exit 98
|
||||
;;
|
||||
esac
|
||||
)"
|
||||
;;
|
||||
esac
|
||||
elif [[ "$method" == "GET" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE"/issues/comments/* ]]; then
|
||||
emit "$(PR_REVIEW_GET_ID="${path##*/}" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
|
||||
state_path = os.environ["PR_REVIEW_COMMENTS"]
|
||||
mode = os.environ["PR_REVIEW_TEST_MODE"]
|
||||
wanted = int(os.environ["PR_REVIEW_GET_ID"])
|
||||
with open(state_path, encoding="utf-8") as handle:
|
||||
comments = json.load(handle)
|
||||
match = next((c for c in comments if c["id"] == wanted), None)
|
||||
if match is None:
|
||||
print("404")
|
||||
print(json.dumps({"message": "not found"}))
|
||||
raise SystemExit(0)
|
||||
if mode == "readback-failure":
|
||||
# The server returns a DIFFERENT body than was created — a genuine
|
||||
# provider-side mismatch the wrapper must reject.
|
||||
match = dict(match, body="different-body")
|
||||
print("200")
|
||||
print(json.dumps(match))
|
||||
PY
|
||||
)"
|
||||
else
|
||||
echo "Unexpected curl request: $method $url" >&2
|
||||
exit 97
|
||||
fi
|
||||
SH
|
||||
chmod +x "$BIN_DIR/curl"
|
||||
|
||||
# Seed persistent server state for a mode before the wrapper runs.
|
||||
seed_state() {
|
||||
local mode="$1"
|
||||
printf '[]' > "$COMMENTS_FILE"
|
||||
rm -f "$SUBMIT_PAYLOAD_FILE"
|
||||
PR_REVIEW_SEED_MODE="$mode" PR_REVIEW_SEED_ACTING="$ACTING_LOGIN" \
|
||||
PR_REVIEW_SEED_HEAD="$HEAD_SHA" python3 - "$REVIEWS_FILE" <<'PY'
|
||||
import json
|
||||
import os
|
||||
import sys
|
||||
|
||||
mode = os.environ["PR_REVIEW_SEED_MODE"]
|
||||
acting = os.environ["PR_REVIEW_SEED_ACTING"]
|
||||
head = os.environ["PR_REVIEW_SEED_HEAD"]
|
||||
|
||||
|
||||
def review(rid, state, commit, login):
|
||||
return {"id": rid, "state": state, "commit_id": commit, "user": {"login": login}}
|
||||
|
||||
|
||||
if mode == "paginated-approve":
|
||||
# 50 pre-existing reviews fill page 1 (limit 50); the review this run submits
|
||||
# becomes id 51 and lands ALONE on page 2, exercising >page-1 pagination in
|
||||
# the enumeration check.
|
||||
reviews = [review(i, "COMMENT", "oldsha0000", acting) for i in range(1, 51)]
|
||||
elif mode == "no-op-concurrent-review":
|
||||
# A concurrent SAME-IDENTITY APPROVED review at the CURRENT head already
|
||||
# exists. The wrapper's own submit will be a no-op; it must fail closed
|
||||
# because no created id is returned — it must not scan and accept this one.
|
||||
reviews = [review(77, "APPROVED", head, acting)]
|
||||
else:
|
||||
reviews = [review(100, "COMMENT", "oldsha0000", acting)]
|
||||
|
||||
with open(sys.argv[1], "w", encoding="utf-8") as handle:
|
||||
json.dump(reviews, handle)
|
||||
PY
|
||||
}
|
||||
|
||||
run_review() {
|
||||
local mode="$1" action="$2" comment="${3:-}"
|
||||
local configured_url="${4:-https://git.mosaicstack.dev}"
|
||||
@@ -288,101 +328,144 @@ run_review() {
|
||||
: > "$TEA_LOG"
|
||||
: > "$CURL_LOG"
|
||||
: > "$OUTPUT_FILE"
|
||||
rm -f "$WORK_DIR/review_calls"
|
||||
seed_state "$mode"
|
||||
(
|
||||
cd "$REPO_DIR"
|
||||
PATH="$BIN_DIR:$PATH" \
|
||||
XDG_CONFIG_HOME="$XDG_DIR" \
|
||||
MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \
|
||||
PR_REVIEW_TEA_LOG="$TEA_LOG" \
|
||||
PR_REVIEW_LOGIN_URL="${configured_url%/}" \
|
||||
PR_REVIEW_CURL_LOG="$CURL_LOG" \
|
||||
PR_REVIEW_REVIEW_CALLS="$WORK_DIR/review_calls" \
|
||||
PR_REVIEW_REVIEWS="$REVIEWS_FILE" \
|
||||
PR_REVIEW_COMMENTS="$COMMENTS_FILE" \
|
||||
PR_REVIEW_SUBMIT_PAYLOAD="$SUBMIT_PAYLOAD_FILE" \
|
||||
PR_REVIEW_TEST_MODE="$mode" \
|
||||
PR_REVIEW_EXPECTED_BODY="$comment" \
|
||||
PR_REVIEW_EXPECTED_API_BASE="$expected_api_base" \
|
||||
PR_REVIEW_API_ROOT="$expected_api_root" \
|
||||
PR_REVIEW_HEAD_SHA="$HEAD_SHA" \
|
||||
PR_REVIEW_ACTING_LOGIN="$ACTING_LOGIN" \
|
||||
PR_REVIEW_FOREIGN_LOGIN="$FOREIGN_LOGIN" \
|
||||
"$SCRIPT_DIR/pr-review.sh" -n 123 -a "$action" ${comment:+-c "$comment"}
|
||||
) > "$OUTPUT_FILE" 2>&1
|
||||
}
|
||||
|
||||
assert_no_tea_write() {
|
||||
# tea must only ever be used for the login list, never to write.
|
||||
if grep -qvE '^login list --output json$' "$TEA_LOG"; then
|
||||
echo "FAIL: wrapper invoked tea for something other than the login list" >&2
|
||||
cat "$TEA_LOG" >&2
|
||||
exit 1
|
||||
fi
|
||||
}
|
||||
|
||||
# Case 1: a plain approve submits a review via REST and verifies it by its exact
|
||||
# provider-returned id (id 101), attributed to the acting identity, pinned to
|
||||
# the PR head, with no separate comment.
|
||||
run_review approve approve
|
||||
grep -q '^pr approve 123 --repo mosaicstack/stack --login mosaicstack$' "$TEA_LOG"
|
||||
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 (paginated) reviews endpoint, plus a PR head
|
||||
# lookup and an acting-identity (GET /user) resolution for attribution.
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews?limit=[0-9]*&page=1$' "$CURL_LOG"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123$' "$CURL_LOG"
|
||||
grep -q 'Approved and verified Gitea PR #123 (review ID 101)' "$OUTPUT_FILE"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/user$' "$CURL_LOG"
|
||||
if grep -q 'comment' "$TEA_LOG"; then
|
||||
echo "Plain approve (no review body) unexpectedly touched comment persistence" >&2
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123$' "$CURL_LOG"
|
||||
grep -q '^POST 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/reviews/101$' "$CURL_LOG"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews?limit=[0-9]*&page=1$' "$CURL_LOG"
|
||||
assert_no_tea_write
|
||||
# The submitted review payload carries the event and the PR head commit_id.
|
||||
PR_REVIEW_HEAD_SHA="$HEAD_SHA" python3 - "$SUBMIT_PAYLOAD_FILE" <<'PY'
|
||||
import json
|
||||
import os
|
||||
import sys
|
||||
|
||||
payload = json.load(open(sys.argv[1], encoding="utf-8"))
|
||||
assert payload["event"] == "APPROVED", payload
|
||||
assert payload["commit_id"] == os.environ["PR_REVIEW_HEAD_SHA"], payload
|
||||
PY
|
||||
# A plain approve (no body) must not POST a comment.
|
||||
if grep -q '/issues/123/comments' "$CURL_LOG"; then
|
||||
echo "FAIL: plain approve unexpectedly posted a comment" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# #865 (invocation attribution): a concurrent APPROVED review at the current
|
||||
# head, authored by a DIFFERENT identity while tea created nothing, must NOT
|
||||
# satisfy verification — id-above-boundary + state + head is only temporal
|
||||
# ordering, not proof THIS reviewer wrote it.
|
||||
if run_review foreign-review approve; then
|
||||
# Case 2: a submitted review NOT authored by the acting identity must FAIL
|
||||
# CLOSED — the exact-id read-back enforces authorship.
|
||||
if run_review author-mismatch-review approve; then
|
||||
echo "FAIL: approve accepted a review authored by a different identity" >&2
|
||||
cat "$OUTPUT_FILE" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q 'Approved and verified' "$OUTPUT_FILE"; then
|
||||
echo "FAIL: read-back matched a different-identity review (attribution bypassed)" >&2
|
||||
echo "FAIL: read-back did not enforce acting-identity authorship" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# #865 (pagination): a genuine matching review that lands beyond page 1 of the
|
||||
# reviews list must still be found by a fully-paginating read-back.
|
||||
run_review paginated-approve approve
|
||||
grep -q 'Approved and verified Gitea PR #123 (review ID 200)' "$OUTPUT_FILE"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews?limit=[0-9]*&page=2$' "$CURL_LOG"
|
||||
|
||||
# #835: tea v0.11.1 defines no --comment/-comment flag on `pr approve`. A
|
||||
# review body supplied alongside approve must be routed through the durable
|
||||
# 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 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"
|
||||
|
||||
# #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 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?limit=[0-9]*&page=1$' "$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"
|
||||
|
||||
if run_review legacy-fallback comment durable-body; then
|
||||
echo "The old nonexistent tea pr comment fallback returned success" >&2
|
||||
# Case 3: a no-op submit with a concurrent SAME-IDENTITY, same-state review at
|
||||
# the current head already present must FAIL CLOSED — the closed concurrency
|
||||
# window. The wrapper must not read back (or accept) the concurrent id 77.
|
||||
if run_review no-op-concurrent-review approve; then
|
||||
echo "FAIL: approve reported success when its submit no-opped but a concurrent review existed" >&2
|
||||
cat "$OUTPUT_FILE" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q '^pr comment ' "$TEA_LOG"; then
|
||||
echo "Wrapper invoked unsupported tea pr comment" >&2
|
||||
if grep -q 'Approved and verified' "$OUTPUT_FILE"; then
|
||||
echo "FAIL: approve accepted a concurrent review for a no-op submit (window not closed)" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q 'Added comment to Gitea PR' "$OUTPUT_FILE"; then
|
||||
echo "Wrapper reported success without durable persistence" >&2
|
||||
if grep -q '/pulls/123/reviews/77$' "$CURL_LOG"; then
|
||||
echo "FAIL: wrapper read back the concurrent review id 77 (illegitimate fallback)" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Case 4: a genuine matching review that lands beyond page 1 of the reviews list
|
||||
# must still be found by the fully-paginating enumeration check.
|
||||
run_review paginated-approve approve
|
||||
grep -q 'Approved and verified Gitea PR #123 (review ID 51)' "$OUTPUT_FILE"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews/51$' "$CURL_LOG"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews?limit=[0-9]*&page=2$' "$CURL_LOG"
|
||||
|
||||
# Case 5: an approve WITH a body carries that body in the review submit itself —
|
||||
# there is no separate detached comment POST.
|
||||
run_review approve approve approve-note
|
||||
grep -q 'Approved and verified Gitea PR #123 (review ID 101)' "$OUTPUT_FILE"
|
||||
PR_REVIEW_EXPECTED_BODY="approve-note" python3 - "$SUBMIT_PAYLOAD_FILE" <<'PY'
|
||||
import json
|
||||
import os
|
||||
import sys
|
||||
|
||||
payload = json.load(open(sys.argv[1], encoding="utf-8"))
|
||||
assert payload["body"] == os.environ["PR_REVIEW_EXPECTED_BODY"], payload
|
||||
PY
|
||||
if grep -q '/issues/123/comments' "$CURL_LOG"; then
|
||||
echo "FAIL: approve-with-body posted a separate comment instead of carrying the body on the review" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Case 6: request-changes requires a body and carries it on the REQUEST_CHANGES
|
||||
# review submit.
|
||||
run_review request-changes request-changes changes-required
|
||||
grep -q 'Requested changes and verified on Gitea PR #123 (review ID 101)' "$OUTPUT_FILE"
|
||||
grep -q '^POST 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/reviews/101$' "$CURL_LOG"
|
||||
PR_REVIEW_EXPECTED_BODY="changes-required" python3 - "$SUBMIT_PAYLOAD_FILE" <<'PY'
|
||||
import json
|
||||
import os
|
||||
import sys
|
||||
|
||||
payload = json.load(open(sys.argv[1], encoding="utf-8"))
|
||||
assert payload["event"] == "REQUEST_CHANGES", payload
|
||||
assert payload["body"] == os.environ["PR_REVIEW_EXPECTED_BODY"], payload
|
||||
PY
|
||||
assert_no_tea_write
|
||||
|
||||
# Case 7: the `comment` action creates a comment via REST and verifies it by its
|
||||
# exact created id, attributed to the acting identity. This also exercises
|
||||
# owner/repo + base-URL resolution across clone-URL shapes.
|
||||
complex_body=$'durable "body"\n-- marker'
|
||||
run_review comment-success comment "$complex_body"
|
||||
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 comment on Gitea PR #123' "$OUTPUT_FILE"
|
||||
if [[ -s "$TEA_LOG" ]]; then
|
||||
echo "REST comment path unexpectedly invoked tea" >&2
|
||||
cat "$TEA_LOG" >&2
|
||||
exit 1
|
||||
fi
|
||||
assert_no_tea_write
|
||||
|
||||
run_review http-success comment durable-body http://git.mosaicstack.dev
|
||||
grep -q '^POST http://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/123/comments$' "$CURL_LOG"
|
||||
@@ -410,23 +493,17 @@ grep -q '^POST https://git.example/api/v1/repos/owner/repo/issues/123/comments$'
|
||||
run_review url-ssh-success comment durable-body https://git.example ssh://git@git.example/owner/repo.git owner/repo
|
||||
grep -q '^POST https://git.example/api/v1/repos/owner/repo/issues/123/comments$' "$CURL_LOG"
|
||||
|
||||
# #850 (follow-up to #812): an SSH remote's transport port (e.g. `ssh://
|
||||
# git@host:2222/...`) must NOT be compared against the configured HTTP(S) API
|
||||
# URL's port -- they identify unrelated properties (SSH daemon port vs. HTTP(S)
|
||||
# provider port) of the same Gitea host. Before the fix, host-match required
|
||||
# the configured URL to carry the identical port, so this failed closed even
|
||||
# though both remote and configured URL name the same host.
|
||||
# #850: an SSH remote's transport port must not be compared against the
|
||||
# configured HTTP(S) API URL's port.
|
||||
run_review ssh-transport-port-success comment durable-body https://git.example ssh://git@git.example:2222/owner/repo.git owner/repo
|
||||
grep -q '^POST https://git.example/api/v1/repos/owner/repo/issues/123/comments$' "$CURL_LOG"
|
||||
|
||||
# #850 (follow-up to #812): an explicit default HTTP(S) port on the remote
|
||||
# (`https://host:443/...`) must be treated as equal to an implicit
|
||||
# (portless) configured URL on BOTH sides -- the pre-fix comparison only
|
||||
# normalized the default port when the REMOTE side was portless, so the
|
||||
# inverse (explicit remote, implicit configured) form failed closed.
|
||||
# #850: an explicit default HTTP(S) port on the remote must equal an implicit
|
||||
# (portless) configured URL.
|
||||
run_review explicit-default-port-success comment durable-body https://git.example https://git.example:443/owner/repo.git owner/repo
|
||||
grep -q '^POST https://git.example/api/v1/repos/owner/repo/issues/123/comments$' "$CURL_LOG"
|
||||
|
||||
# Comment write/read-back failure modes must all fail closed.
|
||||
if run_review write-transport-failure comment durable-body; then
|
||||
echo "Expected provider transport failure to return nonzero" >&2
|
||||
exit 1
|
||||
@@ -444,4 +521,4 @@ if grep -q 'Added and verified comment' "$OUTPUT_FILE"; then
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "pr-review.sh durable Gitea comment regression passed"
|
||||
echo "pr-review.sh REST review + comment create/read-back regression passed"
|
||||
|
||||
Reference in New Issue
Block a user