fix(tools): attribute read-back to acting identity and paginate fully (#865)
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Round 2 remediation for the read-back verification in issue-comment.sh and pr-review.sh. BLOCKER A (invocation attribution): id-above-boundary + content/state match only proves temporal ordering — a concurrent write from a different identity could satisfy it while this tea invocation created nothing. Both wrappers now resolve the acting identity once via curl GET /api/v1/user and additionally require the accepted record's author login to equal that identity. Residual same-identity same-body/state concurrency is documented in-code (tea 0.11.1 emits no reliable created-record id to close it further). BLOCKER B (pagination): the comments and reviews list reads now walk every page (?limit=&page=1,2,… until a short/empty page) for both the pre-write boundary and the post-write read-back, so a record beyond page 1 is still found. Adds regressions: concurrent different-identity write fails closed (comments and reviews); a matching review beyond page 1 is still found. README updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Hermes Agent
@@ -23,6 +23,9 @@ cleanup() {
|
||||
}
|
||||
trap cleanup EXIT
|
||||
|
||||
ACTING_LOGIN="review-bot"
|
||||
FOREIGN_LOGIN="other-writer"
|
||||
|
||||
mkdir -p "$REPO_DIR" "$BIN_DIR"
|
||||
git -C "$REPO_DIR" init -q
|
||||
git -C "$REPO_DIR" remote add origin https://git.mosaicstack.dev/mosaicstack/stack.git
|
||||
@@ -67,7 +70,7 @@ if [[ "$*" == *" -comment "* || "$*" == *" --comment "* || "$*" == *" -comment"
|
||||
fi
|
||||
|
||||
case "${PR_REVIEW_TEST_MODE:-}" in
|
||||
approve)
|
||||
approve|paginated-approve|foreign-review)
|
||||
[[ "$*" == "pr approve 123 --repo mosaicstack/stack --login mosaicstack" ]] || exit 90
|
||||
;;
|
||||
request-changes)
|
||||
@@ -127,6 +130,12 @@ while [[ $# -gt 0 ]]; do
|
||||
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
|
||||
printf '%s %s\n' "$method" "$url" >> "$PR_REVIEW_CURL_LOG"
|
||||
|
||||
write_response() {
|
||||
@@ -144,32 +153,83 @@ case "${PR_REVIEW_TEST_MODE:-}" in
|
||||
write-http-failure)
|
||||
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" == "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.
|
||||
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'
|
||||
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
|
||||
state="APPROVED"
|
||||
[[ "$PR_REVIEW_TEST_MODE" == "request-changes" ]] && state="REQUEST_CHANGES"
|
||||
response=$(PR_REVIEW_STATE="$state" python3 - <<'PY'
|
||||
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'
|
||||
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"},
|
||||
]))
|
||||
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"]
|
||||
acting = os.environ["PR_REVIEW_ACTING_LOGIN"]
|
||||
foreign = os.environ["PR_REVIEW_FOREIGN_LOGIN"]
|
||||
|
||||
|
||||
def review(review_id, review_state, commit, login):
|
||||
return {
|
||||
"id": review_id,
|
||||
"state": review_state,
|
||||
"commit_id": commit,
|
||||
"user": {"login": login},
|
||||
}
|
||||
|
||||
|
||||
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 []
|
||||
else:
|
||||
if page == 1:
|
||||
records = [
|
||||
review(100, "COMMENT", "oldsha0000", acting),
|
||||
review(200, state, "HEADSHA_FEEDFACE", acting),
|
||||
]
|
||||
else:
|
||||
records = []
|
||||
print(json.dumps(records))
|
||||
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
|
||||
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'
|
||||
@@ -222,6 +282,7 @@ run_review() {
|
||||
local remote_url="${5:-https://git.mosaicstack.dev/mosaicstack/stack.git}"
|
||||
local expected_repo="${6:-mosaicstack/stack}"
|
||||
local expected_api_base="${configured_url%/}/api/v1/repos/$expected_repo"
|
||||
local expected_api_root="${configured_url%/}/api/v1"
|
||||
git -C "$REPO_DIR" remote set-url origin "$remote_url"
|
||||
write_credentials "$configured_url"
|
||||
: > "$TEA_LOG"
|
||||
@@ -238,6 +299,9 @@ run_review() {
|
||||
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_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
|
||||
}
|
||||
@@ -246,14 +310,36 @@ 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 reviews endpoint, plus a PR head lookup.
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews$' "$CURL_LOG"
|
||||
# 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 '^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
|
||||
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
|
||||
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
|
||||
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.
|
||||
@@ -268,7 +354,7 @@ grep -q 'Added and verified review comment on Gitea PR #123 (comment ID 456)' "$
|
||||
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$' "$CURL_LOG"
|
||||
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"
|
||||
|
||||
Reference in New Issue
Block a user