diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index 064c4e5aee..df72f616ca 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -5,7 +5,7 @@ run-name: >- github.event.client_payload.pr_number || github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number || 'event' }}@${{ github.event.client_payload.pr_head_sha || github.event.pull_request.head.sha || - github.event.workflow_run.head_sha || github.sha }} + github.event.workflow_run.pull_requests[0].head.sha || github.sha }} on: pull_request_target: @@ -24,8 +24,16 @@ concurrency: github.event.client_payload.target_repository || github.repository }}-${{ github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number || github.event.client_payload.pr_number || - github.run_id }} - cancel-in-progress: true + github.run_id }}-${{ + github.event.client_payload.pr_head_sha || github.event.pull_request.head.sha || + github.event.workflow_run.pull_requests[0].head.sha || github.sha }}-${{ + github.event_name == 'workflow_run' && + github.event.workflow_run.conclusion == 'cancelled' && + format('cancelled-{0}', github.run_id) || + 'actionable' }} + # A cancelled upstream review emits a workflow_run event whose Noema job is + # skipped. It must not cancel a live same-head Noema review before skipping. + cancel-in-progress: ${{ github.event_name != 'workflow_run' || github.event.workflow_run.conclusion != 'cancelled' }} permissions: contents: read @@ -37,8 +45,146 @@ jobs: cancel-closed-pr-runs: if: github.event_name == 'pull_request_target' && github.event.action == 'closed' runs-on: ubuntu-latest + permissions: + actions: write + contents: read + env: + GH_TOKEN: ${{ github.token }} + TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} + CLOSED_PR_NUMBER: ${{ github.event.pull_request.number }} + CURRENT_RUN_ID: ${{ github.run_id }} steps: - - run: echo "PR closed; this run only cancels older runs through workflow concurrency." + - name: Cancel queued and running Noema reviews for the closed pull request + shell: bash + run: | + set -euo pipefail + + # cancel_runs prints the number of runs it matched for $1's status + # on stdout (its only stdout output) so the multi-pass loop below + # can tell whether a pass found anything; all human-facing log + # lines go to stderr so they don't pollute that count. + # + # The runs list is scoped to this repository, not to a specific + # workflow file: noema-review.yml runs against sibling + # repositories only through the organization's required-workflow + # ruleset (README.md's "또 같이" / "siblings call it" section) and + # is never itself committed to those repositories, so + # actions/workflows/noema-review.yml/runs is not guaranteed to + # resolve there -- GitHub's List repository workflows family + # enumerates workflow files that exist in that repository's own + # tree. actions/runs plus the run object's own `.path` field is + # this repo's own already-proven pattern for this exact cross-repo + # cleanup (see strix.yml's identical job). + # Status stays a server-side filter -- bounding each query to only + # the currently active runs -- rather than an unfiltered + # per-workflow fetch filtered client-side, since noema-review.yml + # is this org's central, highest-volume review workflow and an + # unbounded history walk on every PR close is a real rate-limit + # and latency risk here. + cancel_runs() { + local status="$1" + local runs_url="repos/${TARGET_REPOSITORY}/actions/runs?status=${status}&per_page=100" + local runs_json + if ! runs_json="$(gh api --paginate "$runs_url" 2>/tmp/noema-close-gh-error)"; then + echo "::warning::Noema close cleanup could not inspect ${TARGET_REPOSITORY}; leaving runs unchanged." >&2 + sed 's/^/ /' /tmp/noema-close-gh-error >&2 || true + echo 0 + return 0 + fi + local run_ids + # PR-scoped by two independent, OR'd signals -- neither alone + # covers every trigger this job serves. The rendered + # display_title (this workflow's own run-name, embedding the + # target repository/PR number/head SHA) is this workflow's + # original signal, and stays reliable for repository_dispatch + # and workflow_run triggers. But GitHub does not consistently + # render run-name for an organization-required-workflow + # pull_request_target run materialized in a sibling repository + # (Devin Review, PR #1507: "Sibling Noema runs evade + # cancellation") -- `name` and `display_title` can both collapse + # to the bare workflow name and the plain PR title there, + # matching neither the old `.name ==` filter nor the + # display_title prefix below. GitHub's own `pull_requests[]` + # array on the run object closes that gap: it is populated for + # this workflow's pull_request_target runs because the + # noema-review job itself only ever processes same-repository, + # non-fork pull requests (its own `if:` requires + # `head.repo.full_name == github.repository`), so the cross-fork + # "empty pull_requests[]" caveat that rules this field out + # elsewhere in this org's tooling does not apply here. Neither + # signal alone is sufficient for every trigger type, so this + # matches on either one -- never a bare head_sha, which two + # different open PRs can share (e.g. a duplicate PR opened from + # the same branch against another target) and which would let + # closing one cancel the other's still-needed run. Matching by + # PR number rather than by the closed PR's current head SHA also + # means historical-head runs from earlier pushes to this same PR + # are still caught. `.path` pins the workflow identity in place + # of the old `.name ==` filter: unlike `.name` (which, like + # display_title, only carries the bare workflow name for a + # required-workflow-ruleset run), `.path` was independently + # confirmed stable across both native and sibling contexts. + if ! run_ids="$(jq -r --arg pr "$CLOSED_PR_NUMBER" \ + --arg current "$CURRENT_RUN_ID" --arg target "$TARGET_REPOSITORY" ' + .workflow_runs[] + | select((.id | tostring) != $current) + | select(.path == ".github/workflows/noema-review.yml") + | select((.name // "") | startswith("Required Noema Review")) + | select( + ((.display_title // "") | startswith("Required Noema Review " + $target + "#" + $pr + "@")) + or ((.pull_requests // []) | any(.number == ($pr | tonumber))) + ) + | .id + ' <<<"$runs_json")"; then + echo "::warning::Noema close cleanup received invalid run data for ${TARGET_REPOSITORY}; leaving runs unchanged." >&2 + echo 0 + return 0 + fi + local matched=0 + while IFS= read -r run_id; do + [ -n "$run_id" ] || continue + matched=$((matched + 1)) + if gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel" >/dev/null 2>/tmp/noema-close-cancel-error; then + echo "Cancelled Noema run ${run_id} in ${TARGET_REPOSITORY} for closed PR #${CLOSED_PR_NUMBER}." >&2 + else + echo "::warning::Noema close cleanup could not cancel run ${run_id}; it may have finished or the token lacks Actions write access." >&2 + sed 's/^/ /' /tmp/noema-close-cancel-error >&2 || true + fi + done <<<"$run_ids" + echo "$matched" + } + + # A run can transition between the five active statuses between + # one status's fetch and the next (e.g. it is "requested" when the + # already-fetched "queued" list was read, then becomes "queued" + # moments later, after this pass has already moved past checking + # "queued") -- a real GitHub Actions run lifecycle race, not a + # hypothetical. A single sequential sweep can let such a run + # escape cancellation entirely. Re-scan every active status for up + # to three passes: always run at least two full passes (a run that + # slips through every status query in pass 1 has, by definition, + # settled into a checkable status by the time pass 2 queries it + # again), and only skip the third when both prior passes matched + # nothing, bounding the retries so API flakiness cannot loop this + # forever. + max_passes=3 + pass=1 + found_any=0 + while [ "$pass" -le "$max_passes" ]; do + pass_matches=0 + for active_status in queued in_progress requested waiting pending; do + matched="$(cancel_runs "$active_status")" + pass_matches=$((pass_matches + matched)) + done + echo "Noema close cleanup pass ${pass}/${max_passes} matched ${pass_matches} run(s) across active statuses." >&2 + if [ "$pass_matches" -gt 0 ]; then + found_any=1 + fi + if [ "$pass" -ge 2 ] && [ "$pass_matches" -eq 0 ] && [ "$found_any" -eq 0 ]; then + break + fi + pass=$((pass + 1)) + done noema-review: name: noema-review @@ -54,10 +200,17 @@ jobs: && github.event.action != 'closed' && github.event.pull_request.head.repo.full_name == github.repository ) + permissions: + actions: write + checks: read + contents: read + id-token: write + pull-requests: read env: FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.event.client_payload.target_repository || github.repository }} PR_NUMBER: ${{ github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number || github.event.client_payload.pr_number || '' }} + EXPECTED_HEAD: ${{ github.event.client_payload.pr_head_sha || github.event.pull_request.head.sha || github.event.workflow_run.pull_requests[0].head.sha || '' }} steps: - name: Skip events without pull request context if: env.PR_NUMBER == '' @@ -133,6 +286,100 @@ jobs: tar -xzf "$trusted_archive" -C "$GITHUB_WORKSPACE" --strip-components=1 test -f scripts/ci/noema_review_gate.py + - name: Reject a stale trigger before credential or model setup + if: env.PR_NUMBER != '' + env: + GH_TOKEN: ${{ github.token }} + run: | + set -euo pipefail + if [[ ! "$EXPECTED_HEAD" =~ ^[0-9a-f]{40}$ ]]; then + echo "::error::Noema trigger did not provide a canonical lowercase exact head SHA." + exit 1 + fi + live_head="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.head.sha')" + if [ "${live_head,,}" != "${EXPECTED_HEAD,,}" ]; then + echo "::error::Noema trigger is stale; expected ${EXPECTED_HEAD}, observed ${live_head}." + exit 1 + fi + + - name: Cancel superseded Noema runs after live-head validation + if: github.event_name == 'pull_request_target' && env.PR_NUMBER != '' + env: + GH_TOKEN: ${{ github.token }} + CURRENT_RUN_ID: ${{ github.run_id }} + run: | + set -euo pipefail + declare -A seen=() + cancelled=0 + for pass in 1 2; do + for active_status in queued in_progress requested waiting pending; do + if ! runs_json="$(gh api --paginate "repos/${TARGET_REPOSITORY}/actions/runs?status=${active_status}&per_page=100")"; then + echo "::warning::Could not inspect ${active_status} Noema runs for superseded heads." + continue + fi + # See the close-cleanup job's matching comment above cancel_runs's + # own selector for the full rationale: display_title only + # renders this workflow's PR/head-bearing run-name reliably for + # a native trigger, so a sibling-repository required-workflow + # run is additionally matched via GitHub's own pull_requests[] + # array (populated here because noema-review only ever + # processes same-repository, non-fork pull requests), and + # `.path` pins workflow identity where `.name` cannot. The + # live-head exclusion below is independently reinforced with a + # direct `.head_sha` comparison -- the run object's own + # head_sha field, unlike display_title, is populated and + # accurate regardless of run-name rendering, so it protects the + # current run even when its display_title never rendered a + # matching "@$head" suffix to exclude by. + if ! run_ids="$(jq -r --arg pr "$PR_NUMBER" --argjson current "$CURRENT_RUN_ID" \ + --arg target "$TARGET_REPOSITORY" --arg head "$EXPECTED_HEAD" ' + .workflow_runs[] + | select(.id < $current) + | select(.path == ".github/workflows/noema-review.yml") + | select((.name // "") | startswith("Required Noema Review")) + | select( + ((.display_title // "") | startswith("Required Noema Review " + $target + "#" + $pr + "@")) + or ((.pull_requests // []) | any(.number == ($pr | tonumber))) + ) + | select(((.display_title // "") | endswith("@" + $head)) | not) + | select(((.head_sha // "") | ascii_downcase) != ($head | ascii_downcase)) + | .id + ' <<<"$runs_json")"; then + echo "::warning::Could not parse ${active_status} Noema runs for superseded heads." + continue + fi + while IFS= read -r run_id; do + [ -n "$run_id" ] || continue + [ -z "${seen[$run_id]:-}" ] || continue + seen[$run_id]=1 + # A transient failure here (rate limit, network blip) must + # never crash this step under set -e: this is a housekeeping + # cleanup, and letting an ancillary API hiccup fail the whole + # job would block a perfectly valid, live-head review over + # something unrelated to it. Treat "cannot verify" the same + # as "verified stale": stop cancelling rather than risk a + # wrong cancellation, but let the job continue. + if ! live_head="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.head.sha' 2>/tmp/noema-supersede-live-head-error)"; then + echo "::warning::Noema cleanup could not re-verify the live PR head before cancelling run ${run_id}; stopping cleanup rather than risking a wrong cancellation." >&2 + sed 's/^/ /' /tmp/noema-supersede-live-head-error >&2 || true + exit 0 + fi + if [ "${live_head,,}" != "${EXPECTED_HEAD,,}" ]; then + echo "::notice::Noema cleanup stopped because the PR head advanced." + exit 0 + fi + if gh api --method POST "repos/${TARGET_REPOSITORY}/actions/runs/${run_id}/cancel" >/dev/null; then + cancelled=$((cancelled + 1)) + echo "Cancelled superseded Noema run ${run_id} for PR #${PR_NUMBER}." + else + echo "::warning::Could not cancel superseded Noema run ${run_id}; it may already be terminal." + fi + done <<<"$run_ids" + done + echo "Superseded Noema cleanup pass ${pass}/2 complete." + done + echo "Cancelled ${cancelled} superseded Noema run(s) after live-head validation." + - name: Select fail-closed Noema reviewer credential if: env.PR_NUMBER != '' id: noema_credential @@ -327,4 +574,5 @@ jobs: export NOEMA_LLM_VIA_ORCHESTRATOR=1 python3 -m scripts.ci.noema_review_gate \ --repo "$TARGET_REPOSITORY" \ - --pr-number "$PR_NUMBER" + --pr-number "$PR_NUMBER" \ + --expected-head "$EXPECTED_HEAD" diff --git a/.github/workflows/opencode-review-dispatch.yml b/.github/workflows/opencode-review-dispatch.yml index 2aa245e7f2..cdc1245266 100644 --- a/.github/workflows/opencode-review-dispatch.yml +++ b/.github/workflows/opencode-review-dispatch.yml @@ -34,6 +34,7 @@ jobs: name: validate-pr-metadata if: github.event_name == 'repository_dispatch' runs-on: ubuntu-latest + timeout-minutes: 8 permissions: contents: read pull-requests: read @@ -224,6 +225,7 @@ jobs: needs.validate-pr-metadata.result == 'success' && github.event_name == 'repository_dispatch' runs-on: ubuntu-latest + timeout-minutes: 12 permissions: contents: read id-token: write @@ -371,6 +373,7 @@ jobs: && needs.coverage-source-tree.result != 'cancelled' && github.event_name == 'repository_dispatch' runs-on: ubuntu-latest + timeout-minutes: 300 permissions: # The PR tree arrives through a same-run artifact. No repository-content, # identity, secret, or write token is available to untrusted tests. @@ -2309,9 +2312,9 @@ jobs: # 36-minute publication gate, the 18-minute Noema handoff, and setup/cleanup # overhead without truncating a late current-head verdict, handoff, merge # scheduler follow-up, or bounded failure reason. - timeout-minutes: 325 + timeout-minutes: 305 permissions: - actions: read + actions: write checks: read id-token: write contents: read @@ -4003,9 +4006,9 @@ jobs: OPENCODE_MODEL_ATTEMPTS: "1" # Preserve reviews that legitimately need tens of minutes to inspect a # large repository. Changed-file count is not a repository-complexity - # proxy, so every cadence class gets 90 minutes per candidate while the - # bounded provider-pool watchdog remains the outer guard. - OPENCODE_RUN_TIMEOUT_SECONDS: "5400" + # proxy. Let Contextual Orchestrator use the existing total review + # budget; the bounded provider-pool watchdog remains the outer guard. + OPENCODE_RUN_TIMEOUT_SECONDS: "11700" OPENCODE_EXPORT_TIMEOUT_SECONDS: "180" OPENCODE_TOTAL_RETRY_BUDGET_SECONDS: "11700" OPENCODE_POOL_STEP_TIMEOUT_SECONDS: "12000" @@ -4017,22 +4020,22 @@ jobs: OPENCODE_DYNAMIC_REVIEW_CADENCE: "true" OPENCODE_SMALL_CHANGE_FILE_THRESHOLD: "3" OPENCODE_MEDIUM_CHANGE_FILE_THRESHOLD: "20" - OPENCODE_SMALL_CHANGE_RUN_TIMEOUT_SECONDS: "5400" + OPENCODE_SMALL_CHANGE_RUN_TIMEOUT_SECONDS: "11700" OPENCODE_SMALL_CHANGE_TOTAL_BUDGET_SECONDS: "11700" - OPENCODE_MEDIUM_CHANGE_RUN_TIMEOUT_SECONDS: "5400" + OPENCODE_MEDIUM_CHANGE_RUN_TIMEOUT_SECONDS: "11700" OPENCODE_MEDIUM_CHANGE_TOTAL_BUDGET_SECONDS: "11700" - OPENCODE_LARGE_CHANGE_RUN_TIMEOUT_SECONDS: "5400" + OPENCODE_LARGE_CHANGE_RUN_TIMEOUT_SECONDS: "11700" OPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDS: "11700" - OPENCODE_UNKNOWN_CHANGE_RUN_TIMEOUT_SECONDS: "5400" + OPENCODE_UNKNOWN_CHANGE_RUN_TIMEOUT_SECONDS: "11700" OPENCODE_UNKNOWN_CHANGE_TOTAL_BUDGET_SECONDS: "11700" - OPENCODE_DYNAMIC_RUN_TIMEOUT_CAP_SECONDS: "5400" + OPENCODE_DYNAMIC_RUN_TIMEOUT_CAP_SECONDS: "11700" OPENCODE_DYNAMIC_TOTAL_BUDGET_CAP_SECONDS: "11700" OPENCODE_DYNAMIC_MAX_CYCLES_CAP: "1" OPENCODE_FREE_RUN_TIMEOUT_SECONDS: "3600" OPENCODE_DYNAMIC_MAX_CYCLES: "1" CENTRAL_REVIEW_PROCESS_FALLBACK_ELIGIBLE: ${{ steps.central_review_process_fallback_scope.outputs.eligible || 'false' }} CENTRAL_REVIEW_PROCESS_FALLBACK_SCOPE_LABEL: ${{ steps.central_review_process_fallback_scope.outputs.scope_label || 'unsupported' }} - OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_RUN_TIMEOUT_SECONDS: "5400" + OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_RUN_TIMEOUT_SECONDS: "11700" OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_TOTAL_BUDGET_SECONDS: "11700" OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_MAX_CYCLES: "1" OPENCODE_BACKOFF_INITIAL_SECONDS: "30" @@ -7545,6 +7548,7 @@ jobs: echo "::endgroup::" - name: Enforce current-head formal OpenCode review receipt + id: formal_review_receipt if: >- always() && needs.validate-pr-metadata.result == 'success' @@ -7568,6 +7572,61 @@ jobs: --head-sha "$PR_HEAD_SHA" \ "${draft_args[@]}" + - name: Wake exact-head required OpenCode workflow + if: >- + always() + && github.event_name == 'repository_dispatch' + && steps.formal_review_receipt.outcome == 'success' + && needs.validate-pr-metadata.outputs.target_repository != '' + && needs.validate-pr-metadata.outputs.head_sha != '' + && github.event.client_payload.required_run_id != '' + env: + GH_TOKEN: ${{ needs.validate-pr-metadata.outputs.target_repository == github.repository && github.token || secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN }} + GH_REPOSITORY: ${{ needs.validate-pr-metadata.outputs.target_repository }} + PR_HEAD_SHA: ${{ needs.validate-pr-metadata.outputs.head_sha }} + REQUIRED_RUN_ID: ${{ github.event.client_payload.required_run_id }} + WAKE_TOKEN_SOURCE: ${{ needs.validate-pr-metadata.outputs.target_repository == github.repository && 'github-token' || secrets.PR_REVIEW_MERGE_TOKEN != '' && 'PR_REVIEW_MERGE_TOKEN' || secrets.OPENCODE_APPROVE_TOKEN != '' && 'OPENCODE_APPROVE_TOKEN' || 'unavailable' }} + run: | + set -euo pipefail + if [ -z "${GH_TOKEN:-}" ] || [ "$WAKE_TOKEN_SOURCE" = "unavailable" ]; then + echo "::error::Actions-capable wake credential is unavailable. Native runs use github.token; sibling runs require PR_REVIEW_MERGE_TOKEN or OPENCODE_APPROVE_TOKEN." + exit 1 + fi + [[ "$REQUIRED_RUN_ID" =~ ^[1-9][0-9]*$ ]] || { + echo "::error::Required OpenCode run id is missing or non-canonical." + exit 1 + } + # The immutable run id is scoped to GH_REPOSITORY. Revalidate its + # event, central workflow path, and live PR head before rerunning it; + # rendered titles and workflow_url differ between native and + # organization-required workflow contexts. + for attempt in $(seq 1 12); do + run="$(gh api "repos/${GH_REPOSITORY}/actions/runs/${REQUIRED_RUN_ID}")" + required_run="$(printf '%s\n' "$run" | jq -r --arg head "$PR_HEAD_SHA" --argjson run_id "$REQUIRED_RUN_ID" ' + select(.id == $run_id) + | select(.event == "pull_request_target") + | select(.path == ".github/workflows/opencode-review.yml") + | select(.head_sha == $head) + | [(.id // ""), (.status // ""), (.conclusion // "")] + | @tsv + ')" + IFS=$'\t' read -r required_run_id required_status required_conclusion <<<"$required_run" + if [ "$required_status" = "completed" ] && [ "$required_conclusion" = "failure" ]; then + gh api -X POST "repos/${GH_REPOSITORY}/actions/runs/${required_run_id}/rerun-failed-jobs" >/dev/null + echo "Re-ran failed jobs for exact-head Required OpenCode Review run ${required_run_id}." + exit 0 + fi + if [ "$required_status" = "completed" ] && [ "$required_conclusion" = "success" ]; then + echo "Exact-head Required OpenCode Review run ${required_run_id} already succeeded." + exit 0 + fi + if [ "$attempt" -lt 12 ]; then + sleep 5 + fi + done + echo "::error::Formal OpenCode receipt exists, but the exact-head required workflow did not reach a rerunnable failed state." + exit 1 + - name: Publish repository_dispatch OpenCode status if: >- always() diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index f16f5f3106..81faf57757 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -33,6 +33,22 @@ jobs: echo "Required OpenCode workflow materialized without checking out or executing pull-request content." + - name: Reject untrusted fork review resource consumption + env: + PR_ACTION: ${{ github.event.action }} + BASE_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name }} + HEAD_REPOSITORY: ${{ github.event.pull_request.head.repo.full_name }} + run: | + set -euo pipefail + if [ "$PR_ACTION" = "closed" ]; then + echo "PR closed; fork-resource-consumption check is not required." + exit 0 + fi + if [ -z "$BASE_REPOSITORY" ] || [ -z "$HEAD_REPOSITORY" ] || [ "$HEAD_REPOSITORY" != "$BASE_REPOSITORY" ]; then + echo "::error::Long-running required review is restricted to branches in the base repository. A maintainer must materialize an external contribution on a trusted branch before review." + exit 1 + fi + - name: Resolve immutable central policy source id: trusted_source env: @@ -235,66 +251,35 @@ jobs: name: opencode-review needs: [coverage-evidence] runs-on: ubuntu-latest - timeout-minutes: 100 + timeout-minutes: 5 permissions: contents: read pull-requests: read id-token: write steps: - - name: Request current-head OpenCode review execution - if: github.event.action != 'closed' - env: - OIDC_AUDIENCE: opencode-github-action - OPENCODE_API_BASE_URL: https://api.opencode.ai - TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} - BASE_BRANCH: ${{ github.event.pull_request.base.ref }} - run: | - set -euo pipefail - if [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ]; then - echo "::error::OpenCode review dispatch requires GitHub OIDC." - exit 1 - fi - separator='&' - [[ "$ACTIONS_ID_TOKEN_REQUEST_URL" == *\?* ]] || separator='?' - oidc_token="$(curl -fsS -H "Authorization: Bearer ${ACTIONS_ID_TOKEN_REQUEST_TOKEN}" "${ACTIONS_ID_TOKEN_REQUEST_URL}${separator}audience=${OIDC_AUDIENCE}" | jq -r '.value // empty')" - if [ -z "$oidc_token" ]; then - echo "::error::OpenCode review dispatch could not obtain its OIDC token." - exit 1 - fi - app_token="$(curl -fsS -X POST -H "Authorization: Bearer ${oidc_token}" "${OPENCODE_API_BASE_URL}/exchange_github_app_token" | jq -r '.token // empty')" - if [ -z "$app_token" ]; then - echo "::error::OpenCode review dispatch could not obtain its repository-scoped app token." - exit 1 - fi - echo "::add-mask::$app_token" - jq -cn \ - --arg target_repository "$TARGET_REPOSITORY" \ - --arg pr_number "$PR_NUMBER" \ - --arg base_branch "$BASE_BRANCH" \ - '{event_type:"merge-scheduler",client_payload:{target_repository:$target_repository,pr_number:$pr_number,base_branch:$base_branch,max_prs:"1",review_dispatch_limit:"1",trigger_reviews:true,enable_auto_merge:false,update_branches:false,dry_run:false}}' | - GH_TOKEN="$app_token" gh api -X POST repos/ContextualWisdomLab/.github/dispatches --input - - - - name: Fail closed without a current-head OpenCode verdict + - name: Resolve current-head formal OpenCode verdict + id: verdict env: GH_TOKEN: ${{ github.token }} TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} + PR_ACTION: ${{ github.event.action }} run: | set -euo pipefail - if [ "${{ github.event.action }}" = "closed" ]; then + if [ "$PR_ACTION" = "closed" ]; then echo "PR closed; a current-head OpenCode verdict is not required." + echo "verdict=CLOSED" >>"$GITHUB_OUTPUT" exit 0 fi if [ -z "${PR_NUMBER:-}" ] || [ -z "${HEAD_SHA:-}" ]; then echo "::error::Missing PR number or head SHA; cannot verify a current-head OpenCode verdict." exit 1 fi - verdict="" - for attempt in $(seq 1 180); do - reviews="$(gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews")" - verdict="$(printf '%s\n' "$reviews" | jq -r -s --arg sha "$HEAD_SHA" ' + if ! reviews="$(timeout 25 gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews")"; then + reviews="[]" + fi + verdict="$(printf '%s\n' "$reviews" | jq -r -s --arg sha "$HEAD_SHA" ' (add // []) | [ .[] @@ -322,15 +307,62 @@ jobs: empty end ')" - if [ -n "$verdict" ]; then - break - fi - if [ "$attempt" -lt 180 ]; then - sleep 30 - fi - done - if [ -z "$verdict" ]; then + echo "verdict=${verdict}" >>"$GITHUB_OUTPUT" + if [ -n "$verdict" ]; then + echo "Current-head OpenCode verdict: ${verdict}." + fi + + - name: Request current-head OpenCode review execution + if: github.event.action != 'closed' && steps.verdict.outputs.verdict == '' + env: + OIDC_AUDIENCE: opencode-github-action + OPENCODE_API_BASE_URL: https://api.opencode.ai + TARGET_REPOSITORY: ${{ github.event.pull_request.base.repo.full_name || github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + BASE_BRANCH: ${{ github.event.pull_request.base.ref }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_BRANCH: ${{ github.event.pull_request.head.ref }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + if [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ]; then + echo "::error::OpenCode review dispatch requires GitHub OIDC." + exit 1 + fi + separator='&' + [[ "$ACTIONS_ID_TOKEN_REQUEST_URL" == *\?* ]] || separator='?' + oidc_token="$(curl -fsS -H "Authorization: Bearer ${ACTIONS_ID_TOKEN_REQUEST_TOKEN}" "${ACTIONS_ID_TOKEN_REQUEST_URL}${separator}audience=${OIDC_AUDIENCE}" | jq -r '.value // empty')" + if [ -z "$oidc_token" ]; then + echo "::error::OpenCode review dispatch could not obtain its OIDC token." + exit 1 + fi + app_token="$(curl -fsS -X POST -H "Authorization: Bearer ${oidc_token}" "${OPENCODE_API_BASE_URL}/exchange_github_app_token" | jq -r '.token // empty')" + if [ -z "$app_token" ]; then + echo "::error::OpenCode review dispatch could not obtain its repository-scoped app token." + exit 1 + fi + echo "::add-mask::$app_token" + jq -cn \ + --arg target_repository "$TARGET_REPOSITORY" \ + --argjson pr_number "$PR_NUMBER" \ + --arg pr_base_ref "$BASE_BRANCH" \ + --arg pr_base_sha "$BASE_SHA" \ + --arg pr_head_ref "$HEAD_BRANCH" \ + --arg pr_head_sha "$HEAD_SHA" \ + --argjson required_run_id "$GITHUB_RUN_ID" \ + '{event_type:"opencode-review",client_payload:{target_repository:$target_repository,pr_number:$pr_number,pr_base_ref:$pr_base_ref,pr_base_sha:$pr_base_sha,pr_head_ref:$pr_head_ref,pr_head_sha:$pr_head_sha,required_run_id:$required_run_id}}' | + GH_TOKEN="$app_token" gh api -X POST repos/ContextualWisdomLab/.github/dispatches --input - + + - name: Fail closed without a current-head OpenCode verdict + env: + VERDICT: ${{ steps.verdict.outputs.verdict }} + run: | + set -euo pipefail + if [ "$VERDICT" = "CLOSED" ]; then + exit 0 + fi + if [ -z "$VERDICT" ]; then echo "::error::No APPROVED or CHANGES_REQUESTED from opencode-agent on the current head. This required check is not a review and must not succeed until the authenticated dispatch posts a current-head verdict." exit 1 fi - echo "Current-head OpenCode verdict: ${verdict}." + echo "Current-head OpenCode verdict: ${VERDICT}." diff --git a/.gitleaksignore b/.gitleaksignore index a9ee62806c..83eca07ec1 100644 --- a/.gitleaksignore +++ b/.gitleaksignore @@ -2,6 +2,10 @@ # The live tests now construct these token-like strings at runtime; new findings remain blocking. # Historical synthetic OpenAI-key fixture from the provider-diagnostic regression test. b8a18e9bba3c0afb9eeea51d33bc0d5307b733d8:tests/test_opencode_model_pool_runner.py:generic-api-key:87 +# Historical synthetic UUID-shaped fixture (unrecognized-secret-shape regression test) that +# predates commit 3f3bb47, which now constructs it at runtime via fake_secret(*parts); this +# entry only silences the superseded historical commit still reachable in this PR's range. +6657eb76f0e2cf6dab9197cfa861a1f584653aba:tests/test_noema_review_gate.py:generic-api-key:187 995d6b3606e0effe6722c422b369da9ef0171824:tests/test_pr_review_merge_scheduler.py:github-pat:2900 995d6b3606e0effe6722c422b369da9ef0171824:tests/test_pr_review_merge_scheduler.py:github-pat:2908 995d6b3606e0effe6722c422b369da9ef0171824:tests/test_pr_review_merge_scheduler.py:github-pat:2923 diff --git a/.jules/bolt.md b/.jules/bolt.md index b5c165a673..ba3acea9a5 100644 --- a/.jules/bolt.md +++ b/.jules/bolt.md @@ -51,3 +51,9 @@ ## 2026-08-29 - [대용량 텍스트 스캔 시 정규표현식 대신 네이티브 메서드 활용] **Learning:** `scripts/ci/opencode_review_normalize_output.py`의 라벨 스캐닝 루프에서 긴 LLM 리뷰 텍스트를 대상으로 `pattern.finditer()`를 호출하는 패턴이 있었습니다. 마이크로 벤치마크 결과, 단순 문자열 매칭에서는 네이티브 `str.find()`와 `while` 루프를 조합하는 것이 정규표현식 실행 오버헤드 없이 훨씬 빠르다는 것을 확인했습니다. **Action:** 내부 탐색 루프에서 정확히 일치하는 리터럴 문자열(라벨 접두사 등)을 검색할 때는 `re.compile(re.escape(string)).finditer()` 대신 고도로 최적화된 Python 네이티브 `text.find(candidate, index)` 메서드를 사용하십시오. 단, 무한 루프를 방지하기 위해 루프의 모든 분기에서 인덱스가 올바르게 진행되도록 보장해야 합니다. +## 2026-08-29 - [문자열 검색 윈도우 동적 축소를 통한 라벨 스캐닝 최적화] +**Learning:** `scripts/ci/opencode_review_normalize_output.py`의 `label_section` 함수에서 다음 라벨을 찾을 때, 전체 텍스트에 대해 모든 후보 라벨의 위치를 파악하는 방식은 O(N * L) (N=텍스트 길이, L=라벨 개수)의 심각한 오버헤드를 발생시킵니다. +**Action:** 긴 텍스트에서 다음 마커(라벨 등)를 찾을 때, 현재까지 발견된 가장 가까운 다음 마커의 위치(`index`)로 검색 윈도우의 끝(`end = min(end, index)`)을 동적으로 축소하면서 네이티브 `text.find(candidate, start, end)`를 호출하십시오. 이는 중복 스캐닝을 크게 줄입니다. +## 2026-08-31 - [LLM 재시도 루프 시 오차 위치 명시] +**Learning:** `noema_review_gate.py` 등 LLM 응답을 파싱하고 검증하는 로직에서 오류 발생 시 해당 오류 메시지(`str(exc)`)를 프롬프트에 포함하여 다시 LLM을 호출(`repair_error`)하는 구조가 존재합니다. 이때 에러 메시지에 LLM이 잘못 생성한 데이터(예: 존재하지 않는 파일 경로, 일치하지 않는 라인 번호 등)를 구체적으로 포함시키지 않으면, LLM은 무엇이 틀렸는지 알지 못해 동일한 실수를 반복하여 CI 실패를 초래합니다. +**Action:** LLM의 형식 오류나 검증 실패로 인해 `RuntimeError` 등을 발생시킬 때, 단순히 `is not an exact changed-side line`과 같이 이유만 명시하지 말고 `It cited: {location}`처럼 구체적으로 오류를 일으킨 잘못된 LLM 출력을 함께 포함하십시오. diff --git a/CHANGELOG.md b/CHANGELOG.md index 39c61c142b..43020db98e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,128 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Fail closed when the first top-level Noema JSON candidate is malformed, + preventing a later approval object from overriding malformed preface data; + multiple-object output remains supported when its first object is valid. +- Restore the exact-head dispatch contract after the default-branch rollback: + queued requests whose supplied head no longer matches the live pull request + fail before model work, and the workflow security assertions and reviewed + blob pin now enforce that behavior. +- Reject excessively nested Noema LLM JSON responses with an explicit, + string-literal-aware bracket-depth bound (`MAX_JSON_NESTING_DEPTH = 100`), + checked before `json.JSONDecoder.raw_decode` is ever attempted, instead of + relying on `raw_decode`'s own recursion behavior to reject deep input + (review follow-up on #1507): a real 20,000-level-deep payload raises + `RecursionError` from the C-accelerated scanner on Python 3.11-3.13 but + decodes successfully with no exception at all on the Python 3.14 hosted + runner this job actually runs on, so relying on that behavior made the + fail-closed guarantee a property of whichever CPython version happened to + run the job rather than of this code. Restored the excessive-nesting + regression to a real deep payload (not a monkeypatch) now that this bound + makes the real case reproducible everywhere; the synthetic + `RecursionError`-from-the-decoder test remains as supplemental coverage. +- Match JSON delimiter types while discovering Noema verdict candidates, so + malformed wrappers such as `[}` or `{]` cannot release a later nested + object as an apparently top-level verdict. +- Convert JSON decoder recursion failures from deeply nested Noema responses + into the existing bounded, fingerprinted fail-closed diagnostic instead of + allowing an unhandled `RecursionError` to crash the required review. +- Restrict wrapped Noema JSON recovery to top-level brace groups so a valid + nested object cannot escape a malformed outer object and become a verdict. +- Keep Noema's native concurrency head-specific, then explicitly cancel the + same PR's older-head runs only after a `pull_request_target` event proves its + payload SHA is still live. New commits stop obsolete four-hour model calls, + while delayed workflow events and manual reruns of old attempts cannot + cancel the current-head review; cleanup rejects newer run ids and rechecks + the live head before each cancellation. Guard that per-cancellation + live-head re-check against a transient `gh api` failure (Devin review on + #1507): it was an unguarded command substitution under `set -euo + pipefail`, so a rate limit or network blip on that one ancillary call + would exit the whole cleanup step non-zero and fail the job, blocking a + perfectly valid, live-head Noema review over a housekeeping hiccup + unrelated to the review itself. Treat "cannot verify" the same as + "verified stale": stop cancelling further runs, but exit 0 so the job -- + and the actual review later in it -- proceeds. +- Prevent a cancelled upstream `workflow_run` notification from cancelling a + live same-head Noema review and then skipping its own Noema job. The shared + head-specific group remains serialized, but cancelled upstream completions + no longer receive `cancel-in-progress` authority and use a run-unique group, + so GitHub cannot evict an already-pending actionable review either. +- Replace the required OpenCode workflow's two chained 325-minute polling jobs + with event-driven continuation. The required run dispatches the authenticated + multi-hour review, checks once, and fails closed without retaining a hosted + runner; after a formal exact-head receipt is published, the privileged + dispatch reruns only that required run's failed job. Long model and coverage + budgets remain unchanged. Fork PRs still fail closed before dispatch; + maintainers must first materialize them on a trusted base-repository branch. + The required workflow passes its immutable run ID in the authenticated + dispatch; the continuation fetches that target-repository run directly and + revalidates its event, central workflow path, and live PR `head_sha` before + rerunning it, independent of queue duration. Scheduler-originated review + retries now carry the same run ID parsed from the required check's GitHub + Actions details URL, so their valid receipts wake the failed required job too. + The wake step now uses its job-scoped `actions: write` workflow token only for + native runs and requires `PR_REVIEW_MERGE_TOKEN` or + `OPENCODE_APPROVE_TOKEN` for sibling runs; it no longer falls through to the + review-only OpenCode app token or an unusable central workflow token. +- Skip Noema's one-time repair-retry LLM request when the PR head has moved + since the first attempt was fired (CodeRabbit review on #1507): `call_llm` + now takes `expected_head` and re-checks it against a fresh `fetch_pr` + lookup, lowercased like `inspect_and_review`'s existing two stale-head + checks, before firing the retry — avoiding a second, potentially + multi-hour `NOEMA_LLM_TIMEOUT_SECONDS` call for a verdict + `inspect_and_review`'s own post-call check would have discarded anyway. A + new `StaleHeadDuringRepairRetryError` reports this distinctly from the + existing "stale before model work" / "stale before publication" cases, + and `inspect_and_review` treats it the same way: a clean skip, not a + failure. +- Re-pin the reviewed-blob contract test's SHA to the current + `opencode-review-dispatch.yml` content after the review run timeout change, + restoring `test_independent_review_agent_workflow_matches_reviewed_blob`. +- Let Contextual Orchestrator use the full 11,700-second review budget in every + cadence and the central-review fallback, so reviews exceeding two hours are + bounded only by the existing provider-pool watchdog. +- Cancel queued and running Noema reviews from every historical head group when + their pull request closes, preventing abandoned model calls from consuming + runner capacity for the long-running review window. Selection is scoped by PR + number only (the run's structured display title), never by a bare shared + head SHA, so a different open PR that happens to share a commit is never + swept up. The five active-status queries stay repository-scoped and + server-side status-filtered (not a per-workflow-file, unfiltered-then- + client-filtered snapshot, which is not guaranteed to resolve for the + sibling-repository runs this cleanup exists to cancel) and now re-scan for + up to three bounded passes so a run transitioning between statuses + mid-sweep is still caught. +- Reject caller-controlled uppercase Noema trigger SHAs before model work so + equivalent SHA casing cannot create concurrent duplicate reviews. +- Bind Noema workflow concurrency to the triggering PR head so a delayed + OpenCode/Strix completion from an older head cannot cancel the current-head + review run. The trigger head is also checked against the live PR before + credential/model setup and again before review publication, preventing a + stale run from reviewing or publishing against a newer live head. Completion + events use the associated pull request's head rather than the workflow's + trusted base SHA, and hexadecimal comparison is case-insensitive. +- Keep the Noema malformed-response UUID fixture covered by gitleaks without + weakening the secret gate: the historical ignore is limited to the exact + superseded commit, test path, rule, and line, with an executable contract. +- Allow a Contextual Orchestrator-backed Noema review request to run for up to + four hours instead of failing long reviews at a hard-coded 120 seconds. +- Stop logging raw (even regex-scrubbed) LLM response text in Noema's + malformed-JSON fail-closed diagnostic (Devin Review security finding on + PR #1507): `noema-review.yml` is a `pull_request_target` workflow with + public Actions logs, and a finite secret-scrub pattern list cannot + guarantee an LLM-echoed or hallucinated credential in an unrecognized + shape is caught. `extract_json_object` now logs only a content length and + a SHA-256 fingerprint. Also close a related unhandled-crash gap: a + malformed OpenAI-compatible HTTP envelope (non-JSON body, non-object + top-level JSON, wrong-shaped `choices`/`message`, non-string `content`) + previously crashed `call_llm` before it ever reached the JSON-repair + boundary; a new `extract_llm_message_content` validates the envelope + explicitly and now shares the same one-time repair-retry and fail-closed + `RuntimeError` path as a malformed verdict. +- Give Noema one bounded schema-repair request when Contextual Orchestrator + returns malformed verdict JSON, then fail closed with a scrubbed diagnostic + if the corrected response is still invalid. - Harden the review sidecar's per-account catalog cap against silent drift: `contextual_orchestrator_review_launcher.py`'s two `build_zdr_prioritized_catalog` call sites now source their diff --git a/docs/pr-review-and-merge-procedure.md b/docs/pr-review-and-merge-procedure.md index 7866f9851b..e34c3957cb 100644 --- a/docs/pr-review-and-merge-procedure.md +++ b/docs/pr-review-and-merge-procedure.md @@ -21,8 +21,13 @@ stage changes, commit, push, install dependencies, mutate branches, or touch production state. Blocking findings must be source-backed, severity-labeled, impactful, remediable, and include suggested verification. -The OpenCode review job does not widen its own `pull_request_target` job token -to repository-write permission. The scheduler's `GH_TOKEN` merge/read fallback +The OpenCode `pull_request_target` bootstrap does not widen its own token to +repository-write permission. After a formal receipt, the privileged +`repository_dispatch` job grants `actions: write` only so its `github.token` +can rerun a native failed required job. A sibling wake instead requires +`PR_REVIEW_MERGE_TOKEN` or `OPENCODE_APPROVE_TOKEN`; it fails closed without +either and never substitutes the review-only OpenCode app token or the central +repository's workflow token. The scheduler's `GH_TOKEN` merge/read fallback order is `PR_REVIEW_MERGE_TOKEN`, `OPENCODE_APPROVE_TOKEN`, the exchanged OpenCode app token, then the receiving workflow's `github.token`. For repository-dispatch calls that target another repository, the @@ -140,6 +145,18 @@ OpenCode for the same PR head when review evidence is missing or stale. This avoids running PR-head review, CodeGraph, coverage, or PoC code as an unbounded local workflow copy. +The required OpenCode workflow does not poll for the multi-hour model run on a +hosted runner. It dispatches once, checks for an existing exact-head formal +receipt, and otherwise fails closed. After the privileged dispatch publishes +an `APPROVED` or `CHANGES_REQUESTED` receipt, it selects the latest matching +`pull_request_target` run by exact head and reruns only its failed job. Thus the +ruleset-required workflow becomes successful only after the receipt exists, +without reserving a runner while the model works. + +The central dispatch title and validated metadata remain bound to the requested +head. If the pull request advances while queued, validation fails closed and the +scheduler dispatches a new current-head run; publication is exact-head guarded. + Scheduled review-feedback autofix is also centralized. The `PR Review Fix Scheduler` dispatches the central `PR Review Autofix` worker in `ContextualWisdomLab/.github` and passes the target repository, PR number, @@ -154,6 +171,25 @@ Strix keeps `cancel-in-progress: false` so old evidence is not cancelled by a force-push, but PR-scoped concurrency includes the head SHA so an obsolete scan does not serialize newer current-head evidence. +Noema keeps head-specific native concurrency so a delayed workflow event or a +manual rerun of an older attempt cannot cancel the current head. After a +`pull_request_target` run proves its payload SHA still equals the live PR head, +live-head validation explicitly cancels active Noema runs for the same PR's +other heads before credential or model setup, limited to older run ids and a +fresh live-head check before each cancellation. That fresh check fails safe: +an API error stops further cancellation instead of failing the job, so a +transient lookup failure cannot block a live-head review over a housekeeping +hiccup. Each run also compares its +immutable trigger head with the live PR before model work, before a repair +retry, and immediately before publication; a stale run cannot review or +publish against a newer head. + +The trigger mapping is explicit: `pull_request_target` uses `pull_request.head.sha`, +`workflow_run` uses `workflow_run.pull_requests[0].head.sha`, and +`repository_dispatch` uses `client_payload.pr_head_sha`. Missing or malformed +HEAD identity fails closed before reviewer credentials or model capacity are +used. + ## Approve-gate evidence OpenCode approval is evidence-gated. Before approval, the review summary must diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 758ef2961a..76d85b949b 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -1715,6 +1715,635 @@ string, a bare number) confirmed to fail against the pre-fix script (`KeyError: signature as the original round-4 bug) before passing after the fix. 1930 tests pass; 100% coverage and 100% docstring coverage on `scripts/ci/`. +## 2026-08-31 noema-review-gate: malformed LLM JSON crashed the required check instead of failing closed + +The required `noema-review` check on `ContextualWisdomLab/contextual-orchestrator#960` crashed with an +unhandled `json.decoder.JSONDecodeError` inside `extract_json_object`, called from `call_llm` in +`scripts/ci/noema_review_gate.py`. Investigated the canonical-source question first, since this is +exactly the shape of a central-vs-local drift-copy question this repo's own policy addresses: +`contextual-orchestrator` has no `scripts/ci/noema_review_gate.py` committed at all and no +`noema-review.yml` workflow of its own — the required `Required Noema Review` workflow +(`.github/workflows/noema-review.yml`, this repo) materializes this file from a tarball of this repo's +trusted commit SHA into every target repo's runner (`Materialize trusted Noema review gate` step), so the +fix belongs here only; there was no local drift copy in `contextual-orchestrator` to remove either, since +none existed. + +Root cause: `extract_json_object` located a `{...}` substring in the LLM's response content and called +`json.loads()` on it directly with no exception handling. A truncated or malformed model reply (observed: +an unquoted property name partway through the object — exactly `Expecting property name enclosed in +double quotes`) raised `json.JSONDecodeError`, which propagated out of `call_llm`, `inspect_and_review`, +and `main`, past the module's `except RuntimeError` guard in `__main__` (which only catches +`RuntimeError`), crashing the whole `noema-review` job with a raw Python traceback and zero signal about +why the review didn't complete. Every PR org-wide that hit this same LLM-output edge case would hit the +identical unhandled crash, since the same materialized file runs in every target repo. + +Fixed by catching `json.JSONDecodeError` in `extract_json_object` and converting it into the same +`RuntimeError` this file already raises for its other "no usable verdict" cases in `call_llm` +(unsupported decision, missing summary, malformed finding). `call_llm` now gives every invalid verdict +one bounded correction request through its existing repair path; a second invalid response fails closed +through the module's top-level non-zero exit. The error message embeds the raw model response, scrubbed of secrets via +`scrub_sensitive_data` and bounded to a new `MAX_LLM_RESPONSE_LOG_CHARS` (2000 chars), so the job log +still shows *why* the verdict was unusable. (The candidate substring `extract_json_object` extracts is +guaranteed to start with `{`, so per JSON grammar a successful parse can only ever yield an object — a +"valid JSON but not an object" branch would be unreachable dead code under this repo's 100%-coverage gate +and was deliberately not added.) The top-level `__main__` handler was also changed to print +`::error::{exc}` instead of a bare message, matching this repo's own convention in sibling CI gates +(`opencode_review_receipt_gate.py`, `select_nvidia_nim_model.py`). + +Regression tests reproduce the exact reported crash signature at both layers — +`test_extract_json_object_fails_closed_on_malformed_json` (brace-wrapped invalid JSON, mid-object +truncation, secret-scrubbing, length-bounding), `test_call_llm_fails_closed_on_malformed_json_response`, +and `test_call_llm_repairs_one_malformed_json_response` exercise the bounded repair and exhausted-repair +paths. A clean `RuntimeError` propagates only after the corrected response is still invalid. 100% coverage +and 100% docstring coverage on `scripts/ci/`. PR: ContextualWisdomLab/.github#1507. + +The same gate also imposed a hard-coded 120-second HTTP read timeout. A real +Four Pillars review reached that boundary after Contextual Orchestrator had +successfully provisioned and selected a route, then failed with an unhandled +`TimeoutError` before a verdict arrived. Noema review requests now allow the +documented four-hour request window; GitHub's job boundary remains the outer +execution limit. The transport timeout is pinned by the existing call contract +test so a shorter accidental value cannot silently restore the failure. + +## 2026-08-31 noema-review-gate follow-up: fail-closed fix itself still had a public-log secret-leak +edge and an unhandled envelope-crash edge + +Devin Review on PR #1507 found two gaps in the malformed-JSON fail-closed fix above, before that PR +finished its own review cycle — both genuine, not duplicates of the round-4 pattern already recorded. + +**Security (priority): raw model output could still leak an unrecognized-shape credential to a public +log.** The fix above logged the LLM's raw response text through `scrub_sensitive_data` — a finite, +pattern-based regex scrubber (known token/key prefixes, `Bearer`/`token`/`key=` shapes) — into the +`RuntimeError` message that `__main__` prints as `::error::{exc}` on stderr. `noema-review.yml` is a +`pull_request_target` workflow, so that Actions log is public on this org's public repos. A regex +allowlist of known secret *shapes* cannot bound what an LLM might echo back or hallucinate in an +unrecognized shape (mid-sentence, base64-wrapped, or simply a shape nobody anticipated) — no amount of +pattern-list tuning closes that gap, so the fix does not try to. `extract_json_object`'s decode-failure +diagnostic no longer embeds the raw or scrubbed response at all; it logs only a length and a truncated +SHA-256 fingerprint of the (unlogged) content, enough to correlate repeat failures for the same +underlying response without ever exposing its bytes. `MAX_LLM_RESPONSE_LOG_CHARS` (the old +truncate-and-embed bound) was removed as unused. Regression test +`test_extract_json_object_fails_closed_on_malformed_json` was extended to assert this directly: a +credential in a shape none of the `SENSITIVE_DATA_SCRUB_PATTERNS` recognize (a bare UUID-shaped value +mid-sentence, no `token`/`key`/`bearer` marker) is confirmed to survive the old scrubber unmasked, then +confirmed absent from the new diagnostic entirely — as is a known-shape secret, and the raw response text +in general, regardless of input size. + +**Bug: a malformed gateway envelope still crashed before the repair boundary.** `call_llm` only wrapped +`extract_json_object(content)` — parsing the nested verdict string — in the `try` that feeds the #1504 +one-time repair-retry. The lines building `content` from the raw HTTP body (`json.loads(raw)` then four +chained `.get()`/`[0]` accesses) sat *before* that `try`, unguarded: a non-JSON raw body raised an +unhandled `json.JSONDecodeError`, and a syntactically valid but wrong-shaped envelope (top-level JSON +that is a list/`null`/string/number, a non-list `choices`, a non-object `choices[0]` or `message`, or +non-string `content`) raised an unhandled `AttributeError`/`TypeError`/`KeyError` — exactly the class of +crash the malformed-JSON fix above was meant to close, just one layer higher. Fixed with a new +`extract_llm_message_content(raw)` that validates the envelope shape explicitly with `isinstance` checks +at each step (never a broad `except AttributeError`/`TypeError`, so a genuine unrelated bug still +surfaces as itself) and raises the same bounded `RuntimeError` `call_llm` already converts everywhere +else; the call now sits inside the existing repair-retry `try` block, so a malformed envelope gets the +same one repair-retry request a malformed verdict gets before failing closed with a clean diagnostic. A +missing (not malformed) `choices`/`message`/`content` still falls through to an empty string, matching +the original code's leniency for an absent field — `extract_json_object` already fails closed on empty +content. None of the raised messages embed any response bytes, only JSON-value type names. + +Regression tests: direct unit coverage of every `extract_llm_message_content` branch (malformed raw +body, non-object top level, non-list `choices`, non-object `choices[0]`/`message`, non-string `content`, +and the lenient missing-field paths), plus `call_llm` integration tests reproducing the repair-once and +exhausted-repair paths end-to-end (`test_call_llm_repairs_one_malformed_envelope_before_failing_closed`, +`test_call_llm_fails_closed_after_repeated_malformed_envelope`). 100% coverage (branch included) and 100% +docstring coverage on `scripts/ci/`. PR: ContextualWisdomLab/.github#1507 (same PR; addressed before +merge). + +## 2026-08-31 noema-review-gate follow-up round 3: non-UTF-8 gateway replies still crashed before the +repair boundary + +Devin Review's third pass on PR #1507 found one more instance of the same crash-before-repair-boundary +class the round-2 fix above closed for a malformed JSON envelope, plus two informational confirmations +that needed verifying rather than fixing. + +**Bug: a non-UTF-8 response body still crashed before the repair boundary.** `call_llm` decoded the raw +HTTP response with a plain `response.read().decode("utf-8")` sitting *before* the `try` that feeds the +repair-retry — the same unguarded-preamble shape the round-2 envelope fix closed for `json.loads` and the +chained `.get()`/`[0]` accesses, just one step earlier. A gateway reply containing invalid UTF-8 bytes +raised an unhandled `UnicodeDecodeError` before `extract_llm_message_content` or the JSON repair boundary +ever ran, crashing the required review check with a traceback instead of getting the same one-time +schema-repair attempt every other malformed-envelope shape already gets. Fixed with a new +`decode_llm_response_body(raw_bytes)` that converts a `UnicodeDecodeError` into the same bounded +`RuntimeError` `call_llm` already uses elsewhere, called from inside the existing repair-retry `try` +block (`raw = decode_llm_response_body(raw_bytes)`, ahead of `extract_llm_message_content(raw)`). Per the +round-2 security fix, the raised diagnostic never embeds the raw response bytes — not even the +undecodable fragment, since a body containing invalid UTF-8 could still contain a credential-adjacent +byte sequence — only a length and a truncated SHA-256 fingerprint, matching `extract_json_object`'s +no-raw-content pattern exactly. + +Regression tests: `test_decode_llm_response_body_happy_path` and +`test_decode_llm_response_body_fails_closed_on_invalid_utf8` give direct unit coverage of the new +function (including that a secret-shaped prefix and an unrecoverable tail around the bad byte never +appear in the raised message), and `test_call_llm_fails_closed_after_repeated_invalid_utf8_response` +integrates it end-to-end: one repair-retry request, then a clean top-level `RuntimeError` when the retry +response is *also* invalid UTF-8 — never an unhandled traceback. 100% coverage (branch included) and 100% +docstring coverage on `scripts/ci/`. + +**Confirmed correct, no change needed — repair recursion remains bounded.** `call_llm`'s `except +RuntimeError` handler only recurses once: `if repair_error: raise` re-raises immediately on a second +failure instead of recursing again, so total gateway calls per review are capped at two regardless of +which layer (decode, envelope, or verdict JSON) keeps failing. Already covered by +`test_call_llm_fails_closed_after_repeated_malformed_envelope` and the new +`test_call_llm_fails_closed_after_repeated_invalid_utf8_response`, both of which assert exactly two +requests were made. + +**Confirmed correct, no change needed — falsey envelope values still fail closed.** A `choices`, +`message`, or `content` field that is present but falsey-and-wrong-shaped for the lenient branch (e.g. +`choices: false`, `choices: 0`, `choices: ""`, `choices: []`) is treated by `extract_llm_message_content` +the same as an absent field — deliberately lenient, per that function's existing docstring — and resolves +to empty `content`. That empty string is not silently accepted: `extract_json_object` requires content +starting with `{` and raises its own bounded `RuntimeError` ("did not contain a JSON object") for an +empty string, so the falsey-envelope path still fails closed one layer down. Verified directly against +`extract_llm_message_content` + `extract_json_object` for `choices` in `{False, 0, "", []}`. + +PR: ContextualWisdomLab/.github#1507 (same PR; addressed before merge). Devin's own framing marked this +the last expected finding in this decode/parse vein for this PR. + +## 2026-08-31 noema-review-gate stale-trigger guard: workflow_run head misread and case-sensitive SHA +comparison + +Devin Review's next pass on PR #1507 reviewed the stale-trigger guard added around `EXPECTED_HEAD` (the +mechanism that aborts a Noema review run — before any credential/model work or verdict publication — when +its triggering event's head no longer matches the PR's live head) and found two real bugs. Given this +PR's concurrent commit velocity, a sibling session landed the same two fixes to `noema-review.yml` and +`scripts/ci/noema_review_gate.py` (`d74fc4b`/`a5262f3`/`a398a02`/`e4c7a8d`) while this session was still +verifying them; this entry records the independently-confirmed root cause and evidence, plus the +regression tests this session added on top of that already-landed fix (rebased cleanly, no functional +disagreement between the two). + +**Bug 1 (confirmed real): `workflow_run`-triggered reviews always looked stale.** `noema-review.yml` +subscribes to `workflow_run` for `["Required OpenCode Review", "Strix Security Scan"]` — both +`pull_request_target` workflows — so Noema runs as their follow-up. `EXPECTED_HEAD`, the `run-name`, and +the `concurrency` group all read `github.event.workflow_run.head_sha` for that path, but GitHub's +`workflow_run.head_sha` is the base/trusted commit the completing `pull_request_target` job checked out +(its own `github.sha`), not the PR's head — confirmed against GitHub's REST/webhook docs for the +`workflow_run` payload and against this same workflow's own `PR_NUMBER` line, which already reads the +correct PR association via `github.event.workflow_run.pull_requests[0].number`. Every +`workflow_run`-triggered follow-up review was therefore comparing the live PR head against the wrong +(base) commit in `EXPECTED_HEAD` and would almost always find them unequal, aborting the run and silently +skipping the review it exists to produce. Fixed by reusing the same established `pull_requests[0]` pattern +for the head SHA everywhere it appears: `github.event.workflow_run.pull_requests[0].head.sha`, in +`EXPECTED_HEAD`, `run-name`, and the `concurrency` group alike (`docs/pr-review-and-merge-procedure.md`'s +trigger-mapping table updated to match). `pull_requests` is documented to come back empty for cross-fork +PRs; that already degrades safely (`EXPECTED_HEAD` falls through to `''`, and `PR_NUMBER` — sourced from +the same array — already falls through the same way, so the existing "Skip events without pull request +context" step short-circuits before any stale-head comparison runs). + +**Bug 2 (confirmed real): uppercase `--expected-head` was falsely treated as stale.** +`scripts/ci/noema_review_gate.py`'s `--expected-head` regex (`^[0-9a-fA-F]{40}$`) accepts uppercase hex, +and the bash-side guard in `noema-review.yml` accepts it too, but both of the script's live-head +comparisons (`inspect_and_review`'s pre-model-work check against `fetch_pr(...).headRefOid`, and its +pre-publication re-check against a freshly re-fetched `headRefOid`) used a plain case-sensitive `!=` +against GitHub's GraphQL `headRefOid`, which is always lowercase — as did the workflow YAML's own bash +`[ "$live_head" != "$EXPECTED_HEAD" ]` check against the REST `.head.sha` field. A legitimately +uppercase-cased dispatch (e.g. from `client_payload.pr_head_sha`) would be rejected or silently skipped at +every one of these sites even though it named the correct commit. Fixed by lowercasing both sides at +every comparison: `inspect_and_review` normalizes its `expected_head` parameter once +(`expected_head = expected_head.strip().lower()`) and lowercases `headRefOid` at both comparison sites; +the workflow's bash check now compares `"${live_head,,}" != "${EXPECTED_HEAD,,}"`, reusing this repo's +existing `${VAR,,}` lowercase-normalization idiom already used for PR SHAs elsewhere in +`opencode-review-dispatch.yml`. + +Regression tests added by this session on top of the landed fix: `tests/test_noema_orchestrator_workflow_contract.py` adds +`test_workflow_run_expected_head_uses_pull_request_head_not_base_commit` (proves, with distinct base vs. +PR-head SHA values, that the fixed expression resolves to the PR head and not the base commit) and +`test_workflow_run_expected_head_fails_closed_when_pull_requests_is_empty`, plus +`test_stale_trigger_step_compares_expected_head_case_insensitively` and +`test_stale_trigger_step_still_rejects_a_genuinely_different_head`, which execute the workflow's own +extracted bash step against a fake `gh` to prove the case-insensitive fix without weakening genuine +stale-trigger detection. `tests/test_noema_review_gate.py` adds +`test_uppercase_expected_head_is_not_stale_before_model_work` and +`test_uppercase_expected_head_is_not_stale_before_publication`, covering both Python-side comparison +sites end-to-end (through to `submit_review` actually being called), complementing the sibling session's +own `test_expected_head_comparison_is_case_insensitive`. 100% coverage (branch included) and 100% +docstring coverage on `scripts/ci/`. + +PR: ContextualWisdomLab/.github#1507 (same PR; addressed before merge). + +## 2026-09-01 OpenCode contextual-orchestrator runtime ceiling + +Exact-head evidence from four-pillars PRs #35 and #37 showed the required +OpenCode job failing closed after approximately 91 minutes without a verdict. +The central model-pool workflow still capped its contextual-orchestrator +candidate, every changed-file cadence, the dynamic cap, and the central-review +fallback at 5,400 seconds even though the target, pool, and retry budgets already +had capacity for a long-running candidate. Those seven limits now use the full +11,700-second review budget, with an executable step-scoped contract preventing +unrelated numeric strings elsewhere in the workflow from masking a regression. + +PR: ContextualWisdomLab/.github#1507 (same PR; addressed before merge). + +## 2026-08-31 noema-review-gate close-cleanup job: bare head_sha match, single-pass status sweep, and a +workflow-file-scoped endpoint that does not resolve for the sibling repositories the job exists to clean up + +Devin Review's pass on the `cancel-closed-pr-runs` job (the job that cancels still-active "Required Noema +Review" runs when their pull request closes) found two real bugs plus a test-quality gap. Verified against +a fresh clone of `fix/noema-review-gate-json-parse-crash` at commit `03117b7` (the commit that introduced +this job) -- neither was fixed yet at that point. While this session was building its own fix, a concurrent +session landed `e0f542f` ("fix: scope Noema cleanup to closed PR") addressing both findings with a +different mechanism; this session's mandatory pre-push `git fetch && git rebase` surfaced it. Rather than +push a duplicate/conflicting fix, this session verified `e0f542f` independently, found its Bug 2 mechanism +introduces a new regression specific to this job's cross-repository use case, and landed a corrected +version on top of it (`git reset --hard` to `e0f542f` locally, since this session's own prior commit had +never been pushed, then a fresh commit) rather than a competing rewrite. + +**Bug 1 (confirmed real, and correctly fixed by `e0f542f`): bare `head_sha` match let one PR's close +cancel a different PR's still-needed run.** The jq selector's match condition was an OR of three clauses, +the first a bare `.head_sha == $head_sha` with no PR association required. Two different open PRs can +share one head commit (e.g. a duplicate PR opened from the same branch against a different target); +closing one would match and cancel the *other*, unrelated PR's run purely because of the shared commit. +`e0f542f` dropped the bare `head_sha` OR-branch (and the `pull_requests[]` branch alongside it), keeping +only the `display_title` `"target#pr@"` prefix match -- this workflow's own generated run-name, itself +derived from the same PR-number resolution chain the job's other env vars use, so it identifies the +correct PR without depending on GitHub's `pull_requests[]` array (documented empty for cross-fork PRs). +This session's independent re-derivation reached the same conclusion and kept this exact selector logic +unchanged. + +**Bug 2 (confirmed real; `e0f542f`'s fix introduces a different regression for this job's primary use +case): a run could transition between the five active statuses faster than a sequential per-status sweep +could see it.** The original `cancel_runs` was called once per status in a fixed loop, each call issuing +its own `gh api` fetch at a different moment; a run that is e.g. `requested` when the already-fetched +`queued` list was read, then becomes `queued` moments later -- after the loop has already moved past +checking `queued` for that pass -- is a genuine GitHub Actions run lifecycle race that could let an +abandoned run escape cancellation entirely. `e0f542f` fixed this by switching to one unfiltered snapshot +(`.../actions/workflows/noema-review.yml/runs`, no `status` filter, filtered client-side by jq instead), +which does eliminate the race for a query targeting the *central* `.github` repository. It does not for the +job's actual primary case: `noema-review.yml` runs against **sibling** repositories only through the +organization's required-workflow ruleset (`README.md`'s "또 같이" / "siblings call it" section: "GitHub +runs the trusted workflows from `ContextualWisdomLab/.github@main` in that sibling's repository context") +and is never itself committed to those repositories' own `.github/workflows/`. GitHub's `List repository +workflows` / `List workflow runs for a workflow` endpoint family is documented (and, per public reporting +on the predecessor "required workflows" feature's retirement, confirmed to differ) to enumerate workflow +files that exist in that specific repository's own tree; there is no documentation stating a ruleset-only +required workflow sourced from a different repository is addressable this way in the target repository's +context, and this repository's own established pattern for the identical cross-repo cleanup problem +(`strix.yml`'s sibling `cancel-closed-pr-runs` job) deliberately uses the repository-wide, `.name`-filtered +`/actions/runs` endpoint rather than a workflow-file-scoped one. If unresolved for a sibling repository, +`gh api`'s failure is caught by this job's existing fail-open `::warning::...leaving runs unchanged; exit +0` handling, so the job would not error -- it would silently no-op cleanup for every sibling repository, +which is the majority of this job's real invocations and exactly the outcome the whole feature exists to +prevent (the original `03117b7` commit message: abandoned model calls consuming runner capacity for the +two-hour review window). Fixed by keeping `e0f542f`'s selector (display_title-only PR scoping) but +restoring the repository-wide, `status`-server-filtered `/actions/runs` endpoint, and replacing the +original single sequential sweep with a bounded multi-pass re-scan instead of one unfiltered snapshot: +the five-status sweep always runs at least two full passes (a run missed by every status query in pass 1 +has, by definition, settled into a checkable status by the time pass 2 re-queries it), and a third pass +runs only when either of the first two found something to cancel, capped at three passes total. Status +stays a *server-side* filter deliberately -- `noema-review.yml` is this org's central, highest-volume +review workflow (fan-out across every sibling PR event plus every OpenCode/Strix completion), and an +unfiltered fetch of its entire run history on every PR close, filtered only client-side, is a real +rate-limit and latency concern this repository's own `gh api --help`/REST docs give no server-side +multi-status filter to avoid; the bounded-retry, status-filtered design keeps every individual query small +(only the currently active runs) while still closing the race across passes. + +**Test-quality finding (addressed): existing coverage only grep-matched workflow YAML text, never +executed the jq selector or the cancellation loop.** `e0f542f` had already added one such test +(`test_noema_close_cleanup_selects_only_the_closed_pr_from_one_snapshot` in +`tests/test_noema_orchestrator_workflow_contract.py`) executing the real extracted bash against a fake +`gh`; because its fake `gh` answered every call with the same fixture regardless of the requested status, +it implicitly assumed client-side status filtering and needed updating to filter by the `status=` query +parameter (mirroring GitHub's real server-side behavior) once server-side filtering was restored -- +renamed to `test_noema_close_cleanup_selects_only_the_closed_pr_across_shared_display_titles` with that +fix, its shared-head-SHA/different-PR-number assertions otherwise unchanged. Two further tests were added +to `tests/test_noema_review_gate.py`, both executing the workflow's real bash via this repo's established +`_extract_run_block`-plus-`subprocess.run`-with-a-fake-`gh` idiom (matching +`tests/test_noema_orchestrator_workflow_contract.py`'s pattern for this same job): +`test_close_cleanup_selector_is_pr_scoped_not_head_sha_scoped` proves, with two synthetic runs sharing one +head SHA but different PR numbers (42 closing, 43 open), that only PR #42's run is cancelled; and +`test_close_cleanup_survives_a_run_transitioning_between_active_statuses` proves, with a stateful fake +`gh` that only reveals a run under `queued` starting on that status's *second* query, that the fixed +multi-pass sweep still cancels it, and that pass 1 alone finds nothing (`"pass 1/3 matched 0 run(s)"` in +the captured log) -- demonstrating the original single-sweep design would have missed it. All three tests +were confirmed to fail both against the pre-`03117b7` state and, independently, against `e0f542f` alone +(the status-transitioning-run test errors out on `e0f542f`'s workflow-scoped, no-`status`-param URL, which +this test's status-aware fake `gh` cannot resolve into a per-status result -- itself supporting evidence +for the endpoint regression above) before passing against this session's corrected version. + +Validation: `coverage run -m pytest tests -q` -- 2169 passed, 1 skipped, 21 subtests passed; `coverage +report` -- 100% on `scripts/ci/` (no `.py` production files touched; the fix and its tests are entirely in +`.github/workflows/noema-review.yml` and `tests/`); `interrogate` -- 100% docstring coverage (minimum +100.0%, actual 100.0%). The workflow file re-parses clean with `yaml.safe_load`, and the touched `run:` +block passes `bash -n` both as extracted at edit time and as exercised end-to-end by the new subprocess +tests. Full validation was re-run after this PR's isolated-clone protocol's pre-push +`git fetch && git rebase`, given the branch's ongoing concurrent commit velocity. + +PR: ContextualWisdomLab/.github#1507 (same PR; addressed before merge). + +## 2026-08-31 opencode-review.yml required-verdict poller: complete multi-job wait budget + +**Current status: resolved in the same PR.** The investigation below records +the intermediate single-job mitigation and the platform limit it exposed. Its +residual-gap conclusion is superseded by the final design: the required check +dispatches OpenCode directly and chains two 325-minute polling windows, while +the downstream validation, source, coverage, and review jobs have explicit +8-, 12-, 300-, and 305-minute bounds. This covers the full 625-minute +downstream path inside roughly 650 minutes of polling without shortening the +205-minute model-pool budget. Each Reviews API call is capped at 25 seconds and +counts inside a fixed 30-second polling cadence. Fork PRs fail closed during +the short bootstrap job, so untrusted contributors cannot allocate either +long-running wait window; a maintainer must materialize an accepted external +contribution on a base-repository branch first. + +Devin Review's pass on `opencode-review.yml`'s "Fail closed without a current-head OpenCode verdict" +step (the poller the branch-protection-required `opencode-review-target` job uses to wait for +`opencode-review-dispatch.yml` to post a verdict) found a real arithmetic bug: 639 `sleep 30` calls +(the loop never sleeps after its final attempt) sum to 319.5 minutes of polling patience, which is +*less* than `opencode-review-dispatch.yml`'s own `opencode-review-target` job's `timeout-minutes: 325` +-- the job that actually runs the review and posts the verdict this poller is waiting for. The poller +could give up before that job's own declared budget elapses, even before counting the +`validate-pr-metadata` -> `coverage-source-tree` -> `coverage-evidence` chain that job's `needs:` list +requires to finish first, or the dispatch/queueing delay before that chain even starts. Independently +verified the arithmetic (639 x 30 = 19170s = 319.5m < 325m) against a fresh clone at the branch's then +head before making any change. CodeRabbit's independent pass on the same step added a second, distinct +finding: the loop's `sleep 30` calls were the *only* budgeted time -- the up to 640 sequential +`gh api --paginate repos/{repo}/pulls/{number}/reviews` calls themselves had no timeout and no budget +allocation, so one hung connection or a heavily-paginated PR review list could silently consume time +the arithmetic above never accounted for. + +**Investigated the full pipeline before picking new numbers, and found a platform ceiling neither +finding's suggested fix accounted for.** `opencode-review-dispatch.yml`'s own `opencode-review-target` +job carries a job-header comment breaking its 325-minute budget into named line items (12m evidence + +205m provider-pool + 36m publication gate + 18m Noema handoff + ~54m setup/cleanup overhead), and an +existing test (`test_opencode_job_timeout_contains_full_sequential_review_budget` in +`tests/test_opencode_agent_contract.py`) already asserts that composition holds -- left unchanged here. +The three jobs upstream of it in that same workflow's `needs:` chain (`validate-pr-metadata`, +`coverage-source-tree`, `coverage-evidence`) carry no `timeout-minutes` of their own; the only +script-enforced bound inside them is `coverage-evidence`'s three sequential +`timeout --kill-after=20 900` sandboxed test-measurement invocations (Python/R/a third language, +2700s/45m worst case), on top of realistic (not pathological) dispatch-event, runner-provisioning, +Docker-image-build, and git-fetch/artifact-transfer overhead -- a realistic worst-case estimate in the +~90-105 minute range. Summed with the downstream job's own 325-minute budget, a fully safe poller +budget would need to exceed roughly 415-430 minutes. But GitHub-hosted runners (`runs-on: ubuntu-latest`, +used by both the poller job and every job in the chain it waits on) hard-cap **every** job's wall-clock +at 360 minutes regardless of `timeout-minutes` +(; corroborated by +, a report of exactly this "`timeout-minutes: 600` +but killed at 360m anyway" gotcha) -- so no value written into this poller job's `timeout-minutes` can +ever let it wait the full realistic worst case; the platform kills the runner first. This also explains, +retroactively, why the downstream job's own budget was set to 325 rather than something larger: 325 is +already only 35 minutes under that same 360-minute ceiling. + +**Fix: maximize patience within what a single GitHub-hosted job can actually deliver, document the +residual gap explicitly, and treat "one call can't silently be unbounded" as a real, separate defect +worth fixing alongside the budget numbers.** Raised the enclosing `opencode-review-target` job's +`timeout-minutes` from 325 to 355 (5 minutes under the 360-minute hard cap -- the largest value that +stays honored by the platform rather than silently truncated). Raised the poll loop's attempt count from +640 to 661 (`for attempt in $(seq 1 661)`; `sleep 30` interval unchanged), giving 660 sleeps x 30s = 330 +minutes of pure-sleep patience -- now 5 minutes *more* than the downstream job's own 325-minute budget, +closing Devin's specific inequality with an explicit margin, versus falling 5.5 minutes short before. +Addressed CodeRabbit's per-call finding by wrapping the `gh api --paginate` call itself in +`timeout 25`, so no single call (hung connection or an unusually deep multi-page fetch) can consume more +than 25 seconds; a failed or timed-out call now degrades to treating that attempt as "no verdict yet" +(`reviews="[]"`) and continues polling on the next attempt, instead of crashing the whole step under +`set -euo pipefail` the way an unguarded `reviews="$(gh api ...)"` would have. This leaves 25 minutes of +declared slack (355m job timeout minus 330m poll budget) for the dispatch step, cumulative per-call +latency across up to 661 attempts, and runner/shutdown overhead, so the loop's own +`::error::No APPROVED or CHANGES_REQUESTED...` message is the one that fires on genuine exhaustion, +not an abrupt platform-level job-timeout kill with no actionable message. + +**What this fix does and does not close.** It provably fixes Devin's narrow arithmetic complaint (poll +budget now exceeds the downstream job's own declared budget, with margin) and CodeRabbit's per-call +budgeting gap (every `gh api` call is now individually bounded and its failure handled). It does *not* +close the larger realistic-worst-case gap: 330 minutes of patience is still well short of the +~415-430 minute realistic worst case once upstream chain delay is counted, because that full figure +exceeds even the platform's own 360-minute per-job ceiling -- no `timeout-minutes` value fixes that. +Fully closing it needs an architecture change (splitting the wait across multiple short-lived +re-dispatched jobs, e.g. chained through `workflow_run`, rather than one job blocking end-to-end) that +is deliberately out of scope for this budget-sizing fix and is recorded here as an explicit residual +risk rather than silently left implicit. + +**Test-quality finding (addressed): the existing regression test only pinned exact literals +(`"timeout-minutes: 325"`, `"for attempt in $(seq 1 640)"`), which would have needed a matching +hand-edit on every future change and would not have caught a future edit that broke the underlying +relationship while still passing its own literal check.** `tests/test_opencode_required_verdict_regression.py` +now parses the poller's attempt count, sleep interval, per-call timeout, and enclosing job timeout +directly out of `opencode-review.yml`, and the downstream job's `timeout-minutes` directly out of +`opencode-review-dispatch.yml` (same regex shape already used by +`test_opencode_job_timeout_contains_full_sequential_review_budget`), then asserts the arithmetic +relationships rather than the literals: `test_poll_budget_exceeds_downstream_review_job_budget_with_explicit_margin` +asserts the poll budget clears the downstream budget plus an explicit 5-minute margin; +`test_enclosing_job_timeout_has_headroom_above_the_poll_budget` asserts the job's own timeout-minutes +stays at or below the 360-minute GitHub-hosted hard cap and leaves at least 20 minutes of slack above the +pure-sleep budget; `test_poller_gh_api_call_has_an_explicit_per_call_timeout` asserts the per-call +timeout wrapper and the fail-soft `reviews="[]"` fallback are present. Verified these tests actually +catch the original bug (not just pass vacuously) by temporarily reverting the workflow to the pre-fix +640/325 numbers and confirming both budget tests fail with the exact original shortfall +(`330s slack < 1200s minimum`), then restored the fix and re-confirmed all pass. Also added a small +functional smoke test (bash, fake `gh`, tiny timeout/sleep values) exercising the modified loop's exact +structure end-to-end: two simulated hung calls are killed by `timeout` and gracefully treated as +"no verdict yet" without crashing the script, and the loop finds and returns the correct verdict once +`gh` starts succeeding. + +Validation: `coverage run -m pytest tests -q` -- 2173 passed, 1 skipped, 21 subtests passed (up from the +prior 2169-passed baseline by the 3 new tests plus one already landed by a concurrent commit this +session rebased onto); `coverage report` -- 100% on `scripts/ci/` (no `.py` production files touched; the +fix and its tests are entirely in `.github/workflows/opencode-review.yml` and `tests/`); `interrogate` -- +100% docstring coverage (minimum 100.0%, actual 100.0%). `actionlint v1.7.12` (built locally via +`go install`, since no prebuilt binary or cached module was reachable through the outbound proxy) reports +no findings on the modified workflow file (exit 0). `yaml.safe_load` and `bash -n` both re-confirmed +clean on the modified step, and the existing `tests/test_opencode_workflow_shell_syntax.py` suite passes +unchanged. + +PR: ContextualWisdomLab/.github#1507 (same PR; addressed before merge). + +## 2026-08-31 noema-review-gate: repair-retry request fired without re-checking a live-moved PR head + +CodeRabbit's review on PR #1507 found a real efficiency gap in `call_llm`'s one-time repair-retry path. +`inspect_and_review(repo, number, expected_head)` already checks the normalized `expected_head` against +the PR's live `headRefOid` twice -- once before any credential/model work, and again right before +`submit_review` -- but `call_llm` itself had no `expected_head` parameter at all. Its self-recursive +repair-retry branch (`except RuntimeError as exc: if repair_error: raise; return call_llm(..., str(exc))`, +fired once whenever the first attempt's verdict is malformed) went straight to a second, +`NOEMA_LLM_TIMEOUT_SECONDS`-bounded (currently 14,400 seconds) request with no live-head check of its own. +Verified independently from a fresh isolated clone (not the branch's shared working checkout, given three +concurrent actors were pushing to it) before making any change: confirmed both existing checks, confirmed +`call_llm`'s signature had no `expected_head`, and confirmed the recursive retry call site had no head +comparison anywhere on its path. Net effect was wasted compute, not a correctness gap -- the existing +post-call check in `inspect_and_review` already stopped a genuinely stale verdict from publishing -- but a +PR head moving mid-first-attempt could still burn a second, potentially multi-hour LLM call producing a +verdict `inspect_and_review` was always going to discard once `call_llm` returned. + +**Fix.** `expected_head: str` was added to `call_llm`'s signature as a required parameter, positioned +after the other required parameters (`repo`, `number`, `pr`, `diff`, `truncated`) and before the existing +optional, default-valued ones (`review_context`, `changed_paths`, `repair_error`) -- keeping this file's +existing convention of required-then-optional parameter ordering. Inside the repair-retry branch, after +the existing `if repair_error: raise` short-circuit (which already caps retries at one) and before the +recursive call, `call_llm` now re-fetches the live PR via the existing `fetch_pr` helper (no new HTTP +call) and compares its `headRefOid`, lowercased, against `expected_head` -- the same lowercase-normalized +comparison idiom `inspect_and_review`'s own two checks already use. A mismatch raises a new +`StaleHeadDuringRepairRetryError(RuntimeError)` (defined immediately above `call_llm`) with a distinct +message ("...stale before repair retry.") rather than a bare `RuntimeError`, so `inspect_and_review` can +tell a benign stale-head race apart from a genuine review failure and keep treating it as the same kind of +clean, non-error skip (`print(...); return 0`) as its other two stale-head checks -- not as a hard failure +that would reach `main`'s top-level `except RuntimeError` / `::error::` / exit-1 path. `inspect_and_review` +now calls `call_llm` inside a `try`/`except StaleHeadDuringRepairRetryError` for exactly that purpose. +Scope was kept intentionally narrow: this does not touch the separate `submit_review` TOCTOU race +CodeRabbit flagged on the same PR (tracked separately, not a code change), and it does not redesign +`call_llm`'s retry/repair architecture -- one added live-head check on the one existing retry path. + +**Regression tests** (`tests/test_noema_review_gate.py`): `test_call_llm_skips_repair_retry_when_head_moves_before_it_fires` +proves the retry request never fires (`len(open_calls) == 1`) and `StaleHeadDuringRepairRetryError` is +raised with a "stale before repair retry" message when the live head has moved between the first attempt +and the retry decision; `test_call_llm_still_repairs_once_when_head_has_not_moved` proves the existing +one-time repair behavior is unchanged when the head has not moved; `test_inspect_and_review_reports_stale_before_repair_retry_cleanly` +proves `inspect_and_review` converts that exception into a clean `return 0` without ever calling +`submit_review`. Every pre-existing direct `call_llm(...)` call site across `tests/test_noema_review_gate.py`, +`tests/test_noema_review_orchestrator_ssrf.py`, and `tests/test_repository_branch_coverage_review_schedulers.py` +was updated for the new required parameter; call sites that raise before `call_llm`'s HTTP request (URL/ +SSRF validation) needed only the added argument, while call sites that exercise the repair-retry path +needed a `fetch_pr` mock added alongside it so the new live-head check has something to compare against. + +Validation: `coverage run -m pytest tests -q` -- 2174 passed, 1 skipped, 21 subtests passed. Baseline +before this change was 2170 passed; two concurrent sessions' opencode-review.yml poller-budget fixes +landed and were picked up mid-session by this PR's mandatory pre-push `git fetch`/rebase protocol (first +`ddaa917`, widening the poller's own budget past its downstream job, raising the baseline to 2173; then +`4548f93`, which superseded that same-day fix with a different architecture -- two chained polling +windows covering the complete multi-hour path -- landing at 2171 before this change's own 3 new tests). +Both moves produced a `CHANGELOG.md` conflict against this entry's own `[Unreleased]` bullet (resolved by +keeping this session's bullet plus whichever upstream bullet was current at that fetch, dropping the +now-superseded intermediate one); `docs/product-technical-gap-baseline.md` conflicted once and auto-merged +cleanly the second time. `coverage report --show-missing` -- 100% on `scripts/ci/` (`noema_review_gate.py`: +517 stmts, 232 branches, 100%; TOTAL unchanged at 10,600 stmts / 4,252 branches, since neither concurrent +fix touched a `scripts/ci/` production file); `interrogate` -- 100% docstring coverage (minimum 100.0%, +actual 100.0%); `ruff check` on every touched file -- all checks passed. Full validation was re-run after +every rebase, given the branch's ongoing concurrent commit velocity from multiple simultaneous sessions. + +PR: ContextualWisdomLab/.github#1507 (CodeRabbit review on #1507; same PR, addressed before merge). + +Deeply nested wrapped JSON can make Python's decoder raise `RecursionError` +instead of `JSONDecodeError`. The extraction boundary now converts that case +to the same bounded length-and-SHA-256 fail-closed diagnostic, with a regression +test that forces the decoder failure without depending on interpreter-specific +nesting limits. + +### Same-PR old-head model cancellation + +The repair-retry guard prevents a second stale request, but head-specific +workflow concurrency still allowed the first request to occupy a runner for up +to four hours after a new commit. Head-specific native concurrency remains so +a delayed event or manual rerun of an older attempt cannot cancel the current +head. After a live `pull_request_target` event passes the existing live-head +check, it explicitly cancels active runs for the same PR's other heads before +model setup, but only when their run IDs are smaller than its own. This +directional condition prevents an older cleanup racing a push from cancelling +the newer run and closes the stale-compute gap without weakening exact-head +review publication. + +Cancelled upstream review runs exposed a separate same-head race: their +`workflow_run` notifications entered this concurrency group, cancelled a live +native Noema review, and then skipped because the upstream conclusion was +`cancelled`. Merely disabling `cancel-in-progress` is insufficient because +GitHub always replaces the existing pending member of a concurrency group with +the newest pending run. Cancelled notifications therefore use a run-unique +suffix and are also denied cancellation authority. All actionable triggers +remain in the shared head-specific group; successful or failed upstream +completions still serialize and trigger the intended current-head review. + +## 2026-08-31 noema-review-gate: the live-head re-check added to close the above gap was itself an unguarded API call + +Auditing the directional cancellation guard immediately above (run IDs smaller than the current run, plus +a fresh live-head re-check performed again right before each individual cancellation) for robustness -- +not disputing its correctness -- found +`live_head="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.head.sha')"` was a bare +assignment under this step's own `set -euo pipefail`, unlike every other `gh api` call in this same step +and in the sibling `cancel-closed-pr-runs` job, which are all wrapped in `if ! ... ; then warn; +continue/return; fi`. Reproduced concretely: a fake `gh` that fails only this one call (simulating a +transient rate limit or network blip) makes the whole step exit 1, which -- since no later step in this +job declares `continue-on-error` or `if: always()` -- fails the entire `noema-review` job, blocking a +perfectly valid, live-head Noema review over a housekeeping API hiccup unrelated to the review itself +(Devin review on #1507). + +**Fix**: wrap the re-check the same way every other `gh api` call in this file already is -- on failure, +log a `::warning::` and `exit 0` (treat "cannot verify" the same as "verified stale": stop cancelling +further runs, but let the job, and the actual review later in it, proceed). Reproduced the crash against +the pre-fix step with a hand-rolled fake `gh`, confirmed `exit 0` post-fix with the identical fake-failure +fixture, and confirmed the normal (non-failure) cancellation path is unchanged, before folding both +scenarios into `tests/test_noema_review_gate.py` as +`test_superseded_cleanup_survives_a_transient_live_head_lookup_failure`, executing the real, unmodified +production bash (not a reimplementation) via `subprocess.run`, in the same fake-`gh`-fixture idiom +`test_superseded_cleanup_preserves_current_and_newer_run_ids` already established for this step. +`test_noema_concurrency_and_live_head_cleanup_preserve_current_review` was also extended with a docstring +enumerating the four invariants this mechanism now holds together across every review round it took to get +here (new-head cancels old-head; a delayed workflow_run/repository_dispatch trigger never reaches this +step at all; a directional ordering guard stops an older cleanup from racing a newer run; and this +live-head re-check itself fails safe) plus structural assertions for the step's `pull_request_target`-only +gate and the now-guarded (non-bare) live-head re-check -- so a future edit that reintroduces any of these +regressions fails a test immediately rather than requiring another bot-finds-it/human-fixes-it round. + +Validation: `coverage run -m pytest tests -q` -- 2179 passed, 1 skipped, 21 subtests passed (1 new test +plus one extended existing test); `coverage report` -- 100% on `scripts/ci/` (no `.py` production file +touched by this specific fix; the fix and its tests are entirely in `.github/workflows/noema-review.yml`, +`docs/`, and `tests/` -- separately, the unreachable type branch in `extract_json_object` was removed so +the implementation now directly reflects the JSON grammar guarantee); `interrogate` -- 100% docstring +coverage (minimum 100.0%, actual 100.0%); `actionlint` +on the modified workflow -- clean. The touched `run:` block parses with `bash -n` and was exercised +interactively against hand-rolled fake `gh` fixtures for both the crash-reproduction and the fixed +behavior before being folded into the pytest suite. Full validation was re-run after every rebase, given +the branch's ongoing, very high commit velocity from multiple simultaneous sessions converging on this +same ~15-line mechanism throughout the day. + +PR: ContextualWisdomLab/.github#1507 (Devin review on #1507; same PR, addressed before merge). + +The same exact-head review also identified that scanning every opening brace could recover a valid +nested object after its malformed outer object failed to decode. Recovery now considers only top-level +brace groups, preserving lightly wrapped and multiple-object responses while failing closed on nested +escape. A regression test reproduces the former nested-object acceptance directly. An explicit, +string-aware `MAX_JSON_NESTING_DEPTH = 100` check also runs before `raw_decode`, so the limit does not +depend on Python-version-specific `RecursionError` behavior. + +The two chained required-workflow pollers were then replaced after live organization evidence showed +53 concurrent Actions runs and a growing runner queue. The required workflow still dispatches the same +bounded multi-hour OpenCode path and still fails closed without a formal exact-head receipt, but it now +releases its runner after one receipt lookup. Once the privileged dispatch validates the formal receipt, +it selects the latest exact-head `Required OpenCode Review` `pull_request_target` run and calls +`rerun-failed-jobs`; only the small verdict job reruns. This preserves ruleset `18156473`'s required +workflow identity and the two-hour-plus model allowance while removing roughly eleven runner-hours of +polling per PR. The authenticated dispatch carries the immutable triggering required-run ID; the +continuation fetches that target-repository run directly and validates its `pull_request_target` event, +central workflow path, and live PR `head_sha` before rerunning it. This remains correct even when runner +queue delay exceeds the model jobs' declared timeout sum and avoids dependence on context-specific title +or `workflow_url` rendering. Scheduler review retries propagate the same immutable run ID from the +required check's Actions details URL, so the scheduler and direct required-workflow entrypoints share one +continuation contract. Native wake calls use the privileged dispatch job's narrowly scoped `actions: +write` workflow token. Sibling wake calls require `PR_REVIEW_MERGE_TOKEN` or +`OPENCODE_APPROVE_TOKEN` and fail closed when neither is configured; the review-only OpenCode app token +and the central repository's workflow token are never presented as cross-repository Actions credentials. + +## 2026-08-31 `ORCHESTRATOR_PIN_SHA` bumped to carry #925's stream_options/tools fix + +**Context**: `#1451` fixed a separate, org-wide `pingora_edge_policy.py` coverage +gap blocking `opencode-review-dispatch.yml`'s own `coverage-evidence` job for +every `.github`-hosted PR. Once that landed and Strix could actually complete +scans again (via `#1448`'s scoped `LLM_DISABLE_STREAMING` workaround), +`ContextualWisdomLab/contextual-orchestrator#925` — the real root-cause fix for +the gateway's `stream_options.include_usage=true` + `tools` rejection — merged +(`7944a3c`). `.github#1463` reverts `#1448`'s workaround now that the gateway +itself no longer rejects that combination. + +**Devin Review correctly caught a real bug in that revert before merge**: the +review sidecar vendors `contextual-orchestrator` at a *pinned* SHA +(`ORCHESTRATOR_PIN_SHA`), not live `main` — and the pin in place at revert time +(`30c6d71680e659f25a0a433d4726ad0d437f9757`) was cut *before* `#925` merged. +Confirmed by `git merge-base --is-ancestor 30c6d716... 7944a3c` (true). Removing +the Strix-side streaming workaround while the vendored gateway still ran the +old, rejecting code would have restored the exact failure `#1448` existed to +route around — every Strix scan through the sidecar would fail again. + +**Fix**: bumped `ORCHESTRATOR_PIN_SHA` to `7944a3cd98f7b60fba9272e7f89c3977a75af746` +(the `#925` merge commit itself — deliberately not `contextual-orchestrator`'s +later tip, to keep this bump minimal and scoped to exactly the fix this revert +depends on) in the three places this repo's own convention requires kept in +sync: `scripts/ci/contextual_orchestrator_review_sidecar.sh`'s default, +`tests/test_contextual_orchestrator_review_sidecar_contract.py`'s pinned-SHA +contract assertion, and `docs/adr/0003-contextual-orchestrator-vendored-free-zdr.md`'s +"today" reference. Landed in the same PR (`#1463`) as the streaming revert, +not split out, since the revert is unsafe without it. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 90a69bed31..4ffaee4d40 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -6,6 +6,7 @@ import argparse import ast import base64 +import hashlib import ipaddress import json import os @@ -32,6 +33,7 @@ MAX_FILE_CONTEXT_CHARS = 4000 MAX_REVIEW_CONTEXT_CHARS = 24000 MAX_THREAD_BODY_CHARS = 1200 +NOEMA_LLM_TIMEOUT_SECONDS = 4 * 60 * 60 DIFF_HUNK_RE = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@") ORCHESTRATOR_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1"}) @@ -302,7 +304,7 @@ def validate_substantive_verdict( raise RuntimeError(f"Noema reviewed line {index} must be an object") location = (reviewed.get("path"), reviewed.get("line"), reviewed.get("side")) if location not in locations: - raise RuntimeError(f"Noema reviewed line {index} is not an exact changed-side line") + raise RuntimeError(f"Noema reviewed line {index} is not an exact changed-side line. It cited: {location[0]}:{location[1]} ({location[2]})") analysis = reviewed.get("analysis") if not isinstance(analysis, str) or not analysis.strip(): raise RuntimeError(f"Noema reviewed line {index} requires concrete analysis") @@ -330,7 +332,7 @@ def validate_substantive_verdict( raise RuntimeError(f"Noema adversarial probe {index} must be an object") location = (probe.get("path"), probe.get("line"), probe.get("side")) if location not in locations: - raise RuntimeError(f"Noema adversarial probe {index} is not an exact changed-side line") + raise RuntimeError(f"Noema adversarial probe {index} is not an exact changed-side line. It cited: {location[0]}:{location[1]} ({location[2]})") for field in ("hypothesis", "attack_or_counterexample", "evidence"): value = probe.get(field) if not isinstance(value, str) or not value.strip(): @@ -488,16 +490,308 @@ def redirect_request( raise urllib.error.HTTPError(req.full_url, code, msg, headers, fp) +def _json_nesting_within_bound(text: str, start: int, max_depth: int) -> bool: + """Return whether the ``{``/``[`` nesting at ``text[start]`` stays within bound. + + A lightweight, string-literal-aware bracket-type stack: walks forward + from ``start`` (a ``{``), ignoring any ``{``/``[``/``}``/``]`` characters + that appear inside a JSON string literal, and returns ``True`` as soon as + the opening brace's matching close is found without nesting exceeding + ``max_depth``, or ``False`` the moment ``max_depth`` is exceeded. Running + off the end of ``text`` without closing (an unterminated candidate) is + reported as within bound — that shape is already a decode failure + ``json.JSONDecoder.raw_decode`` reports on its own; this function's only + job is bounding nesting *depth*, not validating overall JSON shape. + + A closer that does not match the innermost open bracket's type (a ``]`` + where the enclosing container is a ``{``, or vice versa) is a no-op: it + does not pop the stack. A plain up/down counter that treated ``{``/``[`` + interchangeably would let such a mismatched closer prematurely signal + "the outer bracket is closed" while genuinely deeper structure follows, + under-counting the real nesting depth ``raw_decode`` would encounter on + this exact candidate (Devin review on PR #1507). + + This check runs before ``raw_decode`` is attempted on a candidate, ahead + of and independent of ``json.JSONDecoder``'s own recursion behavior — + see ``extract_json_object``'s docstring for why that behavior cannot be + trusted to reject excessive nesting on its own. + """ + stack: list[str] = [] + in_string = False + escaped = False + for index in range(start, len(text)): + char = text[index] + if in_string: + if escaped: + escaped = False + elif char == "\\": + escaped = True + elif char == '"': + in_string = False + continue + if char == '"': + in_string = True + elif char in "{[": + stack.append(char) + if len(stack) > max_depth: + return False + elif char == "}": + if stack and stack[-1] == "{": + stack.pop() + if not stack: + # Only "{" can empty the stack: text[start] is always + # "{" (this function's own contract), so it is always + # the bottom-most, last-popped element; a "]" popping + # an inner "[" can never reach an empty stack itself. + return True + elif char == "]" and stack and stack[-1] == "[": + stack.pop() + return True + + +MAX_JSON_NESTING_DEPTH = 100 + + def extract_json_object(text: str) -> dict[str, Any]: - """Extract a JSON object from a strict or lightly wrapped LLM response.""" + """Extract a JSON object from a strict or lightly wrapped LLM response. + + Fails closed with ``RuntimeError`` — the same "no usable verdict" failure + path ``call_llm`` already raises for an unsupported decision, a missing + summary, or a malformed finding — instead of letting a malformed or + truncated LLM response's ``json.JSONDecodeError`` propagate as an + unhandled exception and crash the review job. Only top-level brace groups + are candidates: a ``{`` is a candidate only while a bracket-type stack + (tracking ``{``/``[`` opens against their own matching ``}``/``]`` + closes) is empty, so a valid nested object cannot escape a malformed + outer *object or array* wrapper. Every candidate starts at a ``{``, + making each successful parse a JSON object (``dict``); only the decode + failure itself needs converting. Once a top-level candidate begins, a + decode failure rejects the response rather than scanning forward to a + later verdict; multiple objects remain supported only when the first + candidate decodes successfully. + + A closer that cannot legally match the innermost open bracket — nothing + open at all, or the innermost open bracket is the other type — stops + candidate discovery outright instead of being a no-op on the stack. Only + ignoring the mismatch (popping nothing, but continuing to scan) is not + enough: a *later*, otherwise-well-formed ``[``/``]`` or ``{``/``}`` pair + can still legitimately re-close the stack down to empty despite the + earlier mismatch, so a subsequent ``{`` would again be seen as a fresh + top-level candidate even though the response as a whole was never + cleanly-formed JSON (Devin review on PR #1507, e.g. ``[} ] {...}``: the + stray ``}`` is a no-op, but the following ``]`` still validly closes the + ``[``, and the ``{`` after that would wrongly look top-level again). Any + closer this malformed anywhere in the response is treated as proof the + whole response cannot be trusted to contain a clean top-level object + from that point on, not just proof that one bracket group failed to + close. + + The raised diagnostic never embeds the raw (or scrubbed) model response. + This is a ``pull_request_target`` workflow whose Actions logs are public + on this org's public repos, and ``scrub_sensitive_data`` is a finite, + pattern-based scrubber: an LLM can echo back or hallucinate a credential + in a shape none of its patterns recognize (mid-sentence, base64-wrapped, + or simply a shape nobody anticipated). A regex allowlist of known secret + *shapes* cannot be a complete defense, so instead of trying to perfect + it, the raw content is never logged at all. Only a length and a SHA-256 + content fingerprint are logged — enough to correlate repeat failures for + the same underlying (unlogged) response without exposing its bytes. + + Excessive nesting is rejected by an explicit ``_json_nesting_within_bound`` + check against ``MAX_JSON_NESTING_DEPTH`` (100 — generously above the + verdict schema's own real maximum of roughly 5 levels: object -> + ``findings``/``reviewed_lines``/``adversarial_validation.probes`` -> + each list's object entries), evaluated *before* ``raw_decode`` is ever + attempted, rather than by trusting ``json.JSONDecoder``'s own recursion + behavior to raise on deep input. That behavior is not a stable contract: + a real ``depth = max(20_000, sys.getrecursionlimit() * 2)`` nested-array + payload raises ``RecursionError`` from the C-accelerated scanner on + Python 3.11-3.13, but is decoded successfully (no exception at all) on + the Python 3.14.7 hosted runner this job actually runs on (job + 99642234627, commit ``ec23350e``: + ``test_extract_json_object_fails_closed_on_excessive_nesting`` failed + with "DID NOT RAISE RuntimeError" against that exact real payload). + Relying on ``RecursionError`` alone would make this fail-closed guarantee + a property of whichever CPython version happens to run the job, not of + this function. The explicit bound removes that dependency; a residual + ``except RecursionError`` is kept only as defense-in-depth for whatever + lies within the bound (``RecursionError`` is itself a ``RuntimeError`` + subclass, so even an unhandled one here would already surface through + ``call_llm``'s own ``except RuntimeError`` around this call and every + post-decode field read). + """ stripped = text.strip() - if stripped.startswith("{"): - return json.loads(stripped) - start = stripped.find("{") - end = stripped.rfind("}") - if start < 0 or end < start: + decoder = json.JSONDecoder() + decode_error: json.JSONDecodeError | None = None + candidate_starts: list[int] = [] + stack: list[str] = [] + in_string = False + escaped = False + for index, character in enumerate(stripped): + if in_string: + if escaped: + escaped = False + elif character == "\\": + escaped = True + elif character == '"': + in_string = False + continue + if character == '"': + in_string = True + elif character == "{": + if not stack: + candidate_starts.append(index) + stack.append("{") + elif character == "[": + stack.append("[") + elif character == "}": + if not stack or stack[-1] != "{": + # A closer that cannot legally appear here (nothing open, or + # the innermost open bracket is a "[") is proof this response + # is not cleanly-formed JSON at all, not just proof that one + # bracket group failed to close. Stop finding new candidates + # rather than let bracket-type matching alone "resync" past + # it and treat a later, structurally-unrelated { as a fresh + # top-level verdict (Devin review on PR #1507). + break + stack.pop() + elif character == "]": + if not stack or stack[-1] != "[": + break + stack.pop() + + for start in candidate_starts: + if not _json_nesting_within_bound(stripped, start, MAX_JSON_NESTING_DEPTH): + decode_error = json.JSONDecodeError( + f"JSON nesting exceeds the bounded limit ({MAX_JSON_NESTING_DEPTH} levels)", + stripped, + start, + ) + break + try: + candidate, _end = decoder.raw_decode(stripped, start) + except RecursionError: + decode_error = json.JSONDecodeError( + "JSON nesting exceeds decoder limit", stripped, start + ) + break + except json.JSONDecodeError as exc: + decode_error = exc + break + return candidate + + if "{" not in stripped: raise RuntimeError("Noema LLM response did not contain a JSON object") - return json.loads(stripped[start : end + 1]) + + exc = decode_error or json.JSONDecodeError( + "No JSON object could be decoded", stripped, 0 + ) + try: + raise exc + except json.JSONDecodeError as exc: + fingerprint = hashlib.sha256( + stripped.encode("utf-8", errors="surrogatepass") + ).hexdigest()[:16] + raise RuntimeError( + f"Noema LLM response was not valid JSON ({exc}). Raw model output " + "is not logged here (this pull_request_target workflow's logs " + "are public and a finite secret-scrub pattern list cannot " + "guarantee an LLM-echoed or hallucinated credential in an " + f"unrecognized shape is caught): response length={len(stripped)} " + f"chars, sha256={fingerprint}." + ) from exc + + +def extract_llm_message_content(raw: str) -> str: + """Parse and validate the OpenAI-compatible chat-completion HTTP envelope. + + Fails closed with the same bounded ``RuntimeError`` ``call_llm`` already + uses for an unusable verdict, instead of letting a malformed gateway + reply crash the review job before it ever reaches the verdict-JSON + repair boundary handled by ``extract_json_object``. Covers a non-JSON + raw body, a non-object top-level JSON value, a wrong-shaped ``choices`` + or ``message`` field, and non-string ``content`` — each rejected with an + explicit ``isinstance`` check rather than a broad ``except``, so a + genuine programming error elsewhere in this module still surfaces as + itself. A missing or empty ``choices``/``message``/``content`` is left + to fall through to an empty string, matching the original code's + leniency for an absent (not malformed) field; ``extract_json_object`` + already fails closed on empty content. + + None of the raised messages embed any part of the untrusted response + body — only JSON-value type names, which cannot carry a credential. + """ + try: + data = json.loads(raw) + except json.JSONDecodeError as exc: + raise RuntimeError(f"Noema LLM response body was not valid JSON: {exc}") from exc + if not isinstance(data, dict): + raise RuntimeError( + f"Noema LLM response body was not a JSON object (got {type(data).__name__})" + ) + choices = data.get("choices") + if not choices: + choices = [{}] + elif not isinstance(choices, list): + raise RuntimeError( + f"Noema LLM response 'choices' was not a list (got {type(choices).__name__})" + ) + first_choice = choices[0] + if not isinstance(first_choice, dict): + raise RuntimeError( + "Noema LLM response choices[0] was not a JSON object " + f"(got {type(first_choice).__name__})" + ) + message = first_choice.get("message") + if not message: + message = {} + elif not isinstance(message, dict): + raise RuntimeError( + f"Noema LLM response 'message' was not a JSON object (got {type(message).__name__})" + ) + content = message.get("content") + if not content: + content = "" + elif not isinstance(content, str): + raise RuntimeError( + f"Noema LLM response 'content' was not a string (got {type(content).__name__})" + ) + return content.strip() + + +def decode_llm_response_body(raw_bytes: bytes) -> str: + """Decode the raw gateway HTTP response body as UTF-8 text. + + Devin Review bug finding on PR #1507 round 3: a gateway reply containing + invalid UTF-8 used to raise ``UnicodeDecodeError`` at the plain + ``response.read().decode("utf-8")`` call in ``call_llm``, before that + body ever reached ``extract_llm_message_content`` or the verdict-JSON + repair boundary. That crashed the required review check with an + unhandled traceback instead of getting the same one-time schema-repair + retry every other malformed-envelope shape already gets. Call this + inside ``call_llm``'s existing repair-retry ``try`` block so a decode + failure converts to the same bounded ``RuntimeError`` and gets the same + fail-closed treatment. + + The raised diagnostic never embeds the raw response bytes — not even + the undecodable fragment. Only a length and a SHA-256 content + fingerprint are logged, matching ``extract_json_object``'s no-raw-content + pattern: a body containing invalid UTF-8 could still contain a + credential-adjacent byte sequence, and this is a ``pull_request_target`` + workflow whose Actions logs are public on this org's public repos. + """ + try: + return raw_bytes.decode("utf-8") + except UnicodeDecodeError as exc: + fingerprint = hashlib.sha256(raw_bytes).hexdigest()[:16] + raise RuntimeError( + f"Noema LLM response body was not valid UTF-8 ({exc}). Raw " + "response bytes are not logged here (this pull_request_target " + "workflow's logs are public and a finite secret-scrub pattern " + "list cannot guarantee an LLM-echoed or hallucinated credential " + "in an unrecognized byte sequence is caught): response " + f"length={len(raw_bytes)} bytes, sha256={fingerprint}." + ) from exc def _truthy_env(name: str) -> bool: @@ -587,17 +881,34 @@ def reject_private_llm_url(api_url: str) -> None: raise ValueError("URL cannot target internal IP addresses") +class StaleHeadDuringRepairRetryError(RuntimeError): + """Raised when the PR head moves before ``call_llm``'s repair-retry request fires.""" + + def call_llm( repo: str, number: int, pr: dict[str, Any], diff: str, truncated: bool, + expected_head: str, review_context: str = "", changed_paths: Sequence[str] = (), repair_error: str = "", ) -> dict[str, Any]: - """Call the configured OpenAI-compatible LLM endpoint for a review verdict.""" + """Call the configured OpenAI-compatible LLM endpoint for a review verdict. + + ``expected_head`` is the same normalized (lowercase) SHA + ``inspect_and_review`` already checks before model work and before + publication. It is threaded through here so the one-time repair-retry + request below — fired only after the first attempt's verdict was + malformed — can also confirm the PR head has not moved before spending a + second, potentially multi-hour ``NOEMA_LLM_TIMEOUT_SECONDS`` call on a + review that ``inspect_and_review``'s own post-call stale-head check would + discard anyway once this function returns. See ``fetch_pr`` for the live + lookup and ``StaleHeadDuringRepairRetryError`` for how that stale + condition is reported distinctly to the caller. + """ api_url = os.environ.get("NOEMA_LLM_API_URL", "").strip() api_key = os.environ.get("NOEMA_LLM_API_KEY", "").strip() model = os.environ.get("NOEMA_LLM_MODEL", "").strip() or "noema-default" @@ -653,45 +964,50 @@ def call_llm( method="POST", ) opener = urllib.request.build_opener(NoRedirectHandler()) - with opener.open(request, timeout=120) as response: # nosec B310 - raw = response.read().decode("utf-8") - data = json.loads(raw) - content = (((data.get("choices") or [{}])[0].get("message") or {}).get("content") or "").strip() - verdict = extract_json_object(content) - decision = str(verdict.get("decision") or "").strip().lower() - if decision not in {"approve", "request_changes", "comment"}: - raise RuntimeError(f"Noema LLM returned unsupported decision: {decision!r}") - summary = verdict.get("summary") - if not isinstance(summary, str) or not summary.strip(): - raise RuntimeError("Noema LLM response did not contain a substantive summary") - findings = verdict.get("findings") - if not isinstance(findings, list) or any(not isinstance(finding, dict) for finding in findings): - raise RuntimeError("Noema LLM response findings must be a list of objects") - for finding in findings: - if ( - finding.get("severity") not in {"high", "medium", "low"} - or not isinstance(finding.get("file"), str) - or not finding["file"].strip() - or type(finding.get("line")) is not int - or finding["line"] <= 0 - or finding.get("side") not in {"RIGHT", "LEFT"} - or not isinstance(finding.get("message"), str) - or not finding["message"].strip() - ): - raise RuntimeError("Noema LLM response contained a malformed finding") - if decision == "request_changes" and not findings: - raise RuntimeError("Noema LLM request_changes response did not contain a substantive finding") + with opener.open(request, timeout=NOEMA_LLM_TIMEOUT_SECONDS) as response: # nosec B310 + raw_bytes = response.read() try: + raw = decode_llm_response_body(raw_bytes) + content = extract_llm_message_content(raw) + verdict = extract_json_object(content) + decision = str(verdict.get("decision") or "").strip().lower() + if decision not in {"approve", "request_changes", "comment"}: + raise RuntimeError(f"Noema LLM returned unsupported decision: {decision!r}") + summary = verdict.get("summary") + if not isinstance(summary, str) or not summary.strip(): + raise RuntimeError("Noema LLM response did not contain a substantive summary") + findings = verdict.get("findings") + if not isinstance(findings, list) or any(not isinstance(finding, dict) for finding in findings): + raise RuntimeError("Noema LLM response findings must be a list of objects") + for finding in findings: + if ( + finding.get("severity") not in {"high", "medium", "low"} + or not isinstance(finding.get("file"), str) + or not finding["file"].strip() + or type(finding.get("line")) is not int + or finding["line"] <= 0 + or finding.get("side") not in {"RIGHT", "LEFT"} + or not isinstance(finding.get("message"), str) + or not finding["message"].strip() + ): + raise RuntimeError("Noema LLM response contained a malformed finding") + if decision == "request_changes" and not findings: + raise RuntimeError("Noema LLM request_changes response did not contain a substantive finding") validate_substantive_verdict(verdict, diff, changed_paths) except RuntimeError as exc: if repair_error: raise + if str(fetch_pr(repo, number).get("headRefOid") or "").lower() != expected_head: + raise StaleHeadDuringRepairRetryError( + "Pull request head changed during review; stale before repair retry." + ) from exc return call_llm( repo, number, pr, diff, truncated, + expected_head, review_context, changed_paths, str(exc), @@ -779,9 +1095,20 @@ def submit_review(repo: str, number: int, pr: dict[str, Any], actor: str, verdic print(f"Noema {event} review submitted for {repo}#{number} at {head_sha}.") -def inspect_and_review(repo: str, number: int) -> int: - """Inspect PR state and submit Noema's independent LLM review.""" +def inspect_and_review(repo: str, number: int, expected_head: str) -> int: + """Inspect PR state and submit Noema's independent LLM review. + + ``expected_head`` is normalized defensively before the stale-head + comparisons below, and before the one ``call_llm`` performs on its own + repair-retry path (see ``StaleHeadDuringRepairRetryError``). The CLI and + workflow require canonical lowercase SHA input so equivalent casing + cannot split the workflow concurrency group. + """ + expected_head = expected_head.strip().lower() pr = fetch_pr(repo, number) + if str(pr.get("headRefOid") or "").lower() != expected_head: + print("Trigger head is stale; Noema review skipped before model work.") + return 0 actor = current_actor() if not actor: raise RuntimeError("Noema reviewer identity could not be verified") @@ -799,8 +1126,16 @@ def inspect_and_review(repo: str, number: int) -> int: diff, truncated = fetch_diff(repo, number) changed_paths = fetch_changed_file_paths(repo, number) review_context = build_review_context(repo, number, pr) - verdict = call_llm(repo, number, pr, diff, truncated, review_context, changed_paths) - submit_review(repo, number, pr, actor, verdict) + try: + verdict = call_llm(repo, number, pr, diff, truncated, expected_head, review_context, changed_paths) + except StaleHeadDuringRepairRetryError: + print("Pull request head changed during review; Noema review skipped before repair retry.") + return 0 + current_pr = fetch_pr(repo, number) + if str(current_pr.get("headRefOid") or "").lower() != expected_head: + print("Pull request head changed during review; stale verdict was not published.") + return 0 + submit_review(repo, number, current_pr, actor, verdict) return 0 @@ -809,6 +1144,7 @@ def parse_args(argv: list[str]) -> argparse.Namespace: parser = argparse.ArgumentParser() parser.add_argument("--repo", required=True) parser.add_argument("--pr-number", required=True, type=int) + parser.add_argument("--expected-head", required=True) return parser.parse_args(argv) @@ -817,12 +1153,16 @@ def main(argv: list[str]) -> int: args = parse_args(argv) if args.pr_number <= 0: raise SystemExit("--pr-number must be positive") - return inspect_and_review(args.repo, args.pr_number) + if not re.fullmatch(r"[0-9a-f]{40}", args.expected_head): + raise SystemExit( + "--expected-head must be a canonical lowercase 40-character Git SHA" + ) + return inspect_and_review(args.repo, args.pr_number, args.expected_head) if __name__ == "__main__": # pragma: no cover try: raise SystemExit(main(sys.argv[1:])) except RuntimeError as exc: - print(str(exc), file=sys.stderr) + print(f"::error::{exc}", file=sys.stderr) raise SystemExit(1) from exc diff --git a/scripts/ci/opencode_review_normalize_output.py b/scripts/ci/opencode_review_normalize_output.py index 761a7988da..d9b9ec349e 100755 --- a/scripts/ci/opencode_review_normalize_output.py +++ b/scripts/ci/opencode_review_normalize_output.py @@ -974,14 +974,22 @@ def label_starts(candidate: str) -> list[int]: if not starts: return "" start = starts[-1] + len(label) - next_starts = [ - candidate_start - for candidate in APPROVAL_VERIFICATION_LABELS - if candidate != label - for candidate_start in label_starts(candidate) - if candidate_start >= start - ] - end = min(next_starts) if next_starts else len(text) + + end = len(text) + for candidate in APPROVAL_VERIFICATION_LABELS: + if candidate == label: + continue + index = text.find(candidate, start, end) + while index != -1: + if ( + candidate == "coverage:" + and text[max(0, index - 10) : index] == "docstring " + ): + index = text.find(candidate, index + len(candidate), end) + continue + end = min(end, index) + break + return text[start:end] diff --git a/scripts/ci/pr_review_merge_scheduler.py b/scripts/ci/pr_review_merge_scheduler.py index cfaab58b71..6d77596c7c 100644 --- a/scripts/ci/pr_review_merge_scheduler.py +++ b/scripts/ci/pr_review_merge_scheduler.py @@ -66,6 +66,7 @@ } statusCheckRollup { contexts(first: 100) { + pageInfo { hasNextPage endCursor } nodes { __typename ... on CheckRun { @@ -139,6 +140,28 @@ } """ +PR_CONTEXTS_PAGE_QUERY = """\ +query($owner: String!, $name: String!, $number: Int!, $cursor: String!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + statusCheckRollup { + contexts(first: 100, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { + __typename + ... on CheckRun { + name status conclusion startedAt detailsUrl + checkSuite { createdAt workflowRun { workflow { name } } } + } + ... on StatusContext { context state } + } + } + } + } + } +} +""" + OPEN_PRS_PAGE_SIZE = 25 # Defends against a pathological GraphQL pageInfo loop when backfilling a PR's # full review history; 500 pages * 100 reviews/page is far beyond any @@ -158,6 +181,7 @@ "Required OpenCode Review", "OpenCode Review Dispatch", } +OPENCODE_REVIEW_WORKFLOW_PATH = ".github/workflows/opencode-review.yml" RUNNING_CHECK_STATES = {"PENDING", "EXPECTED", "QUEUED", "IN_PROGRESS", "WAITING", "REQUESTED"} FAILED_CHECK_CONCLUSIONS = {"FAILURE", "ERROR", "CANCELLED", "TIMED_OUT", "STARTUP_FAILURE"} ACTION_REQUIRED_CONCLUSIONS = {"ACTION_REQUIRED"} @@ -169,6 +193,7 @@ "OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed." ) ACTIONS_JOB_DETAILS_URL_RE = re.compile(r"/actions/runs/\d+/job/(\d+)(?:[/?#]|$)") +ACTIONS_RUN_DETAILS_URL_RE = re.compile(r"/actions/runs/(\d+)(?:/job/\d+)?(?:[/?#]|$)") DIRECT_MERGE_AUTO_FALLBACK_MARKERS = ( "base branch policy prohibits the merge", "is not mergeable", @@ -855,6 +880,37 @@ def complete_all_pr_reviews(owner: str, name: str, prs: list[dict[str, Any]]) -> ) +def complete_paginated_pr_contexts(repo: str, pr: dict[str, Any]) -> None: + """Load every status-context page before selecting a required workflow run.""" + contexts = ((pr.get("statusCheckRollup") or {}).get("contexts") or {}) + page_info = contexts.get("pageInfo") or {} + nodes = list(contexts.get("nodes") or []) + owner, name = validate_github_repository(repo).split("/", 1) + pages = 0 + while page_info.get("hasNextPage"): + cursor = page_info.get("endCursor") + if not cursor: + raise RuntimeError("Status context pagination did not provide an end cursor") + pages += 1 + if pages > MAX_REVIEW_PAGINATION_PAGES: + raise RuntimeError("Status context pagination exceeded its safety bound") + payload = gh_graphql( + PR_CONTEXTS_PAGE_QUERY, + owner=owner, + name=name, + number=int(pr["number"]), + cursor=cursor, + ) + pull_request = ((payload.get("data") or {}).get("repository") or {}).get( + "pullRequest" + ) or {} + page_contexts = ((pull_request.get("statusCheckRollup") or {}).get("contexts") or {}) + nodes.extend(page_contexts.get("nodes") or []) + page_info = page_contexts.get("pageInfo") or {} + contexts["nodes"] = nodes + contexts["pageInfo"] = page_info + + def github_resource_inaccessible(exc: RuntimeError) -> bool: """Return whether GitHub denied an API read for the current integration token.""" @@ -1275,6 +1331,38 @@ def matching_actions_job_id(pr: dict[str, Any], predicate: Any) -> str | None: return None +def matching_actions_run_id(pr: dict[str, Any], predicate: Any) -> int | None: + """Return the newest matching check-run's workflow run id, if exposed. + + Devin Review finding on PR #1507 ("Older review run remains blocking"): + an earlier version of this function returned the first predicate match + found scanning ``context_nodes`` in reverse, which is only the newest + match when GitHub happens to return the rollup in chronological order -- + not guaranteed, and not true for every real payload. With multiple + same-purpose check runs present (reruns, or two dispatches racing), that + could select an older, already-resolved run while a genuinely newer + failure sat unselected and unrerun. This now ranks every match with the + same ``check_run_recency_key`` signal ``_newest_check_run_per_identity`` + uses to resolve reruns elsewhere in this file, so position in the list + never decides the winner -- only actual recency does. + """ + candidates: list[tuple[tuple[int, datetime, int], int]] = [] + for index, node in enumerate(context_nodes(pr)): + if node.get("__typename") != "CheckRun" or not predicate(node): + continue + match = ACTIONS_RUN_DETAILS_URL_RE.search(node.get("detailsUrl") or "") + if match: + candidates.append( + ( + check_run_recency_key( + node, parse_github_datetime(node.get("startedAt")), index + ), + int(match.group(1)), + ) + ) + return max(candidates)[1] if candidates else None + + def parse_github_datetime(value: str | None) -> datetime | None: """Parse a GitHub API timestamp into an aware UTC datetime.""" if not value: @@ -2531,18 +2619,20 @@ def active_workflow_runs( *, event: str | None = None, created: str | None = None, + head_sha: str | None = None, ) -> list[dict[str, Any]]: """Return workflow runs for a repository, optionally narrowed server-side. - ``event`` and ``created`` map directly onto GitHub's ``List workflow - runs for a repository`` REST query parameters (``event`` selects the - triggering webhook event, ``created`` accepts a date/range qualifier - such as ``>=2026-08-24T00:00:00Z``). Both are omitted by default so - existing callers keep fetching every run for the given statuses - unfiltered; a caller with a naturally bounded lookup -- one whose - target repository's run history only grows, such as a same-head - dispatch search -- should pass them to avoid paginating history it can - never use. + ``event``, ``created``, and ``head_sha`` map directly onto GitHub's + ``List workflow runs for a repository`` REST query parameters (``event`` + selects the triggering webhook event, ``created`` accepts a date/range + qualifier such as ``>=2026-08-24T00:00:00Z``, ``head_sha`` narrows to + runs for one exact commit). All three are omitted by default so existing + callers keep fetching every run for the given statuses unfiltered; a + caller with a naturally bounded lookup -- one whose target repository's + run history only grows, such as a same-head dispatch search, or one + scoped to a single known commit -- should pass them to avoid paginating + history it can never use. """ runs: list[dict[str, Any]] = [] for status in statuses: @@ -2563,6 +2653,8 @@ def active_workflow_runs( args += ["-f", f"event={event}"] if created: args += ["-f", f"created={created}"] + if head_sha: + args += ["-f", f"head_sha={head_sha}"] payload = json.loads(run_github_actions(args)) pages = payload if isinstance(payload, list) else [payload] for page in pages: @@ -2837,6 +2929,56 @@ def cancel_stale_opencode_runs(repo: str, workflow: str, pr: dict[str, Any], *, return [run_id for _, run_id in stale_refs] +def discover_opencode_required_run_id(repo: str, head_sha: str) -> int | None: + """Return the current-head Required OpenCode Review run id via a bounded lookup. + + Devin Review finding on PR #1507 ("Large check rollups never wake"): + ``matching_actions_run_id`` only sees the GraphQL ``statusCheckRollup`` + fragment's first 100 status/check contexts + (``PULL_REQUEST_FIELDS_FRAGMENT``'s ``contexts(first: 100)``). A pull + request already carrying at least 100 contexts -- dozens of CI/security + workflows across several pushes and reruns is realistic in this + organization -- can push the real Required OpenCode Review check run + past that page, so the in-memory scan finds nothing even though the run + exists. This is a REST fallback, not a rewrite of that scan: it is + scoped server-side to the exact triggering event, the exact workflow + file path, and the exact current head SHA (GitHub's ``head_sha`` list + filter), so it stays a bounded, targeted lookup -- never an unfiltered + history walk -- and finds the run whether it is still queued/running or + already completed (the realistic failure mode is a stuck ``failure`` + conclusion on an otherwise-valid exact-head run). + """ + if not GIT_SHA_RE.fullmatch(head_sha): + return None + target_repo = validate_github_repository(repo) + newest_id: int | None = None + newest_started: datetime | None = None + for run_data in active_workflow_runs( + target_repo, + ("queued", "in_progress", "completed"), + event="pull_request_target", + head_sha=head_sha, + ): + if run_data.get("path") != OPENCODE_REVIEW_WORKFLOW_PATH: + continue + if str(run_data.get("head_sha") or "").lower() != head_sha.lower(): + continue + run_id = run_data.get("id") + if not run_id: + continue + started_at = parse_github_datetime( + run_data.get("run_started_at") or run_data.get("created_at") + ) + is_newer = started_at is not None and ( + newest_started is None or started_at > newest_started + ) + if newest_id is None or is_newer: + newest_id = int(run_id) + if started_at is not None: + newest_started = started_at + return newest_id + + def dispatch_opencode_review(repo: str, workflow: str, pr: dict[str, Any], *, dry_run: bool) -> str: """Dispatch trusted OpenCode for the PR head, or report an active run. @@ -2864,6 +3006,20 @@ def dispatch_opencode_review(repo: str, workflow: str, pr: dict[str, Any], *, dr head_ref = validate_git_ref(pr["headRefName"]) target_repo = validate_github_repository(repo) dispatch_repo = repository_dispatch_target(target_repo) + client_payload: dict[str, Any] = { + "target_repository": target_repo, + "pr_number": int(pr["number"]), + "pr_base_ref": base_ref, + "pr_base_sha": base_sha, + "pr_head_ref": head_ref, + "pr_head_sha": head_sha, + } + complete_paginated_pr_contexts(target_repo, pr) + required_run_id = matching_actions_run_id(pr, is_opencode_check_run) + if required_run_id is None: + required_run_id = discover_opencode_required_run_id(target_repo, head_sha) + if required_run_id is not None: + client_payload["required_run_id"] = required_run_id run_github_dispatch( [ "gh", @@ -2877,14 +3033,7 @@ def dispatch_opencode_review(repo: str, workflow: str, pr: dict[str, Any], *, dr stdin=json.dumps( { "event_type": "opencode-review", - "client_payload": { - "target_repository": target_repo, - "pr_number": int(pr["number"]), - "pr_base_ref": base_ref, - "pr_base_sha": base_sha, - "pr_head_ref": head_ref, - "pr_head_sha": head_sha, - }, + "client_payload": client_payload, } ), ) diff --git a/scripts/ci/strix_quick_gate.sh b/scripts/ci/strix_quick_gate.sh index 4f0d7b1ca4..57b81f0ad8 100755 --- a/scripts/ci/strix_quick_gate.sh +++ b/scripts/ci/strix_quick_gate.sh @@ -2651,20 +2651,6 @@ run_strix_once() { if ! resolved_target_path="$(resolve_current_target_path "$TARGET_PATH")"; then return 1 fi - # contextual-orchestrator's gateway deliberately rejects any request that - # combines stream_options.include_usage=true with tools (a correctness - # guarantee against silently-incomplete usage accounting; out of scope to - # change here). Strix's agent loop always streams and always sends tools, - # so every call through that gateway hits the rejection immediately. - # Strix itself ships an opt-in for exactly this: LLM_DISABLE_STREAMING=true - # makes each turn a single non-streaming get_response (stream:false on the - # wire, so stream_options is never sent) replayed as one terminal stream - # event; nothing else about the run loop changes. Scope it narrowly to the - # contextual-orchestrator loopback so other providers keep real streaming. - local strix_disable_streaming="false" - if is_contextual_orchestrator_api_base "$llm_api_base_value"; then - strix_disable_streaming="true" - fi local start_epoch start_epoch="$(date +%s)" local child_llm_api_key="" @@ -2696,7 +2682,6 @@ run_strix_once() { STRIX_CHILD_EXECUTABLE_ROOT="$STRIX_EXECUTABLE_ROOT" \ STRIX_CHILD_EXECUTABLE_SHA256="$STRIX_EXECUTABLE_SHA256" \ STRIX_CHILD_REQUIRE_EXECUTABLE_INTEGRITY="${IS_PR_EVIDENCE_RUN:-false}" \ - STRIX_CHILD_DISABLE_STREAMING="$strix_disable_streaming" \ python3 - "$timeout_seconds" "$resolved_target_path" "$SCAN_MODE" "$STRIX_LOG" "$STRIX_SCAN_WORKING_DIR" <<'PY' import hashlib import hmac @@ -2751,12 +2736,6 @@ child_env["LLM_MODEL"] = os.environ["STRIX_CHILD_MODEL"] if os.environ.get("STRIX_CHILD_LLM_API_KEY"): child_env["LLM_API_KEY"] = os.environ["STRIX_CHILD_LLM_API_KEY"] child_env["STRIX_REPORTS_DIR"] = os.environ["STRIX_CHILD_REPORTS_DIR"] -if os.environ.get("STRIX_CHILD_DISABLE_STREAMING", "").strip().lower() == "true": - # See the comment above strix_disable_streaming's assignment in bash: - # this routes only the contextual-orchestrator gateway through Strix's - # own non-streaming fallback so stream_options is never sent alongside - # tools, without touching how Strix talks to any other provider. - child_env["LLM_DISABLE_STREAMING"] = "true" for key, value in os.environ.items(): if key.startswith("FAKE_STRIX_") and value: child_env[key] = value diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 4053f4fd53..fdfb34bfb9 100644 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -96,6 +96,13 @@ assert_file_not_contains() { fi } +required_workflow_bootstrap_has_if() { + local bootstrap_file="$1" + + awk '/^ required-workflow-bootstrap:$/{p=1; print; next} p && /^ [A-Za-z0-9_-]+:/{exit} p' "$bootstrap_file" | + grep '^[[:space:]]*if:' >/dev/null +} + seal_opencode_test_artifacts() { local runner_temp="$1" local head_sha="$2" @@ -327,9 +334,12 @@ assert_strix_workflow_pr_trigger_hardened() { assert_file_contains "$GATE_SCRIPT" 'child_env["PNPM_CONFIG_IGNORE_SCRIPTS"] = "true"' "strix gate child process disables pnpm lifecycle scripts" assert_file_contains "$GATE_SCRIPT" 'child_env["YARN_ENABLE_SCRIPTS"] = "false"' "strix gate child process disables yarn lifecycle scripts" assert_file_contains "$GATE_SCRIPT" 'child_env["PYTHONWARNINGS"] = "ignore:Pydantic serializer warnings:UserWarning:pydantic.main"' "strix gate child env narrowly filters the known third-party Pydantic serializer warning" - assert_file_contains "$GATE_SCRIPT" 'if is_contextual_orchestrator_api_base "$llm_api_base_value"; then' "strix gate scopes the non-streaming opt-in to the contextual-orchestrator loopback gateway" - assert_file_contains "$GATE_SCRIPT" 'STRIX_CHILD_DISABLE_STREAMING="$strix_disable_streaming"' "strix gate threads the streaming opt-in through to the child process environment" - assert_file_contains "$GATE_SCRIPT" 'child_env["LLM_DISABLE_STREAMING"] = "true"' "strix gate disables Strix's own SDK streaming for the contextual-orchestrator gateway, which rejects stream_options.include_usage alongside tools" + # contextual-orchestrator#925 (merged) fixed the gateway's rejection of + # stream_options.include_usage=true alongside tools -- the actual root + # cause #1448's LLM_DISABLE_STREAMING opt-in routed around. That opt-in is + # reverted (this PR); these guard against it silently reappearing. + assert_file_not_contains "$GATE_SCRIPT" 'STRIX_CHILD_DISABLE_STREAMING="$strix_disable_streaming"' "strix gate no longer threads a streaming opt-in through to the child process environment" + assert_file_not_contains "$GATE_SCRIPT" 'child_env["LLM_DISABLE_STREAMING"] = "true"' "strix gate no longer disables Strix's own SDK streaming for the contextual-orchestrator gateway" assert_file_contains "$GATE_SCRIPT" '[[ "$normalized_changed_file" =~ ^backend/.+\.py$ ]]' "strix gate detects nested backend Python files for PR-scoped import context" assert_file_contains "$GATE_SCRIPT" '[[ "$normalized_changed_file" == scripts/ci/test_*.sh || "$normalized_changed_file" == scripts/ci/*_test.sh ]]' "strix gate excludes large CI test harness scripts from model scan input" assert_file_contains "$GATE_SCRIPT" "Materialized PR-head changed-file scope for Strix scan" "strix gate avoids copying the full PR head tree into privileged scan targets by default" @@ -522,9 +532,31 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { assert_file_not_contains "$workflow_file" "Wait for trusted OpenCode approval review" "opencode pull_request bridge was removed to avoid duplicate required-check resource use" assert_file_not_contains "$workflow_file" "Trusted OpenCode requested changes for head" "opencode pull_request bridge no longer reconsumes stale trusted review state" assert_file_not_contains "$workflow_file" "github.event.pull_request.number == 240" "opencode review workflow must not hard-code repository-specific PR bypasses" - if awk '/^ required-workflow-bootstrap:$/,/^[^ ]/' "$bootstrap_file" | grep -q '^[[:space:]]*if:'; then + # Match against the full awk output rather than letting `grep -q` close its + # end of the pipe on the first match: a large bootstrap job's piped output + # can exceed the OS pipe buffer, and `grep -q`'s early exit can SIGPIPE the + # still-writing awk producer. Under `set -o pipefail` (top of this file) + # that SIGPIPE (128+13=141) outranks grep's own 0 exit, so the `if` + # incorrectly takes the "no match" branch even though the forbidden `if:` + # key was found. Dropping `-q` makes grep read to completion, so it never + # closes the pipe early and the real exit status is preserved. + if required_workflow_bootstrap_has_if "$bootstrap_file"; then record_failure "opencode required workflow bootstrap must not depend on required-workflow event payload fields" fi + local large_bootstrap_fixture + local fixture_line + large_bootstrap_fixture="$(mktemp)" + { + printf '%s\n' 'jobs:' ' required-workflow-bootstrap:' ' if: forbidden' + for ((fixture_line = 0; fixture_line < 20000; fixture_line++)); do + printf '%s\n' ' # padding forces the producer past the pipe buffer' + done + printf '%s\n' ' next-job:' ' runs-on: ubuntu-latest' + } >"$large_bootstrap_fixture" + if ! required_workflow_bootstrap_has_if "$large_bootstrap_fixture"; then + record_failure "opencode required workflow bootstrap condition detection must survive a job block larger than the pipe buffer" + fi + rm -f "$large_bootstrap_fixture" assert_file_contains "$workflow_file" 'github.event.client_payload.target_repository || github.repository' "opencode review scopes concurrency by target repository" assert_file_contains "$workflow_file" "format('pr-{0}', github.event.client_payload.pr_number)" "opencode review scopes repository_dispatch concurrency by current PR" assert_file_not_contains "$workflow_file" "format('pr-{0}-{1}'" "opencode review does not keep stale head-specific concurrency groups" @@ -741,12 +773,12 @@ assert_opencode_review_uses_codegraph_and_contextual_orchestrator() { assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "skipping remaining attempts for this model" "opencode review skips same-model retries after context-window overflow" assert_file_contains "$REPO_ROOT/.github/workflows/strix.yml" "exceeded your current quota" "strix wrapper neutralizes quota-only provider failures without vulnerability reports" assert_file_contains "$REPO_ROOT/scripts/ci/strix_quick_gate.sh" "billing details" "strix quick gate classifies provider quota starvation as infrastructure" - assert_file_contains "$workflow_file" 'timeout-minutes: 325' "opencode review target contains evidence, the bounded long-review pool, publication, Noema handoff, and cleanup overhead" + assert_file_contains "$workflow_file" 'timeout-minutes: 305' "opencode review target contains evidence, the bounded long-review pool, publication, Noema handoff, and cleanup overhead" assert_file_contains "$workflow_file" 'timeout-minutes: 12' "opencode evidence preparation fails closed before it ties up the review queue" assert_file_contains "$workflow_file" 'timeout-minutes: 205' "opencode model pool preserves full-hour candidates within a bounded provider-pool window" assert_file_contains "$workflow_file" 'timeout-minutes: 34' "opencode fast approval publication is bounded around the dynamic image and package/GPU check wait" assert_file_contains "$workflow_file" 'continue-on-error: true' "opencode approval gate still runs after model-pool failure to publish a reason" - assert_file_contains "$workflow_file" 'OPENCODE_RUN_TIMEOUT_SECONDS: "5400"' "opencode primary review preserves legitimate full-hour provider sessions" + assert_file_contains "$workflow_file" 'OPENCODE_RUN_TIMEOUT_SECONDS: "11700"' "opencode primary review uses the full pool review budget" assert_file_contains "$workflow_file" 'OPENCODE_FREE_RUN_TIMEOUT_SECONDS: "3600"' "opencode free-tier failover timeout is hour-class (~3600s)" assert_file_contains "$workflow_file" "CONTEXTUAL_ORCHESTRATOR_BASE_URL" "opencode review uses the gateway endpoint for all model candidates" assert_file_contains "$workflow_file" "CONTEXTUAL_ORCHESTRATOR_TOKEN" "opencode review uses the gateway credential for all model candidates" @@ -918,7 +950,7 @@ assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" ' assert_file_not_contains "$workflow_file" "no model produced a valid review control block" "opencode model-failure path no longer documents a final exhausted state" assert_file_contains "$workflow_file" 'OPENCODE_MODEL_ATTEMPTS: "1"' "opencode primary and fallback paths avoid multi-attempt stalls on one model" assert_file_contains "$workflow_file" 'OPENCODE_MODEL_ATTEMPTS: "1"' "opencode catalog fallback tries each model once before moving on" - assert_file_contains "$workflow_file" 'OPENCODE_RUN_TIMEOUT_SECONDS: "5400"' "opencode catalog fallback preserves legitimate full-hour provider sessions" + assert_file_contains "$workflow_file" 'OPENCODE_RUN_TIMEOUT_SECONDS: "11700"' "opencode catalog fallback uses the full pool review budget" assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "OpenCode %s attempt %s/%s failed" "opencode catalog fallback records per-model retry failures" assert_file_contains "$REPO_ROOT/scripts/ci/run_opencode_review_model_pool.sh" "exponential backoff" "opencode model retry paths use exponential backoff instead of fixed sleeps" assert_file_contains "$workflow_file" '"enabled_providers": ["contextual-orchestrator"]' "opencode review keeps the generated provider set gateway-only" @@ -1500,8 +1532,12 @@ assert_opencode_review_posts_suggested_diffs_inline() { assert_file_contains "$workflow_file" "GitHub did not accept the inline review comments" "opencode review explains anchor failures instead of copying diffs to the PR body" assert_file_contains "$workflow_file" "publish_request_changes_from_control" "opencode review REQUEST_CHANGES path publishes findings from the control JSON" + # Same SIGPIPE-under-pipefail shape as the required-workflow-bootstrap + # check above: read the piped awk range to completion instead of letting + # `grep -q` close the pipe on its first match, which could otherwise + # SIGPIPE a still-writing awk and flip this check's exit status. if awk '/format_request_changes_body\(\)/,/build_request_changes_review_payload\(\)/ { print }' "$workflow_file" | - grep -Fq '```diff'; then + grep -F '```diff' >/dev/null; then record_failure "opencode review PR-level REQUEST_CHANGES body must not contain fenced suggested diffs" fi } @@ -3292,7 +3328,7 @@ set -euo pipefail printf '%s\n' "${STRIX_LLM:-}" >> "${FAKE_STRIX_CALL_LOG:?}" printf '%s\n' "${LLM_API_BASE:-}" >> "${FAKE_STRIX_API_BASE_LOG:?}" if [ -n "${FAKE_STRIX_RUNTIME_ENV_LOG:-}" ]; then - printf 'LLM_TIMEOUT=%s;STRIX_MEMORY_COMPRESSOR_TIMEOUT=%s;STRIX_REASONING_EFFORT=%s;STRIX_LLM_MAX_RETRIES=%s;GEMINI_LOCATION=%s;PYTHONWARNINGS=%s;NPM_CONFIG_IGNORE_SCRIPTS=%s;PNPM_CONFIG_IGNORE_SCRIPTS=%s;YARN_ENABLE_SCRIPTS=%s;UNRELATED_SECRET=%s;LLM_DISABLE_STREAMING=%s\n' \ + printf 'LLM_TIMEOUT=%s;STRIX_MEMORY_COMPRESSOR_TIMEOUT=%s;STRIX_REASONING_EFFORT=%s;STRIX_LLM_MAX_RETRIES=%s;GEMINI_LOCATION=%s;PYTHONWARNINGS=%s;NPM_CONFIG_IGNORE_SCRIPTS=%s;PNPM_CONFIG_IGNORE_SCRIPTS=%s;YARN_ENABLE_SCRIPTS=%s;UNRELATED_SECRET=%s\n' \ "${LLM_TIMEOUT:-}" \ "${STRIX_MEMORY_COMPRESSOR_TIMEOUT:-}" \ "${STRIX_REASONING_EFFORT:-}" \ @@ -3302,8 +3338,7 @@ if [ -n "${FAKE_STRIX_RUNTIME_ENV_LOG:-}" ]; then "${NPM_CONFIG_IGNORE_SCRIPTS:-}" \ "${PNPM_CONFIG_IGNORE_SCRIPTS:-}" \ "${YARN_ENABLE_SCRIPTS:-}" \ - "${UNRELATED_SECRET:-}" \ - "${LLM_DISABLE_STREAMING:-}" >> "${FAKE_STRIX_RUNTIME_ENV_LOG:?}" + "${UNRELATED_SECRET:-}" >> "${FAKE_STRIX_RUNTIME_ENV_LOG:?}" fi target_path="" @@ -5959,13 +5994,6 @@ PY "$runtime_env_log" \ "LLM_TIMEOUT=90;STRIX_MEMORY_COMPRESSOR_TIMEOUT=10;STRIX_REASONING_EFFORT=minimal;STRIX_LLM_MAX_RETRIES=1;GEMINI_LOCATION=GLOBAL;PYTHONWARNINGS=ignore:Pydantic serializer warnings:UserWarning:pydantic.main;NPM_CONFIG_IGNORE_SCRIPTS=true;PNPM_CONFIG_IGNORE_SCRIPTS=true;YARN_ENABLE_SCRIPTS=false;UNRELATED_SECRET=" \ "scenario=$scenario runtime env forwarding" - # Non-contextual-orchestrator providers (gemini here) never see the - # stream-disabling opt-in: it is scoped narrowly to the gateway that - # rejects stream_options.include_usage alongside tools. - assert_file_contains \ - "$runtime_env_log" \ - "LLM_DISABLE_STREAMING=" \ - "scenario=$scenario non-gateway providers keep real streaming" fi if [ "$scenario" = "custom-openai-compatible-preserves-effort" ]; then assert_file_contains \ @@ -5973,16 +6001,6 @@ PY "STRIX_REASONING_EFFORT=minimal" \ "scenario=$scenario custom compatible endpoint effort" fi - if [ "$scenario" = "contextual-orchestrator-gateway-model-qualification" ]; then - # contextual-orchestrator rejects stream_options.include_usage=true - # alongside tools; Strix's agent loop always sends both, so the gate - # routes this gateway through Strix's own LLM_DISABLE_STREAMING opt-in - # (single non-streaming get_response per turn) instead of streaming. - assert_file_contains \ - "$runtime_env_log" \ - "LLM_DISABLE_STREAMING=true" \ - "scenario=$scenario contextual-orchestrator gateway disables SDK streaming to avoid the stream_options+tools rejection" - fi if [ "$scenario" = "report-known-internal-warning-sanitized" ]; then assert_file_not_contains \ diff --git a/tests/test_noema_orchestrator_workflow_contract.py b/tests/test_noema_orchestrator_workflow_contract.py index 75ad5242c0..3b9c5baeef 100644 --- a/tests/test_noema_orchestrator_workflow_contract.py +++ b/tests/test_noema_orchestrator_workflow_contract.py @@ -3,6 +3,7 @@ from __future__ import annotations import os +import json import shutil import subprocess import textwrap @@ -11,6 +12,134 @@ from tests.test_required_workflow_queue_contract import workflow_step, workflow_text +def test_noema_close_cleanup_selects_only_the_closed_pr_across_shared_display_titles( + tmp_path: Path, +) -> None: + """Execute cleanup against a shared-head-SHA fixture and cancel only the closed PR. + + Real jq/bash execution (not text-grepping): PR #7 (closing) and PR #8 + (unrelated, open) both have runs on the same head commit; only #7's + matches the PR-scoped selector cancel_runs applies, and a `completed` + PR #7 run must not be re-cancelled. Runs #104/#105 additionally cover + Devin Review's "Sibling Noema runs evade cancellation" finding on PR + #1507: a required-workflow-ruleset run materialized in a sibling + repository whose `display_title` never rendered this workflow's PR/head + run-name (a plain PR title instead) must still be matched through + GitHub's own `pull_requests[]` array, and only for the closing PR. The + fake `gh` below filters its fixture by the `status=` query parameter, + mirroring GitHub's own server-side status filtering, because the + workflow's cancel_runs deliberately relies on that filtering (see the + run block's own comment) rather than fetching everything and filtering + client-side. + """ + script = textwrap.dedent( + workflow_step( + workflow_text("noema-review.yml"), + "Cancel queued and running Noema reviews for the closed pull request", + ).split(" run: |\n", 1)[1].split("\n noema-review:", 1)[0] + ) + workflow_path = ".github/workflows/noema-review.yml" + runs = { + "workflow_runs": [ + { + "id": 101, + "path": workflow_path, + "name": "Required Noema Review", + "display_title": "Required Noema Review ContextualWisdomLab/demo#7@" + "a" * 40, + "head_sha": "a" * 40, + "status": "requested", + }, + { + "id": 102, + "path": workflow_path, + "name": "Required Noema Review", + "display_title": "Required Noema Review ContextualWisdomLab/demo#8@" + "a" * 40, + "head_sha": "a" * 40, + "status": "queued", + }, + { + "id": 103, + "path": workflow_path, + "name": "Required Noema Review", + "display_title": "Required Noema Review ContextualWisdomLab/demo#7@" + "a" * 40, + "head_sha": "a" * 40, + "status": "completed", + }, + { + "id": 104, + "path": workflow_path, + "name": "Required Noema Review", + "display_title": "Fix an unrelated example bug", + "head_sha": "a" * 40, + "status": "queued", + "pull_requests": [{"number": 7}], + }, + { + "id": 105, + "path": workflow_path, + "name": "Required Noema Review", + "display_title": "A different pull request's title", + "head_sha": "a" * 40, + "status": "queued", + "pull_requests": [{"number": 8}], + }, + ] + } + runs_file = tmp_path / "runs.json" + runs_file.write_text(json.dumps(runs), encoding="utf-8") + calls_file = tmp_path / "calls.txt" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +if [[ "$*" == *"--paginate"* ]]; then + [[ "$*" != *"/actions/workflows/"* ]] || exit 99 + printf '%s\n' "$*" >>"$FAKE_CALLS_FILE" + url="$3" + status="$(printf '%s' "$url" | sed -E 's/.*status=([a-z_]+)&.*/\\1/')" + jq --arg status "$status" '{workflow_runs: [.workflow_runs[] | select(.status == $status)]}' \\ + "$FAKE_RUNS_FILE" +else + printf '%s\n' "$*" >>"$FAKE_CALLS_FILE" +fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( # noqa: S603 + [shutil.which("bash") or "/bin/bash", "-c", script], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "TARGET_REPOSITORY": "ContextualWisdomLab/demo", + "CLOSED_PR_NUMBER": "7", + "CURRENT_RUN_ID": "999", + "FAKE_RUNS_FILE": str(runs_file), + "FAKE_CALLS_FILE": str(calls_file), + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + calls = calls_file.read_text(encoding="utf-8") + # Repository-scoped, status-server-filtered -- never the workflow-file- + # scoped endpoint, which does not resolve for sibling-repository runs. + assert "actions/runs?status=" in calls + assert "/actions/workflows/" not in calls + assert "/actions/runs/101/cancel" in calls + assert "/actions/runs/102/cancel" not in calls + assert "/actions/runs/103/cancel" not in calls + # Devin Review finding on PR #1507 ("Sibling Noema runs evade + # cancellation"): a required-workflow-ruleset run materialized in a + # sibling repository (#104) never renders this workflow's run-name into + # display_title, so it must still be matched via GitHub's own + # pull_requests[] array; a same-shaped run for an unrelated PR (#105) + # must not. + assert "/actions/runs/104/cancel" in calls + assert "/actions/runs/105/cancel" not in calls + + def test_noema_review_credentials_and_llm_use_orchestrator_free() -> None: """Require reviewer credentials and the sidecar; the public NIM hardcode is gone.""" workflow = workflow_text("noema-review.yml") @@ -56,6 +185,135 @@ def test_noema_review_credentials_and_llm_use_orchestrator_free() -> None: assert "secrets: inherit" not in workflow +def _expected_head_from_workflow_run_event(event: dict) -> str: + """Mirror EXPECTED_HEAD's ``||`` fallback chain for a ``workflow_run`` event. + + Reproduces GitHub Actions' short-circuit-on-falsy ``||`` semantics over + the same dotted paths ``noema-review.yml``'s ``EXPECTED_HEAD`` env var + reads, so a test can prove — with concrete, distinct base vs. PR-head SHA + values — which commit the expression actually resolves to, without + needing a live Actions runner to evaluate ``${{ }}`` syntax. + """ + client_payload = event.get("client_payload") or {} + pull_request = event.get("pull_request") or {} + workflow_run = event.get("workflow_run") or {} + pull_requests = workflow_run.get("pull_requests") or [] + workflow_run_pr_head = ( + (pull_requests[0].get("head") or {}).get("sha") if pull_requests else None + ) + return ( + client_payload.get("pr_head_sha") + or (pull_request.get("head") or {}).get("sha") + or workflow_run_pr_head + or "" + ) + + +def test_workflow_run_expected_head_uses_pull_request_head_not_base_commit() -> None: + """EXPECTED_HEAD for a workflow_run completion must resolve the PR head, not the base. + + Devin Review finding on PR #1507: ``github.event.workflow_run.head_sha`` + is the base/trusted commit the completing ``pull_request_target`` + workflow (Required OpenCode Review / Strix Security Scan) checked out — + not the PR head — so every workflow_run-triggered follow-up review used + to fail the stale-trigger gate. The fix reuses this same workflow's own + established pattern for ``PR_NUMBER`` (``pull_requests[0].number``) and + reads the actual PR head from ``pull_requests[0].head.sha`` instead. + """ + workflow = workflow_text("noema-review.yml") + assert ( + "EXPECTED_HEAD: ${{ github.event.client_payload.pr_head_sha || " + "github.event.pull_request.head.sha || " + "github.event.workflow_run.pull_requests[0].head.sha || '' }}" + ) in workflow + assert "EXPECTED_HEAD: ${{ github.event.client_payload.pr_head_sha || github.event.pull_request.head.sha || github.event.workflow_run.head_sha || '' }}" not in workflow + + base_sha = "b" * 40 + pr_head_sha = "a" * 40 + assert base_sha != pr_head_sha + workflow_run_event = { + "workflow_run": { + # The top-level head_sha on a workflow_run object completing a + # pull_request_target run is the base/trusted commit that run + # checked out (its own github.sha) -- not the PR's head. + "head_sha": base_sha, + "pull_requests": [ + {"number": 42, "head": {"sha": pr_head_sha}, "base": {"sha": base_sha}} + ], + } + } + assert _expected_head_from_workflow_run_event(workflow_run_event) == pr_head_sha + assert _expected_head_from_workflow_run_event(workflow_run_event) != base_sha + + +def test_workflow_run_expected_head_fails_closed_when_pull_requests_is_empty() -> None: + """A fork-originated workflow_run (empty pull_requests[]) yields no expected head. + + ``pull_requests`` is documented to come back empty for cross-fork PRs; + EXPECTED_HEAD must fall through to '' rather than fabricate a head, and + PR_NUMBER (already sourced from the same array) falls through the same + way, so the job's existing "Skip events without pull request context" + step still short-circuits the run before any stale-head comparison. + """ + workflow_run_event = {"workflow_run": {"head_sha": "c" * 40, "pull_requests": []}} + assert _expected_head_from_workflow_run_event(workflow_run_event) == "" + + +def _run_stale_trigger_step( + tmp_path: Path, *, expected_head: str, live_head: str +) -> subprocess.CompletedProcess[str]: + """Execute the "Reject a stale trigger" step's bash with a fake `gh` on PATH.""" + bash_executable = shutil.which("bash") or "/bin/bash" + step_script = textwrap.dedent( + workflow_step( + workflow_text("noema-review.yml"), + "Reject a stale trigger before credential or model setup", + ).split(" run: |\n", 1)[1] + ) + fake_gh = tmp_path / "gh" + fake_gh.write_text( + f"#!/usr/bin/env bash\nset -euo pipefail\nprintf '%s' '{live_head}'\n", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + env = { + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "TARGET_REPOSITORY": "ContextualWisdomLab/example", + "PR_NUMBER": "7", + "EXPECTED_HEAD": expected_head, + "GH_TOKEN": "synthetic-token", + } + return subprocess.run( # noqa: S603, S607 + [bash_executable, "-c", step_script], + env=env, + capture_output=True, + text=True, + check=False, + ) + + +def test_stale_trigger_step_rejects_noncanonical_uppercase_head( + tmp_path: Path, +) -> None: + """Reject caller-controlled uppercase SHA before any model work.""" + sha = "a" * 40 + result = _run_stale_trigger_step(tmp_path, expected_head=sha.upper(), live_head=sha) + assert result.returncode == 1 + assert "canonical lowercase exact head SHA" in result.stdout + + +def test_stale_trigger_step_still_rejects_a_genuinely_different_head( + tmp_path: Path, +) -> None: + """A canonical but genuinely different trigger head is still rejected.""" + result = _run_stale_trigger_step( + tmp_path, expected_head="a" * 40, live_head="b" * 40 + ) + assert result.returncode == 1 + assert "Noema trigger is stale" in result.stdout + + def test_noema_visibility_lookup_retries_transient_api_failures() -> None: """Bound transient GitHub API failures without weakening visibility validation.""" workflow = workflow_text("noema-review.yml") diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 338d46ba81..5229605627 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1,12 +1,607 @@ import base64 +import hashlib import json +import os +import shlex +import shutil +import subprocess import sys +import textwrap +from pathlib import Path import pytest from scripts.ci import noema_review_gate as noema +def test_gitleaks_ignore_is_exactly_scoped_to_superseded_uuid_fixture(): + entries = { + line + for line in Path(".gitleaksignore").read_text(encoding="utf-8").splitlines() + if line and not line.startswith("#") + } + fingerprint = ( + "6657eb76f0e2cf6dab9197cfa861a1f584653aba:" + "tests/test_noema_review_gate.py:generic-api-key:187" + ) + assert fingerprint in entries + assert sum("tests/test_noema_review_gate.py" in entry for entry in entries) == 1 + + +def test_noema_concurrency_and_live_head_cleanup_preserve_current_review(): + """Pin the invariants this cancellation mechanism must hold together. + + Several Devin Review rounds landed on this same cancellation mechanism in + one day (see the matching ``docs/product-technical-gap-baseline.md`` + entry for the full narrative), each closing a gap the previous fix left + open: + + 1. A live new-head trigger must cancel a still-running older-head run of + the same PR (proven end to end by + ``test_superseded_cleanup_preserves_current_and_newer_run_ids``, + executing the real production jq selector). + 2. A delayed ``workflow_run``/``repository_dispatch`` completion for an + OLDER head must never cancel a genuinely current run -- pinned here by + the head-inclusive concurrency group assertions below (native + protection, independent of this step) AND by the step-level ``if:`` + gate restricting this explicit cancellation entirely to live + ``pull_request_target`` triggers, so a workflow_run/repository_dispatch + execution never even reaches this step. + 3. A cancellation step whose OWN trigger was confirmed live at the start + of the job must still never cancel a run dispatched AFTER its own + dispatch, even though its own multi-pass scan can take long enough in + wall-clock time for such a run to appear in the active-runs listing: + proven by ``test_superseded_cleanup_preserves_current_and_newer_run_ids`` + (a higher run id survives) and pinned structurally here via the + ``.id < $current`` ordering guard plus the per-cancellation live-head + re-check. + 4. That live-head re-check is a housekeeping safeguard, not the review + itself: a transient failure reading it must stop cleanup without + crashing the step (and thus the whole job) -- proven by + ``test_superseded_cleanup_survives_a_transient_live_head_lookup_failure``. + """ + workflow = Path(".github/workflows/noema-review.yml").read_text(encoding="utf-8") + concurrency = workflow.split("concurrency:", 1)[1].split("permissions:", 1)[0] + assert "github.event.client_payload.pr_head_sha" in concurrency + assert "github.event.pull_request.head.sha" in concurrency + assert "github.event.workflow_run.pull_requests[0].head.sha" in concurrency + assert "github.event.workflow_run.head_sha" not in concurrency + assert "github.event.workflow_run.conclusion == 'cancelled'" in concurrency + assert "format('cancelled-{0}', github.run_id)" in concurrency + assert "'actionable'" in concurrency + assert "cancel-in-progress: ${{" in concurrency + assert "github.event_name != 'workflow_run'" in concurrency + assert "github.event.workflow_run.conclusion != 'cancelled'" in concurrency + assert "Cancel superseded Noema runs after live-head validation" in workflow + assert workflow.index("Reject a stale trigger before credential or model setup") < workflow.index( + "Cancel superseded Noema runs after live-head validation" + ) + cleanup = workflow.split("Cancel superseded Noema runs after live-head validation", 1)[1] + job_header = workflow.split("\n noema-review:", 1)[1].split(" steps:", 1)[0] + assert "actions: write" in job_header + # Invariant 2 (step-level half): only a live pull_request_target trigger + # may even attempt this cancellation -- workflow_run and + # repository_dispatch executions (which can legitimately be delayed by + # hours) skip this step entirely and rely solely on the head-inclusive + # concurrency group above. + assert ( + "if: github.event_name == 'pull_request_target' && env.PR_NUMBER != ''" + in cleanup + ) + assert 'select(.id < $current)' in cleanup + # The live-head re-check must be error-guarded (an `if !` command + # substitution), never a bare assignment under set -euo pipefail -- a + # transient failure here must stop cleanup, not crash the whole job. + assert cleanup.count('live_head="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}"') == 1 + assert ( + 'if ! live_head="$(gh api "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}" --jq \'.head.sha\'' + in cleanup + ) + assert "could not re-verify the live PR head before cancelling" in cleanup + assert '"${live_head,,}" != "${EXPECTED_HEAD,,}"' in cleanup + assert 'endswith("@" + $head)' in cleanup + assert "| not)" in cleanup + + +def test_noema_superseded_cleanup_selects_only_other_heads_of_same_pr(): + """Execute the workflow's jq selector against current, sibling, and foreign runs. + + ``$current`` must be passed with ``--argjson`` (a number), matching the + production invocation (``--argjson current "$CURRENT_RUN_ID"``): jq's + type ordering ranks every number below every string, so passing it as a + string via ``--arg`` would make the selector's directional ``.id < + $current`` guard vacuously true for every fixture row regardless of the + actual id values, silently proving nothing about that guard (caught by + review on PR #1507). With the numeric type restored, the fixture's ids + must also be realistic: GitHub Actions run ids increase monotonically + over time, so the "current" run (the latest trigger) has the *highest* + id here, and the superseded same-PR sibling has a lower one — the + opposite of this fixture's original (also-wrong) ordering, under which + the directional guard's own vacuous-true bug happened to still produce + the expected output for an unrelated reason.""" + jq = shutil.which("jq") + if jq is None: + pytest.skip("jq is required to execute the production cleanup selector") + workflow = Path(".github/workflows/noema-review.yml").read_text(encoding="utf-8") + start_marker = '--arg target "$TARGET_REPOSITORY" --arg head "$EXPECTED_HEAD" \'\n' + start = workflow.index(start_marker) + len(start_marker) + end = workflow.index('\n \' <<<"$runs_json"', start) + selector = workflow[start:end] + workflow_path = ".github/workflows/noema-review.yml" + runs = { + "workflow_runs": [ + {"id": 98, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review owner/repo#7@old"}, + {"id": 99, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review owner/repo#8@old"}, + {"id": 100, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review owner/repo#7@current"}, + {"id": 97, "name": "Other", "display_title": "Required Noema Review owner/repo#7@old"}, + ] + } + result = subprocess.run( + [jq, "-r", "--arg", "pr", "7", "--argjson", "current", "100", "--arg", "target", "owner/repo", "--arg", "head", "current", selector], + input=json.dumps(runs), + text=True, + capture_output=True, + check=True, + ) + assert result.stdout.splitlines() == ["98"] + assert "github.event.workflow_run.head_sha" not in workflow + assert "EXPECTED_HEAD:" in workflow + assert "--expected-head \"$EXPECTED_HEAD\"" in workflow + assert '"${live_head,,}" != "${EXPECTED_HEAD,,}"' in workflow + assert workflow.index("Reject a stale trigger before credential or model setup") < workflow.index( + "Select fail-closed Noema reviewer credential" + ) + + +def test_noema_superseded_cleanup_matches_a_sibling_run_by_pull_requests_array(): + """A sibling-repo run whose display_title never rendered is still matched. + + Devin Review, PR #1507 ("Sibling Noema runs evade cancellation"): a + required-workflow-ruleset run materialized in a sibling repository can + carry the bare workflow name in ``name`` and the plain PR title (not + this workflow's rendered run-name) in ``display_title`` -- exactly the + shape ``tests/test_opencode_required_verdict_regression.py`` documents + for the analogous OpenCode wake selector, and confirmed live against + real sibling-repository runs during this fix. The selector must still + match such a run via GitHub's own ``pull_requests[]`` array and exclude + the live head via the direct ``head_sha`` comparison, since the head is + also never embedded in a display_title that never rendered it. + """ + jq = shutil.which("jq") + if jq is None: + pytest.skip("jq is required to execute the production cleanup selector") + workflow = Path(".github/workflows/noema-review.yml").read_text(encoding="utf-8") + start_marker = '--arg target "$TARGET_REPOSITORY" --arg head "$EXPECTED_HEAD" \'\n' + start = workflow.index(start_marker) + len(start_marker) + end = workflow.index('\n \' <<<"$runs_json"', start) + selector = workflow[start:end] + workflow_path = ".github/workflows/noema-review.yml" + current_head = "b" * 40 + old_head = "a" * 40 + runs = { + "workflow_runs": [ + { + "id": 98, + "path": workflow_path, + "name": "Required Noema Review", + "display_title": "Fix an unrelated example bug", + "head_sha": old_head, + "pull_requests": [{"number": 7}], + }, + { + "id": 99, + "path": workflow_path, + "name": "Required Noema Review", + "display_title": "A different pull request's title", + "head_sha": old_head, + "pull_requests": [{"number": 8}], + }, + { + "id": 100, + "path": workflow_path, + "name": "Required Noema Review", + "display_title": "Same PR, current push", + "head_sha": current_head, + "pull_requests": [{"number": 7}], + }, + { + "id": 97, + "path": ".github/workflows/strix.yml", + "name": "Required Noema Review", + "display_title": "Fix an unrelated example bug", + "head_sha": old_head, + "pull_requests": [{"number": 7}], + }, + ] + } + result = subprocess.run( + [ + jq, "-r", + "--arg", "pr", "7", + "--argjson", "current", "101", + "--arg", "target", "owner/repo", + "--arg", "head", current_head, + selector, + ], + input=json.dumps(runs), + text=True, + capture_output=True, + check=True, + ) + assert result.stdout.splitlines() == ["98"] + + +def test_noema_close_event_cancels_historical_head_runs(): + """Close cleanup must cancel active Noema runs across prior head groups.""" + workflow = Path(".github/workflows/noema-review.yml").read_text(encoding="utf-8") + cleanup = workflow.split(" cancel-closed-pr-runs:", 1)[1].split( + " noema-review:", 1 + )[0] + assert "actions: write" in cleanup + assert "Cancel queued and running Noema reviews for the closed pull request" in cleanup + assert 'select((.name // "") | startswith("Required Noema Review"))' in cleanup + assert 'select(.path == ".github/workflows/noema-review.yml")' in cleanup + assert "CLOSED_PR_NUMBER" in cleanup + assert "CURRENT_RUN_ID" in cleanup + assert "/actions/runs/${run_id}/cancel" in cleanup + # Devin Review finding on PR #1507 (bug 1, "Sibling Noema runs evade + # cancellation"): GitHub does not consistently render this workflow's + # run-name for an organization-required-workflow run materialized in a + # sibling repository, so display_title alone (an exact `.name ==` + # filter alone, too) can never match a sibling PR's runs. Selection is + # PR-scoped by two independent, OR'd signals: the generated + # display_title where GitHub does render it, and GitHub's own + # pull_requests[] array otherwise -- reliably populated here because + # this job only ever processes same-repository, non-fork pull requests + # (unlike the general cross-fork case elsewhere in this org's tooling, + # where pull_requests[] is documented to come back empty). Never a bare + # head_sha, which two different open PRs can share. + assert ".head_sha == $head_sha" not in cleanup + assert "--arg head_sha" not in cleanup + assert ( + '((.display_title // "") | startswith("Required Noema Review " + ' + '$target + "#" + $pr + "@"))' + ) in cleanup + assert ( + 'or ((.pull_requests // []) | any(.number == ($pr | tonumber)))' + ) in cleanup + # Devin Review finding on PR #1507 (bug 2): a single sequential sweep + # across the five active statuses could miss a run that transitioned + # between statuses mid-sweep. Re-scan until a pass converges, bounded. + # actions/runs (repo-wide, status server-filtered) is kept rather than + # an unfiltered actions/workflows/noema-review.yml/runs snapshot: that + # workflow-file-scoped endpoint is not guaranteed to resolve for + # sibling-repository runs, since noema-review.yml is never itself + # committed to those repositories (it applies there only through the + # organization's required-workflow ruleset). + assert 'runs_url="repos/${TARGET_REPOSITORY}/actions/runs?status=${status}&per_page=100"' in cleanup + assert "max_passes=3" in cleanup + assert 'while [ "$pass" -le "$max_passes" ]; do' in cleanup + assert 'if [ "$pass" -ge 2 ] && [ "$pass_matches" -eq 0 ] && [ "$found_any" -eq 0 ]; then' in cleanup + + +def _extract_run_block(workflow_text: str, step_name: str) -> str: + """Extract one step's ``run: |`` body from workflow YAML by indentation. + + Matches the extraction helper already used by + ``tests/test_opencode_workflow_shell_syntax.py`` and + ``tests/test_strix_repository_visibility_contract.py`` for the same + purpose: find the named step, locate its ``run: |`` block, and collect + lines until indentation returns to (or below) the block's own level -- + which correctly stops at the end of the block even when, as here, the + step is the last (only) one in its job and the next line at the step's + own indentation belongs to a different job entirely. + """ + lines = workflow_text.splitlines() + step_index = next( + index for index, line in enumerate(lines) if line.strip() == f"- name: {step_name}" + ) + run_index = next( + index + for index in range(step_index + 1, len(lines)) + if lines[index].strip() == "run: |" + ) + run_indent = len(lines[run_index]) - len(lines[run_index].lstrip()) + block_lines: list[str] = [] + for line in lines[run_index + 1 :]: + if line.strip() and len(line) - len(line.lstrip()) <= run_indent: + break + block_lines.append(line[run_indent + 2 :] if len(line) >= run_indent + 2 else "") + return "\n".join(block_lines) + "\n" + + +def _close_cleanup_script() -> str: + """Extract the close-cleanup step's real bash body from the workflow.""" + workflow = Path(".github/workflows/noema-review.yml").read_text(encoding="utf-8") + return _extract_run_block( + workflow, "Cancel queued and running Noema reviews for the closed pull request" + ) + + +def _superseded_cleanup_script() -> str: + """Extract the live-head supersession step's real bash body.""" + workflow = Path(".github/workflows/noema-review.yml").read_text(encoding="utf-8") + return _extract_run_block( + workflow, "Cancel superseded Noema runs after live-head validation" + ) + + +def test_superseded_cleanup_preserves_current_and_newer_run_ids(tmp_path: Path) -> None: + """Execute cleanup and cancel only the same PR's older, different-head run.""" + current_head = "b" * 40 + workflow_path = ".github/workflows/noema-review.yml" + runs = {"workflow_runs": [ + {"id": 100, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#7@" + "a" * 40}, + {"id": 199, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#7@" + current_head}, + {"id": 201, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#7@" + "c" * 40}, + {"id": 99, "path": workflow_path, "name": "Required Noema Review", "display_title": "Required Noema Review ContextualWisdomLab/example#8@" + "a" * 40}, + ]} + fixture = tmp_path / "runs.json" + fixture.write_text(json.dumps(runs), encoding="utf-8") + calls = tmp_path / "calls.txt" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$FAKE_CALLS" +if [[ "$*" == *"/pulls/7"* ]]; then printf '%s\n' "$EXPECTED_HEAD"; exit 0; fi +if [[ "$*" == *"actions/runs?status="* ]]; then cat "$FAKE_RUNS"; exit 0; fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( # noqa: S603 + [shutil.which("bash") or "/bin/bash", "-c", _superseded_cleanup_script()], + env={**os.environ, "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "TARGET_REPOSITORY": "ContextualWisdomLab/example", "PR_NUMBER": "7", + "EXPECTED_HEAD": current_head, "CURRENT_RUN_ID": "200", + "FAKE_RUNS": str(fixture), "FAKE_CALLS": str(calls)}, + capture_output=True, text=True, check=False, + ) + assert result.returncode == 0, result.stderr + recorded = calls.read_text(encoding="utf-8") + assert "/actions/runs/100/cancel" in recorded + assert "/actions/runs/199/cancel" not in recorded + assert "/actions/runs/201/cancel" not in recorded + assert "/actions/runs/99/cancel" not in recorded + + +def test_superseded_cleanup_survives_a_transient_live_head_lookup_failure( + tmp_path: Path, +) -> None: + """A transient live-head re-check failure must stop cleanup, not crash the step. + + The live-head re-check this step performs before every single + cancellation is a housekeeping safeguard, not the review itself. Before + this fix, `live_head="$(gh api ...)"` was an unguarded command + substitution under `set -euo pipefail`: a transient `gh api` failure + (rate limit, network blip) on that one call would exit the whole step + non-zero, failing this job and blocking a perfectly valid, live-head + Noema review over an ancillary API hiccup unrelated to the review + itself (Devin Review finding on PR #1507). The fix treats "cannot + verify" the same as "verified stale": stop cancelling further runs, but + exit 0 so the job -- and the actual review later in it -- proceeds. + """ + current_head = "b" * 40 + runs = { + "workflow_runs": [ + { + "id": 100, + "path": ".github/workflows/noema-review.yml", + "name": "Required Noema Review", + "display_title": "Required Noema Review ContextualWisdomLab/example#7@" + "a" * 40, + }, + ] + } + fixture = tmp_path / "runs.json" + fixture.write_text(json.dumps(runs), encoding="utf-8") + calls = tmp_path / "calls.txt" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + """#!/usr/bin/env bash +set -euo pipefail +printf '%s\n' "$*" >>"$FAKE_CALLS" +if [[ "$*" == *"/pulls/7"* ]]; then echo "gh: transient error" >&2; exit 1; fi +if [[ "$*" == *"actions/runs?status="* ]]; then cat "$FAKE_RUNS"; exit 0; fi +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( # noqa: S603 + [shutil.which("bash") or "/bin/bash", "-c", _superseded_cleanup_script()], + env={**os.environ, "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "TARGET_REPOSITORY": "ContextualWisdomLab/example", "PR_NUMBER": "7", + "EXPECTED_HEAD": current_head, "CURRENT_RUN_ID": "200", + "FAKE_RUNS": str(fixture), "FAKE_CALLS": str(calls)}, + capture_output=True, text=True, check=False, + ) + assert result.returncode == 0, ( + f"a transient live-head lookup failure must not crash this step " + f"(it would fail the whole job); stderr={result.stderr!r}" + ) + assert "/actions/runs/100/cancel" not in calls.read_text(encoding="utf-8") + assert "could not re-verify the live PR head" in result.stderr + + +def _write_fake_gh(tmp_path: Path, *, body: str) -> dict[str, str]: + """Write a fake `gh` executable and return a PATH-prefixed env base for it.""" + fake_gh = tmp_path / "gh" + fake_gh.write_text(f"#!/usr/bin/env bash\nset -euo pipefail\n{body}\n", encoding="utf-8") + fake_gh.chmod(0o755) + return { + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "GH_TOKEN": "synthetic-token", + "TARGET_REPOSITORY": "ContextualWisdomLab/example", + "CLOSED_PR_NUMBER": "42", + "CURRENT_RUN_ID": "999", + } + + +def test_close_cleanup_selector_is_pr_scoped_not_head_sha_scoped(tmp_path: Path) -> None: + """Real jq execution: a shared head SHA must not leak cancellation across PRs. + + Devin Review finding on PR #1507 (bug 1). Two open pull requests (#42, + the one closing, and #43, unrelated) share one head commit -- a real, + if uncommon, GitHub scenario (e.g. a duplicate PR opened from the same + branch against a different target). Only PR #42's run may be cancelled; + PR #43's run, identical except for its PR association, must survive + untouched. This pipes representative run JSON through the workflow's + actual jq selector rather than grep-matching the YAML text. The fake + `gh` here answers every status query with the same fixture (status + filtering is not what this test is about); the status-filtering + contract is covered separately below. + """ + shared_head = "d" * 40 + fixture = { + "workflow_runs": [ + { + "id": 100, + "path": ".github/workflows/noema-review.yml", + "name": "Required Noema Review", + "display_title": ( + f"Required Noema Review ContextualWisdomLab/example#42@{shared_head}" + ), + }, + { + "id": 200, + "path": ".github/workflows/noema-review.yml", + "name": "Required Noema Review", + "display_title": ( + f"Required Noema Review ContextualWisdomLab/example#43@{shared_head}" + ), + }, + ] + } + fixture_path = tmp_path / "fixture.json" + fixture_path.write_text(json.dumps(fixture), encoding="utf-8") + cancel_log = tmp_path / "cancelled-run-ids.txt" + cancel_log.write_text("", encoding="utf-8") + + env = _write_fake_gh( + tmp_path, + body=textwrap.dedent( + f"""\ + if [ "$1" = api ] && [ "$2" = --paginate ]; then + cat {shlex.quote(str(fixture_path))} + exit 0 + elif [ "$1" = api ] && [ "$2" = --method ] && [ "$3" = POST ]; then + run_id="$(printf '%s' "$4" | sed -E 's#.*/runs/([0-9]+)/cancel#\\1#')" + printf '%s\\n' "$run_id" >> {shlex.quote(str(cancel_log))} + exit 0 + fi + echo "unexpected gh invocation: $*" >&2 + exit 1 + """ + ), + ) + + bash_executable = shutil.which("bash") or "/bin/bash" + result = subprocess.run( # noqa: S603 + [bash_executable, "-c", _close_cleanup_script()], + env=env, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + + cancelled_ids = { + line.strip() for line in cancel_log.read_text(encoding="utf-8").splitlines() if line.strip() + } + assert cancelled_ids == {"100"}, ( + f"expected only PR #42's run (100) cancelled, got {cancelled_ids}; " + f"stderr={result.stderr}" + ) + + +def test_close_cleanup_survives_a_run_transitioning_between_active_statuses( + tmp_path: Path, +) -> None: + """Real bash execution: a run that changes status mid-sweep is still cancelled. + + Devin Review finding on PR #1507 (bug 2). A run for the closed PR is not + yet visible under any active status on the sweep's first pass (modeling + it being "requested" when the already-fetched "queued" list was read, + then becoming "queued" moments later, after the loop had already moved + past checking "queued" for that pass) and only becomes visible, under + "queued", starting with the *second* query for that status. A single + sequential sweep (the pre-fix behavior) would find zero matches and + leave this run running forever; the fixed multi-pass sweep must still + cancel it. This also exercises the status query parameter end to end + (the fake `gh` here filters by it, unlike the test above). + """ + fixture = { + "workflow_runs": [ + { + "id": 300, + "path": ".github/workflows/noema-review.yml", + "name": "Required Noema Review", + "display_title": ( + f"Required Noema Review ContextualWisdomLab/example#42@{'d' * 40}" + ), + } + ] + } + fixture_path = tmp_path / "fixture.json" + fixture_path.write_text(json.dumps(fixture), encoding="utf-8") + cancel_log = tmp_path / "cancelled-run-ids.txt" + cancel_log.write_text("", encoding="utf-8") + state_dir = tmp_path / "state" + state_dir.mkdir() + + env = _write_fake_gh( + tmp_path, + body=textwrap.dedent( + f"""\ + if [ "$1" = api ] && [ "$2" = --paginate ]; then + url="$3" + status="$(printf '%s' "$url" | sed -E 's/.*status=([a-z_]+)&.*/\\1/')" + counter_file={shlex.quote(str(state_dir))}"/count-${{status}}" + count=0 + [ -f "$counter_file" ] && count="$(cat "$counter_file")" + count=$((count + 1)) + printf '%s' "$count" > "$counter_file" + if [ "$status" = queued ] && [ "$count" -eq 2 ]; then + cat {shlex.quote(str(fixture_path))} + else + echo '{{"workflow_runs": []}}' + fi + exit 0 + elif [ "$1" = api ] && [ "$2" = --method ] && [ "$3" = POST ]; then + run_id="$(printf '%s' "$4" | sed -E 's#.*/runs/([0-9]+)/cancel#\\1#')" + printf '%s\\n' "$run_id" >> {shlex.quote(str(cancel_log))} + exit 0 + fi + echo "unexpected gh invocation: $*" >&2 + exit 1 + """ + ), + ) + + bash_executable = shutil.which("bash") or "/bin/bash" + result = subprocess.run( # noqa: S603 + [bash_executable, "-c", _close_cleanup_script()], + env=env, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + + cancelled_ids = { + line.strip() for line in cancel_log.read_text(encoding="utf-8").splitlines() if line.strip() + } + assert cancelled_ids == {"300"}, ( + f"the status-transitioning run must still be cancelled; got {cancelled_ids}; " + f"stderr={result.stderr}" + ) + # Prove the race is real: pass 1 alone (the pre-fix, single-sweep + # behavior) found nothing, so only the fixed multi-pass loop caught it. + assert "pass 1/3 matched 0 run(s)" in result.stderr + assert "pass 2/3 matched 1 run(s)" in result.stderr + + def fake_secret(*parts: str) -> str: return "".join(parts) @@ -147,6 +742,600 @@ def app_identity(args, **kwargs): noema.extract_json_object("not-json") +def test_extract_json_object_balances_wrapped_and_multiple_objects(): + """Decode one complete object without joining unrelated brace-bearing text.""" + verdict = {"decision": "approve", "summary": "balanced { text }"} + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object( + "prose {not JSON} before " + json.dumps(verdict) + " after {brace prose}" + ) + assert noema.extract_json_object( + json.dumps(verdict) + "\n" + json.dumps({"decision": "comment"}) + ) == verdict + escaped = {"decision": "approve", "summary": 'escaped " { text }'} + assert noema.extract_json_object(json.dumps(escaped)) == escaped + + +def test_extract_json_object_rejects_approval_after_malformed_top_level_candidate(): + """A malformed first candidate must not release a later approval verdict.""" + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object( + '{"broken": invalid} {"decision":"approve","summary":"later"}' + ) + + +def test_extract_json_object_rejects_nested_recovery_from_malformed_outer_object(): + """A valid nested object must not escape its malformed outer object.""" + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object( + 'prefix {"broken": {"decision":"approve","summary":"nested"} trailing' + ) + + +def test_extract_json_object_rejects_nested_recovery_from_malformed_outer_array(): + """A valid nested object must not escape a malformed outer *array* either. + + Candidate discovery must track ``[``/``]`` depth alongside ``{``/``}``: + without it, the inner object's own ``{`` is wrongly seen at depth zero + (only brace nesting was tracked) and treated as a fresh top-level + candidate, letting a complete inner object "recover" out of an + unterminated outer array — the same class of bug + ``test_extract_json_object_rejects_nested_recovery_from_malformed_outer_object`` + covers for an outer object wrapper.""" + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object( + '[{"decision":"comment","summary":"ok","findings":[]}' + ) + + +@pytest.mark.parametrize("payload", ['[} {"decision":"approve"}', '{] {"decision":"approve"}']) +def test_extract_json_object_rejects_recovery_after_mismatched_delimiter(payload): + """A mismatched closer must not release a nested verdict candidate.""" + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object(payload) + + +def test_extract_json_object_rejects_nested_recovery_via_a_mismatched_closer(): + """A stray closer of the wrong bracket type must not fake-close a wrapper. + + Candidate discovery uses a bracket-*type* stack, not a plain up/down + counter: a ``]`` only pops an innermost ``[``, and a ``}`` only pops an + innermost ``{``. A plain counter that treated any closer as -1 would let + a mismatched closer (which cannot legitimately close the container it + appears in) prematurely signal "back to depth zero," so a later nested + recovery object's own ``{`` would wrongly be seen as a fresh top-level + candidate (Devin review on PR #1507). Covers both mismatch directions: + a stray ``]`` inside an unterminated ``{``, and a stray ``}`` inside an + unterminated ``[``.""" + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object( + '{"broken": ]{"decision":"comment","summary":"ok","findings":[]}' + ) + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object( + '{"broken": [}{"decision":"comment","summary":"ok","findings":[]}' + ) + + +def test_extract_json_object_stops_discovery_after_any_mismatched_closer(): + """A mismatched closer must poison the *rest* of discovery, not just the + bracket group it appears in. + + Merely making a mismatched closer a stack no-op (ignored rather than + popped) is not enough on its own: a later, otherwise-well-formed pair + can still validly re-close the stack down to empty despite the earlier + mismatch, so a subsequent { would again look like a fresh top-level + candidate. ``[} ] {...}`` -- the stray } is a no-op against the open [, + but the following ] still legitimately closes that [, and the { after + it would wrongly look top-level again if discovery kept scanning + (Devin review on PR #1507). No candidate must be found past the + mismatch at all.""" + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object( + '[} ] {"decision":"comment","summary":"ok","findings":[]}' + ) + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object( + '{] } {"decision":"comment","summary":"ok","findings":[]}' + ) + + +def test_json_nesting_within_bound_does_not_undercount_past_a_mismatched_closer(): + """A mismatched closer must not make the bound-check think a candidate + closed early, undercounting nesting that raw_decode would still walk + through when this candidate is actually decoded. Covers both mismatch + directions: a stray ``]`` inside an unterminated ``{``, and a stray + ``}`` inside an unterminated ``[``.""" + deep = "[" * 5 + stray_bracket = '{"a": ]' + deep + "0" + "]" * 5 + "}" + assert noema._json_nesting_within_bound(stray_bracket, 0, 100) is True + assert noema._json_nesting_within_bound(stray_bracket, 0, 3) is False + + stray_brace = '{"a": [}0]}' + assert noema._json_nesting_within_bound(stray_brace, 0, 100) is True + + +def test_extract_json_object_fails_closed_on_a_real_deep_payload(): + """A genuinely deep JSON payload must fail closed on this job's own runtime. + + Deliberately real input, not a monkeypatch: ``json.JSONDecoder.raw_decode``'s + own recursion behavior is not a stable contract across Python versions — + a real ``depth = max(20_000, sys.getrecursionlimit() * 2)`` nested array + raises ``RecursionError`` on Python 3.11-3.13 but decodes successfully + (no exception) on the Python 3.14 runner this job actually runs on (see + ``extract_json_object``'s docstring for the verifying CI evidence). This + test proves the *explicit* ``MAX_JSON_NESTING_DEPTH`` bound rejects real + excessive nesting regardless of which behavior the running interpreter + happens to have, rather than proving only that a raised RecursionError is + handled (see the sibling ``..._on_a_recursion_error_from_the_decoder`` + test below for that narrower, supplemental contract).""" + depth = max(20_000, sys.getrecursionlimit() * 2) + nested = '{"decision":' + ("[" * depth) + "0" + ("]" * depth) + "}" + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object(nested) + + +def test_extract_json_object_accepts_nesting_within_the_bound(): + """A legitimately nested verdict (well under the depth bound) still decodes.""" + nested = '{"decision":' + ("[" * 10) + "0" + ("]" * 10) + "}" + assert noema.extract_json_object(nested) == {"decision": json.loads("[" * 10 + "0" + "]" * 10)} + + +def test_json_nesting_within_bound_handles_escaped_quotes_inside_strings(): + """An escaped quote inside a string must not be mistaken for the string's + terminator: unrelated bracket characters that happen to follow inside the + same string value must not be miscounted as real nesting depth, or a + shallow, valid verdict would be wrongly rejected as excessively nested.""" + payload = '{"decision": "abc\\"' + ("[" * 200) + 'def"}' + assert json.loads(payload) == {"decision": 'abc"' + ("[" * 200) + "def"} + assert noema.extract_json_object(payload) == json.loads(payload) + + +def test_extract_json_object_fails_closed_on_a_recursion_error_from_the_decoder(monkeypatch): + """Supplemental coverage: an actual RecursionError from raw_decode (should + the running interpreter ever raise one within the bound) still reaches + the same bounded, scrubbed diagnostic as every other decode failure + here, on top of the explicit depth bound proven above.""" + def reject_deep_json(_decoder, _text, _start=0): + raise RecursionError("maximum recursion depth exceeded") + + monkeypatch.setattr(json.JSONDecoder, "raw_decode", reject_deep_json) + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object('{"item": {}}') + + +def test_extract_json_object_fails_closed_on_malformed_json(): + """A brace-wrapped but syntactically invalid LLM response must raise the + same fail-closed RuntimeError this module uses for other unusable-verdict + cases, never an unhandled json.JSONDecodeError (the reported CI crash). + + Devin Review security finding on PR #1507: the raised diagnostic must + never embed the raw (even scrubbed) model response, because this is a + public ``pull_request_target`` job and the finite scrub-pattern list + cannot guarantee an LLM-echoed or hallucinated credential in an + unrecognized shape is caught. Only a length and a content fingerprint + are logged.""" + # Reproduces "Expecting property name enclosed in double quotes": an + # unquoted/truncated key inside an otherwise brace-wrapped object. + malformed = '{"decision":"approve", trailing garbage not: "quoted}' + with pytest.raises(RuntimeError, match="was not valid JSON") as excinfo: + noema.extract_json_object(malformed) + assert not isinstance(excinfo.value, json.JSONDecodeError) + message = str(excinfo.value) + # The raw response text must never appear in the diagnostic. + assert "approve" not in message + assert "trailing garbage" not in message + # A bounded, non-secret correlation diagnostic replaces it instead. + assert f"response length={len(malformed)} chars" in message + assert "sha256=" in message + fingerprint = hashlib.sha256(malformed.encode("utf-8")).hexdigest()[:16] + assert fingerprint in message + + # A response truncated mid-object hits the same decode failure. + truncated = '{"decision":"approve","summary":"looks fine so far,' + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.extract_json_object(truncated) + + # A credential in a shape the finite scrub-pattern list does NOT + # recognize (no "token"/"key"/"bearer" marker, no known provider prefix + # — just a bare UUID-shaped value mid-sentence) must still never reach + # the raised message, because raw content is never embedded at all. + unrecognized_shape_secret = fake_secret( + "3f29e1a7-8b44-4c1d", "-9e77-2a5f9c001234" + ) + leaky = ( + '{"decision":"approve","summary":"use internal id ' + f"{unrecognized_shape_secret} to correlate, trailing garbage" + ) + # Confirm this test is not vacuous: the existing finite regex scrubber + # really does miss this shape. + assert unrecognized_shape_secret in (noema.scrub_sensitive_data(leaky) or "") + with pytest.raises(RuntimeError) as leaky_excinfo: + noema.extract_json_object(leaky) + leaky_message = str(leaky_excinfo.value) + assert unrecognized_shape_secret not in leaky_message + assert "approve" not in leaky_message + assert "ghp_" not in leaky_message + + # A known-shape secret (would have matched the old finite scrubber too) + # must also never appear, now that raw content is omitted outright. + known_shape_leaky = '{"decision":"approve","summary":"token ghp_' + "a" * 36 + '", bad' + with pytest.raises(RuntimeError) as known_excinfo: + noema.extract_json_object(known_shape_leaky) + assert "ghp_" not in str(known_excinfo.value) + + # Long malformed content produces a bounded diagnostic regardless of + # input size — never logged in full, and never truncated-and-embedded + # either; the diagnostic length does not grow with the input. + huge = '{"decision":"approve", ' + ("x" * 5000) + " bad" + with pytest.raises(RuntimeError) as huge_excinfo: + noema.extract_json_object(huge) + huge_message = str(huge_excinfo.value) + assert "x" * 100 not in huge_message + assert len(huge_message) < 500 + assert f"response length={len(huge)} chars" in huge_message + + # Devin Review follow-up finding: a malformed verdict containing an + # escaped lone surrogate (valid inside a Python/JSON string, but not + # representable in strict UTF-8) must not crash the fingerprint + # computation itself with an unhandled UnicodeEncodeError -- it must + # still fail closed with the same bounded RuntimeError. + surrogate_bearing = '{"decision":"approve", "note": "\ud800", trailing bad' + with pytest.raises(RuntimeError, match="was not valid JSON") as surrogate_excinfo: + noema.extract_json_object(surrogate_bearing) + assert not isinstance(surrogate_excinfo.value, UnicodeEncodeError) + surrogate_message = str(surrogate_excinfo.value) + assert "sha256=" in surrogate_message + assert f"response length={len(surrogate_bearing)} chars" in surrogate_message + + +def test_extract_llm_message_content_happy_paths(): + """A well-formed envelope returns its stripped content; a missing (not + malformed) choices/message/content field is treated leniently, matching + the pre-fix code's behavior for an absent field.""" + envelope = json.dumps({"choices": [{"message": {"content": " {\"decision\":\"approve\"} "}}]}) + assert noema.extract_llm_message_content(envelope) == '{"decision":"approve"}' + + assert noema.extract_llm_message_content(json.dumps({})) == "" + assert noema.extract_llm_message_content(json.dumps({"choices": []})) == "" + assert noema.extract_llm_message_content(json.dumps({"choices": [{}]})) == "" + assert noema.extract_llm_message_content(json.dumps({"choices": [{"message": None}]})) == "" + assert ( + noema.extract_llm_message_content(json.dumps({"choices": [{"message": {"content": None}}]})) + == "" + ) + + +def test_extract_llm_message_content_fails_closed_on_malformed_raw_body(): + """Devin Review bug finding on PR #1507: a malformed raw HTTP body must + raise the same bounded RuntimeError call_llm's repair path already uses + for a malformed verdict, never an unhandled json.JSONDecodeError.""" + with pytest.raises(RuntimeError, match="response body was not valid JSON"): + noema.extract_llm_message_content("not json at all") + + +@pytest.mark.parametrize("body", ["[]", "null", '"just a string"', "5"]) +def test_extract_llm_message_content_fails_closed_on_non_object_top_level(body): + """A syntactically valid but non-object top-level JSON value (array, + null, bare string, bare number) must fail closed instead of crashing on + the next `.get(...)` call, exactly as Devin's finding described.""" + with pytest.raises(RuntimeError, match="response body was not a JSON object"): + noema.extract_llm_message_content(body) + + +@pytest.mark.parametrize("choices", [{"a": 1}, "choices-as-string", 5]) +def test_extract_llm_message_content_fails_closed_on_wrong_shaped_choices(choices): + """A present-but-wrong-shaped (non-list) 'choices' field must fail + closed instead of crashing on `choices[0]`.""" + with pytest.raises(RuntimeError, match="'choices' was not a list"): + noema.extract_llm_message_content(json.dumps({"choices": choices})) + + +@pytest.mark.parametrize("first_choice", [None, 1, "text"]) +def test_extract_llm_message_content_fails_closed_on_wrong_shaped_choice_element(first_choice): + """A choices[0] that is not a JSON object must fail closed instead of + crashing on `.get("message")`.""" + with pytest.raises(RuntimeError, match=r"choices\[0\] was not a JSON object"): + noema.extract_llm_message_content(json.dumps({"choices": [first_choice]})) + + +@pytest.mark.parametrize("message", [[1, 2], "text", 5]) +def test_extract_llm_message_content_fails_closed_on_wrong_shaped_message(message): + """A present-but-wrong-shaped (non-object) 'message' field must fail + closed instead of crashing on `.get("content")`.""" + with pytest.raises(RuntimeError, match="'message' was not a JSON object"): + noema.extract_llm_message_content(json.dumps({"choices": [{"message": message}]})) + + +@pytest.mark.parametrize("content", [5, [1, 2], {"a": 1}]) +def test_extract_llm_message_content_fails_closed_on_non_string_content(content): + """A present-but-non-string 'content' field must fail closed instead of + crashing on `.strip()`.""" + with pytest.raises(RuntimeError, match="'content' was not a string"): + noema.extract_llm_message_content( + json.dumps({"choices": [{"message": {"content": content}}]}) + ) + + +def test_decode_llm_response_body_happy_path(): + """A well-formed UTF-8 response body decodes normally.""" + assert noema.decode_llm_response_body("hello world".encode("utf-8")) == "hello world" + + +def test_decode_llm_response_body_fails_closed_on_invalid_utf8(): + """Devin Review bug finding on PR #1507 round 3: a gateway reply + containing invalid UTF-8 must raise the same bounded RuntimeError + call_llm's repair path already uses for a malformed envelope, never an + unhandled UnicodeDecodeError. The raised message must never embed the + raw response bytes — even an attempted-decode fragment near the bad + byte — matching extract_json_object's no-raw-content pattern, since a + body containing invalid UTF-8 could still contain a credential-adjacent + byte sequence.""" + secret_like_prefix = b"token=ghp_deadbeef1234567890" + raw_bytes = secret_like_prefix + bytes([0xFF]) + b"unrecoverable tail bytes" + with pytest.raises(RuntimeError) as excinfo: + noema.decode_llm_response_body(raw_bytes) + message = str(excinfo.value) + assert "not valid UTF-8" in message + assert "ghp_" not in message + assert "unrecoverable tail" not in message + fingerprint = hashlib.sha256(raw_bytes).hexdigest()[:16] + assert f"response length={len(raw_bytes)} bytes" in message + assert f"sha256={fingerprint}" in message + + +def test_call_llm_repairs_one_malformed_envelope_before_failing_closed(monkeypatch): + """The envelope-level fail-closed path integrates with the existing + verdict-repair boundary: a malformed gateway reply gets one repair-retry + request before failing closed, exactly like a malformed verdict JSON + already does.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + bodies = iter( + ( + "not-json-at-all", + json.dumps( + { + "choices": [ + { + "message": { + "content": json.dumps( + {"decision": "comment", "summary": "Recovered", "findings": []} + ) + } + } + ] + } + ), + ) + ) + requests = [] + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + return next(bodies).encode() + + def open_response(_opener, request, **_kwargs): + requests.append(json.loads(request.data)) + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + + verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + + assert verdict["summary"] == "Recovered" + assert len(requests) == 2 + assert "prior verdict was rejected" in requests[1]["messages"][1]["content"] + + +def test_call_llm_skips_repair_retry_when_head_moves_before_it_fires(monkeypatch): + """CodeRabbit finding on PR #1507: ``expected_head`` is checked before + model work and before publication, but the one-time repair-retry request + inside ``call_llm`` used to fire unconditionally on a malformed first + verdict, even if the PR head had already moved. That burns a second, + potentially multi-hour ``NOEMA_LLM_TIMEOUT_SECONDS`` call on a review + ``inspect_and_review``'s own post-call stale-head check would discard + anyway. ``call_llm`` must instead re-check the live head via ``fetch_pr`` + before the retry request and fail closed with + ``StaleHeadDuringRepairRetryError`` — cleanly, not a crash — issuing only + the one doomed first request.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + open_calls = [] + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + # Malformed: missing "choices" triggers call_llm's fail-closed + # RuntimeError path on the very first attempt. + return b"[]" + + def open_response(_opener, request, **_kwargs): + open_calls.append(request) + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + # The live PR head has moved on since the trigger fetched "head". + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr(headRefOid="new")) + + with pytest.raises(noema.StaleHeadDuringRepairRetryError, match="stale before repair retry"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + # Only the first, already-doomed request was made — the repair-retry + # request never fired once the live head no longer matched. + assert len(open_calls) == 1 + + +def test_call_llm_still_repairs_once_when_head_has_not_moved(monkeypatch): + """A matching live head must not block the existing one-time repair + retry — this is a narrow addition to the existing repair boundary, not a + behavior change for the unstale case.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + contents = iter( + ( + "not-json-at-all", + json.dumps({"decision": "comment", "summary": "Recovered", "findings": []}), + ) + ) + open_calls = [] + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + content = next(contents) + return json.dumps({"choices": [{"message": {"content": content}}]}).encode() + + def open_response(_opener, request, **_kwargs): + open_calls.append(request) + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr(headRefOid="head")) + + verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + + assert verdict["summary"] == "Recovered" + assert len(open_calls) == 2 + + +def test_inspect_and_review_reports_stale_before_repair_retry_cleanly(monkeypatch): + """``inspect_and_review`` must treat a stale-during-repair-retry signal + exactly like its own pre-model and pre-publication stale checks: a clean + skip (return 0), never an unhandled exception or a published review.""" + pr = make_pr() + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) + monkeypatch.setattr(noema, "current_actor", lambda: "noema") + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value: "context") + + def fake_call_llm(*args, **kwargs): + raise noema.StaleHeadDuringRepairRetryError( + "Pull request head changed during review; stale before repair retry." + ) + + monkeypatch.setattr(noema, "call_llm", fake_call_llm) + monkeypatch.setattr( + noema, + "submit_review", + lambda *args, **kwargs: pytest.fail("stale-during-repair verdict must not publish"), + ) + + assert noema.inspect_and_review("owner/repo", 7, "head") == 0 + + +def test_call_llm_fails_closed_after_repeated_malformed_envelope(monkeypatch): + """Two consecutive malformed envelopes must produce a single clean + top-level RuntimeError diagnostic, never an unhandled traceback — but + the first still gets a repair-retry request like a malformed verdict + would.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + open_calls = [] + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + # Top-level JSON is a bare list — no "choices" object to speak of. + return b"[]" + + def open_response(_opener, request, **_kwargs): + open_calls.append(request) + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + + with pytest.raises(RuntimeError, match="response body was not a JSON object"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + assert len(open_calls) == 2 + + +def test_call_llm_fails_closed_after_repeated_invalid_utf8_response(monkeypatch): + """Devin Review bug finding on PR #1507 round 3: a gateway reply + containing invalid UTF-8 bytes used to raise UnicodeDecodeError before + extract_llm_message_content or the verdict-JSON repair boundary ever + ran, crashing the required review check with an unhandled traceback. + It must instead integrate with the existing repair-retry boundary + exactly like a malformed JSON envelope already does: one repair-retry + request, then a single clean top-level RuntimeError when the retry + response is *also* invalid UTF-8 — never an unhandled traceback.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + open_calls = [] + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + # Invalid UTF-8: a lone continuation byte with no lead byte. + return b"not utf-8 at all: \x80\x81\xfe" + + def open_response(_opener, request, **_kwargs): + open_calls.append(request) + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + + with pytest.raises(RuntimeError, match="response body was not valid UTF-8"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + # One initial request plus exactly one repair-retry request — not an + # unbounded retry loop, and not a crash on the first attempt. + assert len(open_calls) == 2 + assert "prior verdict was rejected" in json.loads(open_calls[1].data)["messages"][1]["content"] + + +@pytest.mark.parametrize("choices", [{"a": 1}, 5]) +def test_call_llm_fails_closed_on_wrong_shaped_gateway_choices(monkeypatch, choices): + """A malformed (non-list) choices field surfaces through call_llm's + fail-closed path rather than crashing the required review job.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + return json.dumps({"choices": choices}).encode() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", lambda *args, **kwargs: Response()) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + + with pytest.raises(RuntimeError, match="'choices' was not a list"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + + @pytest.mark.parametrize( ("actor", "installation_id", "source"), [ @@ -266,15 +1455,16 @@ def read(self): def test_call_llm_handles_configuration_and_verdicts(monkeypatch): monkeypatch.setattr(noema, "validate_substantive_verdict", lambda *_args: None) pr = make_pr() + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) monkeypatch.delenv("NOEMA_LLM_API_URL", raising=False) monkeypatch.delenv("NOEMA_LLM_API_KEY", raising=False) with pytest.raises(RuntimeError, match="not configured"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") monkeypatch.setenv("NOEMA_LLM_API_URL", "file:///etc/passwd") monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret") with pytest.raises(ValueError, match="URL scheme must be http or https"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") monkeypatch.setenv("NOEMA_LLM_API_KEY", "secret") @@ -283,6 +1473,7 @@ def test_call_llm_handles_configuration_and_verdicts(monkeypatch): def fake_urlopen(request, timeout): seen["url"] = request.full_url + seen["timeout"] = timeout seen["body"] = json.loads(request.data.decode("utf-8")) return FakeResponse( { @@ -313,9 +1504,10 @@ def open(self, request, timeout=None): return self.call_func(request, timeout) monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *args: FakeOpener(fake_urlopen)) - verdict = noema.call_llm("owner/repo", 1, pr, "diff", True, "extra review context") + verdict = noema.call_llm("owner/repo", 1, pr, "diff", True, "head", "extra review context") assert verdict["decision"] == "approve" assert seen["url"] == "https://llm.example.test/chat" + assert seen["timeout"] == 14400 assert seen["body"]["model"] == "review-model" assert "extra review context" in seen["body"]["messages"][1]["content"] @@ -328,32 +1520,32 @@ def fake_urlopen_defer(request, timeout=None): lambda *args: FakeOpener(fake_urlopen_defer) ) with pytest.raises(RuntimeError, match="unsupported decision"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") # Test case-insensitive valid URL monkeypatch.setenv("NOEMA_LLM_API_URL", "HTTPS://llm.example.test/chat") monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *args: FakeOpener(fake_urlopen)) - assert noema.call_llm("owner/repo", 1, pr, "diff", True)["decision"] == "approve" + assert noema.call_llm("owner/repo", 1, pr, "diff", True, "head")["decision"] == "approve" # Test invalid scheme (and no original URL in error) monkeypatch.setenv("NOEMA_LLM_API_URL", "file:///etc/passwd") with pytest.raises(ValueError, match="URL scheme must be http or https"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") # Test localhost rejection monkeypatch.setenv("NOEMA_LLM_API_URL", "http://localhost/chat") with pytest.raises(ValueError, match="URL cannot target localhost"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") # Test missing hostname monkeypatch.setenv("NOEMA_LLM_API_URL", "http:///chat") with pytest.raises(ValueError, match="URL must have a valid hostname"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") # Test internal IP rejection monkeypatch.setenv("NOEMA_LLM_API_URL", "http://169.254.169.254/chat") with pytest.raises(ValueError, match="URL cannot target internal IP addresses"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") import socket original_getaddrinfo = socket.getaddrinfo @@ -366,7 +1558,7 @@ def fake_getaddrinfo(host, port, *args, **kwargs): return original_getaddrinfo(host, port, *args, **kwargs) monkeypatch.setattr(socket, "getaddrinfo", fake_getaddrinfo) with pytest.raises(ValueError, match="URL cannot target internal IP addresses"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") # Test unresolved hostname does not break monkeypatch.setenv("NOEMA_LLM_API_URL", "http://unresolved.example.com/chat") @@ -374,7 +1566,7 @@ def fake_getaddrinfo_error(host, port, *args, **kwargs): raise socket.gaierror("Name or service not known") monkeypatch.setattr(socket, "getaddrinfo", fake_getaddrinfo_error) monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *args: FakeOpener(fake_urlopen)) - assert noema.call_llm("owner/repo", 1, pr, "diff", True)["decision"] == "approve" + assert noema.call_llm("owner/repo", 1, pr, "diff", True, "head")["decision"] == "approve" # Test invalid IP string from getaddrinfo (unlikely but theoretically possible) monkeypatch.setenv("NOEMA_LLM_API_URL", "http://weird-dns.example.com/chat") @@ -383,7 +1575,7 @@ def fake_getaddrinfo_invalid_ip(host, port, *args, **kwargs): return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", ("not_an_ip", 0))] return original_getaddrinfo(host, port, *args, **kwargs) monkeypatch.setattr(socket, "getaddrinfo", fake_getaddrinfo_invalid_ip) - assert noema.call_llm("owner/repo", 1, pr, "diff", True)["decision"] == "approve" + assert noema.call_llm("owner/repo", 1, pr, "diff", True, "head")["decision"] == "approve" def test_noema_redirect_handler_rejects_redirects(): @@ -418,7 +1610,7 @@ def raise_gaierror(host, port, *args, **kwargs): monkeypatch.setattr(socket, "getaddrinfo", raise_gaierror) with pytest.raises(ValueError, match="must start with http:// or https://"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") def test_call_llm_rejects_non_http_parsed_scheme(monkeypatch): @@ -430,7 +1622,7 @@ def test_call_llm_rejects_non_http_parsed_scheme(monkeypatch): monkeypatch.setattr(noema.urllib.parse, "urlparse", lambda _: parsed) with pytest.raises(ValueError, match="URL scheme must be http or https"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") def test_format_findings_and_submit_review(monkeypatch): @@ -477,7 +1669,7 @@ def test_inspect_and_review_skip_paths(monkeypatch): monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok", "findings": []}) monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) - assert noema.inspect_and_review("owner/repo", 7) == 0 + assert noema.inspect_and_review("owner/repo", 7, "head") == 0 assert calls cases = [ @@ -488,17 +1680,17 @@ def test_inspect_and_review_skip_paths(monkeypatch): calls.clear() monkeypatch.setattr(noema, "fetch_pr", lambda repo, number, pr=pr: pr) monkeypatch.setattr(noema, "current_actor", lambda actor=actor: actor) - assert noema.inspect_and_review("owner/repo", 7) == 0 + assert noema.inspect_and_review("owner/repo", 7, "head") == 0 assert calls == [] monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: clean_pr) monkeypatch.setattr(noema, "current_actor", lambda: "") with pytest.raises(RuntimeError, match="identity could not be verified"): - noema.inspect_and_review("owner/repo", 7) + noema.inspect_and_review("owner/repo", 7, "head") monkeypatch.setattr(noema, "current_actor", lambda: "opencode-agent") with pytest.raises(RuntimeError, match="independent reviewer credential"): - noema.inspect_and_review("owner/repo", 7) + noema.inspect_and_review("owner/repo", 7, "head") def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatch): @@ -516,7 +1708,85 @@ def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatc monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok"}) monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) - assert noema.inspect_and_review("owner/repo", 7) == 0 + assert noema.inspect_and_review("owner/repo", 7, "head") == 0 + assert calls + + +def test_stale_trigger_stops_before_identity_or_model_work(monkeypatch): + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr(headRefOid="new")) + monkeypatch.setattr( + noema, + "current_actor", + lambda: pytest.fail("stale execution must stop before identity lookup"), + ) + assert noema.inspect_and_review("owner/repo", 7, "old") == 0 + + +def test_expected_head_comparison_is_case_insensitive(monkeypatch): + seen = [] + monkeypatch.setattr( + noema, + "fetch_pr", + lambda repo, number: make_pr(headRefOid="a" * 40, isDraft=True), + ) + monkeypatch.setattr(noema, "current_actor", lambda: seen.append("actor") or "noema") + assert noema.inspect_and_review("owner/repo", 7, "A" * 40) == 0 + assert seen == ["actor"] + + +def test_head_movement_stops_before_review_publication(monkeypatch): + pull_requests = iter((make_pr(), make_pr(headRefOid="new"))) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) + monkeypatch.setattr(noema, "current_actor", lambda: "noema") + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr( + noema, + "call_llm", + lambda *args, **kwargs: {"decision": "approve", "summary": "ok"}, + ) + monkeypatch.setattr( + noema, + "submit_review", + lambda *args, **kwargs: pytest.fail("stale verdict must not publish"), + ) + assert noema.inspect_and_review("owner/repo", 7, "head") == 0 + + +def test_uppercase_expected_head_is_not_stale_before_model_work(monkeypatch): + """An uppercase --expected-head must match GitHub's lowercase live SHA (Devin Review, PR #1507).""" + pr = make_pr(headRefOid="abc123def0") + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) + monkeypatch.setattr(noema, "current_actor", lambda: "noema") + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value: "context") + monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok"}) + calls = [] + monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) + + assert noema.inspect_and_review("owner/repo", 7, "ABC123DEF0") == 0 + assert calls + + +def test_uppercase_expected_head_is_not_stale_before_publication(monkeypatch): + """The pre-publication re-check must also compare case-insensitively.""" + pull_requests = iter((make_pr(headRefOid="abc123def0"), make_pr(headRefOid="abc123def0"))) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) + monkeypatch.setattr(noema, "current_actor", lambda: "noema") + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: ["tool.py"]) + monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr: "context") + monkeypatch.setattr( + noema, + "call_llm", + lambda *args, **kwargs: {"decision": "approve", "summary": "ok"}, + ) + calls = [] + monkeypatch.setattr(noema, "submit_review", lambda *args, **kwargs: calls.append(args)) + + assert noema.inspect_and_review("owner/repo", 7, "ABC123DEF0") == 0 assert calls @@ -535,8 +1805,72 @@ def read(self): return json.dumps({"choices": [{"message": {"content": '{"decision":"approve"}'}}]}).encode() monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", lambda *args, **kwargs: Response()) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) with pytest.raises(RuntimeError, match="substantive summary"): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False) + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + + +def test_call_llm_fails_closed_on_malformed_json_response(monkeypatch): + """Reproduces the reported CI crash: an LLM response whose content is + truncated/malformed JSON must fail the review cleanly through call_llm's + existing RuntimeError path, never as an unhandled json.JSONDecodeError.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + # Malformed: an unquoted property name after the decision key, + # matching "Expecting property name enclosed in double quotes". + malformed_content = '{"decision":"approve", trailing garbage not: "quoted}' + return json.dumps({"choices": [{"message": {"content": malformed_content}}]}).encode() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", lambda *args, **kwargs: Response()) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + with pytest.raises(RuntimeError, match="was not valid JSON"): + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + + +def test_call_llm_repairs_one_malformed_json_response(monkeypatch): + """Ask once for corrected JSON before failing the required review closed.""" + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example/v1/chat/completions") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + contents = iter( + ( + '{"decision":"approve", trailing garbage not: "quoted}', + json.dumps({"decision": "comment", "summary": "Repaired JSON", "findings": []}), + ) + ) + requests = [] + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return None + + def read(self): + content = next(contents) + return json.dumps({"choices": [{"message": {"content": content}}]}).encode() + + def open_response(_opener, request, **_kwargs): + requests.append(json.loads(request.data)) + return Response() + + monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", open_response) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) + + verdict = noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") + + assert verdict["summary"] == "Repaired JSON" + assert len(requests) == 2 + assert "prior verdict was rejected" in requests[1]["messages"][1]["content"] @pytest.mark.parametrize("message", [[], {}, 0, " "]) @@ -560,8 +1894,9 @@ def read(self): return json.dumps({"choices": [{"message": {"content": json.dumps(verdict)}}]}).encode() monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", lambda *args, **kwargs: Response()) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) with pytest.raises(RuntimeError, match="malformed finding"): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False) + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") @pytest.mark.parametrize( @@ -593,8 +1928,9 @@ def read(self): return json.dumps({"choices": [{"message": {"content": json.dumps(verdict)}}]}).encode() monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", lambda *args, **kwargs: Response()) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) with pytest.raises(RuntimeError, match=error): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False) + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") def test_call_llm_rejects_generic_approve_without_changed_line_evidence(monkeypatch): @@ -613,8 +1949,9 @@ def read(self): return json.dumps({"choices": [{"message": {"content": json.dumps(verdict)}}]}).encode() monkeypatch.setattr(noema.urllib.request.OpenerDirector, "open", lambda *args, **kwargs: Response()) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) with pytest.raises(RuntimeError, match="parseable changed-line evidence"): - noema.call_llm("owner/repo", 7, make_pr(), "diff", False) + noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") def test_call_llm_repairs_one_rejected_changed_line_verdict(monkeypatch): @@ -688,13 +2025,14 @@ def read(self): class Opener: def open(self, request, timeout): - assert timeout == 120 + assert timeout == noema.NOEMA_LLM_TIMEOUT_SECONDS payloads.append(json.loads(request.data)) return Response(invalid if len(payloads) == 1 else valid) monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *_args: Opener()) + monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: make_pr()) - assert noema.call_llm("owner/repo", 7, make_pr(), diff, False)["decision"] == "approve" + assert noema.call_llm("owner/repo", 7, make_pr(), diff, False, "head")["decision"] == "approve" assert len(payloads) == 2 assert "trusted validator" in payloads[1]["messages"][1]["content"] @@ -995,14 +2333,36 @@ def test_format_review_evidence_renders_only_structured_entries(): def test_parse_args_and_main(monkeypatch): - parsed = noema.parse_args(["--repo", "owner/repo", "--pr-number", "9"]) + parsed = noema.parse_args( + ["--repo", "owner/repo", "--pr-number", "9", "--expected-head", "a" * 40] + ) assert parsed.repo == "owner/repo" assert parsed.pr_number == 9 + assert parsed.expected_head == "a" * 40 seen = [] - monkeypatch.setattr(noema, "inspect_and_review", lambda repo, number: seen.append((repo, number)) or 0) - assert noema.main(["--repo", "owner/repo", "--pr-number", "9"]) == 0 - assert seen == [("owner/repo", 9)] + monkeypatch.setattr( + noema, + "inspect_and_review", + lambda repo, number, head: seen.append((repo, number, head)) or 0, + ) + assert ( + noema.main( + ["--repo", "owner/repo", "--pr-number", "9", "--expected-head", "a" * 40] + ) + == 0 + ) + assert seen == [("owner/repo", 9, "a" * 40)] with pytest.raises(SystemExit, match="--pr-number must be positive"): - noema.main(["--repo", "owner/repo", "--pr-number", "0"]) + noema.main( + ["--repo", "owner/repo", "--pr-number", "0", "--expected-head", "a" * 40] + ) + with pytest.raises(SystemExit, match="--expected-head must be a canonical lowercase"): + noema.main( + ["--repo", "owner/repo", "--pr-number", "9", "--expected-head", "bad"] + ) + with pytest.raises(SystemExit, match="--expected-head must be a canonical lowercase"): + noema.main( + ["--repo", "owner/repo", "--pr-number", "9", "--expected-head", "A" * 40] + ) diff --git a/tests/test_noema_review_orchestrator_ssrf.py b/tests/test_noema_review_orchestrator_ssrf.py index 01bc30c9ac..cfe7f3c10d 100644 --- a/tests/test_noema_review_orchestrator_ssrf.py +++ b/tests/test_noema_review_orchestrator_ssrf.py @@ -137,24 +137,24 @@ def open(self, request, timeout=None): return self.call_func(request, timeout) monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *args: FakeOpener(fake_urlopen)) - verdict = noema.call_llm("owner/repo", 1, pr, "diff", False) + verdict = noema.call_llm("owner/repo", 1, pr, "diff", False, "head") assert verdict["decision"] == "approve" assert seen["url"] == "http://127.0.0.1:18080/v1/chat/completions" assert seen["model"] == "orchestrator/free" monkeypatch.setenv("NOEMA_LLM_API_URL", "http://127.0.0.1:9/evil") with pytest.raises(ValueError, match="URL cannot target internal IP addresses"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") monkeypatch.delenv("CONTEXTUAL_ORCHESTRATOR_BASE_URL", raising=False) monkeypatch.setenv("NOEMA_LLM_VIA_ORCHESTRATOR", "1") monkeypatch.setenv("NOEMA_LLM_API_URL", "http://[::1]:18080/v1/chat/completions") with pytest.raises(ValueError, match="URL cannot target internal IP addresses"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") monkeypatch.setenv("NOEMA_LLM_API_URL", "http://localhost:18080/v1/chat/completions") with pytest.raises(ValueError, match="URL cannot target localhost"): - noema.call_llm("owner/repo", 1, pr, "diff", False) + noema.call_llm("owner/repo", 1, pr, "diff", False, "head") def test_reject_private_llm_url_scheme_hostname_and_public_dns(monkeypatch): diff --git a/tests/test_opencode_agent_contract.py b/tests/test_opencode_agent_contract.py index 79fdba39aa..35409b42bb 100644 --- a/tests/test_opencode_agent_contract.py +++ b/tests/test_opencode_agent_contract.py @@ -1107,9 +1107,9 @@ def test_opencode_repository_dispatch_authorization_is_fail_closed(): "ALLOWED_DISPATCH_TARGETS": ( "ContextualWisdomLab/.github,ContextualWisdomLab/naruon" ), - "TARGET_REPOSITORY": "ContextualWisdomLab/naruon", - "PR_NUMBER": "1085", - } + "TARGET_REPOSITORY": "ContextualWisdomLab/naruon", + "PR_NUMBER": "1085", + } authorized = subprocess.run( ["bash", "-c", shell], @@ -1897,7 +1897,7 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): assert "CENTRAL_REVIEW_PROCESS_FALLBACK_ELIGIBLE" in workflow assert "CENTRAL_REVIEW_PROCESS_FALLBACK_SCOPE_LABEL" in workflow assert ( - 'OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_RUN_TIMEOUT_SECONDS: "5400"' + 'OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_RUN_TIMEOUT_SECONDS: "11700"' in workflow ) assert ( @@ -1975,7 +1975,7 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): r"Prepare bounded OpenCode review evidence[\s\S]{0,120}timeout-minutes: 12", workflow, ) - assert re.search(r"opencode-review-target:[\s\S]*?timeout-minutes: 325", workflow) + assert re.search(r"opencode-review-target:[\s\S]*?timeout-minutes: 305", workflow) assert "timeout-minutes: 12" in workflow assert re.search( r"Run OpenCode PR Review model pool[\s\S]{0,240}timeout-minutes: 205", workflow @@ -1984,7 +1984,7 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): assert 'OPENCODE_MEDIUM_CHANGE_TOTAL_BUDGET_SECONDS: "11700"' in workflow assert 'OPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDS: "11700"' in workflow assert 'OPENCODE_UNKNOWN_CHANGE_TOTAL_BUDGET_SECONDS: "11700"' in workflow - assert 'OPENCODE_RUN_TIMEOUT_SECONDS: "5400"' in workflow + assert 'OPENCODE_RUN_TIMEOUT_SECONDS: "11700"' in workflow assert 'OPENCODE_TOTAL_RETRY_BUDGET_SECONDS: "11700"' in workflow assert 'OPENCODE_POOL_STEP_TIMEOUT_SECONDS: "12000"' in workflow assert ( @@ -2020,7 +2020,7 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): ) assert 'OPENCODE_MODEL_CANDIDATES: "contextual-orchestrator/orchestrator/free"' in workflow assert 'OPENCODE_MODEL_ATTEMPTS: "1"' in workflow - assert 'OPENCODE_RUN_TIMEOUT_SECONDS: "5400"' in workflow + assert 'OPENCODE_RUN_TIMEOUT_SECONDS: "11700"' in workflow assert 'OPENCODE_EXPORT_TIMEOUT_SECONDS: "180"' in workflow assert 'OPENCODE_TOTAL_RETRY_BUDGET_SECONDS: "11700"' in workflow assert 'OPENCODE_POOL_STEP_TIMEOUT_SECONDS: "12000"' in workflow @@ -2030,15 +2030,15 @@ def test_workflow_provisions_sandbox_tool_and_reviewer_agent(): "OPENCODE_CHANGED_FILES_FILE: ${{ runner.temp }}/opencode-changed-files.txt" in workflow ) - assert 'OPENCODE_SMALL_CHANGE_RUN_TIMEOUT_SECONDS: "5400"' in workflow + assert 'OPENCODE_SMALL_CHANGE_RUN_TIMEOUT_SECONDS: "11700"' in workflow assert 'OPENCODE_SMALL_CHANGE_TOTAL_BUDGET_SECONDS: "11700"' in workflow - assert 'OPENCODE_MEDIUM_CHANGE_RUN_TIMEOUT_SECONDS: "5400"' in workflow + assert 'OPENCODE_MEDIUM_CHANGE_RUN_TIMEOUT_SECONDS: "11700"' in workflow assert 'OPENCODE_MEDIUM_CHANGE_TOTAL_BUDGET_SECONDS: "11700"' in workflow - assert 'OPENCODE_LARGE_CHANGE_RUN_TIMEOUT_SECONDS: "5400"' in workflow + assert 'OPENCODE_LARGE_CHANGE_RUN_TIMEOUT_SECONDS: "11700"' in workflow assert 'OPENCODE_LARGE_CHANGE_TOTAL_BUDGET_SECONDS: "11700"' in workflow - assert 'OPENCODE_UNKNOWN_CHANGE_RUN_TIMEOUT_SECONDS: "5400"' in workflow + assert 'OPENCODE_UNKNOWN_CHANGE_RUN_TIMEOUT_SECONDS: "11700"' in workflow assert 'OPENCODE_UNKNOWN_CHANGE_TOTAL_BUDGET_SECONDS: "11700"' in workflow - assert 'OPENCODE_DYNAMIC_RUN_TIMEOUT_CAP_SECONDS: "5400"' in workflow + assert 'OPENCODE_DYNAMIC_RUN_TIMEOUT_CAP_SECONDS: "11700"' in workflow assert 'OPENCODE_DYNAMIC_TOTAL_BUDGET_CAP_SECONDS: "11700"' in workflow assert 'OPENCODE_DYNAMIC_MAX_CYCLES_CAP: "1"' in workflow assert 'OPENCODE_FREE_RUN_TIMEOUT_SECONDS: "3600"' in workflow @@ -2342,6 +2342,27 @@ def timeout_minutes(pattern: str) -> int: ) +def test_contextual_orchestrator_uses_outer_pool_budget() -> None: + """Do not impose a shorter per-process cutoff on orchestration.""" + workflow = Path(".github/workflows/opencode-review-dispatch.yml").read_text( + encoding="utf-8" + ) + model_pool = workflow.split(" - name: Run OpenCode PR Review model pool", 1)[ + 1 + ].split(" - name: Exchange OpenCode app token for review writes", 1)[0] + timeout_variables = ( + "OPENCODE_RUN_TIMEOUT_SECONDS", + "OPENCODE_SMALL_CHANGE_RUN_TIMEOUT_SECONDS", + "OPENCODE_MEDIUM_CHANGE_RUN_TIMEOUT_SECONDS", + "OPENCODE_LARGE_CHANGE_RUN_TIMEOUT_SECONDS", + "OPENCODE_UNKNOWN_CHANGE_RUN_TIMEOUT_SECONDS", + "OPENCODE_DYNAMIC_RUN_TIMEOUT_CAP_SECONDS", + "OPENCODE_CENTRAL_REVIEW_PROCESS_FALLBACK_RUN_TIMEOUT_SECONDS", + ) + for variable in timeout_variables: + assert f'{variable}: "11700"' in model_pool + + def test_opencode_approval_gate_shell_is_parseable(): """Guard the large inline approval shell against YAML-valid syntax breaks.""" if os.name == "nt": @@ -2660,7 +2681,10 @@ def test_opencode_privileged_review_security_boundaries_are_fail_closed(): '^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$ ]]' ) in metadata_step assert '[ "$live_head_repository" != "$TARGET_REPOSITORY" ]' not in metadata_step - assert '[ "$SUPPLIED_HEAD_SHA" = "$live_head_sha" ]' in metadata_step + assert 'mismatches+=("head_sha")' in metadata_step + assert '[ "$SUPPLIED_HEAD_REF" = "$live_head_ref" ]' in metadata_step + assert "proceeding with the live head" not in metadata_step + assert "head_sha=%s\\n' \"$live_head_sha\"" in metadata_step assert ( 'live_visibility="$(jq -r \'.base.repo.visibility // empty | ascii_downcase\'' ) in metadata_step diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index ff73d1e0b0..7fb4456f56 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -3,8 +3,10 @@ from __future__ import annotations import json +import os import shutil import subprocess +import textwrap from pathlib import Path import pytest @@ -12,6 +14,7 @@ HEAD = "a" * 40 WORKFLOW = Path(".github/workflows/opencode-review.yml") +DISPATCH_WORKFLOW = Path(".github/workflows/opencode-review-dispatch.yml") STATUS_HELPER = Path("scripts/ci/opencode_dispatch_status.py") @@ -102,14 +105,19 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non assert "Request current-head OpenCode review execution" in workflow assert "repos/ContextualWisdomLab/.github/dispatches" in workflow assert "exchange_github_app_token" in workflow + assert "Reject untrusted fork review resource consumption" in workflow + assert "github.event.pull_request.head.repo.full_name" in workflow target_job = workflow.split(" opencode-review-target:\n", 1)[1] - assert "timeout-minutes: 100" in target_job.split(" steps:\n", 1)[0] + assert "timeout-minutes: 5" in target_job.split(" steps:\n", 1)[0] + assert "for attempt in" not in workflow + assert "opencode-review-wait-window-one" not in workflow assert "id-token: write" in target_job.split(" steps:\n", 1)[0] - assert 'event_type:"merge-scheduler"' in workflow - assert "trigger_reviews:true" in workflow - assert "for attempt in $(seq 1 180)" in target_job - assert "sleep 30" in target_job - assert "enable_auto_merge:false" in workflow + assert "steps.verdict.outputs.verdict == ''" in target_job + assert 'event_type:"opencode-review"' in workflow + assert 'sleep "$remaining_seconds"' not in workflow + assert workflow.count("timeout 25 gh api --paginate") == 1 + assert workflow.count('if ! reviews="$(timeout 25 gh api') == 1 + assert workflow.count('reviews="[]"') == 1 assert 'gh api --paginate "repos/${TARGET_REPOSITORY}/pulls/${PR_NUMBER}/reviews"' in workflow assert "github.event.pull_request.head.sha" in workflow assert "This required check is not a review and must not succeed" in workflow @@ -117,3 +125,170 @@ def test_required_workflow_cannot_succeed_with_an_echo_only_placeholder() -> Non "Review approval remains a separate current-head PR review requirement" not in workflow ) + + +def test_formal_receipt_reruns_failed_required_job_without_runner_polling() -> None: + """A formal receipt wakes the failed required run instead of polling for hours.""" + required = WORKFLOW.read_text(encoding="utf-8") + dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") + assert "for attempt in" not in required + assert "rerun-failed-jobs" in dispatched + assert '--argjson required_run_id "$GITHUB_RUN_ID"' in required + assert "required_run_id:$required_run_id" in required + assert "id: formal_review_receipt" in dispatched + assert "steps.formal_review_receipt.outcome == 'success'" in dispatched + assert "github.event.client_payload.required_run_id != ''" in dispatched + assert 'gh api "repos/${GH_REPOSITORY}/actions/runs/${REQUIRED_RUN_ID}"' in dispatched + assert "select(.id == $run_id)" in dispatched + assert 'select(.event == "pull_request_target")' in dispatched + assert 'select(.path == ".github/workflows/opencode-review.yml")' in dispatched + assert "select(.head_sha == $head)" in dispatched + wake_step = dispatched.split("Wake exact-head required OpenCode workflow", 1)[1].split("\n\n - name:", 1)[0] + target_job = dispatched.split(" opencode-review-target:\n", 1)[1] + target_permissions = target_job.split(" env:\n", 1)[0] + assert "actions: write" in target_permissions + assert ( + "needs.validate-pr-metadata.outputs.target_repository == " + "github.repository && github.token" + ) in wake_step + assert "steps.opencode_app_token.outputs.token" not in wake_step + assert "WAKE_TOKEN_SOURCE" in wake_step + assert '"$WAKE_TOKEN_SOURCE" = "unavailable"' in wake_step + assert "--paginate" not in wake_step + # Identity is the immutable target-repository run id plus event/path/head; + # do not depend on context-specific title or workflow_url rendering. + assert "display_title ==" not in wake_step + assert ".name | startswith(" not in wake_step + assert 'workflow_url | contains("/actions/required_workflows/")' not in wake_step + + +def wake_selector(run: dict[str, object], *, head: str = HEAD, run_id: int = 42) -> str: + """Execute the wake step's run-validation jq program in isolation.""" + jq = shutil.which("jq") + if jq is None: + pytest.skip("jq is required to execute the production wake selector") + dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") + marker = """jq -r --arg head "$PR_HEAD_SHA" --argjson run_id "$REQUIRED_RUN_ID" '""" + start = dispatched.index(marker) + len(marker) + end = dispatched.index("\n ')", start) + result = subprocess.run( + [jq, "-r", "--arg", "head", head, "--argjson", "run_id", str(run_id), dispatched[start:end]], + input=json.dumps(run), + text=True, + capture_output=True, + check=False, + ) + assert result.returncode == 0, result.stderr + return result.stdout.strip() + + +def required_run(*, run_id: int = 42, head_sha: str = HEAD, path: str = ".github/workflows/opencode-review.yml") -> dict[str, object]: + """Build one realistic single-run GET REST API record. + + Mirrors the real shape a sibling repo sees for a run injected by the org's + required-workflow ruleset (this repo's actual central-hub use case): `name` + is the bare workflow name and `display_title` is a plain PR title, with no + PR number or head SHA embedded in either -- unlike a native same-repo + trigger, where both fields carry the rendered `run-name`. + """ + return { + "id": run_id, + "head_sha": head_sha, + "event": "pull_request_target", + "name": "Required OpenCode Review", + "display_title": "Fix an unrelated example bug", + "path": path, + "workflow_url": ( + "https://api.github.com/repos/ContextualWisdomLab/example" + "/actions/required_workflows/9" + ), + "status": "completed", + "conclusion": "failure", + } + + +def test_wake_selector_matches_the_referenced_run_without_name_or_display_title() -> None: + """The exact-id, exact-head run is matched using only id/event/path/head_sha.""" + assert wake_selector(required_run()) == "42\tcompleted\tfailure" + + +def test_wake_selector_rejects_a_referenced_run_with_a_different_head() -> None: + """A referenced run whose head_sha has moved on (Devin Review, PR #1507: + + 'another PR or head') must not be treated as the current PR's required run + -- the realistic failure mode for an id-based reference, e.g. a superseded + run or a stale/forged required_run_id. + """ + assert wake_selector(required_run(head_sha="b" * 40)) == "" + + +def test_wake_selector_rejects_a_referenced_run_for_a_different_workflow() -> None: + """A referenced run for a different required workflow (Strix) is rejected.""" + assert wake_selector(required_run(path=".github/workflows/strix.yml")) == "" + + +def test_formal_receipt_wakes_the_exact_head_failed_required_run(tmp_path: Path) -> None: + """Execute the production wake script end-to-end against a fake GitHub API.""" + dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") + step = dispatched.split(" - name: Wake exact-head required OpenCode workflow\n", 1)[1] + run_block = step.split(" run: |\n", 1)[1].split("\n\n - name:", 1)[0] + script = textwrap.dedent(run_block) + calls = tmp_path / "calls" + fake_gh = tmp_path / "gh" + fake_gh.write_text( + f"""#!/usr/bin/env bash +set -euo pipefail +printf '%s\\n' "$*" >>"$FAKE_CALLS" +if [[ "$*" == *"actions/runs/42/rerun-failed-jobs"* ]]; then exit 0; fi +if [[ "$*" == *"actions/runs/42"* ]]; then printf '%s\\n' '{json.dumps(required_run())}'; exit 0; fi +exit 1 +""", + encoding="utf-8", + ) + fake_gh.chmod(0o755) + result = subprocess.run( # noqa: S603 + [shutil.which("bash") or "/bin/bash", "-c", script], + env={ + **os.environ, + "PATH": f"{tmp_path}{os.pathsep}{os.environ.get('PATH', '')}", + "FAKE_CALLS": str(calls), + "GH_REPOSITORY": "ContextualWisdomLab/example", + "GH_TOKEN": "actions-write-token", + "PR_HEAD_SHA": HEAD, + "REQUIRED_RUN_ID": "42", + "WAKE_TOKEN_SOURCE": "PR_REVIEW_MERGE_TOKEN", + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + recorded = calls.read_text(encoding="utf-8") + assert "actions/runs/42/rerun-failed-jobs" in recorded + assert "repos/ContextualWisdomLab/example/actions/runs/42" in recorded + assert "--paginate" not in recorded + + +def test_sibling_formal_receipt_fails_closed_without_actions_token() -> None: + """A sibling wake without either Actions-capable PAT fails before GitHub I/O.""" + dispatched = DISPATCH_WORKFLOW.read_text(encoding="utf-8") + step = dispatched.split(" - name: Wake exact-head required OpenCode workflow\n", 1)[1] + script = textwrap.dedent( + step.split(" run: |\n", 1)[1].split("\n\n - name:", 1)[0] + ) + result = subprocess.run( # noqa: S603 + [shutil.which("bash") or "/bin/bash", "-c", script], + env={ + **os.environ, + "GH_TOKEN": "", + "GH_REPOSITORY": "ContextualWisdomLab/example", + "PR_HEAD_SHA": HEAD, + "REQUIRED_RUN_ID": "42", + "WAKE_TOKEN_SOURCE": "unavailable", + }, + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 1 + assert "Actions-capable wake credential is unavailable" in result.stdout diff --git a/tests/test_pr_review_autofix_nvidia_nim_contract.py b/tests/test_pr_review_autofix_nvidia_nim_contract.py index 3dcfe2cdd8..b2e29b9c13 100644 --- a/tests/test_pr_review_autofix_nvidia_nim_contract.py +++ b/tests/test_pr_review_autofix_nvidia_nim_contract.py @@ -19,7 +19,7 @@ DOCTORING_RECORD = Path("docs/doctoring/hourly-nvidia-nim-autofix.md") CHANGELOG = Path("CHANGELOG.md") REVIEW_DISPATCH_WORKFLOW = Path(".github/workflows/opencode-review-dispatch.yml") -REVIEW_DISPATCH_BLOB_SHA = "2aa245e7f2a053a4c0b7a9cc8bac0d5d44d38092" +REVIEW_DISPATCH_BLOB_SHA = "cdc1245266403f0b238558ecbab528d1557412dd" def _workflow_text(path: Path) -> str: diff --git a/tests/test_pr_review_merge_scheduler.py b/tests/test_pr_review_merge_scheduler.py index b3dced78d4..919566aeb2 100644 --- a/tests/test_pr_review_merge_scheduler.py +++ b/tests/test_pr_review_merge_scheduler.py @@ -1487,6 +1487,10 @@ def test_context_review_and_check_helpers(monkeypatch): no_job_url = make_pr(statusCheckRollup={"contexts": {"nodes": [opencode_check()]}}) assert sched.matching_actions_job_id(no_job_url, sched.is_opencode_context) is None + assert sched.matching_actions_run_id(check_jobs, sched.is_opencode_context) == 1 + assert sched.matching_actions_run_id(check_jobs, sched.is_strix_context) == 2 + assert sched.matching_actions_run_id(no_job_url, sched.is_opencode_context) is None + assert sched.parse_github_datetime(None) is None assert sched.parse_github_datetime("not-a-date") is None assert sched.parse_github_datetime("2026-06-25T07:00:00Z") == datetime(2026, 6, 25, 7, 0, tzinfo=timezone.utc) @@ -1698,6 +1702,186 @@ def test_context_review_and_check_helpers(monkeypatch): assert not sched.is_opencode_review(opencode_review(login="human")) +def test_matching_actions_run_id_selects_by_recency_not_list_position(): + """Devin Review finding on PR #1507 ("Older review run remains blocking"). + + Two same-purpose OpenCode check runs are present, listed *older-first* + -- the opposite of the ordering the previous ``reversed(...)``-plus- + first-match implementation silently depended on. The genuinely newer + run (a later ``checkSuite.createdAt``) must still win, proving + selection is driven by ``check_run_recency_key``, not by position in + ``context_nodes``. + """ + older_first = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [ + { + "__typename": "CheckRun", + "name": "opencode-review", + "status": "COMPLETED", + "conclusion": "SUCCESS", + "startedAt": "2026-06-25T07:05:00Z", + "detailsUrl": "https://github.com/owner/repo/actions/runs/501/job/1", + "checkSuite": { + "createdAt": "2026-06-25T07:00:00Z", + "workflowRun": {"workflow": {"name": "OpenCode Review"}}, + }, + }, + { + "__typename": "CheckRun", + "name": "opencode-review", + "status": "COMPLETED", + "conclusion": "FAILURE", + "startedAt": "2026-06-25T08:05:00Z", + "detailsUrl": "https://github.com/owner/repo/actions/runs/502/job/2", + "checkSuite": { + "createdAt": "2026-06-25T08:00:00Z", + "workflowRun": {"workflow": {"name": "OpenCode Review"}}, + }, + }, + ] + } + } + ) + assert sched.matching_actions_run_id(older_first, sched.is_opencode_check_run) == 502 + + newer_first = make_pr( + statusCheckRollup={ + "contexts": {"nodes": list(reversed(older_first["statusCheckRollup"]["contexts"]["nodes"]))} + } + ) + assert sched.matching_actions_run_id(newer_first, sched.is_opencode_check_run) == 502 + + +def test_discover_opencode_required_run_id_bounded_head_scoped_lookup(monkeypatch): + """Devin Review finding on PR #1507 ("Large check rollups never wake"). + + ``matching_actions_run_id`` only sees the first 100 GraphQL rollup + contexts. When the required run falls outside that page (simulated + here by an empty rollup), ``discover_opencode_required_run_id`` must + still find it through a REST lookup scoped server-side to the exact + event, workflow path, and head SHA -- never an unfiltered history walk + -- and must ignore a same-head run for a different workflow path and a + same-path run for a different head. + """ + head_sha = "a" * 40 + other_head = "b" * 40 + calls = [] + + def fake_active_workflow_runs(repo, statuses, *, event=None, created=None, head_sha=None): + calls.append((repo, tuple(statuses), event, created, head_sha)) + return [ + { + "id": 601, + "path": ".github/workflows/strix.yml", + "head_sha": head_sha, + "run_started_at": "2026-06-25T07:00:00Z", + }, + { + "id": 602, + "path": sched.OPENCODE_REVIEW_WORKFLOW_PATH, + "head_sha": other_head, + "run_started_at": "2026-06-25T07:00:00Z", + }, + { + "id": 603, + "path": sched.OPENCODE_REVIEW_WORKFLOW_PATH, + "head_sha": head_sha, + "run_started_at": "2026-06-25T06:00:00Z", + }, + { + "id": 604, + "path": sched.OPENCODE_REVIEW_WORKFLOW_PATH, + "head_sha": head_sha, + "run_started_at": "2026-06-25T09:00:00Z", + }, + ] + + monkeypatch.setattr(sched, "active_workflow_runs", fake_active_workflow_runs) + + assert sched.discover_opencode_required_run_id("owner/repo", head_sha) == 604 + assert len(calls) == 1 + repo, statuses, event, created, called_head = calls[0] + assert repo == "owner/repo" + assert set(statuses) == {"queued", "in_progress", "completed"} + assert event == "pull_request_target" + assert called_head == head_sha + + assert sched.discover_opencode_required_run_id("owner/repo", "not-a-sha") is None + assert len(calls) == 1 + + +def test_discover_opencode_required_run_id_ranks_and_skips_edge_case_rows(monkeypatch): + """Cover the recency comparison's edge cases the happy path above does not. + + A matching row with no ``id`` must be skipped entirely (never crash on + ``int(None)``); a matching row with no timestamp may still become the + first candidate; and a later matching row with an *older* timestamp + than the current best must not displace it. + """ + head_sha = "a" * 40 + + def fake_active_workflow_runs(repo, statuses, *, event=None, created=None, head_sha=None): + return [ + # No timestamp at all: still becomes the first candidate. + {"id": 801, "path": sched.OPENCODE_REVIEW_WORKFLOW_PATH, "head_sha": head_sha}, + # A real timestamp: newer than "no timestamp", becomes the new best. + { + "id": 802, + "path": sched.OPENCODE_REVIEW_WORKFLOW_PATH, + "head_sha": head_sha, + "run_started_at": "2026-02-01T00:00:00Z", + }, + # Matches path/head but has no id: must be skipped, not crash. + { + "id": None, + "path": sched.OPENCODE_REVIEW_WORKFLOW_PATH, + "head_sha": head_sha, + "run_started_at": "2026-03-01T00:00:00Z", + }, + # Older than the current best (802): must not displace it. + { + "id": 803, + "path": sched.OPENCODE_REVIEW_WORKFLOW_PATH, + "head_sha": head_sha, + "run_started_at": "2026-01-01T00:00:00Z", + }, + ] + + monkeypatch.setattr(sched, "active_workflow_runs", fake_active_workflow_runs) + + assert sched.discover_opencode_required_run_id("owner/repo", head_sha) == 802 + + +def test_dispatch_opencode_review_falls_back_to_bounded_discovery(monkeypatch): + """Scheduler dispatch uses the bounded fallback only when the rollup misses.""" + monkeypatch.setenv("GITHUB_ACTIONS", "true") + monkeypatch.setenv("GH_TOKEN", "opencode-app-token") + calls = [] + monkeypatch.setattr( + sched, "active_opencode_run_refs", lambda repo, workflow, pr: ([], []) + ) + monkeypatch.setattr(sched, "force_cancel_workflow_run_refs", lambda refs: None) + monkeypatch.setattr( + sched, + "discover_opencode_required_run_id", + lambda repo, head_sha: calls.append((repo, head_sha)) or 999, + ) + dispatch_calls = [] + monkeypatch.setattr( + sched, "run_github_dispatch", lambda args, stdin=None: dispatch_calls.append(stdin) + ) + + head_sha = "a" * 40 + pr = make_pr(headRefOid=head_sha, baseRefOid="b" * 40) + result = sched.dispatch_opencode_review("owner/repo", "OpenCode Review", pr, dry_run=False) + + assert result == "dispatched" + assert calls == [("owner/repo", head_sha)] + assert json.loads(dispatch_calls[0])["client_payload"]["required_run_id"] == 999 + + def test_central_progress_ignores_required_workflow_checkrun_placeholder( monkeypatch, ): @@ -3939,7 +4123,15 @@ def fake_run(args, stdin=None): ] assert calls[9][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] assert calls[10][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] - assert calls[11] == [ + # calls[11:14]: the bounded discover_opencode_required_run_id fallback + # (matching_actions_run_id found nothing in this PR's empty rollup). + for offset, status in enumerate(("queued", "in_progress", "completed")): + discover_call = calls[11 + offset] + assert discover_call[:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] + assert f"status={status}" in discover_call + assert "event=pull_request_target" in discover_call + assert f"head_sha={head_sha}" in discover_call + assert calls[14] == [ "gh", "api", "-X", @@ -4194,7 +4386,17 @@ def fake_run_with_env(args, *, stdin=None, env=None): ] assert calls[6][0][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] assert calls[7][0][:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] - assert calls[8][0] == [ + # calls[8:11]: the bounded discover_opencode_required_run_id fallback + # (matching_actions_run_id found nothing in the empty rollup), scoped to + # the exact head SHA across the three statuses that can hold the + # required run. + for offset, status in enumerate(("queued", "in_progress", "completed")): + discover_call = calls[8 + offset][0] + assert discover_call[:5] == ["gh", "api", "--method", "GET", "repos/owner/repo/actions/runs"] + assert f"status={status}" in discover_call + assert "event=pull_request_target" in discover_call + assert f"head_sha={'a' * 40}" in discover_call + assert calls[11][0] == [ "gh", "api", "-X", @@ -4223,7 +4425,20 @@ def fake_run_with_env(args, *, stdin=None, env=None): monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REPOSITORY", "ContextualWisdomLab/.github") monkeypatch.setenv("SCHEDULER_REQUIRED_WORKFLOW_REF", "main") - pr = make_pr(baseRefName="develop", baseRefOid=base_sha, headRefOid=head_sha) + pr = make_pr( + baseRefName="develop", + baseRefOid=base_sha, + headRefOid=head_sha, + statusCheckRollup={ + "contexts": { + "nodes": [ + opencode_check( + details_url="https://github.com/owner/repo/actions/runs/42/job/101" + ) + ] + } + }, + ) sched.dispatch_strix_evidence("owner/repo", "Strix Security Scan", pr, dry_run=False) sched.dispatch_opencode_review("owner/repo", "OpenCode Review", pr, dry_run=False) @@ -4270,6 +4485,7 @@ def fake_run_with_env(args, *, stdin=None, env=None): "pr_base_sha": base_sha, "pr_head_ref": "feature", "pr_head_sha": head_sha, + "required_run_id": 42, }, } @@ -4513,6 +4729,92 @@ def fake_run(args, stdin=None): assert not any(call[:3] == ["gh", "workflow", "run"] for call in calls) +def test_complete_paginated_pr_contexts_finds_required_run_after_first_100(monkeypatch): + """Load later GraphQL pages before required-run selection.""" + required = opencode_check(details_url="https://github.com/owner/repo/actions/runs/142/job/2") + required["checkSuite"]["createdAt"] = "2026-08-31T02:00:00Z" + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [{"__typename": "StatusContext", "context": f"check-{i}"} for i in range(100)], + "pageInfo": {"hasNextPage": True, "endCursor": "page-2"}, + } + } + ) + monkeypatch.setattr( + sched, + "gh_graphql", + lambda *args, **kwargs: { + "data": { + "repository": { + "pullRequest": { + "statusCheckRollup": { + "contexts": { + "nodes": [required], + "pageInfo": {"hasNextPage": False, "endCursor": None}, + } + } + } + } + } + }, + ) + sched.complete_paginated_pr_contexts("owner/repo", pr) + assert len(sched.context_nodes(pr)) == 101 + assert sched.matching_actions_run_id(pr, sched.is_opencode_check_run) == 142 + + +def test_complete_paginated_pr_contexts_rejects_missing_cursor(): + """Fail closed when GitHub advertises a page without a cursor.""" + pr = make_pr( + statusCheckRollup={ + "contexts": {"nodes": [], "pageInfo": {"hasNextPage": True}} + } + ) + with pytest.raises(RuntimeError, match="did not provide an end cursor"): + sched.complete_paginated_pr_contexts("owner/repo", pr) + + +def test_complete_paginated_pr_contexts_bounds_repeated_pages(monkeypatch): + """Fail closed when GitHub never terminates status-context pagination.""" + pr = make_pr( + statusCheckRollup={ + "contexts": { + "nodes": [], + "pageInfo": {"hasNextPage": True, "endCursor": "loop"}, + } + } + ) + calls = [] + + def repeated_page(*args, **kwargs): + calls.append((args, kwargs)) + return { + "data": { + "repository": { + "pullRequest": { + "statusCheckRollup": { + "contexts": { + "nodes": [], + "pageInfo": { + "hasNextPage": True, + "endCursor": "loop", + }, + } + } + } + } + } + } + + monkeypatch.setattr(sched, "gh_graphql", repeated_page) + monkeypatch.setattr(sched, "MAX_REVIEW_PAGINATION_PAGES", 1) + with pytest.raises(RuntimeError, match="exceeded its safety bound"): + sched.complete_paginated_pr_contexts("owner/repo", pr) + assert len(calls) == 1 + assert calls[0][1]["cursor"] == "loop" + + @pytest.mark.parametrize( ("workflow_name", "run_title"), [ diff --git a/tests/test_repository_branch_coverage_javascript_and_noema.py b/tests/test_repository_branch_coverage_javascript_and_noema.py index 6eb5ab9baf..99793a4dfa 100644 --- a/tests/test_repository_branch_coverage_javascript_and_noema.py +++ b/tests/test_repository_branch_coverage_javascript_and_noema.py @@ -5,8 +5,6 @@ import json import subprocess from pathlib import Path -from typing import Any - import pytest from scripts.ci import javascript_coverage_gate as js_gate diff --git a/tests/test_repository_branch_coverage_review_schedulers.py b/tests/test_repository_branch_coverage_review_schedulers.py index 85cdc0b964..4928e18046 100644 --- a/tests/test_repository_branch_coverage_review_schedulers.py +++ b/tests/test_repository_branch_coverage_review_schedulers.py @@ -63,11 +63,11 @@ class Opener: """Open one deterministic provider response.""" def open(self, _request: Any, timeout: int) -> Response: - assert timeout == 120 + assert timeout == noema.NOEMA_LLM_TIMEOUT_SECONDS return Response() monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *_args: Opener()) - verdict = noema.call_llm("owner/repo", 1, {"headRefOid": "a" * 40}, "diff", False) + verdict = noema.call_llm("owner/repo", 1, {"headRefOid": "a" * 40}, "diff", False, "a" * 40) assert verdict["decision"] == "approve" diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index b3eac37fac..5a295da25f 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -241,7 +241,15 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: assert "github.event.pull_request.base.repo.full_name" in concurrency_contract assert "github.repository" in concurrency_contract assert "github.event.pull_request.number" in workflow - assert "cancel-in-progress: true" in workflow + if filename == "noema-review.yml": + assert "cancel-in-progress: ${{" in concurrency_contract + assert "github.event_name != 'workflow_run'" in concurrency_contract + assert ( + "github.event.workflow_run.conclusion != 'cancelled'" + in concurrency_contract + ) + else: + assert "cancel-in-progress: true" in workflow if filename in { "close-empty-pr.yml", "security-scan.yml", @@ -254,7 +262,26 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: assert "opencode-review-bootstrap-" in concurrency_contract elif filename == "noema-review.yml": assert "github.event.workflow_run.pull_requests[0].number" in concurrency_contract - assert "github.event_name }}" not in concurrency_contract + assert "github.event.pull_request.head.sha" in concurrency_contract + assert "github.event.workflow_run.pull_requests[0].head.sha" in concurrency_contract + assert "github.event.workflow_run.head_sha" not in concurrency_contract + assert "github.event.client_payload.pr_head_sha" in concurrency_contract + assert "github.event.workflow_run.conclusion == 'cancelled'" in ( + concurrency_contract + ) + assert "format('cancelled-{0}', github.run_id)" in concurrency_contract + assert "'actionable'" in concurrency_contract + procedure = ( + REPO_ROOT / "docs" / "pr-review-and-merge-procedure.md" + ).read_text(encoding="utf-8") + assert "head-specific native concurrency" in procedure + assert "live-head validation explicitly cancels" in procedure + for source in ( + "`pull_request_target` uses `pull_request.head.sha`", + "`workflow_run` uses `workflow_run.pull_requests[0].head.sha`", + "`repository_dispatch` uses `client_payload.pr_head_sha`", + ): + assert source in procedure else: if filename in {"codeql-pr.yml", "osv-scanner-pr.yml", "scorecard-pr.yml"}: assert "github.event_name == 'pull_request'" in concurrency_contract @@ -262,7 +289,8 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: assert ( "github.event_name == 'pull_request_target'" in concurrency_contract ) - assert "github.event.pull_request.head.sha" not in concurrency_contract + if filename != "noema-review.yml": + assert "github.event.pull_request.head.sha" not in concurrency_contract assert "format('pr-{0}-{1}'" not in concurrency_contract @@ -399,27 +427,30 @@ def test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs() - assert "closed" in workflow assert "cancel-closed-pr-runs:" in workflow - if filename == "strix.yml": - assert "Cancel queued and running scans for the closed pull request" in workflow - assert ( - "secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN " - "|| github.token" - ) in workflow - assert "DISPATCH_REPOSITORY" not in workflow - assert "CLOSED_PR_HEAD_SHA" in workflow - assert 'select(.event == "pull_request_target")' in workflow - assert 'select(.event == "repository_dispatch")' not in workflow + if filename in {"strix.yml", "noema-review.yml"}: + noun = "scans" if filename == "strix.yml" else "Noema reviews" + assert f"Cancel queued and running {noun} for the closed pull request" in workflow assert "leaving runs unchanged" in workflow - assert ( - "for active_status in queued in_progress requested waiting pending" - in workflow - ) + next_job = "strix" if filename == "strix.yml" else "noema-review" cleanup_job = workflow.split(" cancel-closed-pr-runs:", 1)[1].split( - " strix:", 1 + f" {next_job}:", 1 )[0] assert "actions: write" in cleanup_job assert "actions/checkout" not in cleanup_job assert "cleanup skipped" not in cleanup_job + if filename == "strix.yml": + assert "CLOSED_PR_HEAD_SHA" in workflow + assert ( + "for active_status in queued in_progress requested waiting pending" + in workflow + ) + assert ( + "secrets.PR_REVIEW_MERGE_TOKEN || secrets.OPENCODE_APPROVE_TOKEN " + "|| github.token" + ) in workflow + assert "DISPATCH_REPOSITORY" not in workflow + assert 'select(.event == "pull_request_target")' in workflow + assert 'select(.event == "repository_dispatch")' not in workflow else: assert ( "PR closed; this run only cancels older runs through workflow concurrency." @@ -496,7 +527,9 @@ def test_noema_triggers_serialize_one_review_per_pull_request() -> None: assert "github.event.pull_request.number || github.event.workflow_run.pull_requests[0].number" in concurrency_contract assert "github.event.client_payload.pr_number" in concurrency_contract - assert "github.event_name }}" not in concurrency_contract + assert "github.event.workflow_run.conclusion == 'cancelled'" in concurrency_contract + assert "format('cancelled-{0}', github.run_id)" in concurrency_contract + assert "'actionable'" in concurrency_contract def test_noema_review_credentials_and_orchestrator_configuration_fail_closed() -> None: