From 686c881fe4932dc3a7e28fd6249890eeff2d98d2 Mon Sep 17 00:00:00 2001 From: "jason.woltje" Date: Fri, 17 Jul 2026 19:44:56 +0000 Subject: [PATCH] fix: fetch actual Gitea PR head for Codex reviews (#815) --- .../mosaic/framework/tools/codex/README.md | 4 +- .../mosaic/framework/tools/codex/common.sh | 59 ++++--- .../tools/codex/test-pr-diff-context.sh | 158 ++++++++++++++++++ packages/mosaic/package.json | 3 +- 4 files changed, 197 insertions(+), 27 deletions(-) create mode 100644 packages/mosaic/framework/tools/codex/test-pr-diff-context.sh diff --git a/packages/mosaic/framework/tools/codex/README.md b/packages/mosaic/framework/tools/codex/README.md index b01ee75..03eb834 100644 --- a/packages/mosaic/framework/tools/codex/README.md +++ b/packages/mosaic/framework/tools/codex/README.md @@ -70,6 +70,8 @@ Security vulnerability review focusing on: ~/.config/mosaic/tools/codex/codex-security-review.sh -n 42 ``` +PR mode resolves the provider's PR diff rather than relying on the caller's checked-out branch. On Gitea, it fetches the base and `refs/pull//head` refs and diffs those explicit refs. If the refs cannot be fetched or the resulting diff is empty, the command exits nonzero before Codex runs or a review is posted. + ### Review Against Base Branch ```bash @@ -253,7 +255,7 @@ Run the script from inside a git repository. ### "No changes found to review" -The specified mode (--uncommitted, --base, etc.) found no changes to review. +The specified non-PR mode (`--uncommitted`, `--base`, etc.) found no changes to review. PR mode instead fails closed with an actionable error when it cannot construct a non-empty provider diff; verify the PR number, remote, provider login, and ref access before retrying. ### "Codex produced no output" diff --git a/packages/mosaic/framework/tools/codex/common.sh b/packages/mosaic/framework/tools/codex/common.sh index 197f1ec..2241405 100755 --- a/packages/mosaic/framework/tools/codex/common.sh +++ b/packages/mosaic/framework/tools/codex/common.sh @@ -44,38 +44,47 @@ build_diff_context() { diff_text=$(git show "$value" 2>/dev/null) ;; pr) - # For PRs, we need to fetch the PR diff - detect_platform + # Provider detection writes its result to stdout; suppress it so it cannot + # be mistaken for diff content when this function is used in a substitution. + detect_platform >/dev/null if [[ "$PLATFORM" == "github" ]]; then - diff_text=$(gh pr diff "$value" 2>/dev/null) + diff_text=$(gh pr diff "$value" 2>/dev/null) || { + echo "Error: Failed to fetch the diff for PR #${value}." >&2 + return 1 + } elif [[ "$PLATFORM" == "gitea" ]]; then - # tea doesn't have a direct pr diff command, use git - local pr_base - pr_base=$(tea pr list --fields index,base --output simple 2>/dev/null | grep "^${value}" | awk '{print $2}') - if [[ -n "$pr_base" ]]; then - diff_text=$(git diff "${pr_base}...HEAD" 2>/dev/null) - else - # Fallback: fetch PR info via API - local repo_info - repo_info=$(get_repo_info) - local remote_url - remote_url=$(git remote get-url origin 2>/dev/null) - local host - host=$(echo "$remote_url" | sed -E 's|.*://([^/]+).*|\1|; s|.*@([^:]+).*|\1|') - diff_text=$(curl -s "https://${host}/api/v1/repos/${repo_info}/pulls/${value}" \ - -H "Authorization: token $(tea login list --output simple 2>/dev/null | head -1 | awk '{print $2}')" \ - 2>/dev/null | jq -r '.diff_url // empty') - if [[ -n "$diff_text" && "$diff_text" != "null" ]]; then - diff_text=$(curl -s "$diff_text" 2>/dev/null) - else - diff_text=$(git diff "main...HEAD" 2>/dev/null) - fi + local pr_base base_ref pr_head_ref + pr_base=$(tea pr list --fields index,base --output simple 2>/dev/null | awk -v pr="$value" '$1 == pr { print $2; exit }') + if [[ -z "$pr_base" ]]; then + echo "Error: Could not resolve the base branch for Gitea PR #${value}." >&2 + return 1 fi + + base_ref="refs/remotes/origin/${pr_base}" + pr_head_ref="refs/remotes/origin/pr/${value}/head" + if ! git fetch --quiet origin \ + "+refs/heads/${pr_base}:${base_ref}" \ + "+refs/pull/${value}/head:${pr_head_ref}"; then + echo "Error: Failed to fetch the base and head refs for Gitea PR #${value}." >&2 + return 1 + fi + diff_text=$(git diff "${base_ref}...${pr_head_ref}") || { + echo "Error: Failed to diff the fetched refs for Gitea PR #${value}." >&2 + return 1 + } + else + echo "Error: Unsupported git platform while resolving PR #${value}." >&2 + return 1 fi ;; esac - echo "$diff_text" + if [[ "$mode" == "pr" && -z "${diff_text//[[:space:]]/}" ]]; then + echo "Error: Unable to construct a non-empty diff for PR #${value}; verify the PR refs and provider access." >&2 + return 1 + fi + + printf '%s\n' "$diff_text" } # Format JSON findings as markdown for PR comments diff --git a/packages/mosaic/framework/tools/codex/test-pr-diff-context.sh b/packages/mosaic/framework/tools/codex/test-pr-diff-context.sh new file mode 100644 index 0000000..5189358 --- /dev/null +++ b/packages/mosaic/framework/tools/codex/test-pr-diff-context.sh @@ -0,0 +1,158 @@ +#!/bin/bash +# Hermetic regression coverage for Gitea PR diff construction and fail-closed reviews. +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +TMP_DIR=$(mktemp -d) +trap 'rm -rf "$TMP_DIR"' EXIT + +fail() { + echo "not ok - $*" >&2 + exit 1 +} + +assert_contains() { + local haystack="$1" needle="$2" + if [[ "$haystack" != *"$needle"* ]]; then + printf 'actual output:\n%s\n' "$haystack" >&2 + fail "expected output to contain: $needle" + fi +} + +# Prevent CI-provided repository context from leaking into the fixture repositories. +unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY \ + GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_COMMON_DIR +export GIT_AUTHOR_NAME="Codex Fixture" +export GIT_AUTHOR_EMAIL="codex-fixture@example.test" +export GIT_COMMITTER_NAME="$GIT_AUTHOR_NAME" +export GIT_COMMITTER_EMAIL="$GIT_AUTHOR_EMAIL" +export GITEA_LOGIN="fixture" +export GITEA_TOKEN="fixture-token" +export GITEA_URL="file://$TMP_DIR" + +create_pr_fixture() { + local fixture_root="$1" head_mode="$2" + local origin="$fixture_root/origin.git" + local seed="$fixture_root/seed" + local work="$fixture_root/work" + local base_sha head_sha + + mkdir -p "$fixture_root" + git init --quiet --bare "$origin" + git init --quiet --initial-branch=release/next "$seed" + printf 'base\n' > "$seed/pr-change.ts" + git -C "$seed" add pr-change.ts + git -C "$seed" commit --quiet -m "fixture base" + base_sha=$(git -C "$seed" rev-parse HEAD) + git -C "$seed" remote add origin "$origin" + git -C "$seed" push --quiet origin release/next + git --git-dir="$origin" symbolic-ref HEAD refs/heads/release/next + + if [[ "$head_mode" == "changed" ]]; then + git -C "$seed" switch --quiet -c feature/pr-795 + printf 'actual-pr-change\n' > "$seed/pr-change.ts" + git -C "$seed" commit --quiet -am "fixture PR head" + head_sha=$(git -C "$seed" rev-parse HEAD) + git -C "$seed" push --quiet origin HEAD:refs/pull/795/head + else + head_sha="$base_sha" + git --git-dir="$origin" update-ref refs/pull/795/head "$head_sha" + fi + + # Gitea's provider-owned PR head ref now exists in the local bare origin. + git clone --quiet "$origin" "$work" + printf '%s\n' "$work" +} + +FAKE_BIN="$TMP_DIR/bin" +mkdir -p "$FAKE_BIN" +cat > "$FAKE_BIN/tea" <<'STUB' +#!/bin/bash +if [[ "$*" == "pr list --fields index,base --output simple" ]]; then + printf '795 release/next\n' + exit 0 +fi +exit 1 +STUB +cat > "$FAKE_BIN/codex" <<'STUB' +#!/bin/bash +printf 'CODEX %s\n' "$*" >> "$CODEX_LOG" +exit 99 +STUB +chmod +x "$FAKE_BIN/tea" "$FAKE_BIN/codex" +export PATH="$FAKE_BIN:$PATH" + +# The valid fixture is a fresh clone on the non-main base. The PR head exists only +# at refs/pull/795/head, so local HEAD cannot accidentally satisfy the assertion. +if [[ "${1:-all}" != "fail-closed" ]]; then + VALID_WORK=$(create_pr_fixture "$TMP_DIR/valid" changed) + ( + cd "$VALID_WORK" + # shellcheck source=common.sh + source "$SCRIPT_DIR/common.sh" + diff_context=$(build_diff_context pr 795) + assert_contains "$diff_context" "actual-pr-change" + + base_sha=$(git rev-parse refs/remotes/origin/release/next) + head_sha=$(git rev-parse refs/remotes/origin/pr/795/head) + local_sha=$(git rev-parse HEAD) + [[ "$local_sha" == "$base_sha" ]] || fail "fixture clone is not on the PR base" + [[ "$head_sha" != "$base_sha" ]] || fail "fixture PR head does not differ from its base" + git show-ref --verify --quiet refs/remotes/origin/pr/795/head || \ + fail "fetched PR head ref is missing" + [[ "$(git diff --name-only "${base_sha}...${head_sha}")" == "pr-change.ts" ]] || \ + fail "explicit PR refs do not contain the fixture change" + if git show-ref --verify --quiet refs/heads/main || \ + git show-ref --verify --quiet refs/remotes/origin/main; then + fail "fixture unexpectedly contains a main ref" + fi + ) + echo "ok - Gitea PR mode fetches and diffs explicit non-main base and PR head refs" +fi + +# Build an empty PR entirely inside another local repository. Both review wrappers +# must emit the PR-numbered error before Codex or the stubbed post path can execute. +if [[ "${1:-all}" != "pr-head" ]]; then + EMPTY_WORK=$(create_pr_fixture "$TMP_DIR/empty" empty) + SANDBOX="$TMP_DIR/sandbox" + mkdir -p "$SANDBOX/tools/codex/schemas" "$SANDBOX/tools/git" + cp "$SCRIPT_DIR/common.sh" \ + "$SCRIPT_DIR/codex-code-review.sh" \ + "$SCRIPT_DIR/codex-security-review.sh" \ + "$SANDBOX/tools/codex/" + cp "$SCRIPT_DIR/schemas/code-review-schema.json" \ + "$SCRIPT_DIR/schemas/security-review-schema.json" \ + "$SANDBOX/tools/codex/schemas/" + cp "$SCRIPT_DIR/../git/detect-platform.sh" "$SANDBOX/tools/git/" + + cat > "$SANDBOX/tools/git/pr-review.sh" <<'STUB' +#!/bin/bash +printf 'POST %s\n' "$*" >> "$POST_LOG" +STUB + chmod +x "$SANDBOX/tools/git/pr-review.sh" + + POST_LOG="$TMP_DIR/post.log" + CODEX_LOG="$TMP_DIR/codex.log" + export POST_LOG CODEX_LOG + + for review_kind in code security; do + : > "$POST_LOG" + : > "$CODEX_LOG" + review_script="$SANDBOX/tools/codex/codex-${review_kind}-review.sh" + set +e + ( + cd "$EMPTY_WORK" + "$review_script" -n 795 + ) >"$TMP_DIR/${review_kind}.stdout" 2>"$TMP_DIR/${review_kind}.stderr" + review_status=$? + set -e + + stderr_text=$(cat "$TMP_DIR/${review_kind}.stderr") + [[ "$review_status" -ne 0 ]] || fail "${review_kind} review returned success for an empty PR diff" + [[ ! -s "$CODEX_LOG" ]] || fail "Codex ran for an empty ${review_kind} PR diff" + [[ ! -s "$POST_LOG" ]] || fail "${review_kind} review auto-post ran for an empty PR diff" + assert_contains "$stderr_text" "Error:" + assert_contains "$stderr_text" "PR #795" + echo "ok - empty ${review_kind} PR diff fails closed before Codex and auto-post" + done +fi diff --git a/packages/mosaic/package.json b/packages/mosaic/package.json index 47ec2e3..0de2b3d 100644 --- a/packages/mosaic/package.json +++ b/packages/mosaic/package.json @@ -24,7 +24,8 @@ "build": "tsc", "lint": "eslint src", "typecheck": "tsc --noEmit", - "test": "vitest run --passWithNoTests" + "test": "vitest run --passWithNoTests && pnpm run test:framework-shell", + "test:framework-shell": "bash framework/tools/codex/test-pr-diff-context.sh" }, "dependencies": { "@mosaicstack/brain": "workspace:*",