From 0115d92fdab480077723bcc27513947a14d03984 Mon Sep 17 00:00:00 2001 From: ms-lead-reviewer Date: Mon, 20 Jul 2026 00:36:48 -0500 Subject: [PATCH] fix(#812): use supported gitea comment REST read-back --- docs/scratchpads/812-pr-review-comment.md | 42 ++-- .../mosaic/framework/tools/git/pr-review.sh | 79 ++++++-- .../tools/git/test-pr-review-gitea-comment.sh | 191 +++++++++++++----- 3 files changed, 226 insertions(+), 86 deletions(-) diff --git a/docs/scratchpads/812-pr-review-comment.md b/docs/scratchpads/812-pr-review-comment.md index 3a4437f7..28ebec14 100644 --- a/docs/scratchpads/812-pr-review-comment.md +++ b/docs/scratchpads/812-pr-review-comment.md @@ -7,7 +7,7 @@ ## Objective -Make the Gitea `comment` action in `packages/mosaic/framework/tools/git/pr-review.sh` use tea v0.11.1's supported `tea comment` command and report success only after provider read-back verifies the created comment against the intended repository, PR, and exact body. +Make the Gitea `comment` action in `packages/mosaic/framework/tools/git/pr-review.sh` use the supported Gitea comments REST API and report success only after provider read-back verifies the created comment against the intended repository, PR, and exact body. ## Plan @@ -16,34 +16,36 @@ Make the Gitea `comment` action in `packages/mosaic/framework/tools/git/pr-revie 3. Implement the minimal supported write plus ID-based provider read-back. 4. Document that wrapper write output is not durable provenance until read-back succeeds. 5. Run focused regression tests, touched-package tests, and repository quality gates. -6. Run independent review, remediate findings, queue-guard, push, and open a PR. Stop at PR-open for coordinator review/merge. +6. Remediate review findings, queue-guard, and push for coordinator-owned independent review. Do not open or merge a PR. ## Progress checkpoints -- [x] RED regression committed and reported to mosaic-100 (`3585cdfa`) -- [x] Initial minimal fix implemented (`c3a83833`) -- [x] Focused and package tests green after dependency build -- [ ] Independent review passed -- [ ] PR opened and reported with exact head SHA +- [x] RED regression committed and reported to mosaic-100 (rebased commit `770e3f57`) +- [x] Initial minimal fix implemented (rebased commit `ea7f8c57`) +- [x] Rebased cleanly onto main `627cf2bb387f7c84a532d88819903a7679ce0d72` +- [x] Codex blocker remediated by replacing unsupported `tea api` with authenticated REST write/read-back +- [x] Focused, package, and repository gates green +- [ ] Coordinator-owned independent review pending after push +- [x] No PR opened; no self-review or self-merge ## Tests run -- RED: `bash packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh` failed against the old path because `tea pr comment` fell back and the wrapper returned success. -- GREEN: all `packages/mosaic/framework/tools/git/test-*.sh` harnesses passed. -- `pnpm build` passed (23/23 tasks). -- `pnpm --filter @mosaicstack/mosaic test` passed (67 files, 1278 tests). -- Package and root typecheck/lint/format gates passed. -- Codex security review found no issues. -- Codex code review requested changes: tea v0.11.1 has no supported `tea api`; replace read-back with the existing host-scoped authenticated curl/API path when work resumes. +- RED after rebase: the regression harness failed against `origin/main` with status 1 after reproducing the old `tea pr comment` zero-exit fallback and false success echo. +- GREEN at resumed head: the same harness passed with REST POST 201 plus GET 200 read-back. +- All `packages/mosaic/framework/tools/git/test-*.sh` harnesses passed. +- `shellcheck -x` passed for the changed scripts; `bash -n` passed. +- Manifest resolver returned `framework` for `tools/git/test-pr-review-gitea-comment.sh`. +- `pnpm test` passed (43/43 Turbo tasks; Mosaic 75 files/1434 tests; Gateway 56 files/628 tests plus documented skips). +- `pnpm typecheck` passed (42/42 tasks), `pnpm lint` passed (23/23), and `pnpm format:check` passed. +- Firewall checks found no user-home paths or operator identities in changed shipped files; no token value is logged or echoed. ## Risks / blockers -- **Coordination hold:** Mos directed #812 to remain parked until in-flight #789 reaches terminal state because #789 actively uses `pr-review.sh`. Branch must be pushed but no PR opened. -- Initial implementation's `tea api` read-back is not compatible with tea v0.11.1 and must be remediated before PR. -- Tea output shape must provide a created comment ID in JSON; failure to parse must fail closed. -- Read-back must not accept a pre-existing comment with the same body. -- Existing approve/request-changes behavior must remain unchanged and covered. +- No active implementation blocker. #789 reached terminal merged state and the coordination hold was lifted. +- REST transport failures, non-201 writes, malformed/missing created IDs, non-200 read-backs, and read-back mismatches all fail closed. +- Existing approve/request-changes behavior remains covered. +- Independent exact-head review remains coordinator-owned. ## Final verification evidence -Parked pending #789 terminal. No PR opened. +Acceptance criteria re-verified locally after clean rebase. Branch will be force-pushed with lease for coordinator verification; no PR opened. diff --git a/packages/mosaic/framework/tools/git/pr-review.sh b/packages/mosaic/framework/tools/git/pr-review.sh index f0bf6b89..20a37fbd 100755 --- a/packages/mosaic/framework/tools/git/pr-review.sh +++ b/packages/mosaic/framework/tools/git/pr-review.sh @@ -5,6 +5,7 @@ set -e SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=packages/mosaic/framework/tools/git/detect-platform.sh source "$SCRIPT_DIR/detect-platform.sh" # Parse arguments @@ -85,7 +86,10 @@ if [[ "$PLATFORM" == "github" ]]; then elif [[ "$PLATFORM" == "gitea" ]]; then case $ACTION in approve) - tea pr approve "$PR_NUMBER" $(get_gitea_repo_args) ${COMMENT:+--comment "$COMMENT"} + repo=$(get_repo_slug) + host=$(get_remote_host) + login=$(get_gitea_login_for_host "$host") + tea pr approve "$PR_NUMBER" --repo "$repo" --login "$login" ${COMMENT:+--comment "$COMMENT"} echo "Approved Gitea PR #$PR_NUMBER" ;; request-changes) @@ -93,7 +97,10 @@ elif [[ "$PLATFORM" == "gitea" ]]; then echo "Error: Comment required for request-changes" exit 1 fi - tea pr reject "$PR_NUMBER" $(get_gitea_repo_args) --comment "$COMMENT" + repo=$(get_repo_slug) + host=$(get_remote_host) + login=$(get_gitea_login_for_host "$host") + tea pr reject "$PR_NUMBER" --repo "$repo" --login "$login" --comment "$COMMENT" echo "Requested changes on Gitea PR #$PR_NUMBER" ;; comment) @@ -104,42 +111,80 @@ elif [[ "$PLATFORM" == "gitea" ]]; then repo=$(get_repo_slug) host=$(get_remote_host) - login=$(get_gitea_login_for_host "$host") + token=$(get_gitea_token "$host") || { + echo "Error: Gitea token not found for comment persistence" >&2 + exit 1 + } + api_base="https://$host/api/v1/repos/$repo" + payload=$(COMMENT_BODY="$COMMENT" python3 -c ' +import json +import os - comment_json=$(tea comment "$PR_NUMBER" "$COMMENT" --login "$login" --repo "$repo" --output json) - comment_id=$(printf '%s' "$comment_json" | python3 -c ' +print(json.dumps({"body": os.environ["COMMENT_BODY"]})) +') + write_response_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-write.XXXXXX") + readback_response_file=$(mktemp "${TMPDIR:-/tmp}/mosaic-pr-review-readback.XXXXXX") + trap 'rm -f "$write_response_file" "$readback_response_file"' EXIT + + if ! write_status=$(curl -sS -o "$write_response_file" -w '%{http_code}' \ + -X POST \ + -H "Authorization: token $token" \ + -H 'Content-Type: application/json' \ + -d "$payload" \ + "$api_base/issues/$PR_NUMBER/comments"); then + echo "Error: Gitea comment write transport failed" >&2 + exit 1 + fi + if [[ "$write_status" != "201" ]]; then + echo "Error: Gitea comment write failed with HTTP $write_status" >&2 + exit 1 + fi + + comment_id=$(python3 - "$write_response_file" <<'PY' import json import sys try: - comment = json.load(sys.stdin) - if isinstance(comment, list) and len(comment) == 1: - comment = comment[0] + with open(sys.argv[1], encoding="utf-8") as response: + comment = json.load(response) comment_id = comment.get("id") if isinstance(comment, dict) else None if not isinstance(comment_id, int) or comment_id <= 0: raise ValueError("missing positive comment id") -except (json.JSONDecodeError, ValueError) as error: +except (OSError, json.JSONDecodeError, ValueError) as error: print(f"Error: could not identify created Gitea comment: {error}", file=sys.stderr) raise SystemExit(1) print(comment_id) -') +PY +) + + if ! readback_status=$(curl -sS -o "$readback_response_file" -w '%{http_code}' \ + -H "Authorization: token $token" \ + "$api_base/issues/comments/$comment_id"); then + echo "Error: Gitea comment read-back transport failed" >&2 + exit 1 + fi + if [[ "$readback_status" != "200" ]]; then + echo "Error: Gitea comment read-back failed with HTTP $readback_status" >&2 + exit 1 + fi - readback_json=$(tea api --login "$login" "/repos/$repo/issues/comments/$comment_id") EXPECTED_COMMENT_ID="$comment_id" EXPECTED_COMMENT_BODY="$COMMENT" EXPECTED_REPO="$repo" EXPECTED_PR_NUMBER="$PR_NUMBER" \ - python3 -c ' + python3 - "$readback_response_file" <<'PY' import json import os import sys from urllib.parse import urlparse try: - comment = json.load(sys.stdin) + with open(sys.argv[1], encoding="utf-8") as response: + comment = json.load(response) + if not isinstance(comment, dict): + raise ValueError("response is not a comment object") expected_id = int(os.environ["EXPECTED_COMMENT_ID"]) expected_body = os.environ["EXPECTED_COMMENT_BODY"] expected_repo = os.environ["EXPECTED_REPO"] expected_pr = os.environ["EXPECTED_PR_NUMBER"] - issue_url = comment.get("issue_url", "") if isinstance(comment, dict) else "" - issue_path = urlparse(issue_url).path.rstrip("/") + issue_path = urlparse(comment.get("issue_url", "")).path.rstrip("/") expected_suffix = f"/repos/{expected_repo}/issues/{expected_pr}" if comment.get("id") != expected_id: raise ValueError("comment id mismatch") @@ -147,10 +192,10 @@ try: raise ValueError("comment body mismatch") if not issue_path.endswith(expected_suffix): raise ValueError("repository or PR mismatch") -except (json.JSONDecodeError, KeyError, TypeError, ValueError) as error: +except (OSError, json.JSONDecodeError, KeyError, TypeError, ValueError) as error: print(f"Error: Gitea comment persistence verification failed: {error}", file=sys.stderr) raise SystemExit(1) -' <<< "$readback_json" +PY echo "Added and verified comment on Gitea PR #$PR_NUMBER (comment ID $comment_id)" ;; diff --git a/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh b/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh index 5dd1ccca..8887a065 100644 --- a/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh +++ b/packages/mosaic/framework/tools/git/test-pr-review-gitea-comment.sh @@ -7,8 +7,10 @@ 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" -LOG_FILE="$WORK_DIR/tea.log" +TEA_LOG="$WORK_DIR/tea.log" +CURL_LOG="$WORK_DIR/curl.log" OUTPUT_FILE="$WORK_DIR/output.log" +CREDENTIALS_FILE="$WORK_DIR/credentials.json" cleanup() { rm -rf "$WORK_DIR" @@ -19,11 +21,22 @@ 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 +cat > "$CREDENTIALS_FILE" <<'JSON' +{ + "gitea": { + "mosaicstack": { + "url": "https://git.mosaicstack.dev", + "token": "test-only-placeholder" + } + } +} +JSON + cat > "$BIN_DIR/tea" <<'SH' #!/usr/bin/env bash set -euo pipefail -printf '%s\n' "$*" >> "$PR_REVIEW_TEST_LOG" +printf '%s\n' "$*" >> "$PR_REVIEW_TEA_LOG" if [[ "$*" == "login list --output json" ]]; then printf '%s\n' '[{"name":"mosaicstack","url":"https://git.mosaicstack.dev"}]' @@ -37,45 +50,15 @@ case "${PR_REVIEW_TEST_MODE:-}" in request-changes) [[ "$*" == "pr reject 123 --repo mosaicstack/stack --login mosaicstack --comment changes-required" ]] || exit 91 ;; - legacy-fallback) + legacy-fallback|comment-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 - if [[ "$*" == comment* ]]; then - printf '%s\n' 'supported comment write deliberately unavailable' >&2 - exit 23 - fi + echo "Unexpected tea command: $*" >&2 exit 92 ;; - comment-success|readback-failure) - if [[ "$*" == "comment 123 durable-body --login mosaicstack --repo mosaicstack/stack --output json" ]]; then - printf '%s\n' '{"id":456,"body":"durable-body"}' - exit 0 - fi - if [[ "$*" == "api --login mosaicstack /repos/mosaicstack/stack/issues/comments/456" ]]; then - if [[ "$PR_REVIEW_TEST_MODE" == "readback-failure" ]]; then - printf '%s\n' '{"id":456,"body":"different-body","issue_url":"https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/123"}' - else - printf '%s\n' '{"id":456,"body":"durable-body","issue_url":"https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/123"}' - fi - exit 0 - fi - if [[ "$*" == pr\ comment* ]]; then - # Reproduce the old false-positive fallback if the wrapper regresses. - printf '%s\n' 'INDEX TITLE STATE' - exit 0 - fi - exit 93 - ;; - write-failure) - if [[ "$*" == "comment 123 durable-body --login mosaicstack --repo mosaicstack/stack --output json" ]]; then - printf '%s\n' 'provider rejected comment' >&2 - exit 24 - fi - exit 94 - ;; *) exit 95 ;; @@ -83,25 +66,130 @@ esac SH chmod +x "$BIN_DIR/tea" +cat > "$BIN_DIR/curl" <<'SH' +#!/usr/bin/env bash +set -euo pipefail + +output_file="" +method="GET" +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 + ;; + esac +done + +printf '%s %s\n' "$method" "$url" >> "$PR_REVIEW_CURL_LOG" + +write_response() { + local status="$1" body="$2" + [[ -n "$output_file" ]] || exit 96 + printf '%s' "$body" > "$output_file" + 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"}' + ;; + comment-success|readback-failure) + if [[ "$method" == "POST" && "$url" == "https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/123/comments" ]]; then + 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" == "https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/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({ + "id": 456, + "body": os.environ["PR_REVIEW_BODY"], + "issue_url": "https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/123", +})) +PY +) + write_response 200 "$response" + else + echo "Unexpected curl request: $method $url" >&2 + exit 97 + fi + ;; + *) + exit 98 + ;; +esac +SH +chmod +x "$BIN_DIR/curl" + run_review() { local mode="$1" action="$2" comment="${3:-}" - : > "$LOG_FILE" + : > "$TEA_LOG" + : > "$CURL_LOG" : > "$OUTPUT_FILE" ( cd "$REPO_DIR" PATH="$BIN_DIR:$PATH" \ - PR_REVIEW_TEST_LOG="$LOG_FILE" \ + MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \ + PR_REVIEW_TEA_LOG="$TEA_LOG" \ + PR_REVIEW_CURL_LOG="$CURL_LOG" \ PR_REVIEW_TEST_MODE="$mode" \ + PR_REVIEW_EXPECTED_BODY="$comment" \ "$SCRIPT_DIR/pr-review.sh" -n 123 -a "$action" ${comment:+-c "$comment"} ) > "$OUTPUT_FILE" 2>&1 } run_review approve approve -grep -q '^pr approve 123 --repo mosaicstack/stack --login mosaicstack$' "$LOG_FILE" +grep -q '^pr approve 123 --repo mosaicstack/stack --login mosaicstack$' "$TEA_LOG" grep -q 'Approved Gitea PR #123' "$OUTPUT_FILE" run_review request-changes request-changes changes-required -grep -q '^pr reject 123 --repo mosaicstack/stack --login mosaicstack --comment changes-required$' "$LOG_FILE" +grep -q '^pr reject 123 --repo mosaicstack/stack --login mosaicstack --comment changes-required$' "$TEA_LOG" grep -q 'Requested changes on Gitea PR #123' "$OUTPUT_FILE" if run_review legacy-fallback comment durable-body; then @@ -109,7 +197,7 @@ if run_review legacy-fallback comment durable-body; then cat "$OUTPUT_FILE" >&2 exit 1 fi -if grep -q '^pr comment ' "$LOG_FILE"; then +if grep -q '^pr comment ' "$TEA_LOG"; then echo "Wrapper invoked unsupported tea pr comment" >&2 exit 1 fi @@ -118,20 +206,25 @@ if grep -q 'Added comment to Gitea PR' "$OUTPUT_FILE"; then exit 1 fi -run_review comment-success comment durable-body -grep -q '^comment 123 durable-body --login mosaicstack --repo mosaicstack/stack --output json$' "$LOG_FILE" -grep -q '^api --login mosaicstack /repos/mosaicstack/stack/issues/comments/456$' "$LOG_FILE" +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 run_review write-failure comment durable-body; then - echo "Expected provider write failure to return nonzero" >&2 - exit 1 -fi -if grep -q 'Added and verified comment' "$OUTPUT_FILE"; then - echo "Write failure reported durable success" >&2 +if [[ -s "$TEA_LOG" ]]; then + echo "REST comment path unexpectedly invoked tea" >&2 + cat "$TEA_LOG" >&2 exit 1 fi +if run_review write-transport-failure comment durable-body; then + echo "Expected provider transport failure to return nonzero" >&2 + exit 1 +fi +if run_review write-http-failure comment durable-body; then + echo "Expected non-201 provider write to return nonzero" >&2 + exit 1 +fi if run_review readback-failure comment durable-body; then echo "Expected mismatched provider read-back to return nonzero" >&2 exit 1